>From ff4266f2630d083af4d74c9e77babaee3f382f51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A1draig=20Brady?= Date: Fri, 16 May 2014 09:50:24 +0100 Subject: [PATCH] chroot: with --userspec clear root's supplemental groups It's dangerous and confusing to leave root's supplemental groups in place when specifying other users with --userspec. In the edge case that that is desired one can explicitly specify --groups. Also we implicitly set the system defined supplemental groups for a user. The existing mechanism where supplemental groups needed to be explicitly specified is confusing and not general when the lookup needs to be done within the chroot. Also we extend the --groups syntax slightly to allow clearing the set of supplementary groups using --groups=''. * src/chroot.c (setgroups): On systems without supplemental groups, clearing then is a noop and so should return success. (main): Lookup the primary GID with getpwuid() when just a numeric uid is specified, and also infer the USERNAME from this call, needed when we're later looking up the supplemental groups for a user. Support clearing supplemental groups, either implicitly for unknown users, or explicitly when --groups='' is specified. * tests/misc/chroot-credentials.sh: Various new test cases * doc/coreutils.texi (chroot invocation): Adjust for the new behavior. * NEWS: Mention the change in behavior. --- NEWS | 3 + doc/coreutils.texi | 7 ++- src/chroot.c | 115 +++++++++++++++++++++++++++++++++---- tests/misc/chroot-credentials.sh | 90 +++++++++++++++++++++-------- 4 files changed, 176 insertions(+), 39 deletions(-) diff --git a/NEWS b/NEWS index 35d0bda..db427ac 100644 --- a/NEWS +++ b/NEWS @@ -83,6 +83,9 @@ GNU coreutils NEWS -*- outline -*- chroot with an argument of "/" no longer implicitly changes the current directory to "/", allowing changing only user credentials for a command. + chroot --userspec will now unset supplemental groups associated with root, + and instead use the supplemental groups of the specified user. + ls with none of LS_COLORS or COLORTERM environment variables set, will now honor an empty or unknown TERM environment variable, and not output colors even with --colors=always. diff --git a/doc/coreutils.texi b/doc/coreutils.texi index 789cd68..592f4a6 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 look-up +implicit in the @option{--userspec} option. @end table diff --git a/src/chroot.c b/src/chroot.c index a2debac..6b16060 100644 --- a/src/chroot.c +++ b/src/chroot.c @@ -20,11 +20,13 @@ #include #include #include +#include #include #include "system.h" #include "error.h" #include "ignore-value.h" +#include "mgetgroups.h" #include "quote.h" #include "userspec.h" #include "xstrtol.h" @@ -38,6 +40,11 @@ # define MAXGID GID_T_MAX #endif +static inline bool uid_unset (uid_t uid) { return uid == (uid_t) -1; } +static inline bool gid_unset (gid_t gid) { return gid == (gid_t) -1; } +#define uid_set(x) (!uid_unset (x)) +#define gid_set(x) (!gid_unset (x)) + enum { GROUPS = UCHAR_MAX + 1, @@ -56,10 +63,20 @@ static struct option const long_opts[] = #if ! HAVE_SETGROUPS /* At least Interix lacks supplemental group support. */ static int -setgroups (size_t size _GL_UNUSED, gid_t const *list _GL_UNUSED) +setgroups (size_t size, gid_t const *list _GL_UNUSED) { - errno = ENOTSUP; - return -1; + if (size == 0) + { + /* Return success when clearing supplemental groups + as ! HAVE_SETGROUPS should only be the case on + platforms that don't support supplemental groups. */ + return 0; + } + else + { + errno = ENOTSUP; + return -1; + } } #endif @@ -180,7 +197,8 @@ main (int argc, char **argv) int c; /* Input user and groups spec. */ - char const *userspec = NULL; + char *userspec = NULL; + char const *username = NULL; char const *groups = NULL; /* Parsed user and group IDs. */ @@ -203,8 +221,16 @@ main (int argc, char **argv) switch (c) { case USERSPEC: - userspec = optarg; - break; + { + userspec = optarg; + /* Treat 'user:' just like 'user' + as we lookup the primary group by default + (and support doing so for UIDs as well as names. */ + size_t userlen = strlen (userspec); + if (userlen && userspec[userlen - 1] == ':') + userspec[userlen - 1] = '\0'; + break; + } case GROUPS: groups = optarg; @@ -232,12 +258,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 by the chroot command itself. */ if (userspec) ignore_value (parse_user_spec (userspec, &uid, &gid, NULL, NULL)); + + /* If no gid is supplied or looked up, do so now. + Also lookup the username for use with getgroups. */ + if (uid_set (uid) && (! groups || gid_unset (gid))) + { + const struct passwd *pwd; + if ((pwd = getpwuid (uid))) + { + if (gid_unset (gid)) + gid = pwd->pw_gid; + username = pwd->pw_name; + } + } + if (groups && *groups) ignore_value (parse_additional_groups (groups, &out_gids, &n_gids, false)); +#if HAVE_SETGROUPS + else if (! groups && gid_set (gid) && username) + { + int ngroups = xgetgroups (username, gid, &out_gids); + if (0 < ngroups) + n_gids = ngroups; + } +#endif if (chroot (argv[optind]) != 0) error (EXIT_CANCELED, errno, _("cannot change root directory to %s"), @@ -271,40 +321,79 @@ main (int argc, char **argv) { char const *err = parse_user_spec (userspec, &uid, &gid, NULL, NULL); - if (err && uid == -1 && gid == -1) + if (err && uid_unset (uid) && gid_unset (gid)) error (EXIT_CANCELED, errno, "%s", err); } + /* If no gid is supplied or looked up, do so now. + Also lookup the username for use with getgroups. */ + if (uid_set (uid) && (! groups || gid_unset (gid))) + { + const struct passwd *pwd; + if ((pwd = getpwuid (uid))) + { + if (gid_unset (gid)) + gid = pwd->pw_gid; + username = pwd->pw_name; + } + else if (gid_unset (gid)) + { + error (EXIT_CANCELED, errno, + _("no group specified for unknown uid: %d"), (int) uid); + } + } + GETGROUPS_T *gids = out_gids; GETGROUPS_T *in_gids = NULL; if (groups && *groups) { if (parse_additional_groups (groups, &in_gids, &n_gids, !n_gids) != 0) { - /* If look-up outside the chroot worked, then go with those gids. */ if (! n_gids) fail = true; + /* else look-up outside the chroot worked, then go with those. */ } else gids = in_gids; } +#if HAVE_SETGROUPS + else if (! groups && gid_set (gid) && username) + { + int ngroups = xgetgroups (username, gid, &in_gids); + if (ngroups <= 0) + { + if (! n_gids) + { + fail = true; + error (0, errno, _("failed to get supplemental groups")); + } + /* else look-up outside the chroot worked, then go with those. */ + } + else + { + n_gids = ngroups; + gids = in_gids; + } + } +#endif - 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; diff --git a/tests/misc/chroot-credentials.sh b/tests/misc/chroot-credentials.sh index 904696d..d50704c 100755 --- a/tests/misc/chroot-credentials.sh +++ b/tests/misc/chroot-credentials.sh @@ -27,6 +27,18 @@ grep '^#define HAVE_SETGROUPS 1' "$CONFIG_HEADER" >/dev/null \ root=$(id -nu 0) || skip_ "Couldn't look up root username" +# verify numeric IDs looked up similarly to names +NON_ROOT_UID=$(id -u $NON_ROOT_USERNAME) +NON_ROOT_GID=$(id -g $NON_ROOT_USERNAME) + +# "uid:" is supported (unlike chown etc.) since we treat it like "uid" +chroot --userspec=$NON_ROOT_UID: / true || fail=1 + +# verify that invalid groups are diagnosed +for g in ' ' ',' '0trail'; do + test "$(chroot --groups="$g" / id -G)" && fail=1 +done + # Verify that root credentials are kept. test $(chroot / whoami) = "$root" || fail=1 test "$(groups)" = "$(chroot / groups)" || fail=1 @@ -37,41 +49,69 @@ whoami_after_chroot=$( ) test "$whoami_after_chroot" != "$root" || fail=1 -if test "$HAVE_SETGROUPS"; then - # Verify that there are no additional groups. - id_G_after_chroot=$( - chroot --userspec=$NON_ROOT_USERNAME:$NON_ROOT_GROUP \ - --groups=$NON_ROOT_GROUP / id -G - ) - test "$id_G_after_chroot" = $NON_ROOT_GROUP || fail=1 +# Verify that when specifying only a group we don't change the +# list of supplemental groups +test "$(chroot --userspec=:$NON_ROOT_GROUP / id -G)" = \ + "$NON_ROOT_GID $(id -G)" || fail=1 + +if ! test "$HAVE_SETGROUPS"; then + Exit $fail fi -# Verify that when specifying only the user name we get the current -# primary group ID. -test "$(chroot --userspec=$NON_ROOT_USERNAME / id -g)" = "$(id -g)" \ - || fail=1 + +# Verify that there are no additional groups. +id_G_after_chroot=$( + chroot --userspec=$NON_ROOT_USERNAME:$NON_ROOT_GROUP \ + --groups=$NON_ROOT_GROUP / id -G +) +test "$id_G_after_chroot" = $NON_ROOT_GROUP || fail=1 + +# Verify that when specifying only the user name we get all their groups +test "$(chroot --userspec=$NON_ROOT_USERNAME / id -G)" = \ + "$(id -G $NON_ROOT_USERNAME)" || fail=1 + +# Ditto with trailing : on the user name. +test "$(chroot --userspec=$NON_ROOT_USERNAME: / id -G)" = \ + "$(id -G $NON_ROOT_USERNAME)" || fail=1 + +# Verify that when specifying only the user and clearing supplemental groups +# that we only get the primary group +test "$(chroot --userspec=$NON_ROOT_USERNAME --groups='' / id -G)" = \ + "$(id -g $NON_ROOT_USERNAME)" || fail=1 + +# Verify that when specifying only the UID we get all their groups +test "$(chroot --userspec=$NON_ROOT_UID / id -G)" = \ + "$(id -G $NON_ROOT_USERNAME)" || fail=1 + +# Verify that when specifying only the user and clearing supplemental groups +# that we only get the primary group. Note this variant with prepended '+' +# results in no lookups in the name database which could be useful depending +# on your chroot setup. +test "$(chroot --userspec=+$NON_ROOT_UID:+$NON_ROOT_GID --groups='' / id -G)" =\ + "$(id -g $NON_ROOT_USERNAME)" || fail=1 # Verify that when specifying only a group we get the current user ID test "$(chroot --userspec=:$NON_ROOT_GROUP / id -u)" = "$(id -u)" \ || fail=1 -# verify that invalid groups are diagnosed -for g in ' ' ',' '0trail'; do - test "$(chroot --groups="$g" / id -G)" && fail=1 -done +# verify that arbitrary numeric IDs are supported +test "$(chroot --userspec=1234:+5678 --groups=' +8765,4321' / id -G)" \ + || fail=1 -if test "$HAVE_SETGROUPS"; then - # verify that arbitrary numeric IDs are supported - test "$(chroot --userspec=1234:+5678 --groups=' +8765,4321' / id -G)" \ - || fail=1 +# demonstrate that extraneous commas are supported +test "$(chroot --userspec=1234:+5678 --groups=',8765,,4321,' / id -G)" \ + || fail=1 + +# demonstrate that --groups is not cumulative +test "$(chroot --groups='invalid ignored' --groups='' / id -G)" \ + || fail=1 - # demonstrate that extraneous commas are supported - test "$(chroot --userspec=1234:+5678 --groups=',8765,,4321,' / id -G)" \ - || fail=1 +if ! id -u +12342; then + # Ensure supplemental groups cleared from some arbitrary unknown ID + test "$(chroot --userspec=+12342:+5678 / id -G)" = '5678' || fail=1 - # demonstrate that --groups is not cumlative - test "$(chroot --groups='invalid ignored' --groups='' / id -G)" \ - || fail=1 + # Ensure we fail when we don't know what groups to set for an unknown ID + chroot --userspec=+12342 / true && fail=1 fi Exit $fail -- 1.7.7.6