Message-ID: <c0d77327b15e84df19a019300347063a0b74e1a5.camel@HansenPartnership.com>
Date: Thu, 22 Feb 2024 01:33:14 +0100
From: James Bottomley <James.Bottomley@...senPartnership.com>
To: Kent Overstreet <kent.overstreet@...ux.dev>
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
 lsf-pc@...ts.linux-foundation.org, Christian Brauner
 <christian@...uner.io>,  Stéphane Graber
 <stgraber@...raber.org>
Subject: Re: [LSF TOPIC] beyond uidmapping, & towards a better security model

On Wed, 2024-02-21 at 18:01 -0500, Kent Overstreet wrote:
> On Tue, Feb 20, 2024 at 10:53:58PM -0500, James Bottomley wrote:
> > On Tue, 2024-02-20 at 19:25 -0500, Kent Overstreet wrote:
> > > On Mon, Feb 19, 2024 at 09:26:25AM -0500, James Bottomley wrote:
> > > > I would have to say that changing kuid for a string doesn't
> > > > really
> > > > buy us anything except a load of complexity for no very real
> > > > gain. 
> > > > However, since the current kuid is u32 and exposed uid is u16
> > > > and
> > > > there is already a proposal to make use of this somewhat in the
> > > > way
> > > > you envision,
> > > 
> > > Got a link to that proposal?
> > 
> > I think this is the latest presentation on it:
> > 
> > https://fosdem.org/2024/schedule/event/fosdem-2024-3217-converting-filesystems-to-support-idmapped-mounts/
> > 
> > > 
> > > > there might be a possibility to re-express kuid as an array
> > > > of u16s without much disruption.  Each adjacent pair could
> > > > represent the owner at the top and the userns assigned uid
> > > > underneath.  That would neatly solve the nesting problem the
> > > > current upper 16 bits proposal has.
> > > 
> > > At a high level, there's no real difference between a variable
> > > length
> > > integer, or a variable length array of integers, or a string.
> > 
> > Right, so the advantage is the kernel already does an integer
> > comparison all over the place.
> > 
> > > But there's real advantages to getting rid of the string <->
> > > integer
> > > identifier mapping and plumbing strings all the way through:
> > > 
> > >  - creating a new sub-user can be done with nothing more than the
> > > new
> > >    username version of setuid(); IOW, we can start a new named
> > > subuser
> > >    for e.g. firefox without mucking with _any_ system state or
> > > tables
> > > 
> > >  - sharing filesystems between machines is always a pita because
> > >    usernames might be the same but uids never are - let's kill
> > > that
> > > off,
> > >    please
> > > 
> > > Doing anything as big as an array of integers is going to be a
> > > major
> > > compatibiltiy break anyways, so we might as well do it right.
> > 
> > I'm not really convinced it's right.  Strings are trickier to
> > handle and compare than integer arrays and all of the above can be
> > done by either.
> 
> Strings are just arrays of integers, and anyways this stuff would be
> within helpers.

Length limits and comparisons are the problem

> 
> But what you're not seeing is the beauty and simplicity of killing
> the mapping layer.

Well, that's the problem: you don't for certain use cases.  That's what
I've been trying to explain.  For the fully unprivileged use case,
sure, it all works (as does the upper 32 bits proposal or the integer
array ... equally well.

Once you're representing to the userns contained entity they have a
privileged admin that can write to the fsimage as an apparently
privileged user then the problems begin.

> When usernames are strings all the way into the kernel, creating and
> switching to a new user is a single syscall. You can't do that if
> users are small integer identifiers to the kernel; you have to create
> a new entry in /etc/passwd or some equivalent, and that is strictly
> required in order to avoid collisions. Users also can't be ephemeral.
> 
> To sketch out an example of how this would work, say we've got a new
> set_subuser() syscall and the username equivalent of chown().
> 
> Now if we want to run firefox as a subuser, giving it access only
> .local/state/firefox, we'd do the following sequence of syscalls
> within the start of the new firefox process:
> 
> mkdir(".local/state/firefox");
> chown_subuser(".local/state/firefox", "firefox"); /* now owned by
> $USER.firefox */
> set_subuser("firefox");
> 
> If we want to guarantee uniqueness, we'd append a UUID to the
> subusername for the chown_subuser() call, and then for subsequent
> invocations read it with statx() (or subuser enabled equivalent) for
> the set_subuser() call.
> 
> Now firefox is running in a sandbox, where it has no access to the
> rest of your home directory - unless explicitly granted with normal
> ACLs. And the sandbox requires no system configuration; rm -rfing the
> .local/state/firefox directory cleans everything up.
> 
> And these trivially nest: Firefox itself wants to sandbox individual
> tabs from each other, so firefox could run each sub-process as a
> different subuser.
> 
> This is dead easy compared to what we've been doing.

The above is the unprivileged use case.  It works, but it's not all we
have to support.

> > > > However, neither proposal would get us out of the problem of
> > > > mount mapping because we'd have to keep the filesystem
> > > > permission check on the owning uid unless told otherwise.
> > > 
> > > Not sure I follow?
> > 
> > Mounting a filesystem inside a userns can cause huge security
> > problems if we map fs root to inner root without the admin blessing
> > it.  Think of binding /bin into the userns and then altering one of
> > the root owned binaries as inner root: if the permission check
> > passes, the change appears in system /bin.
> 
> So with this proposal mount mapping becomes "map all users on this
> filesystem to subusers of username x". That's a much simpler mapping
> than mapping integer ranges to integer ranges, much easier to verify
> that there aren't accidental root escpes.

That doesn't work for the privileged container run in unprivileged
userns containment use case because we need a mapping from inner to
outer root.

> > > And it wouldn't have to be administrator assigned. Some
> > > administrator assignment might be required for the username <->
> > > 16 bit uid mapping, but if those mappings are ephemeral (i.e. if
> > > we get filesystems persistently storing usernames, which is easy
> > > enough with xattrs) then that just becomes "reserve x range of
> > > the 16 bit uid space for ephemeral translations".
> > 
> > *if* the user names you're dealing with are all unprivileged.  When
> > we have a mix of privileged and unprivileged users owning the
> > files, the problems begin.
> 
> Yes, all subusers are unprivilidged - only one username, the empty
> username (which we'd probably map to root) maps to existing uid 0.

But, as I said above, that's only a subset of the use cases.  The
equally big use case is figuring out how to run privileged containers
in a deprivileged mode and yet still allow them to update images (and
other things).

The unprivileged case is also solved by the proposal I referred you to,
so it's unclear what the advantage of strings (or even int arrays) adds
to this unless we can get a better handle on the privileged container
use case with them.

James

Powered by blists - more mailing lists