Hello,
On Wed, Aug 07, 2013 at 11:04:59AM -0700, ebiederm@xxxxxxxxxxxx wrote:
> Serge Hallyn <serge.hallyn@xxxxxxxxxx> writes:
> > Quoting Nicolas François (nicolas.francois@xxxxxxxxxxxxxxx):
> >> On Tue, Aug 06, 2013 at 02:54:03PM +0000, serge@xxxxxxxxxx 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?
At config time, we can check that uid_t and gid_t allow for 32 bit IDs.
> >> 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?
>
> It sounded to me like he was suggesting newuidmap/newgidmap should make
> an appropriate pam call. I am not familiar with that part of pam so I
> don't know what would be an appropriate pam call.
>
> The closest analog I can think of is newgrp, and to my knowledge newgrp
> does not cal into pam.
I was thinking about something like in chsh. But the analogy with newgrp
is better. So lets forget about it.
> >> 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 :)
>
> There are slightly funny things with setuid and setcap.
>
> The short version however is that once in a user namespace you will
> never have more permissions than your the root user in your user
> namespace.
>
> So from the outside of the user namespace you can't do anything bad even
> with nesting. Aka there is no defined means to escapte privileges from
> inside a user namespace to anything privileged outside a user namespace.
>
> From inside the user namespace you can only follow /etc/subuid or
> /etc/subgid (when setting up a nested user namespace). And if that is a
> concern the root user of a user namespace can just create a new mount
> namespace and mount over the top of them.
>
> So I don't see anything to concern about.
OK.
> >> 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.
Lets ignore it currently. It will be users/admins' responsibility to setup
the right ranges usable in the caller environment.
> >> 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.
>
> Yeah. I should follow up and see what has happend with all of that.
I will add references to these 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.
>
> I don't have the context handy, but I think that was general paranoia.
> On the lines of if something strange is going on don't allow it.
This test ("getgid() != pw->pw_gid") can be bypassed by executing
newuidmap in another shell, with the right gid.
I propose to remove it.
> >> (pw->pw_uid != st.st_uid) ||
That part is needed.
Is there also the need to check that the process cannot change its
effective 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.
OK for the test on UID, but I don't get the point for checking the GID.
Note that we can also put this restriction and wait for a user request.
Also regarding this block, the permission of /proc/<pid> are checked.
I've read that there is the concept of a namespace owner (uid/gid of
/proc/<pid>/uid_map. Is it also something to be checked?
> >> 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?
A small paragraph above each function would be perfect.
> >> 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
> >> ...
>
> Scratching my head. When ranges are bounded by two numbers it gets
> weird. All I remember for certain is I deliberately choose the file
> format of uid, length so that it would be hard to make off by one
> mistakes in the files.
I will create tests for the boundary conditions.
> >> 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.
>
> These were fairly arbitrary defines, with no sanity checks and I didn't
> add any. I just had sensible defaults. As that tends to be the unix
> way. You can have rope to shoot yourself if you really want it.
>
> I really have no opinion on the need for sanity checks one way or another.
I can add a warning in pwck (can't find of a better place (useradd?)).
> >> 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.
>
> I believe the question was. If /etc/subuid does not exist and we add a
> new user do we want to add an entry in /etc/subuid. Similarly for
> /etc/subgid.
>
> I followed convention in shadow and figured that if /etc/subuid and
> /etc/subgid are not present we don't want to add subuids and subgids.
>
> I don't expect we want to add subuids and subgids for system users by
> default but as Serge says there may be instances where we want to add
> them manually.
OK. That would change useradd (disabling setting sub*id when rflg is set).
We could add options to useradd to change this default, or just let the
user use usermod.
> >> 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?
>
> I believe it was either useradd or usermod that I modified so that I
> could add subuids and subgids for an existing user manually.
>
> As for upgrades I don't know. Serge with your distro hat on do you see
> a use for automatically giving user on a system subids and subgids when
> they upgrade versions of shadow?
>
> >> 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.
>
> I know I deliberately allowed (but would never create by default)
> multiple users having access to the same range of subordinate uids.
>
> Shared read-only system images may benefit from such a practice.
>
> In a similar vein there may be uses of being able to log in to a system
> with what is ordinarily a subordinate uid.
>
> The distinction is a distinction of use not a distinction made in the
> kernel.
>
> That said having overlaps is usually a mistake and a warning in the
> manual process may be called for in case someone simply didn't see
> something.
OK.
> >> * 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.
>
> I hope that helps put some context to things. I am totally not looking
> at the userspace issues around newuidmap and newgidmap right now, and I
> appreciate others being able to look at it and make things better.
Thanks for your feedback.
--
Nekral
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/containers
