Message-ID: <a185b384-5ef1-5e53-2cc1-066391424dcb@tycho.nsa.gov>
Date:   Tue, 25 Sep 2018 12:39:55 -0400
From:   Stephen Smalley <sds@...ho.nsa.gov>
To:     Paul Moore <paul@...l-moore.com>, takondra@...co.com
Cc:     linux-kernel@...r.kernel.org, selinux@...ho.nsa.gov,
        xe-linux-external@...co.com
Subject: Re: [RFC PATCH] selinux: add a fallback to defcontext for native
 labeling

On 09/25/2018 12:03 PM, Paul Moore wrote:
> On Tue, Sep 25, 2018 at 9:58 AM Stephen Smalley <sds@...ho.nsa.gov> wrote:
>> On 09/25/2018 01:45 AM, Taras Kondratiuk wrote:
>>> Quoting Paul Moore (2018-09-24 20:46:57)
>>>> On Fri, Sep 21, 2018 at 10:39 AM Stephen Smalley <sds@...ho.nsa.gov> wrote:
>>>>> On 09/20/2018 06:59 PM, Taras Kondratiuk wrote:
>>>>>> Quoting Stephen Smalley (2018-09-20 07:49:12)
>>>>>>> On 09/19/2018 10:41 PM, Taras Kondratiuk wrote:
>>>>>>>> Quoting Stephen Smalley (2018-09-19 12:00:33)
>>>>>>>>> On 09/19/2018 12:52 PM, Taras Kondratiuk wrote:
>>>>
>>>> ...
>>>>
>>>>>> IMO it would be more consistent if defcontext cover all "unlabeled"
>>>>>> groups. It seems unlikely to me that somebody who currently uses
>>>>>> defcontext can somehow rely on mapping invalid labels to unlabeled
>>>>>> instead of default context.
>>>>>
>>>>> Yes, and that seems more consistent with the current documentation in
>>>>> the mount man page for defcontext=.
>>>>>
>>>>> I'd be inclined to change selinux_inode_notifysecctx() to call
>>>>> security_context_to_sid_default() directly instead of using
>>>>> selinux_inode_setsecurity() and change security_context_to_sid_core()
>>>>> and sidtab_search_core() as suggested above to save and use the def_sid
>>>>> instead of SECINITSID_UNLABELED always (initializing the context def_sid
>>>>> to SECINITSID_UNLABELED as the default).  selinux_inode_setsecurity() we
>>>>> should leave unchanged, or if we change it at all, it should be more
>>>>> like the handling in selinux_inode_setxattr().  The notifysecctx hook is
>>>>> invoked by the filesystem to notify the security module of the file's
>>>>> existing security context, so in that case we always want the _default
>>>>> behavior, whereas the setsecurity hook is invoked by the vfs or the
>>>>> filesystem to set the security context of a file to a new value, so in
>>>>> that case we would only use the _force interface if the caller had
>>>>> CAP_MAC_ADMIN.
>>>>>
>>>>> Paul, what say you?  NB This would be a user-visible behavior change for
>>>>> mounts specifying defcontext= on xattr filesystems; files with invalid
>>>>> contexts will then show up with the defcontext value instead of the
>>>>> unlabeled context.  If that's too risky, then we'd need a flag or
>>>>> something to security_context_to_sid_default() to distinguish the
>>>>> behaviors and only set it when called from selinux_inode_notifysecctx().
>>>>
>>>> Visible changes like this are always worrisome, even though I think it
>>>> is safe to assume that the defcontext option is not widely used.  I'd
>>>> feel much better if this change was opt-in.
>>>>
>>>> Which brings about it's own problems.  We have the policy capability
>>>> functionality, but that is likely a poor fit for this as the policy
>>>> capabilities are usually controlled by the Linux distribution while
>>>> the mount options are set by the system's administrator when the
>>>> filesystem is mounted.  We could add a toggle somewhere in selinuxfs,
>>>> but I really dislike that idea, and would prefer to find a different
>>>> solution if possible.  I'm not sure how much flak we would get for
>>>> introducing a new mount option, but perhaps that is the best way to
>>>> handle this: defcontext would continue to behave as it does now, but
>>>> new option X would behave as mentioned in this thread.
>>>>
>>>> Thoughts?
>>>
>>> The new option X will also mean "default context", so it will be pretty
>>> hard to come up with a different but a sensible name. I'm afraid that
>>> having two options with hardly distinguishable semantics will be very
>>> confusing.
>>>
>>> What about a kernel config option that modifies the behavior of
>>> defcontext?
>>
>> First, the existing documentation for defcontext= leaves open the door
>> to the proposed new behavior.  From mount(8):
>> "You can set the default security context for  unlabeled  files  using
>> defcontext= option.  This overrides the value set for unlabeled files in
>> the policy and requires a filesystem that supports xattr labeling."
>>
>> "Unlabeled files" could just mean files without any xattr, or it could
>> mean all files that either lack an xattr or have an invalid one under
>> the policy, since both sets of files are currently mapped to the
>> unlabeled context.
> 
> This may be true for the major SELinux policies being shipped by
> distributions, but can we say this holds true for *all* SELinux
> policies in use today?
> 
>> Second, under what conditions would this situation break existing
>> userspace?  The admin would have had to mount an xattr filesystem with
>> defcontext= specified, and that filesystem would have to both contain
>> files without any xattrs and files with invalid ones (otherwise how they
>> are treated by the kernel is irrelevant), and the policy would need to
>> distinguish access to files without xattrs vs files with invalid ones.
>> Seems unlikely.
> 
> I think you answered your own question.  Yes, it is unlikely, but I
> *really* dislike changing visible behavior like this without some
> explicit action on behalf of the user/admin.  We've done it in the
> past and it has left me uneasy each time.
> 
>> Third, the fact that policy maintainers remapped both SECINITSID_FILE
>> (the default value for defcontext) and SECINITSID_UNLABELED (used for
>> invalid contexts) to the unlabeled context (getting rid of file_t as a
>> separate type, aliased to unlabeled_t) long ago suggests that there is
>> no meaningful difference there.
> 
> Related to the comments above.
> 
>> I'm inclined to just change the behavior for defcontext= unconditionally
>> and have it apply to both native and xattr labeling.  If that's a no-go,
>> then the simplest solution is to just leave defcontext= behavior
>> unchanged for xattr labeling and only implement the new semantics for
>> native labeling.  That's just a matter of adding a flag to
>> security_context_to_sid_default() and only setting it when calling from
>> selinux_inode_notifysecctx().
> 
> Neither option is very appealing to me, but that doesn't mean I'm saying "no".
> 
>  From a sanity and consistency point of view I think option #1 (change
> the defcontext behavior) is a better choice, and I tend to favor this
> consistency even with the understanding that it could result in some
> unexpected behavior for users.  However, if we get complaints, I'm
> going to revert this without a second thought.

In that case, I'd suggest splitting it into two patches; first one only 
enables the new behavior for native labeling filesystems (as per the 
above, via a flag to security_context_to_sid_default), and the second 
patch drops the flag and does it unconditionally.  Then you can always 
revert the latter without affecting the former.

> 
> So to answer your question Taras, go ahead and prepare a patch so we
> can take a look.  A bit of fair warning that it might get delayed
> until after the upcoming merge window since we are already at -rc5; I
> want this to have plenty of time in -next.
> 
> Thanks guys.
> 

Powered by blists - more mailing lists