Re: RFC: dropping privs in chroot --user

[Pádraig Brady]

On 05/17/2014 01:45 AM, Bernhard Voelker wrote:
> 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

That was there already, and I was wondering why myself.
I'll adjust to exit ASAP lest the user think previous
failing actions were optional.

Also I agree with your other points and will adjust accordingly.

Excellent review.

thanks,
Pádraig.