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

[Kees Cook <keescook@chromium.org>]

On Wed, Apr 10, 2019 at 10:47 AM Jann Horn <jannh@google.com> wrote:
>
> On Wed, Apr 10, 2019 at 7:24 PM Kees Cook <keescook@chromium.org> wrote:
> > On Wed, Apr 10, 2019 at 9:56 AM Micah Morton <mortonm@chromium.org> wrote:
> > > From: Jann Horn <jannh@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