Re: [RFC PATCH 1/3] setgroups: new mode 'shadow' for /proc/PID/setgroups

[Christian Brauner]

On Mon, May 17, 2021 at 11:17:13AM -0500, Serge Hallyn wrote:
> On Mon, May 17, 2021 at 04:33:21PM +0200, Christian Brauner wrote:
> > On Mon, May 17, 2021 at 03:30:16PM +0200, Giuseppe Scrivano wrote:
> > > >> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> > > >> index 8d62863721b0..b1940b63f7ac 100644
> > > >> --- a/kernel/user_namespace.c
> > > >> +++ b/kernel/user_namespace.c
> > > >> @@ -123,6 +123,7 @@ int create_user_ns(struct cred *new)
> > > >>  		ns->ucount_max[i] = INT_MAX;
> > > >>  	}
> > > >>  	ns->ucounts = ucounts;
> > > >> +	ns->shadow_group_info = get_current_groups();
> > > >
> > > > If userns u1 unshares u2 with shadow set, then when u2 unshares
> > > > u3, should u3 get the same shadowed set that u2 has, or should it
> > > > get all of u2's groups as u3's initial shadow set?
> > > 
> > > good question.  Thinking more of it, I think a reasonable interface is
> > > to expect a child userns to inherit the same shadow groups as its parent
> > > userns.  If "shadow" is written again to the /proc/PID/setgroups file
> > > then it grows shadow groups set to include the ones the userns had at
> > > creation time (which includes the parent shadow groups).  What do you
> > > think of it?  I'll play more with this idea and see if it works.
> > 
> > So when I initially looked at that proposal I was neither "yay" or "nay"
> > since it seemed useful to people and it looked somewhat straightforward
> > to implement.
> > 
> > But I do have concerns now after seeing this. The whole
> > /proc/<pid>/setgroups API is terrible in the first place and causes even
> > more special-casing in container runtimes then there already is. But it
> > fixes a security issue so ok we'll live with it.
> > 
> > But I'm not happy about extending its format to include more options. I
> > really don't want the precedent of adding magic keywords into this file.
> > 
> > Which brings me to my second concern. I think starting to magically
> > inherit group ids isn't a great idea. It's got a lot of potential for
> > confusion.
> > 
> > The point Serge here made makes this pretty obvious imho. I don't think
> > introducing the complexities of magic group inheritance is something we
> > should do.
> > 
> > Alternative proposal, can we solve this in userspace instead?
> > 
> > As has been pointed out there is a solution to this problem already
> > which is to explicitly map those groups through, i.e. punch holes for
> > the groups to be inherited.
> > 
> > So can't we introduce a new mode for newgidmap by e.g. introducing
> > another /etc/setgroups file or something similar that can be configured
> > by the administrator. It could take options, e.g. "shadow=always" which
> > could mean "everyone must inherit their groups" so newgidmap will punch
> > holes for the caller's groups when writing the gid mapping. We could
> > also extend this by making newgidmap take a command line switch so it's
> > on a case-by-case basis.
> > 
> > This is even more flexible since you could extend the new /etc/setgroups
> > file to specify a list of groups that must always be preserved. I'd
> > rather see something like this rather than a magic inheritance switch.
> > 
> > Christian
> 
> That sounds reasonable, but my concern is that admins currently using
> groups to deny file access will need to take extra steps to maintain
> that guarantee.  I think that falls under the category of a regression.
> Unless we make 'shadow=always' the default.  But then *that* will regress
> users who currently do not want that feature :)

I think this should simply be up to the administrator.

> 
> Anyway, if we do go this route - or maybe a login.defs option
> "ALLOW_UNPRIV_GROUPS_DROP" - perhaps we can also add a new /etc/subauxgroups
> file (TODO find a better name) which admins who are in the know can use to say
> "hallyn 2000" meaning "user hallyn cannot drop group 2000"

That sounds like something useful. Ideally integrated with the newly
added libsubid.

Christian