Re: CVS commit: src/sys/kern

[Taylor R Campbell <campbell+netbsd-source-changes-d%mumble.net@localhost>]

   Date: Wed, 28 Dec 2016 17:36:04 +0000
   From: coypu%SDF.ORG@localhost

   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.