Re: [RFC][PATCH 0/8] Mount, FS, Block and Keyrings notifications [ver #2]

From: Stephen Smalley
Date: Wed Jun 05 2019 - 09:52:16 EST

On 6/5/19 12:19 AM, Andy Lutomirski wrote:
On Tue, Jun 4, 2019 at 6:18 PM Stephen Smalley
<stephen.smalley@xxxxxxxxx> wrote:

On Tue, Jun 4, 2019 at 4:58 PM Andy Lutomirski <luto@xxxxxxxxxx> wrote:

On Tue, Jun 4, 2019 at 1:39 PM David Howells <dhowells@xxxxxxxxxx> wrote:

Andy Lutomirski <luto@xxxxxxxxxx> wrote:

Here's a set of patches to add a general variable-length notification queue
concept and to add sources of events for:

I asked before and didn't see a response, so I'll ask again. Why are you
paying any attention at all to the creds that generate an event?

Casey responded to you. It's one of his requirements.

It being a "requirement" doesn't make it okay.

However, the LSMs (or at least SELinux) ignore f_cred and use current_cred()
when checking permissions. See selinux_revalidate_file_permission() for
example - it uses current_cred() not file->f_cred to re-evaluate the perms,
and the fd might be shared between a number of processes with different creds.

That's a bug. It's arguably a rather severe bug. If I ever get
around to writing the patch I keep thinking of that will warn if we
use creds from invalid contexts, it will warn.

No, not a bug. Working as designed. Initial validation on open, but revalidation upon read/write if something has changed since open (process SID differs from opener, inode SID has changed, policy has changed). Current subject SID should be used for the revalidation. It's a MAC vs DAC difference.

Can you explain how the design is valid, then? Consider nasty cases like this:

$ sudo -u lotsofgarbage 2>/dev/whatever

(sorry for the previous html email; gmail or my inability to properly use it strikes again!)

Here we have four (or more) opportunities to say no:
1) Upon selinux_inode_permission(), when checking write access to /dev/whatever in the context of the shell process,
2) Upon selinux_file_open(), when checking and caching the open and write access for shell to /dev/whatever in the file security struct,
3) Upon selinux_bprm_committing_creds() -> flush_unauthorized_files(), when revalidating write access to /dev/whatever in the context of sudo,
4) Upon selinux_file_permission() -> selinux_revalidate_file_permission(), when revalidating write access to /dev/whatever in the context of sudo.

If any of those fail, then access is denied, so unless both the shell and sudo are authorized to write to /dev/whatever, it is a no-go. NB Only the shell context requires open permission here; the sudo context only needs write.

It is certainly the case that drivers, fs code, and other core code
MUST NOT look at current_cred() in the context of syscalls like
open(). Jann, I, and others have found quite a few rootable bugs of
this sort. What makes MAC special here?

Do you mean syscalls like write(), not open()? I think your concern is that they apply some check only during write() and not open() and therefore are susceptible to confused deputy scenario above. In contrast we are validating access at open, transfer/inherit, and use. If we use file->f_cred instead of current_cred() in selinux_revalidate_file_permission() and the current process SID differs from that of the opener, we'll never apply a check for the actual security context performing the write(), so information can flow in violation of the MAC policy.

I would believe there are cases where auditing write() callers makes
some sense, but anyone reading those logs needs to understand that the
creds are dubious at best.