Re: [RFC PATCH] selinux: add a fallback to defcontext for native labeling

From: Stephen Smalley
Date: Tue Sep 25 2018 - 12:38:00 EST

On 09/25/2018 12:03 PM, Paul Moore wrote:
On Tue, Sep 25, 2018 at 9:58 AM Stephen Smalley <sds@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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.