>From c51ccbd8b320da21b8d60c5fa323f55a01f72371 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A1draig=20Brady?= 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