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

[Paul Moore]

On Fri, Dec 13, 2019 at 9:07 AM Ondrej Mosnacek <omosnace at redhat.com> wrote:
> 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.

That's good to hear, I know it is going to be difficult, but I think
we can all agree it's the right thing to do.  Any idea when you might
have a better idea of the time involved?

Next week I think I'm going to put together a RFC patch that marks
CONFIG_SECURITY_SELINUX_DISABLE as deprecated, and displays a warning
when it is used at runtime.  Later on when we have a better idea of
the removal date, we can start adding delays when it is used to help
get people to migrate to the cmdline approach.

-- 
paul moore
www.paul-moore.com