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.
>From c51ccbd8b320da21b8d60c5fa323f55a01f72371 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]> Date: Mon, 3 Mar 2014 01:54:36 +0000 Subject: [PATCH] chroot: improve --userspec and --groups lookup - Support arbitrary numbers in --groups, consistent with what is already done for --userspec - Avoid lookups entirely for --groups items with a leading '+' - Support names that are actually numbers in --groups - Lookup both inside and outside the chroot with inside taking precedence. The lookup outside may load required libraries to complete the lookup inside the chroot. This can happen for example with a 32 bit chroot on a 64 bit system, where the 32 bit NSS plugins within the chroot fail to load. * src/chroot.c (parse_additional_groups): A new function refactored from set_addition_groups(), to just do the parsing. The actual setgroups() call is separated out for calling from the chroot later. (main): Call parse_user_spec() and parse_additional_groups() both outside and inside the chroot for the reasons outlined above. * tests/misc/chroot-credentials.sh: Ensure arbitrary numeric IDs can be specified without causing lookup errors. * NEWS: Mention the improvements. --- NEWS | 4 + src/chroot.c | 118 ++++++++++++++++++++++++++------------ tests/misc/chroot-credentials.sh | 6 ++ 3 files changed, 92 insertions(+), 36 deletions(-) diff --git a/NEWS b/NEWS index d867784..f04f7fd 100644 --- a/NEWS +++ b/NEWS @@ -35,6 +35,10 @@ GNU coreutils NEWS -*- outline -*- ** Improvements + chroot has better --userspec and --group lookups, with numeric IDs never + causing name lookup errors. Also lookups are first done outside the chroot, + in case the lookup within the chroot fails due to library conflicts etc. + stat and tail work better with HFS+ and HFSX. stat -f --format=%T now reports the file system type, and tail -f now uses inotify for files, rather than the default of issuing a warning and reverting to polling. diff --git a/src/chroot.c b/src/chroot.c index 50bb253..7545f83 100644 --- a/src/chroot.c +++ b/src/chroot.c @@ -24,6 +24,7 @@ #include "system.h" #include "error.h" +#include "ignore-value.h" #include "quote.h" #include "userspec.h" #include "xstrtol.h" @@ -63,13 +64,17 @@ setgroups (size_t size _GL_UNUSED, gid_t const *list _GL_UNUSED) } #endif -/* Call setgroups to set the supplementary groups to those listed in GROUPS. - GROUPS is a comma separated list of supplementary groups (names or numbers). - Parse that list, converting any names to numbers, and call setgroups on the - resulting numbers. Upon any failure give a diagnostic and return nonzero. +/* Determine the group IDs for the specified supplementary GROUPS, + which is a comma separated list of supplementary groups (names or numbers). + Allocate an array for the parsed IDs and store it in PGIDS, + which may be allocated even on parse failure. + Update the number of parsed groups in PN_GIDS on success. + Upon any failure return nonzero, and issue diagnostic if SHOW_ERRORS is true. Otherwise return zero. */ + static int -set_additional_groups (char const *groups) +parse_additional_groups (char const *groups, GETGROUPS_T **pgids, + size_t *pn_gids, bool show_errors) { GETGROUPS_T *gids = NULL; size_t n_gids_allocated = 0; @@ -84,7 +89,18 @@ set_additional_groups (char const *groups) unsigned long int value; if (xstrtoul (tmp, NULL, 10, &value, "") == LONGINT_OK && value <= MAXGID) - g = getgrgid (value); + { + while (isspace (to_uchar (*tmp))) + tmp++; + if (*tmp != '+') + { + /* Handle the case where the name is numeric. */ + g = getgrnam (tmp); + if (g != NULL) + value = g->gr_gid; + } + g = (struct group *)! NULL; /* We've got a group from the number. */ + } else { g = getgrnam (tmp); @@ -94,9 +110,15 @@ set_additional_groups (char const *groups) if (g == NULL) { - error (0, errno, _("invalid group %s"), quote (tmp)); ret = -1; - continue; + + if (show_errors) + { + error (0, errno, _("invalid group %s"), quote (tmp)); + continue; + } + + break; } if (n_gids == n_gids_allocated) @@ -106,19 +128,17 @@ set_additional_groups (char const *groups) if (ret == 0 && n_gids == 0) { - error (0, 0, _("invalid group list %s"), quote (groups)); + if (show_errors) + error (0, 0, _("invalid group list %s"), quote (groups)); ret = -1; } + *pgids = gids; + if (ret == 0) - { - ret = setgroups (n_gids, gids); - if (ret) - error (0, errno, _("failed to set additional groups")); - } + *pn_gids = n_gids; free (buffer); - free (gids); return ret; } @@ -159,9 +179,17 @@ int main (int argc, char **argv) { int c; + + /* Input user and groups spec. */ char const *userspec = NULL; char const *groups = NULL; + /* Parsed user and group IDs. */ + uid_t uid = -1; + gid_t gid = -1; + GETGROUPS_T *out_gids = NULL; + size_t n_gids = 0; + initialize_main (&argc, &argv); set_program_name (argv[0]); setlocale (LC_ALL, ""); @@ -198,6 +226,15 @@ main (int argc, char **argv) usage (EXIT_CANCELED); } + /* We have to lookup users and groups twice. + - First, outside the chroot to load potentially necessary passwd/group + parsing plugins (e.g. NSS); + - Second, inside chroot to redo the parsing in case IDs are different. */ + if (userspec) + ignore_value (parse_user_spec (userspec, &uid, &gid, NULL, NULL)); + if (groups) + ignore_value (parse_additional_groups (groups, &out_gids, &n_gids, false)); + if (chroot (argv[optind]) != 0) error (EXIT_CANCELED, errno, _("cannot change root directory to %s"), argv[optind]); @@ -227,36 +264,45 @@ main (int argc, char **argv) Diagnose any failures. If any have failed, exit before execvp. */ if (userspec) { - uid_t uid = -1; - gid_t gid = -1; char const *err = parse_user_spec (userspec, &uid, &gid, NULL, NULL); - if (err) + if (err && uid == -1 && gid == -1) error (EXIT_CANCELED, errno, "%s", err); + } - if (groups && set_additional_groups (groups)) - fail = true; - - if (gid != (gid_t) -1 && setgid (gid)) + GETGROUPS_T *gids = out_gids; + GETGROUPS_T *in_gids = NULL; + if (groups) + { + if (parse_additional_groups (groups, &in_gids, &n_gids, !n_gids) != 0) { - error (0, errno, _("failed to set group-ID")); - fail = true; + /* If lookup outside the chroot worked, then go with those gids. */ + if (! n_gids) + fail = true; } + else + gids = in_gids; + } - if (uid != (uid_t) -1 && setuid (uid)) - { - error (0, errno, _("failed to set user-ID")); - fail = true; - } + if (gids && setgroups (n_gids, gids) != 0) + { + error (0, errno, _("failed to set additional groups")); + fail = true; } - else + + free (in_gids); + free (out_gids); + + if (gid != (gid_t) -1 && setgid (gid)) + { + error (0, errno, _("failed to set group-ID")); + fail = true; + } + + if (uid != (uid_t) -1 && setuid (uid)) { - /* Yes, this call is identical to the one above. - However, when --userspec and --groups groups are used together, - we don't want to call this function until after parsing USER:GROUP, - and it must be called before setuid. */ - if (groups && set_additional_groups (groups)) - fail = true; + error (0, errno, _("failed to set user-ID")); + fail = true; } if (fail) diff --git a/tests/misc/chroot-credentials.sh b/tests/misc/chroot-credentials.sh index 2b859d8..56f8ce7 100755 --- a/tests/misc/chroot-credentials.sh +++ b/tests/misc/chroot-credentials.sh @@ -50,4 +50,10 @@ test "$(chroot --userspec=$NON_ROOT_USERNAME / id -g)" = "$(id -g)" \ test "$(chroot --userspec=:$NON_ROOT_GROUP / id -u)" = "$(id -u)" \ || fail=1 +if grep '^#define HAVE_SETGROUPS 1' "$CONFIG_HEADER" >/dev/null; then + # verify that arbitrary numeric IDs are supported + test "$(chroot --userspec=1234:+5678 --groups=" +8765,4321" / id -G)" \ + || fail=1 +fi + Exit $fail -- 1.7.7.6
