Re: RFC: dropping privs in chroot --user

[Bernhard Voelker]

On 05/16/2014 10:59 PM, Pádraig Brady wrote:

Thanks for the detailed tests.

> [[ chroot --user=+5000 / id -G ]]
> before: 0 1 2 3 4 6 10
> after:  src/chroot: failed to get primary group

While the logic behind may be okay, our users will
probably be confused with the above change in behavior.
I propose to change the error diagnostic to give a hint
that the user must specify the group explicitly when
using a numeric UID.


> diff --git a/doc/coreutils.texi b/doc/coreutils.texi
> index 789cd68..bfaae9f 100644
> --- a/doc/coreutils.texi
> +++ b/doc/coreutils.texi
> @@ -16112,12 +16112,17 @@ By default, @var{command} is run with the same 
> credentials
>  as the invoking process.
>  Use this option to run it as a different @var{user} and/or with a
>  different primary @var{group}.
> +If a @var{user} is specified then the supplementary groups
> +are set according to the system defined list for that user,
> +unless overridden with the @option{--groups} option.
>  
>  @item --groups=@var{groups}
>  @opindex --groups
> -Use this option to specify the supplementary @var{groups} to be
> +Use this option to override the supplementary @var{groups} to be
>  used by the new process.
>  The items in the list (names or numeric IDs) must be separated by commas.
> +Use @samp{--groups=''} to disable the supplementary group lookup

Didn't we use s/lookup/look-up/ recently, see v8.22-38-ge972be3?

> diff --git a/src/chroot.c b/src/chroot.c
> index a2debac..a679658 100644
> --- a/src/chroot.c
> +++ b/src/chroot.c
> @@ -232,12 +250,36 @@ main (int argc, char **argv)
>        /* We have to look up users and groups twice.
>          - First, outside the chroot to load potentially necessary 
> passwd/group
>            parsing plugins (e.g. NSS);
> -        - Second, inside chroot to redo parsing in case IDs are different.  
> */
> +        - Second, inside chroot to redo parsing in case IDs are different.
> +          Within chroot lookup is the main justification for having
> +          the --user option supported cy the chroot command itself.  */

s/cy/by/


> -  if (gids && setgroups (n_gids, gids) != 0)
> +  if ((uid_set (uid) || groups) && setgroups (n_gids, gids) != 0)
>      {
> -      error (0, errno, _("failed to set additional groups"));
> +      error (0, errno, _("failed to %s supplemental groups"),
> +             gids ? "set" : "clear");
>        fail = true;
>      }
>  
>    free (in_gids);
>    free (out_gids);
>  
> -  if (gid != (gid_t) -1 && setgid (gid))
> +  if (gid_set (gid) && setgid (gid))
>      {
>        error (0, errno, _("failed to set group-ID"));
>        fail = true;
>      }
>  
> -  if (uid != (uid_t) -1 && setuid (uid))
> +  if (uid_set (uid) && setuid (uid))
>      {
>        error (0, errno, _("failed to set user-ID"));
>        fail = true;

Is there a reason not to exit immediately?
IMHO it looks strange (not to say worrying) to get 3 error messages:

  $ src/chroot --user=+5000:123 / id -G
  src/chroot: failed to clear supplemental groups: Operation not permitted
  src/chroot: failed to set group-ID: Operation not permitted
  src/chroot: failed to set user-ID: Operation not permitted

Otherwise: nice work, thanks!

Have a nice day,
Berny