On 12/12/19 11:03 AM, Ondrej Mosnacek wrote:
On Thu, Dec 12, 2019 at 2:14 PM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
On 12/12/19 6:49 AM, Ondrej Mosnacek wrote:
On Wed, Dec 11, 2019 at 8:12 PM Stephen Smalley <sds@xxxxxxxxxxxxx> 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).
