[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: RFC: dropping privs in chroot --user
From: |
Bernhard Voelker |
Subject: |
Re: RFC: dropping privs in chroot --user |
Date: |
Sat, 17 May 2014 02:45:56 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0 |
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
Otherwise: nice work, thanks!
Have a nice day,
Berny