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. -- Ondrej Mosnacek <omosnace at redhat dot com> Software Engineer, Security Technologies Red Hat, Inc.
