Date: Wed, 28 Dec 2016 17:36:04 +0000 From: co...@sdf.org On Wed, Dec 28, 2016 at 12:05:58AM +0000, Roy Marples wrote: > Can you please explain how the security model was broken?
intention with securelevel is to do less things kernel-side if it is raised (which, I hope, reduces our attack surface). It's true that moving the kauth call expanded the attack surface a little bit. Now we have to worry about: 1. Unprivileged users causing kernconfig_lock to be called via module load. This doesn't seem likely to be a problem -- and this is almost certainly not the only path by which unprivileged users can cause kernconfig_lock to be called. 2. Consequences of changing the lock order between kernconfig_lock and anything inside kauth. But kernconfig lock is recursive, so it seems unlikely that there are any adverse consequences to this, even if kauth could -- ill-advisedly, perhaps -- autoload security model modules. 3. Unprivileged users causing module_lookup to be called. Maybe if there are a lot of modules this is a problem, but we could replace the linear module_list by a balanced tree. There's a call to strcmp in module_lookup -- but both parameters are guaranteed NUL-terminated. (If the kernel's stored module names are not NUL-terminated, we have other problems. handle_modctl_load uses copyinstr to get the pathname parameter, which guarantees NUL termination.) I don't see any attack vectors that this enables, or anything else that needs to be audited as a consequence of roy's change. But feel free to point out anything I missed. I don't think it's worth adding this complexity for better npfctl warnings (it's just a warning and doesn't change its behaviour). If you want, I can modify npfctl not to warn for the EPERM case. I'm not sure whether that is better. Warnings are an important part of human/computer interaction. They have consequences, even if they don't change the immediate computational effects. In this case, we need to distinguish EPERM from EEXIST -- or, we need to distinguish `the module is already loaded' from `the module is not and cannot be loaded'. If you would like to propose an alternative way for npf to distinguish these cases, I suggest you first sketch a way or give a patch to do that; then we can weigh the merits of the two alternatives as a constructive process. In any case, please talk with core before reverting any commit that doesn't obviously have immediate serious consequences.
