Re: chroot's userspec option

[Pádraig Brady]

On 02/28/2014 01:27 PM, Pádraig Brady wrote:
> On 02/28/2014 10:34 AM, Ken Werner wrote:
>> On 02/27/2014 05:45 PM, Pádraig Brady wrote:
>>> On 02/27/2014 03:48 PM, Ken Werner wrote:
>>>> Hi,
>>>>
>>>> I noticed when using chroot's --userspec option the Gnulib's 
>>>> parse_user_spec function gets called that leads the glibc to dlopen 
>>>> libnss_compat.so.2 (probably getpwnam() that triggers the libc's NSS 
>>>> mechanism). Since parse_user_spec is called after the chroot system call 
>>>> the new root directory will be searched. I guess this means that the 
>>>> chroot utility attempts to parse the user spec in the "guest" environment. 
>>>> Is this behavior intended? In my case the chroot environment contains a 
>>>> libnss_compat.so.2 that's not compatible and the chroot utility fails with:
>>>>
>>>> /usr/bin/chroot: relocation error: /lib/libnss_compat.so.2: symbol 
>>>> _nss_files_parse_pwent, version GLIBC_2.0 not defined in file libc.so.6 
>>>> with link time reference
>>>>
>>>> As soon as I LD_PRELOAD libnss_compat.so.2 the "host" environment is used 
>>>> to parse the user spec. If this is the intended behavior it would be 
>>>> better if chroot calls the parse_user_spec prior issuing the chroot 
>>>> syscall. Any thoughts? :)
>>>
>>> This issue was noted previously with an explicit workaround:
>>>    http://lists.gnu.org/archive/html/coreutils/2011-07/msg00057.html
>>>
>>> Then again with an implicit workaround:
>>>    http://lists.gnu.org/archive/html/coreutils/2012-05/msg00018.html
>>>
>>> I had mentioned an amendment to that but there was no response.
>>>
>>> I'll look at a fix now to do:
>>>
>>> t_ids = parse_user_spec(); //outside chroot
>>> ids = parse_user_spec(); //inside chroot
>>> if (!ids)
>>>    ids = t_ids;
>>
>> Thank you for providing those pointers! I have to admit that it's still not 
>> clear to me whether the userspec option is supposed to lookup the user/group 
>> using the A) the old or B) the new root. In case of A) the fix would be call 
>> parse_user_spec prior switching to the new root. While B) is not trivial to 
>> support imho. The way it's implemented by now assumes the libc's NSS plugins 
>> of the new root are compatible to the libc of the old root. As you noticed 
>> that's not the case when chrooting into a 32bit userland on a 64bit system 
>> (and there are many more cases).
>>
>> Since I do not really depend on the uid/gid lookup I wondered why getpwnam() 
>> and getgrnam() are still called even if numeric IDs are provided rather than 
>> the names. It turns out the code [1] only skips the lookup if the IDs are 
>> prefixed with '+'. For example:
>>   chroot --userspec=+1234:+1234 /path/to/new/root
>>
>> Unfortunately the --groups option doesn't have a way to skip the lookup 
>> currently [2]. It calls getgrgid/getgrnam that probably trigger libc's NSS 
>> plugins as well.
>>
>> I guess the first thing would be to discuss and decide which approach is the 
>> desired one - then to post patches that changes the docs+code accordingly. 
>> /me ducks ;)
>>
>> [1] Gnulib's userspec.c:parse_user_spec calls parse_with_separator that 
>> skips the lookup in case the first char is a '+':
>> http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/userspec.c;h=1be9266eb54638a2624d0a9205d8e68fd516205e;hb=HEAD#l160
>>
>> [2] Coretutil's chroot.c:main calls set_additional_groups() that calls 
>> getgrgid/getgrnam:
>> http://git.savannah.gnu.org/gitweb/?p=coreutils.git;a=blob;f=src/chroot.c;h=50bb2537ea7df4b963e151bbcb54c217533f32d0;hb=HEAD#l66
> 
> Thanks for looking into the details.
> 
> Yes the `chroot --userspec=+1234:+1234 --groups=+123,+456 /new/root` 
> technique is a good way
> to specify that the lookup is done outside. I.E. one can do the name lookup 
> outside like:
> 
>   group_ids() { printf '%s\n' "$@" | xargs -n1 id -gr | sed 's/^/+/' | paste 
> -s -d,; }
>   chroot --userspec=+$(id -ur user):+$(id -gr user) --groups=$(group_ids 
> group1 group2) /new/root
> 
> Now the above is a bit awkward, but also not the usual case.
> I.E. one only needs to specify a +number when _enforcing_ outside lookup.
> I.E. that's only needed when there are different IDs inside and out,
> and we want to use the outside ones. We'll document this at least.
> We should also document that this is more secure in the unusual
> case where the whole chroot is untrusted, as less code is executed
> between the chroot() and the setuid().
> 
> Now to support that, --groups will need to be adjusted as you mention.
> Currently is does getgrgid() to better validate the passed group IDs,
> by providing diagnostics for a particular invalid ID in the list.
> Now that could be changed to only do the lookup if the setgroups fails.
> Also I notice that --groups assumes an ID if numeric, even without a leading 
> +.
> That's inconsistent, so we should fix that up too (we already lookup the
> name in all cases now, so adding the + constraint to avoid that will
> not be adding new restrictions).
> 
> So with the above in place we can enforce lookup outside.
> But again needing that is unusual, and we can just do name
> lookups as normal like:
> 
>  t_ids = parse_user_spec(); //outside chroot
>  ids = parse_user_spec(); //inside chroot in case different
>  if (!ids)
>     ids = t_ids;

Proposed patch is attached.

thanks,
Pádraig.

chroot-better-id-lookup.patch
Description: Text Data