Re: [PATCH] chroot specify user/group feature

[Jim Meyering]

Giuseppe Scrivano wrote:
> Thank you Jim, Pádraig and Andreas for all your suggestions.  I took all
> of them in consideration.  I temporary lent some code from setuidgid.c
> and the additional groups are allocated dinamically.
>
> I think that in the future the `set_additional_groups' function should
> be moved in a separate library, so it can be shared with setuidgid that
> at the moment doesn't support group names but only group IDs and this is
> what setuidgid will get back.

Thank you.

a few nits.
Some were exposed by running this:

  ./configure --enable-gcc-warnings && make && make syntax-check

I rewrote parts of the .texinfo documentation and removed a paragraph,
since now that the new options are documented, it seemed redundant.
I'll merge these 4 change sets into yours and push late today or
tomorrow, to give people time for any additional feedback.

>From fe77420a0ed029100f413173be79c678cd346064 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 25 May 2009 14:55:39 +0200
Subject: [PATCH 1/4] sort things, cpp-indent

---
 src/chroot.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/chroot.c b/src/chroot.c
index 3e0b30d..7ca9dc4 100644
--- a/src/chroot.c
+++ b/src/chroot.c
@@ -37,19 +37,19 @@
 #define AUTHORS proper_name ("Roland McGrath")

 #ifndef MAXGID
-#define MAXGID GID_T_MAX
+# define MAXGID GID_T_MAX
 #endif

 enum
 {
-  USERSPEC = UCHAR_MAX + 1,
-  GROUPS
+  GROUPS = UCHAR_MAX + 1,
+  USERSPEC
 };

 static struct option const long_opts[] =
 {
-  {"userspec", required_argument, NULL, USERSPEC},
   {"groups", required_argument, NULL, GROUPS},
+  {"userspec", required_argument, NULL, USERSPEC},
   {GETOPT_HELP_OPTION_DECL},
   {GETOPT_VERSION_OPTION_DECL},
   {NULL, 0, NULL, 0}
--
1.6.3.1.241.g24356


>From 5a0ca8823504e1eea922ab83a284f63c31c900a6 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 25 May 2009 15:03:36 +0200
Subject: [PATCH 2/4] don't include xalloc.h: system.h already does that

---
 src/chroot.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/src/chroot.c b/src/chroot.c
index 7ca9dc4..b801220 100644
--- a/src/chroot.c
+++ b/src/chroot.c
@@ -29,7 +29,6 @@
 #include "quote.h"
 #include "userspec.h"
 #include "xstrtol.h"
-#include "xalloc.h"

 /* The official name of this program (e.g., no `g' prefix).  */
 #define PROGRAM_NAME "chroot"
--
1.6.3.1.241.g24356


>From d4e8934ce07683d09ae42dae27868c3681f5986e Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 25 May 2009 15:15:49 +0200
Subject: [PATCH 3/4] Avoid new warning, fix diagnostic, remove curly braces

  cc1: warnings being treated as errors
  chroot.c: In function 'main':
  chroot.c:213: error: format not a string literal and \
    no format arguments [-Wformat-security]

correct copy-paste-o:
  failed setuid would diagnose setgid failure

Remove curly braces around single-stmt blocks.
---
 src/chroot.c |   17 +++++------------
 1 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/src/chroot.c b/src/chroot.c
index b801220..788a1fc 100644
--- a/src/chroot.c
+++ b/src/chroot.c
@@ -209,26 +209,19 @@ main (int argc, char **argv)
       char const *err = parse_user_spec (userspec, &uid, &gid, &user, &group);

       if (err)
-        error (EXIT_FAILURE, errno, err);
+        error (EXIT_FAILURE, errno, "%s", err);

       free (user);
       free (group);

-      if (groups)
-        {
-          if (set_additional_groups (groups))
-            error (0, errno, _("cannot set additional groups"));
-        }
+      if (groups && set_additional_groups (groups))
+        error (0, errno, _("failed to set additional groups"));

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

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

   /* Execute the given command.  */
--
1.6.3.1.241.g24356


>From 6de3363e51770876065f6e70abac847b4bb3bcc6 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 25 May 2009 15:46:36 +0200
Subject: [PATCH 4/4] tweak documentation

---
 doc/coreutils.texi |   22 ++++++++--------------
 1 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 3371ba5..97ea830 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -14162,27 +14162,21 @@ chroot invocation

 @table @samp

address@hidden address@hidden
address@hidden address@hidden:@var{group}]
 @opindex --userspec
-Use the @var{userspec} user and group in the new environment.
address@hidden is given in the form @var{user}:@var{group}, the group
-can be omitted and in this case the original one will be used.
+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}.

 @itemx address@hidden
 @opindex --groups
-Specify an additional set of groups @var{groups} to be used by the
-chroot process.
+Use this option to specify 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.

 @end table

-By default, @command{chroot} retains the user credentials---usually those
-of the super-user---in the new environment.
-You can modify this behavior via the @option{--userspec=USER:GROUP} option,
-to specify the desired user and/or primary group.
-Supplementary groups can be configured using the @option{--groups=group_list}
-option.  If either @option{--userspec} or @option{--groups} is omitted,
-then the original values are kept.
-
 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
--
1.6.3.1.241.g24356