Re: GPIO revisited

[Marc Balmer <marc%msys.ch@localhost>]

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.