Re: RFC: dropping privs in chroot --user

[Pádraig Brady]

On 05/17/2014 12:48 PM, Pádraig Brady wrote:
> 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.

I also noticed another inconsistency where --user=500: was rejected
while --user=500 was not. They should both mean the same thing
with the new syntax anyway, so I've adjusted so that both area allowed.

I split the change to avoid multiple diagnostics to a separate patch
since it's a different change to the existing code.

Both attached here and I'll push later.

thanks,
Pádraig.

chroot-clear-root-groups.patch
Description: Text Data