[PATCH] LSM: allow an LSM to disable all hooks at once

[Ondrej Mosnacek]

On Wed, Dec 11, 2019 at 8:12 PM Stephen Smalley <sds at tycho.nsa.gov> wrote:
> On 12/11/19 1:35 PM, Casey Schaufler wrote:
> > On 12/11/2019 8:42 AM, Kees Cook wrote:
> >> On Wed, Dec 11, 2019 at 09:29:10AM -0500, Stephen Smalley wrote:
> >>> On 12/11/19 9:08 AM, Ondrej Mosnacek wrote:
> >>>> Instead of deleting the hooks from each list one-by-one (which creates
> >>>> some bad race conditions), allow an LSM to provide a reference to its
> >>>> "enabled" variable and check this variable before calling the hook.
> >>>>
> >>>> As a nice side effect, this allows marking the hooks (and other stuff)
> >>>> __ro_after_init unconditionally. Since SECURITY_WRITABLE_HOOKS no longer
> >>>> makes sense, remove it and instead use SECURITY_SELINUX_DISABLE directly
> >>>> for turning on the runtime disable functionality, to emphasize that this
> >>>> is only used by SELinux and is meant to be removed in the future.
> >>> Is this fundamentally different/better than adding if (!selinux_enabled)
> >>> return 0; to the beginning of every SELinux hook function?  And as I noted
> >>> to Casey in the earlier thread, that provides an additional easy target to
> >>> kernel exploit writers for neutering SELinux with a single kernel write
> >>> vulnerability. OTOH, they already have selinux_state.enforcing and friends,
> >>> and this new one would only be if SECURITY_SELINUX_DISABLE=y.
> >> Yeah, I agree here -- we specifically do not want there to be a trivial
> >> way to disable LSMs at runtime. CONFIG_SECURITY_SELINUX_DISABLE should
> >> be considered deprecated IMO, and we don't want to widen its features.
> >
> > In /etc/selinux/config SELINUX=disabled is documented as "No SELinux
> > policy is loaded". How about if instead of blocking policy load and
> > removing the hooks it just blocked policy load? It may be appropriate
> > to tweak the code a bit to perform better in the no-policy loaded
> > case, but my understanding is that the system should work. That would
> > address backward compatibility concerns and allow removal of
> > security_delete_hooks(). I don't think this would have the same
> > exposure of resetting selinux_enabled.
>
> I think that comment stems from before runtime disable was first
> implemented in the kernel, when the only option was to leave SELinux
> enabled with no policy loaded.  Fedora didn't consider that (or
> selinux=0) to be acceptable alternatives, which is why we have runtime
> disable today.

Do you happen to remember the reasons why it wasn't acceptable? We are
ready to start pushing for disabling SECURITY_SELINUX_DISABLE in
Fedora, but we're not sure why it is so crucial. Knowing what we need
to address before disabling/removing it would help a lot.

> selinux_state.initialized reflects whether a policy has
> been loaded.  With a few exceptions in certain hook functions, it is
> only checked by the security server service functions
> (security/selinux/ss/services.c) prior to accessing the policydb.  So
> there is a lot of SELinux processing that would still occur in that
> situation unless we added if (!selinux_state.initialized) return 0;
> checks to all the hook functions, which would create the same exposure
> and would further break the SELinux-enabled case (we need to perform
> some SELinux processing pre-policy-load to allocate blobs and track what
> tasks and objects require delayed security initialization when policy
> load finally occurs).

I think what Casey was suggesting is to add another flag that would
switch from "no policy loaded, but we expect it to be loaded
eventually" to "no policy loaded and we don't expect/allow it to be
loaded any more", which is essentially equivalent to checking
selinux_enabled in each hook, which you had already brought up.

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.