On 05/17/2014 12:48 PM, Pádraig Brady wrote: > On 05/17/2014 01:45 AM, Bernhard Voelker wrote: >> On 05/16/2014 10:59 PM, Pádraig Brady wrote: >> >> Thanks for the detailed tests. >> >>> [[ chroot --user=+5000 / id -G ]] >>> before: 0 1 2 3 4 6 10 >>> after: src/chroot: failed to get primary group >> >> While the logic behind may be okay, our users will >> probably be confused with the above change in behavior. >> I propose to change the error diagnostic to give a hint >> that the user must specify the group explicitly when >> using a numeric UID. >> >> >>> 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 >> >> Didn't we use s/lookup/look-up/ recently, see v8.22-38-ge972be3? >> >>> diff --git a/src/chroot.c b/src/chroot.c >>> index a2debac..a679658 100644 >>> --- a/src/chroot.c >>> +++ b/src/chroot.c >>> @@ -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. */ >> >> s/cy/by/ >> >> >>> - 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; >> >> Is there a reason not to exit immediately? >> IMHO it looks strange (not to say worrying) to get 3 error messages: >> >> $ src/chroot --user=+5000:123 / id -G >> src/chroot: failed to clear supplemental groups: Operation not permitted >> src/chroot: failed to set group-ID: Operation not permitted >> src/chroot: failed to set user-ID: Operation not permitted > > That was there already, and I was wondering why myself. > I'll adjust to exit ASAP lest the user think previous > failing actions were optional. > > Also I agree with your other points and will adjust accordingly. > > Excellent review.
I also noticed another inconsistency where --user=500: was rejected while --user=500 was not. They should both mean the same thing with the new syntax anyway, so I've adjusted so that both area allowed. I split the change to avoid multiple diagnostics to a separate patch since it's a different change to the existing code. Both attached here and I'll push later. thanks, Pádraig.
>From 387fa2a51d56d4a886bd16a6514654bdba94753e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com> Date: Sun, 18 May 2014 16:48:28 +0100 Subject: [PATCH] chroot: exit immediately upon failure * src/chroot.c (main): Consistently exit with failure status immediately upon hitting a terminal issue, rather than diagnosing multiple issues lest users think previous failing actions are optional. --- src/chroot.c | 30 +++++++----------------------- 1 files changed, 7 insertions(+), 23 deletions(-) diff --git a/src/chroot.c b/src/chroot.c index 6b16060..0ded25d 100644 --- a/src/chroot.c +++ b/src/chroot.c @@ -313,8 +313,6 @@ 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) @@ -350,7 +348,7 @@ main (int argc, char **argv) if (parse_additional_groups (groups, &in_gids, &n_gids, !n_gids) != 0) { if (! n_gids) - fail = true; + exit (EXIT_CANCELED); /* else look-up outside the chroot worked, then go with those. */ } else @@ -363,10 +361,8 @@ main (int argc, char **argv) if (ngroups <= 0) { if (! n_gids) - { - fail = true; - error (0, errno, _("failed to get supplemental groups")); - } + error (EXIT_CANCELED, errno, + _("failed to get supplemental groups")); /* else look-up outside the chroot worked, then go with those. */ } else @@ -378,29 +374,17 @@ main (int argc, char **argv) #endif if ((uid_set (uid) || groups) && setgroups (n_gids, gids) != 0) - { - error (0, errno, _("failed to %s supplemental groups"), - gids ? "set" : "clear"); - fail = true; - } + error (EXIT_CANCELED, errno, _("failed to %s supplemental groups"), + gids ? "set" : "clear"); free (in_gids); free (out_gids); if (gid_set (gid) && setgid (gid)) - { - error (0, errno, _("failed to set group-ID")); - fail = true; - } + error (EXIT_CANCELED, errno, _("failed to set group-ID")); if (uid_set (uid) && setuid (uid)) - { - error (0, errno, _("failed to set user-ID")); - fail = true; - } - - if (fail) - exit (EXIT_CANCELED); + error (EXIT_CANCELED, errno, _("failed to set user-ID")); /* Execute the given command. */ execvp (argv[0], argv); -- 1.7.7.6