two new chroot bugs

[Jim Meyering]

Eric Blake wrote:
> According to Jim Meyering on 5/26/2009 7:21 AM:
>> Merged and pushed.
>> Thanks again.
>
> Would it be worth starting to patch the testsuite to replace 'setuidgid -g
> list usr cmd arg' with 'chroot --user usr --groups=list / cmd arg' in
> order to give this feature more exposure and reduce our dependence on
> uninstalled apps?

Yes.  It would be good to get more coverage for installed tools.

However, I've just noticed that the new code in chroot.c needs work.

Two problems:
  - set*ID failure did not evoke non-zero exit
  - "chroot --u=N / id -a" currently uses "gid" uninitialized
    i.e., when --userspec=USER (without ":GROUP")
    Same for --u=:1 (i.e, group ID with no user-ID)

    $ sudo ./chroot --u=0 / id -g
    3496015512

Here are patches, for review.
I'll add tests tomorrow.

>From 63a1039e21fbf127e052f1b4d80176e3a2386d2a Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Wed, 27 May 2009 22:06:04 +0200
Subject: [PATCH] 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..ab35944 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);
+      unsigned int fail = 0;

       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;
+        }

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

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

   /* Execute the given command.  */
--
1.6.3.1.268.g94d6d1

>From 6752c02be6bc760e92687e5ae71e93513f5b84da Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Wed, 27 May 2009 23:06:15 +0200
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.
---
 src/chroot.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/chroot.c b/src/chroot.c
index ab35944..8bc4e59 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;
         }

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

-      if (uid && setuid (uid))
+      if (uid != (uid_t) -1 && setuid (uid))
         {
           error (0, errno, _("failed to set user-ID"));
           ++fail;
--
1.6.3.1.268.g94d6d1