diff options
| -rw-r--r-- | src/chroot.c | 61 | ||||
| -rwxr-xr-x | tests/chroot/credentials | 2 |
2 files changed, 40 insertions, 23 deletions
diff --git a/src/chroot.c b/src/chroot.c index 39b3acf03..12d282b49 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 b76edea7d..58c098f53 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 |