Ready for merge.

On Wed, Apr 10, 2019 at 11:20 AM Kees Cook [off-list ref] wrote:

On Wed, Apr 10, 2019 at 10:47 AM Jann Horn [off-list ref] wrote:

quoted

On Wed, Apr 10, 2019 at 7:24 PM Kees Cook [off-list ref] wrote:

quoted

On Wed, Apr 10, 2019 at 9:56 AM Micah Morton [off-list ref] wrote:

quoted

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!

quoted

quoted

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