Date:   Fri, 21 May 2021 16:03:22 +0200
From:   Snaipe <snaipe@...sta.com>
To:     christian.brauner@...ntu.com
Cc:     dwalsh@...hat.com, ebiederm@...ssion.com, gscrivan@...hat.com,
        linux-kernel@...r.kernel.org, serge@...lyn.com,
        Snaipe <snaipe@...sta.com>
Subject: Re: [RFC PATCH 1/3] setgroups: new mode 'shadow' for /proc/PID/setgroups

Christian Brauner <christian.brauner@...ntu.com> writes:
> On Mon, May 17, 2021 at 03:30:16PM +0200, Giuseppe Scrivano wrote:
> > "Serge E. Hallyn" <serge@...lyn.com> writes:
> > > 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.

To be fair, we already magically inherit group ids -- that's what not calling
setgroups after entering the userns and setting up the {u,g}id maps does.
The new shadow mode does not really cause inheritance, but it does provide
a way for userspace programs to keep these unmapped groups around after
a setgroups, which is currently impossible, and quite sad for the reasons
I outlined in my other email[1].

> 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.

I don't think it really addresses the problem. If you explicitly map these
groups through, then it's still trivial for userspace programs to simply
drop them after the gid map is written. It just solves the problem of
having control over your additional groups in the userns, which, it turns
out, is already configurable by the system administrator via /etc/subgid:

    host$ id
    uid=1000(snaipe) gid=1000(snaipe) groupes=1000(snaipe),998(wheel)

    host$ cat /etc/subgid
    user:1000000:1100000
    user:998:1

    host$ unshare -fU --map-user=0 sh
    ns# echo $$
    3720498

    host$ newgidmap 3725680 0 1000 1 1 1000000 1100000 1100001 998 1

    ns# id
    uid=0(root) gid=0(root) groupes=0(root),65534(nobody),1100001

This is only fine if we're not interested in supporting both negative-
access permission bits and ACLs. It also means application writers and
system administrators cannot force unprivileged users to stay in their
groups even after giving them control of their own user namespace.

[1]: https://lore.kernel.org/lkml/20210510160233.2266000-1-snaipe@arista.com/

-- 
Snaipe

Powered by blists - more mailing lists