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

[Ondrej Mosnacek <omosnace@redhat.com>]

On Thu, Dec 12, 2019 at 2:14 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On 12/12/19 6:49 AM, Ondrej Mosnacek wrote:
> > On Wed, Dec 11, 2019 at 8:12 PM Stephen Smalley <sds@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.
>
> IIRC, they considered the selinux=0 kernel boot parameter to be
> inadequate because of the difficulty in changing kernel boot parameters
> on certain platforms (IBM?).  The no-policy-loaded alternative still
> left a lot of SELinux processing in place, so users would still end up
> paying memory and runtime overheads for no benefit if we only skipped
> policy load.

Thanks, I was worried that there was also something more tricky than
this. We could make adding-removing the kernel parameter easier on
Fedora by creating and maintaining a tool that would be able to do it
reliably across the supported arches. That shouldn't be too hard,
hopefully.

>
> >> 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.
>
> Yep.  if (!selinux_enabled) return 0; or if (selinux_state.disabled)
> return 0; under #ifdef CONFIG_SECURITY_SELINUX_DISABLE in every hook
> might be the best option until it can be removed altogether; avoids
> impacting the LSM framework or any other security module, preserves the
> existing functionality, fairly low overhead on the SELinux-disabled case.

OK, so I'll put together another patch that removes all the
security_delete_hooks() stuff and adds the checks.

>
> NB selinux_enabled was originally just for selinux=0 handling and thus
> remains global (not per selinux-namespace).  selinux_state.disabled is
> only for runtime disable via selinuxfs, which could be applied per
> selinux-namespace if/when selinux namespaces are ever completed and
> merged. Aside from clearing selinux_enabled in selinux_disable() and
> logging selinux_enabled in sel_write_enforce() - which seems pointless
> by the way, there are no other uses of selinux_enabled outside of __init
> code AFAICS.  I think at one time selinux_enabled was exported for use
> by other kernel code related to SECMARK or elsewhere but that was
> eliminated/generalized for other security modules.  So it seems like we
> could always make selinux_enabled itself ro_after_init, stop clearing it
> in selinux_disable() since nothing will be testing it, and just use
> selinux_state.disabled in the hooks (and possibly in the
> sel_write_enforce audit log).

Yes, that sounds reasonable.

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