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.

4 files changed, 176 insertions, 39 deletions

diff --git a/NEWS b/NEWS
index 93e3a0980..ced356c8e 100644
--- a/NEWS
+++ b/NEWS
@@ -85,6 +85,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 789cd68e5..592f4a647 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 a2debac1c..6b1606055 100644
--- a/src/chroot.c
+++ b/src/chroot.c
@@ -20,11 +20,13 @@
 #include <getopt.h>
 #include <stdio.h>
 #include <sys/types.h>
+#include <pwd.h>
 #include <grp.h>
 
 #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 904696d1c..d50704ccc 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