[Toybox] [patch] add su

[Rob Landley]

On 08/12/2013 11:20:40 PM, Strake wrote:
> =46rom 4693d474e5cdb95898bb0759eaac6a0d693ee33d Mon Sep 17 00:00:00  
> 2001
> From: Strake <strake888 at gmail.com>
> Date: Mon, 12 Aug 2013 23:08:53 +0000
> Subject: [PATCH] add su
> 
> ---
>  toys/pending/su.c | 98  
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 98 insertions(+)
>  create mode 100644 toys/pending/su.c
> 
> diff --git a/toys/pending/su.c b/toys/pending/su.c
> new file mode 100644
> index 0000000..7f0866d
> --- /dev/null
> +++ b/toys/pending/su.c
> @@ -0,0 +1,98 @@
> +/* su.c - switch user
> + *
> + * Copyright 2013 CE Strake <strake888 at gmail.com>
> + *
> + * See  
> http://refspecs.linuxfoundation.org/LSB_4.1.0/LSB-Core-generic/LSB-Core-generic/su.html
> +
> +USE_SU(NEWTOY(su, "lmpc:s:", TOYFLAG_BIN))
> +
> +config SU
> +  bool "su"
> +  default n
> +  help
> +    usage: su [-lmp] [-c cmd] [-s shell] [user [argu...]]

I tend to have things like CMD SHELL USER be all caps to show it's not  
a literal argument. I also tend to say ARGS instead of argu (there's a  
couple existing instances in the tree, trying to be consistent).

I need to do a cleanup pass actually. df has [FILESYSTEM ...] when it  
should probably have FILESYSTEM[...]. And of course chgrp has file...  
chmod has FILE... cut has [FILE]... xzcat has [filename...] and so on.  
(It'd be nice there was a standard on this. I'm sort of copying what  
the man pages do but it's cargo cult documenting...)

Right, throw another todo item on the heap.

> +    Switch to given user, or root if not given, and call a shell with
> the given arguments.
> +
> +    options:
> +      -s  shell to call
> +      -c  command to pass to shell with -c
> +
> +    flags:
> +      -l      login shell
> +      -(m|p)  preserve environment
> +*/
> +
> +#define FOR_su
> +#include "toys.h"
> +
> +GLOBALS(
> +  char *sArgu;
> +  char *cArgu;
> +)

I'm leaning more and more towards just having the argument variable be  
the argument letter unless there's an immediately more obvious name.  
TT.s is sort of its own namespace...

> +extern char **environ;

That should be in toys.h

> +static void deny () {
> +  printf ("Denied\n");
> +  xexit ();
> +}
> +
> +void su_main () {
> +  char *name, *passhash, **argu, **argv;
> +  struct passwd *up;
> +  struct spwd *shp;
> +  long ii;
> +
> +  if (toys.optc && strcmp ("-", toys.optargs[0]) == 0) {
> +    toys.optflags |= FLAG_l;
> +    toys.optc--; toys.optargs++;
> +  }
> +
> +  if (toys.optc) {
> +    name = toys.optargs[0];
> +    toys.optc--; toys.optargs++;
> +  }

This is a common enough idiom I'm starting to think there should be an  
xwrap function for it. Hmmm...

> +  else name = "root";
> +  shp = getspnam (name);
> +  if (!shp) perror_exit ("failed to find password");

See earlier discussion on minimal error messages aimed at non-english  
speakers who have a hard enough time learning the posix command names  
in a non-native alphabet. It's linked from  
http://landley.net/toybox/cleanup.html which I _really_ need to catch  
up on the writeups for...

The root user is uid 0, which could be _named_ anything. (Crazy people  
say "wheel".) So we need a getwpuid(0) else getpwnam(name) and then do  
the shadow lookup based on that name.

I'm curious what bionic/android does with this. First version I checked  
user accounts weren't really implemented. I know they don't have an  
/etc/passwd file. (Last time I implemented this stuff I parsed  
/etc/passwd and /etc/shadow myself, but when I looked into android they  
had some funky database stored in extended attributes or something; I  
forget the details. Everybody keeps thinking the windows registry was  
somehow a good idea...)

> +  switch (shp -> sp_pwdp[0]) {
> +  case '!': deny ();
> +  case '$': break;
> +  default : error_exit ("bad password format");
> +  }
> +
> +  if (read_password (toybuf, sizeof (toybuf), "Password: ") != 0)
> perror_exit ("failed to read password");
> +
> +  passhash = crypt (toybuf, shp -> sp_pwdp);
> +  if (!passhash) perror_exit ("failed to compute password hash");
> +  for (ii = 0; toybuf[ii]; ii++) toybuf[ii] = 0;

Proper paranoia would probably swap those last two lines.

Hmmm, should the suid plumbing in main.c mlockall() when we don't drop  
privs?

> +  if (strcmp (passhash, shp -> sp_pwdp) != 0) deny ();
> +
> +  up = getpwnam (name);
> +  if (!up) perror_exit ("failed to getpwnam");
> +
> +  if (setuid (up -> pw_uid) < 0) perror_exit ("failed to setuid");
> +  if (chdir  (up -> pw_dir) < 0) perror_exit ("failed to chdir");

xsetuid() and xchdir() are in lib/xwrap.c.

> +  argu = xmalloc (sizeof (char *)*(toys.optc + 4));
> +  argv = argu;
> +  argv[0] = toys.optflags & FLAG_s ? TT.sArgu : up -> pw_shell;

Reading man page, "restricted shell", ala /etc/shell. Lovely. (If you  
set a shell to /dev/null the user can't log in, _and_ can't run  
anything via su either...)

I note that bash's behavior changes if argv[0] starts with "-".

> +  if (toys.optflags & FLAG_c) {
> +    argv[1] = toys.optflags & FLAG_l ? "-lc" : "-c";
> +    argv[2] = TT.cArgu;
> +    argv += 2;
> +  }
> +  else if (toys.optflags & FLAG_l) (argv++)[1] = "-l";

Is there a reason you can't do -l -c as two arguments and just test  
once?

> +  for (ii = 0; ii < toys.optc; ii++) argv[ii + 1] = toys.optargs[ii];

Probably memcpy.

> +  if (execve (argu[0], argu,
> +              toys.optflags & FLAG_l ? (char *[]){
> +                xmsprintf ( "HOME=%s", up -> pw_dir),
> +                xmsprintf ("SHELL=%s", up -> pw_shell),
> +                xmsprintf ( "USER=%s", up -> pw_name),
> +                xmsprintf ( "TERM=%s", getenv ("TERM")),
> +                0
> +              } : environ) < 0) perror_exit ("failed to exec %s",  
> argu[0]);

You never have to check the return value of execve(). I dunno why they  
bother having one. If it returns, it failed.

I'll try to take a swipe at cleaning this up and moving it out of  
pending.

Thanks,

Rob