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

[Stephen Smalley]

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.

>> 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.

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).