On 05/13/2014 04:04 PM, Pádraig Brady wrote:
> Both setuidgid and runuser behave as I would expect
> and drop the supplemental groups of the root user:
>
> # runuser padraig -c "id -G"
> 500 10 489 491
>
> # ~padraig/git/coreutils/src/setuidgid padraig id -G
> 500 10 489 491
>
> However chroot does not:
>
> # chroot --user=padraig: / id -G
> 500 0 1 2 3 4 6 10
>
> # chroot --user=padraig / id -G
> 0 500 1 2 3 4 6 10
>
> That's at least unexpected and could
> be considered a bug I think.
> If I'm missing nothing I'll send a patch soon.
So with chroot you needed to explicitly set the supplemental groups
with --groups, which was awkward, unexpected and not general
since there was no way to lookup the supplemental groups for
a user _within_ the chroot.
The attached patch changes things as follows.
thanks,
Pádraig.
[[ chroot / id -G ]]
before: 0 1 2 3 4 6 10
after: 0 1 2 3 4 6 10
[[ chroot --user=padraig / id -G ]]
before: 0 500 1 2 3 4 6 10
after: 500 10 489 491
[[ chroot --user=padraig: / id -G ]]
before: 500 0 1 2 3 4 6 10
after: 500 10 489 491
[[ chroot --user=padraig:nobody / id -G ]]
before: 99 500 0 1 2 3 4 6 10
after: 99 500 10 489 491
[[ chroot --user=padraig --groups='' / id -G ]]
before: chroot: invalid group list `'
after: 500
[[ chroot --user=padraig:nobody --groups='' / id -G ]]
before: chroot: invalid group list `'
after: 99 500
[[ chroot --user=+500 / id -G ]]
before: 0 500 1 2 3 4 6 10
after: 500 10 489 491
[[ chroot --user=+500:+500 / id -G ]]
before: 500 0 1 2 3 4 6 10
after: 500 10 489 491
[[ chroot --user=+500:nobody / id -G ]]
before: 99 500 0 1 2 3 4 6 10
after: 99 500 10 489 491
[[ chroot --user=+500:nobody --groups='' / id -G ]]
before: chroot: invalid group list `'
after: 99 500
[[ chroot --user=+500 --groups='' / id -G ]]
before: chroot: invalid group list `'
after: 500
[[ chroot --user=+500:+500 --groups='' / id -G ]]
before: chroot: invalid group list `'
after: 500
[[ chroot --user=+5000 / id -G ]]
before: 0 1 2 3 4 6 10
after: src/chroot: failed to get primary group
[[ chroot --user=+5000:+5000 / id -G ]]
before: 5000 0 1 2 3 4 6 10
after: 5000
[[ chroot --user=+5000:+5000 --groups='' / id -G ]]
before: chroot: invalid group list `'
after: 5000
[[ chroot --user=:padraig / id -G ]]
before: 500 0 1 2 3 4 6 10
after: 500 0 1 2 3 4 6 10
[[ chroot --user=:padraig / id -u ]]
before: 0
after: 0
>From e854853faadb51486ce868489a6db959f99b7577 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Fri, 16 May 2014 09:50:24 +0100
Subject: [PATCH] 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.
---
NEWS | 3 +
doc/coreutils.texi | 7 ++-
src/chroot.c | 100 +++++++++++++++++++++++++++++++++----
tests/misc/chroot-credentials.sh | 90 ++++++++++++++++++++++++---------
4 files changed, 163 insertions(+), 37 deletions(-)
diff --git a/NEWS b/NEWS
index 35d0bda..db427ac 100644
--- a/NEWS
+++ b/NEWS
@@ -83,6 +83,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 789cd68..bfaae9f 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 lookup
+implicit in the @option{--userspec} option.
@end table
diff --git a/src/chroot.c b/src/chroot.c
index a2debac..a679658 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. */
@@ -232,12 +250,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 cy 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 +313,76 @@ 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, _("failed to get primary group"));
+ }
+
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 904696d..18b6075 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 an invalid user spec as it is for chown etc.
+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
--
1.7.7.6
