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. 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. -- paul moore www.paul-moore.com _______________________________________________ Selinux mailing list Selinux@xxxxxxxxxxxxx To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx. To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.
