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
