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

[Stephen Smalley]

On 12/12/19 11:03 AM, Ondrej Mosnacek wrote:
> On Thu, Dec 12, 2019 at 2:14 PM 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:
>>>>>>>> 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.

And maybe we should rename selinux_enabled to selinux_enabled_boot for 
clarity and to match selinux_enforcing_boot (similarly renamed earlier).