tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: GPIO revisited



Thanks for your feedback, my comment are inlin.

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.

There is no caching at all, kauth_authorize_device is called for every ioctl() call.

You authorize an action when you are performing that action.

That is what the code does.

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.

I will change that, then.

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...

I will look into this.

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. :)

Once we know how to handle it definitely the secmodel_securelevel(9) manpage needs to be updated, I just simply forgot about that ;)

- Marc


Thanks,

-e.



Home | Main Index | Thread Index | Old Index