chroot: improve --userspec and --groups look-up

- Support arbitrary numbers in --groups, consistent with
  what is already done for --userspec
- Avoid look-ups entirely for --groups items with a leading '+'
- Support names that are actually numbers in --groups
- Ignore an empty --groups="" option for consistency with --userspec
- Look up both inside and outside the chroot with inside taking
  precedence.  The look-up outside may load required libraries
  to complete the look-up 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 look-up errors.
* NEWS: Mention the improvements.
* THANKS.in: Add Norihiro Kamae who initially reported the issue
with a proposed patch.
Also thanks to Dmitry V. Levin for his diagnosis and sample patch.

5 files changed, 138 insertions, 56 deletions

diff --git a/NEWS b/NEWS
index d86778473..62966b2fa 100644
--- a/NEWS
+++ b/NEWS
@@ -35,6 +35,10 @@ GNU coreutils NEWS                                    -*- outline -*-
 
 ** Improvements
 
+  chroot has better --userspec and --group look-ups, with numeric IDs never
+  causing name look-up errors.  Also look-ups are first done outside the chroot,
+  in case the look-up 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/THANKS.in b/THANKS.in
index fb7d6e084..561d18ce2 100644
--- a/THANKS.in
+++ b/THANKS.in
@@ -469,6 +469,7 @@ Nima Nikzad                         nnikzad@ucla.edu
 Noah Friedman                       friedman@splode.com
 Noel Cragg                          noel@red-bean.com
 Norbert Kiesel                      nkiesel@tbdnetworks.com
+Norihiro Kamae                      norihiro@nagater.net
 Olatunji Oluwabukunmi Ruwase        tjruwase@stanford.edu
 Olav Morkrid                        olav@funcom.com
 Ole Laursen                         olau@hardworking.dk
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 7ba8cd4d9..e5e27eb88 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -221,7 +221,7 @@ Common Options
 * Block size::                   Block size
 * Floating point::               Floating point number representation
 * Signal specifications::        Specifying signals
-* Disambiguating names and IDs:: chgrp and chown owner and group syntax
+* Disambiguating names and IDs:: chgrp, chown, chroot, id: user and group syntax
 * Random sources::               Sources of random data
 * Target directory::             Target directory
 * Trailing slashes::             Trailing slashes
@@ -736,7 +736,7 @@ name.
 * Block size::                  BLOCK_SIZE and --block-size, in some programs.
 * Floating point::              Floating point number representation.
 * Signal specifications::       Specifying signals using the --signal option.
-* Disambiguating names and IDs:: chgrp and chown owner and group syntax
+* Disambiguating names and IDs:: chgrp, chown, chroot, id: user and group syntax
 * Random sources::              --random-source, in some programs.
 * Target directory::            Specifying a target directory, in some programs.
 * Trailing slashes::            --strip-trailing-slashes, in some programs.
@@ -1135,20 +1135,20 @@ also support at least eight real-time signals called @samp{RTMIN},
 @samp{RTMIN+1}, @dots{}, @samp{RTMAX-1}, @samp{RTMAX}.
 
 @node Disambiguating names and IDs
-@section chown and chgrp: Disambiguating user names and IDs
+@section chown, chgrp, chroot, id: Disambiguating user names and IDs
 @cindex user names, disambiguating
 @cindex user IDs, disambiguating
 @cindex group names, disambiguating
 @cindex group IDs, disambiguating
 @cindex disambiguating group names and IDs
 
-Since the @var{owner} and @var{group} arguments to @command{chown} and
-@command{chgrp} may be specified as names or numeric IDs, there is an
+Since the @var{user} and @var{group} arguments to these commands
+may be specified as names or numeric IDs, there is an
 apparent ambiguity.
 What if a user or group @emph{name} is a string of digits?
 @footnote{Using a number as a user name is common in some environments.}
 Should the command interpret it as a user name or as an ID@?
-POSIX requires that @command{chown} and @command{chgrp}
+POSIX requires that these commands
 first attempt to resolve the specified string as a name, and
 only once that fails, then try to interpret it as an ID@.
 This is troublesome when you want to specify a numeric ID, say 42,
@@ -1157,9 +1157,9 @@ and it must work even in a pathological situation where
 Simply invoking @code{chown 42 F}, will set @file{F}s owner ID to
 1000---not what you intended.
 
-GNU @command{chown} and @command{chgrp} provide a way to work around this,
-that at the same time may result in a significant performance improvement
-by eliminating a database look-up.
+GNU @command{chown}, @command{chgrp}, @command{chroot}, and @command{id}
+provide a way to work around this, that at the same time may result in a
+significant performance improvement by eliminating a database look-up.
 Simply precede each numeric user ID and/or group ID with a @samp{+},
 in order to force its interpretation as an integer:
 
@@ -1169,8 +1169,7 @@ chgrp +$numeric_group_id another-file
 chown +0:+0 /
 @end example
 
-GNU @command{chown} and @command{chgrp}
-skip the name look-up process for each @samp{+}-prefixed string,
+The name look-up process is skipped for each @samp{+}-prefixed string,
 because a string containing @samp{+} is never a valid user or group name.
 This syntax is accepted on most common Unix systems, but not on Solaris 10.
 
@@ -14538,8 +14537,9 @@ running it if no user is specified.  Synopsis:
 id [@var{option}]@dots{} [@var{user}]
 @end example
 
-@var{user} can be either a user ID or a name, with name lookup
+@var{user} can be either a user ID or a name, with name look-up
 taking precedence unless the ID is specified with a leading @samp{+}.
+@xref{Disambiguating names and IDs}.
 
 @vindex POSIXLY_CORRECT
 By default, it prints the real user ID, real group ID, effective user ID
@@ -16109,6 +16109,13 @@ The items in the list (names or numeric IDs) must be separated by commas.
 
 @end table
 
+The user and group name look-up performed by the @option{--userspec}
+and @option{--groups} options, is done both outside and inside
+the chroot, with successful look-ups inside the chroot taking precedence.
+If the specified user or group items are intended to represent a numeric ID,
+then a name to ID resolving step is avoided by specifying a leading @samp{+}.
+@xref{Disambiguating names and IDs}.
+
 Here are a few tips to help avoid common problems in using chroot.
 To start with a simple example, make @var{command} refer to a statically
 linked binary.  If you were to use a dynamically linked executable, then
diff --git a/src/chroot.c b/src/chroot.c
index 50bb2537e..36912a5d3 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 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 the parsing in case IDs are different.  */
+  if (userspec)
+    ignore_value (parse_user_spec (userspec, &uid, &gid, NULL, NULL));
+  if (groups && *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 && *groups)
+    {
+      if (parse_additional_groups (groups, &in_gids, &n_gids, !n_gids) != 0)
         {
-          error (0, errno, _("failed to set group-ID"));
-          fail = true;
+          /* If look-up 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 2b859d8ac..904696d1c 100755
--- a/tests/misc/chroot-credentials.sh
+++ b/tests/misc/chroot-credentials.sh
@@ -22,7 +22,10 @@ print_ver_ chroot
 
 require_root_
 
-root=$(id -nu 0) || skip_ "Couldn't lookup root username"
+grep '^#define HAVE_SETGROUPS 1' "$CONFIG_HEADER" >/dev/null \
+  && HAVE_SETGROUPS=1
+
+root=$(id -nu 0) || skip_ "Couldn't look up root username"
 
 # Verify that root credentials are kept.
 test $(chroot / whoami) = "$root" || fail=1
@@ -34,20 +37,41 @@ whoami_after_chroot=$(
 )
 test "$whoami_after_chroot" != "$root" || 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
+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
+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
+  || 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
+
+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 cumlative
+  test "$(chroot --groups='invalid ignored' --groups='' / id -G)" \
     || fail=1
+fi
 
 Exit $fail