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 <meyer...@redhat.com> 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 <meyer...@redhat.com> 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 <meyer...@redhat.com> 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 <meyer...@redhat.com> 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 _______________________________________________ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils