Re: two new chroot bugs

[Jim Meyering]

Jim Meyering wrote:
> Jim Meyering wrote:
> ...
>> Here are patches, for review.
>> I'll add tests tomorrow.
>
> That latter log wasn't complete.
>
>> Subject: [PATCH] chroot: don't set bogus user-ID or group-ID for --u=U or 
>> --u=:G
>>
>> * src/chroot.c (main): Initialize both "uid" and "gid".  To -1.
>> This also allows one to set the primary group-ID to 0, in case
>> it's not that already.
>
> Here's what I have, now:
>
>     chroot: don't set bogus user-ID or group-ID for --u=U: or --u=:G
>
>     * src/chroot.c (main): Initialize both "uid" and "gid".  To -1.
>     This also allows one to set the user-ID or primary group-ID to 0,
>     in case it's not that already.

I've added a test to exercise --u=U and --u=:G,
but didn't find a way to provoke setgid or setuid failure.
I was unable to exercise the --groups=G1,G2,G3 code when
not also specifying --user=... so I've fixed that, too.
Also, an empty group list was not diagnosed, and only the
first invalid group name was diagnosed.  Now all are.

>From cfe8e7a0001a10d4deceb6065134fe8c402d0368 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Thu, 28 May 2009 22:36:05 +0200
Subject: [PATCH 1/4] tests: use "nobody" as the default group name in chroot 
test

* tests/test-lib.sh (NON_ROOT_GROUP): Use "nobody", not "nogroup".
---
 tests/test-lib.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tests/test-lib.sh b/tests/test-lib.sh
index a765bd6..e5eed8d 100644
--- a/tests/test-lib.sh
+++ b/tests/test-lib.sh
@@ -204,7 +204,7 @@ require_root_()
 {
   uid_is_privileged_ || skip_test_ "must be run as root"
   NON_ROOT_USERNAME=${NON_ROOT_USERNAME=nobody}
-  NON_ROOT_GROUP=${NON_ROOT_GROUP=nogroup}
+  NON_ROOT_GROUP=${NON_ROOT_GROUP=nobody}
 }

 skip_if_root_() { uid_is_privileged_ && skip_test_ "must be run as non-root"; }
--
1.6.3.1.279.gd4bf4


>From 9e17c951e9e55c2769e99412110d9f0b2c0a835b Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Wed, 27 May 2009 22:06:04 +0200
Subject: [PATCH 2/4] chroot: set-*-ID failure must provoke nonzero exit before 
execvp

* src/chroot.c (main): Exit upon set-group-ID or set-user-ID failure.
---
 src/chroot.c |   21 ++++++++++++++++++---
 1 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/src/chroot.c b/src/chroot.c
index 788a1fc..dccddd7 100644
--- a/src/chroot.c
+++ b/src/chroot.c
@@ -207,6 +207,7 @@ main (int argc, char **argv)
       char *user;
       char *group;
       char const *err = parse_user_spec (userspec, &uid, &gid, &user, &group);
+      bool fail = false;

       if (err)
         error (EXIT_FAILURE, errno, "%s", err);
@@ -214,14 +215,28 @@ main (int argc, char **argv)
       free (user);
       free (group);

+      /* Attempt to set all three: supplementary groups, group ID, user ID.
+         Diagnose any failures.  If any have failed, exit before execvp.  */
       if (groups && set_additional_groups (groups))
-        error (0, errno, _("failed to set additional groups"));
+        {
+          error (0, errno, _("failed to set additional groups"));
+          fail = true;
+        }

       if (gid && setgid (gid))
-        error (0, errno, _("failed to set group-ID"));
+        {
+          error (0, errno, _("failed to set group-ID"));
+          fail = true;
+        }

       if (uid && setuid (uid))
-        error (0, errno, _("failed to set user-ID"));
+        {
+          error (0, errno, _("failed to set user-ID"));
+          fail = true;
+        }
+
+      if (fail)
+        exit (EXIT_FAILURE);
     }

   /* Execute the given command.  */
--
1.6.3.1.279.gd4bf4


>From 18baffc2b5c6bbe538ee92c658697c026c697786 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Wed, 27 May 2009 23:06:15 +0200
Subject: [PATCH 3/4] chroot: don't set bogus user-ID or group-ID for --u=U: or 
--u=:G

* src/chroot.c (main): Initialize both "uid" and "gid".  To -1.
This also allows one to set the user-ID or primary group-ID to 0,
in case it's not that already.
* tests/chroot/credentials: Test for the above.
---
 src/chroot.c             |    8 ++++----
 tests/chroot/credentials |    9 +++++++++
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/src/chroot.c b/src/chroot.c
index dccddd7..39b3acf 100644
--- a/src/chroot.c
+++ b/src/chroot.c
@@ -202,8 +202,8 @@ main (int argc, char **argv)

   if (userspec)
     {
-      uid_t uid;
-      gid_t gid;
+      uid_t uid = -1;
+      gid_t gid = -1;
       char *user;
       char *group;
       char const *err = parse_user_spec (userspec, &uid, &gid, &user, &group);
@@ -223,13 +223,13 @@ main (int argc, char **argv)
           fail = true;
         }

-      if (gid && setgid (gid))
+      if (gid != (gid_t) -1 && setgid (gid))
         {
           error (0, errno, _("failed to set group-ID"));
           fail = true;
         }

-      if (uid && setuid (uid))
+      if (uid != (uid_t) -1 && setuid (uid))
         {
           error (0, errno, _("failed to set user-ID"));
           fail = true;
diff --git a/tests/chroot/credentials b/tests/chroot/credentials
index 23d66bd..b76edea 100755
--- a/tests/chroot/credentials
+++ b/tests/chroot/credentials
@@ -40,4 +40,13 @@ test "$(chroot --userspec=$NON_ROOT_USERNAME:$NON_ROOT_GROUP 
/ whoami)" != root
 test "$(chroot --userspec=$NON_ROOT_USERNAME:$NON_ROOT_GROUP --groups= / id 
-nG)"\
     = $NON_ROOT_GROUP || fail=1

+# 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 when specifying only a group we get the current user ID
+test "$(chroot --userspec=:$NON_ROOT_GROUP / id -u)" = "$(id -u)" \
+    || fail=1
+
 Exit $fail
--
1.6.3.1.279.gd4bf4


>From ba0d82933a34f0a076c3503073b022d1365c8601 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Fri, 29 May 2009 08:44:56 +0200
Subject: [PATCH 4/4] chroot: make --groups= work without --userspec=;  be more 
robust

* src/chroot.c (set_additional_groups): Add comments.
Given an empty or all-comma group list, diagnose it and return nonzero.
When more than one group is invalid, diagnose all of them,
not just the first.
(main): Honor --groups= also when --userspec= is not specified.
Now that set_additional_groups consistently diagnoses its failures,
don't diagnose it separately here.
* tests/chroot/credentials: Do not invoke with an empty group list.
---
 src/chroot.c             |   61 +++++++++++++++++++++++++++++----------------
 tests/chroot/credentials |    2 +-
 2 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/src/chroot.c b/src/chroot.c
index 39b3acf..12d282b 100644
--- a/src/chroot.c
+++ b/src/chroot.c
@@ -54,7 +54,11 @@ static struct option const long_opts[] =
   {NULL, 0, NULL, 0}
 };

-/* Groups is a comma separated list of additional groups.  */
+/* 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.
+   Otherwise return zero.  */
 static int
 set_additional_groups (char const *groups)
 {
@@ -63,7 +67,7 @@ set_additional_groups (char const *groups)
   size_t n_gids = 0;
   char *buffer = xstrdup (groups);
   char const *tmp;
-  int ret;
+  int ret = 0;

   for (tmp = strtok (buffer, ","); tmp; tmp = strtok (NULL, ","))
     {
@@ -71,9 +75,7 @@ set_additional_groups (char const *groups)
       unsigned long int value;

       if (xstrtoul (tmp, NULL, 10, &value, "") == LONGINT_OK && value <= 
MAXGID)
-        {
-          g = getgrgid (value);
-        }
+        g = getgrgid (value);
       else
         {
           g = getgrnam (tmp);
@@ -83,9 +85,9 @@ set_additional_groups (char const *groups)

       if (g == NULL)
         {
-          error (0, errno, _("cannot find group %s"), tmp);
-          free (buffer);
-          return 1;
+          error (0, errno, _("invalid group %s"), quote (tmp));
+          ret = -1;
+          continue;
         }

       if (n_gids == n_gids_allocated)
@@ -93,16 +95,24 @@ set_additional_groups (char const *groups)
       gids[n_gids++] = value;
     }

-  free (buffer);
+  if (ret == 0 && n_gids == 0)
+    {
+      error (0, 0, _("invalid group list %s"), quote (groups));
+      ret = -1;
+    }

-  ret = setgroups (n_gids, gids);
+  if (ret == 0)
+    {
+      ret = setgroups (n_gids, gids);
+      if (ret)
+        error (0, errno, _("failed to set additional groups"));
+    }

+  free (buffer);
   free (gids);
-
   return ret;
 }

-
 void
 usage (int status)
 {
@@ -200,6 +210,10 @@ main (int argc, char **argv)
       argv += optind + 1;
     }

+  bool fail = false;
+
+  /* Attempt to set all three: supplementary groups, group ID, user ID.
+     Diagnose any failures.  If any have failed, exit before execvp.  */
   if (userspec)
     {
       uid_t uid = -1;
@@ -207,7 +221,6 @@ main (int argc, char **argv)
       char *user;
       char *group;
       char const *err = parse_user_spec (userspec, &uid, &gid, &user, &group);
-      bool fail = false;

       if (err)
         error (EXIT_FAILURE, errno, "%s", err);
@@ -215,13 +228,8 @@ main (int argc, char **argv)
       free (user);
       free (group);

-      /* Attempt to set all three: supplementary groups, group ID, user ID.
-         Diagnose any failures.  If any have failed, exit before execvp.  */
       if (groups && set_additional_groups (groups))
-        {
-          error (0, errno, _("failed to set additional groups"));
-          fail = true;
-        }
+        fail = true;

       if (gid != (gid_t) -1 && setgid (gid))
         {
@@ -234,10 +242,19 @@ main (int argc, char **argv)
           error (0, errno, _("failed to set user-ID"));
           fail = true;
         }
-
-      if (fail)
-        exit (EXIT_FAILURE);
     }
+  else
+    {
+      /* 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;
+    }
+
+  if (fail)
+    exit (EXIT_FAILURE);

   /* Execute the given command.  */
   execvp (argv[0], argv);
diff --git a/tests/chroot/credentials b/tests/chroot/credentials
index b76edea..58c098f 100755
--- a/tests/chroot/credentials
+++ b/tests/chroot/credentials
@@ -37,7 +37,7 @@ test "$(chroot --userspec=$NON_ROOT_USERNAME:$NON_ROOT_GROUP 
/ whoami)" != root
     || fail=1

 # Verify that there are no additional groups.
-test "$(chroot --userspec=$NON_ROOT_USERNAME:$NON_ROOT_GROUP --groups= / id 
-nG)"\
+test "$(chroot --userspec=$NON_ROOT_USERNAME:$NON_ROOT_GROUP 
--groups=$NON_ROOT_GROUP / id -nG)"\
     = $NON_ROOT_GROUP || fail=1

 # Verify that when specifying only the user name we get the current
--
1.6.3.1.279.gd4bf4