Re: GPIO revisited

[Elad Efrat <elad%NetBSD.org@localhost>]

Marc Balmer wrote:

> The most preeminent change from a security point of view is...
>     ... that the gpio layout can be locked by integration with the 
> kauth(9) framework, preventing layout changes at securelevel > 0 
> (securlevel being handled through the securelevel kauth module)

There are several issues with the diff in that regard. You (a) misuse
the API and (b) weaken the security of the system by doing something you 
shouldn't in the securelevel listener.

For (a), this is the relevant diff: (gpio.c)

+       cred = kauth_cred_get();
+       pinset = kauth_authorize_device(cred, KAUTH_DEVICE_GPIO_PINSET,
+           NULL, NULL, NULL, NULL) == KAUTH_RESULT_ALLOW ? 1 : 0;

This ignores the way kauth(9) is used in NetBSD and treats it like it 
was suser(). I only recently had to refactor some portions of the
network stacks to remove such abomination of authorizing an action,
caching the result, and using it in the future -- and your diff adds
just that, which is unacceptable.

You authorize an action when you are performing that action.

Aside from that, you are for some reason testing the return value of
kauth_authorize_device() against KAUTH_RESULT_ALLOW. The latter is
internal, and the authorization wrappers, as the documentation suggests,
can return either 0 or EPERM: (see kauth(9)

    An authorization request can return one of two possible values. Zero
    indicates success -- the operation is allowed; EPERM (see errno(2))
    indicates failure -- the operation is denied.

For (b), this is the relevant diff: (secmodel_securelevel.c)

+       case KAUTH_DEVICE_GPIO_PINSET:
+               if (securelevel > 0)
+                       result = KAUTH_RESULT_DENY;
+               else
+                       result = KAUTH_RESULT_ALLOW;
+               break;

The securelevel secmodel should not (and at the moment, does not) allow
any operation. All it does is deny operations under certain
securelevels, but it should not, under any circumstance, allow an
operation to take place explicitly. A quick grep will verify that this
is the current state of things.

What I suggest at the moment is do what we do for other such cases (for
example, the bind & firewall/NAT code), and add an "always allow" + a
comment in secmodel_bsd44_suser.c. This is not the best solution, but
until I get a chance to implement one it'll have to do...

Beyond that, I'd appreciate if you added the implications of securelevel
wrt/GPIO to secmodel_securelevel(9), to keep it in sync with the code. I
would also suggest to reword the GPIO documentation to suggest that
securelevel is not always present -- perhaps something along the lines
of "if the securelevel secmodel is present, then ..." -- but that's up
to you, really. :)

Thanks,

-e.