On 12/12/2019 8: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. I endorse this approach enthusiastically. > >> 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. >
