summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorPádraig Brady <P@draigBrady.com>2014-05-16 09:50:24 +0100
committerPádraig Brady <P@draigBrady.com>2014-05-21 11:18:26 +0100
commitce0c08b52d893f6cad7ae9b7b59968406c85eeb9 (patch)
tree379c01901adbe45d9cf5fbbb11887c357ad5abec
parent99960eeab9bf7fb479ab9f5342fc12a1fae629e6 (diff)
downloadcoreutils-ce0c08b52d893f6cad7ae9b7b59968406c85eeb9.tar.xz
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.
-rw-r--r--NEWS3
-rw-r--r--doc/coreutils.texi7
-rw-r--r--src/chroot.c115
-rwxr-xr-xtests/misc/chroot-credentials.sh90
4 files changed, 176 insertions, 39 deletions
diff --git a/NEWS b/NEWS
index 93e3a0980..ced356c8e 100644
--- a/NEWS
+++ b/NEWS
@@ -85,6 +85,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 789cd68e5..592f4a647 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 look-up
+implicit in the @option{--userspec} option.
@end table
diff --git a/src/chroot.c b/src/chroot.c
index a2debac1c..6b1606055 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. */
@@ -203,8 +221,16 @@ main (int argc, char **argv)
switch (c)
{
case USERSPEC:
- userspec = optarg;
- break;
+ {
+ userspec = optarg;
+ /* Treat 'user:' just like 'user'
+ as we lookup the primary group by default
+ (and support doing so for UIDs as well as names. */
+ size_t userlen = strlen (userspec);
+ if (userlen && userspec[userlen - 1] == ':')
+ userspec[userlen - 1] = '\0';
+ break;
+ }
case GROUPS:
groups = optarg;
@@ -232,12 +258,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 by 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 +321,79 @@ 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,
+ _("no group specified for unknown uid: %d"), (int) uid);
+ }
+ }
+
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 904696d1c..d50704ccc 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 supported (unlike chown etc.) since we treat it like "uid"
+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