lkml.org 
[lkml]   [2019]   [Jun]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC][PATCH 0/8] Mount, FS, Block and Keyrings notifications [ver #2]
From
Date
On 6/5/19 12:19 AM, Andy Lutomirski wrote:
> On Tue, Jun 4, 2019 at 6:18 PM Stephen Smalley
> <stephen.smalley@gmail.com> wrote:
>>
>> On Tue, Jun 4, 2019 at 4:58 PM Andy Lutomirski <luto@kernel.org> wrote:
>>>
>>> On Tue, Jun 4, 2019 at 1:39 PM David Howells <dhowells@redhat.com> wrote:
>>>>
>>>> Andy Lutomirski <luto@kernel.org> 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.

\
 
 \ /
  Last update: 2019-06-05 15:48    [W:5.522 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site