[PATCH 07/10] LSM: SafeSetID: rewrite userspace API to atomic updates

[Micah Morton]

Ready for merge.

On Wed, Apr 10, 2019 at 11:20 AM Kees Cook <keescook at chromium.org> wrote:
>
> On Wed, Apr 10, 2019 at 10:47 AM Jann Horn <jannh at google.com> wrote:
> >
> > On Wed, Apr 10, 2019 at 7:24 PM Kees Cook <keescook at chromium.org> wrote:
> > > On Wed, Apr 10, 2019 at 9:56 AM Micah Morton <mortonm at chromium.org> wrote:
> > > > From: Jann Horn <jannh at google.com>
> > > >
> > > > The current API of the SafeSetID LSM uses one write() per rule, and applies
> > > > each written rule instantly. This has several downsides:
> > > >
> > > >  - While a policy is being loaded, once a single parent-child pair has been
> > > >    loaded, the parent is restricted to that specific child, even if
> > > >    subsequent rules would allow transitions to other child UIDs. This means
> > > >    that during policy loading, set*uid() can randomly fail.
> > > >  - To replace the policy without rebooting, it is necessary to first flush
> > > >    all old rules. This creates a time window in which no constraints are
> > > >    placed on the use of CAP_SETUID.
> > > >  - If we want to perform sanity checks on the final policy, this requires
> > > >    that the policy isn't constructed in a piecemeal fashion without telling
> > > >    the kernel when it's done.
> > > >
> > > > Other kernel APIs - including things like the userns code and netfilter -
> > > > avoid this problem by performing updates atomically. Luckily, SafeSetID
> > > > hasn't landed in a stable (upstream) release yet, so maybe it's not too
> > > > late to completely change the API.
> > > >
> > > > The new API for SafeSetID is: If you want to change the policy, open
> > > > "safesetid/whitelist_policy" and write the entire policy,
> > > > newline-delimited, in there.
> > >
> > > So the entire policy is expected to be sent in a single write() call?
> > >
> > > open()
> > > write(policy1)
> > > write(policy2)
> > > close()
> > >
> > > means only policy2 is active?
> >
> > No; if you do that, the first write() sets policy1, and the second
> > write() fails with -EINVAL because of the "if (*ppos != 0) return
> > -EINVAL;" in safesetid_file_write() (which already exists in the
> > current version of the LSM).
>
> Ah yes, thanks! I missed that check. Good!
>
> >
> > > I thought policy was meant to be built
> > > over time? i.e. new policy could get appended to existing?
> >
> > That's what the current API does; as I've explained in the commit
> > message, I think that that's a bad idea.
>
> Okay, sounds fine. It wasn't clear to me from the commit message if
> you meant "write the whole policy during a single open/close" or
> "write whole policy with a single initial write".
>
> --
> Kees Cook