Re: [Pkg-shadow-devel] [PATCH 00/11] pkg-shadow support subordinate ids with user namespaces

[Serge Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>]

Quoting Nicolas François (nicolas.francois-Fa7rcPG4DJn7nK0/Xc0eeg@public.gmane.org):
> Hi,
> 
> On Tue, Aug 06, 2013 at 02:54:03PM +0000, serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org wrote:
> > 
> > I rebased and pushed the patchset yesterday.
> 
> Thanks (this removes me the burden of finding how to merge a branch).
> 
> I started to review and read about user namespaces (thanks to Michael!)
> o
> Here are some questions/remarks:
> 
>  1] It would be nice to be able to disable the new tools / manpages /
>     options on some systems (i.e. they only make sense on Linux, with
>     recent Kernel). It would be OK for me if it is enabled by default.
>     Do you agree?

Makes sense.

>  2] I think we can assume that UID/GIDs are (at least) 32 bits, right?
>     (or at least when the feature is not disabled - see point 1)

Hm, what about systems compiled with CONFIG_UID16?

>  3] For PAM versions of shadow, I think it would be nice to authenticate
>     the caller of newuidmap / newgidmap. Do you agree?

I'm not sure what you mean.  Do you mean that pam_something.so could
otherwise call newuidmap/newgidmap before being authenticated?

>  4] What happens when newuidmap / newgidmap are used within a new namespace?
>     Is there a risk to do something wrong by recursively creating shells in
>     cascaded namespace and using these tools? (I don't think so)
>     Is there a way to enforce that it would only be used in the top-most
>     namespace? (Maybe it's not needed to forbid it)

The kernel user namespace implementation is designed to support nesting.

Actually I'm not 100% sure, but I think that setuid-root is currently
only honored in the initial user namespace.  If that were changed,
then calling newuidmap from a child user namespace would only give
the caller privilege over his child user namespace.  Therefore he
could only delegate userids from his own (non-init) namespace to
his own descendent user namespaces.

calling newuidmap on a pid in the initial user namespace results in
-EPERM (as otherwise an unprivileged user could remap privileged
tasks to his permitted subuids :)

>     It would be worth to have at least a paragraph about it in the
>     manpages. (i.e. IDs in /etc/sub*id are according to the caller's
>     namespace)

Agreed.

>  5] It would be worth documenting on which processes newuidmap / newgidmap
>     can act.
>     With newuidmap, an user can set the UID mapping for any process with
>     restrictions on the process and on the requested ranges.
>     We could also mention that the kernel enforce also restriction (5
>     lines at most; can only be written once)

Note that there are under-development very detailed manpages for
namespaces(7) and user_namespaces(7).  I agree the newuidmap
manpage should document what it can act, a one or two line addition
to DESCRIPTION.  Then it should simply refer to the other manpages.

>  5.1] The restriction on the caller or processes seems to be the followings:
>     if ((getuid() != pw->pw_uid) ||   // not needed (already enforced
>                                       // by get_my_pwent()) - but does not
>                                       // harm.
>         (getgid() != pw->pw_gid) ||   // I don't understand this
>                                       // restriction. If I execute a
>                                       // shell with another GID (e.g. using
>                                       // newgrp) why should it be denied?

I'm not sure.  I don't know whether Eric was following conventions
elsewhere in the code, or had specific attacks in mind.

>         (pw->pw_uid != st.st_uid) ||
>         (pw->pw_gid != st.st_gid)) {  // Is it also needed? What would be
>                                       // the problem

IIUC uid 1000, authorized to use subuids 100000-110000, could otherwise
use newuidmap to remap a namespace owned by uid 1001.

>  6] Documenting the APIs would be useful. Especially the ones in
>     lib/subordinateio.c (after 5 minutes, I get lost with the
>     is_range_free, range_exists, find_range, have_range, ...)

In-line (above each function) or elsewhere?

>  7] I'm not sure about the handling of limits of ranges.
>     * First, I'm not sure what is intended. (e.g. is SUB_GID_MAX included in
>     the range)
>     * In libmisc/find_new_sub_uids.c: Is it intended to forbid min == max
>       (I'm not claiming this is very useful, but if
>       SUB_GID_MIN=SUB_GID_MAX and SUB_GID_COUNT is set to 1, this should
>       be valid to set a single mapping for one UID)

Agreed that sounds like it should be allowed.

>     * In find_free_range(), min==max is also forbidden.
>     * In find_free_range(), the check (high > low) forbids to reuse a hole
>       of just 1 UID starting at min
>     ...
> 
>  8] Is there a need to check somewhere that that UID_MAX < SUB_UID_MIN in
>     login.defs? Or is it valid to have overlaps?

That sounds like a dangerous condition to me, and I had assumed it was
already checked somewhere.

>  9] Add manpage NOTE that newuidmap / newgidmap can be used only once on a
>     given process?

Agreed.

> 10] useradd adds sub*id when the /etc/sub*id files are present, right?
>     Is it also correct/intended for system users?
>     I think there should be options to enable or disable the feature.
>     (I would have chosen the first solution, but I can also understand
>     that we consider that the presence of the files is a request to use
>     the feature by default)

Not sure on both of those.  Actually given the (artificial) definition
of a system user in useradd(8), it sounds to me like subuids could be
useful for system users on a system where tasks get started in a userns
by boot scripts using a system userid.

> 11] Ranges will be defined for new users. Do you think there would be a
>     need for another tool to initialize a system for this feature?
>     (similar to pwconv)

Do you mean to add ranges for existing users?

> 12] There could be additions in pwck:
>      * check that users in /etc/sub*id exist
>      * check overlaps?

Overlaps may actually be a feature in some cases, as a way to allow
users to share data.

>      * check overlaps with UIDs?
>      * ...
> 
> I do not think that they are blocking points. (i.e. there could be a
> release without a solution)

I'll wait for comments or ("no time for a comment") on any of these
from Eric, then look at addressing those that I can.

thanks,
-serge