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

[Ondrej Mosnacek]

On Thu, Dec 12, 2019 at 8:01 PM Paul Moore <paul at paul-moore.com> wrote:
> On Thu, Dec 12, 2019 at 1:48 PM Stephen Smalley <sds at tycho.nsa.gov> wrote:
> > On 12/12/19 1:18 PM, Paul Moore wrote:
> > > On Thu, Dec 12, 2019 at 1:10 PM Stephen Smalley <sds at tycho.nsa.gov> wrote:
> > >>
> > >> On 12/12/19 1:09 PM, Paul Moore wrote:
> > >>> On Thu, Dec 12, 2019 at 12:57 PM Stephen Smalley <sds at tycho.nsa.gov> wrote:
> > >>>> On 12/12/19 12:54 PM, Paul Moore wrote:
> > >>>>> On Thu, Dec 12, 2019 at 8:14 AM Stephen Smalley <sds at 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 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:
> > >>>>>
> > >>>>> ...
> > >>>>>
> > >>>>>>>> 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.
> > >>>>>
> > >>>>> Just so I'm understanding this thread correctly, the above change
> > >>>>> (adding enabled checks to each SELinux hook implementation) is only
> > >>>>> until Fedora can figure out a way to deprecate and remove the runtime
> > >>>>> disable?
> > >>>>
> > >>>> That's my understanding.  In the interim, Android kernels should already
> > >>>> be disabling CONFIG_SECURITY_SELINUX_DISABLE and other distros may
> > >>>> choose to disable it as long as they don't care about supporting SELinux
> > >>>> runtime disable.
> > >>>
> > >>> Okay, I just wanted to make sure I wasn't missing something.
> > >>> Honestly, I'd rather Fedora just go ahead and do whatever it is they
> > >>> need to do to turn off CONFIG_SECURITY_SELINUX_DISABLE (it sounds like
> > >>> they have a plan and are working on it), I'm not overly excited about
> > >>> temporarily cluttering up the code with additional "enabled" checks
> > >>> when the status quo works, even if it is less than ideal.
> > >>
> > >> The status quo is producing kernel crashes, courtesy of LSM stacking
> > >> changes...
> > >
> > > How prevalent are these crashes?
> > >
> > > This also only happens when disabling SELinux at runtime, yes?
> > > Something we've advised against for some time now and are working to
> > > eliminate?  Let's just get rid of the runtime disable *soon*, and if
> > > we need a stop-gap fix let's just go with the hook reordering since
> > > that seems to minimize the impact, if not resolve it.
> >
> > Not optimistic given that the original bug was opened 2.5+ years ago,
> > commenters identified fairly significant challenges to removing the
> > support, and no visible progress was ever made.  I suppose the hook
> > reordering will do but seems fragile and hacky.  Whatever.
>
> Based on Ondrej's comments in this thread it sounded as if there was
> some renewed interest in actually eliminating it from Fedora this
> time.  If that's not the case, perhaps it's time to start that effort
> back up, and if we can't ever get away from the runtime disable then
> we can take the step of admitting that everything is terrible and we
> can consider some of the options presented in this thread.

Yes, we are quite determined to do what it takes to remove it. It may
go more smoothly than expected, or it may get unexpectedly
complicated. It's hard to tell at this point.

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