[Libguestfs] [PATCH 1/2] Add -u and -g options to febootstrap-supermin-helper

[Richard W.M. Jones]

On Fri, Oct 01, 2010 at 04:50:32PM +0100, Matthew Booth wrote:
> 
> Bash automatically resets euid to uid when it executes. This can mean that the
> effective user id of a program at the point it calls febootstrap-supermin-helper
> can be lost if any part of execution chain involved bash. This in turn can
> result in:
> 
> * the generation of an incorrect checksum, which contains the uid.
> * the generation of supermin files with differing owners
> 
> The -u and -g options allow the caller to pass in an explicit user and group to
> run as. These will be used when generating a checksum. Additionally,
> febootstrap-supermin-helper will set(u|g)id as appropriate if they are given for
> non-checksum output. This ensures all generated files will have the correct
> ownership, regardless of how they are created.


There are a few problems.

The documentation needs to be updated (helper/febootstrap-super-helper.pod).

> -           PACKAGE_STRING, hostcpu, modpath, geteuid ());
> +           PACKAGE_STRING, hostcpu, modpath, run_uid);

I think you should just use geteuid() here, and change the code below
so that -u and -g perform the setuid/setgid unconditionally if they
are set.

The only thing this would stop you from doing is using the -u option
as non-root to generate a checksum for some other user, but I can't
see how that is helpful to anyone.

This makes the patch much smaller and simpler.

> +uid_t run_uid;
> +uid_t run_gid;

So you won't need these ...

>  enum { HELP_OPTION = CHAR_MAX + 1 };
>  
> -static const char *options = "f:k:vV";
> +static const char *options = "f:u:g:k:vV";

These should be in alnum order, same as for the current options.

> @@ -84,12 +96,70 @@ usage (const char *progname)
>            progname, progname, progname, progname, progname, progname);
>  }
>  
> +static int
> +parseint (const char *id, const char *progname)
> +{
> +  char *invalid;
> +  long int val = strtol (id, &invalid, 10);
> +  if (*invalid != '\0') {
> +    fprintf (stderr, "%s is not a valid uid\n", id);
> +    usage (progname);
> +    exit (EXIT_FAILURE);
> +  }
> +  return val;
> +}

Not sure if this is correct.  The manpage for strtol checks for a
bunch of other conditions.  Could be better to use xstrtol from gnulib.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://et.redhat.com/~rjones/virt-top