Re: chroot's userspec option

[Mike Frysinger]

On Sat 01 Mar 2014 09:21:33 Ken Werner wrote:
> On 02/28/2014 02: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:
> >>>> 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;
> 
> I would have thought in case numerid IDs are specified there's no need
> to perform any outside nor inside lookup. When prefixing the IDs using
> '+' the chroot utility just uses the provided numbers for the
> setuid/setgid calls - that's fine as it is. It'd be great if the
> --groups option would also recognize this notation and the documentation
> have a note on that.

well, in POSIX, chown and such utilities require the acct to be looked up even 
when it's numeric.  it is valid to have an account named "1234" whose uid is 
"10".  so it's a bit weird that some of the coreutils tools would resolve 1234 
to 10, but not others.
-mike

signature.asc
Description: This is a digitally signed message part.