On Sat, Apr 11, 2009 at 9:37 PM, Christos Zoulas
<christos%zoulas.com@localhost> wrote:
> | The default result is "defer", and if the listener wants to explicitly
> | allow, it returns "allow". If, eventually, no listener returns "allow",
> | or at least one listener returns "deny", the operation is denied.
> |
> | Do you want to change this logic? (if yes, why?)
>
> Look for default: in secmodel_44_suser.c. There are a lot of DEFER's
> but there are a couple of ALLOWS and quite a few that do nothing.
> I think that all the defaults should be treated the same way or there
> should be a big fat comment explaining why this is not the case :-)
You'll notice that in every listener, the result is initially set to
DEFER, so the "empty" defaults are actually okay, and the ones with
DEFER should be (carefully) fixed. I'll do them in a different commit so
it'll be easy to follow.
I found four ALLOWs that should not be there:
- implicit ALLOW for "normal" port binding requests
(KAUTH_REQ_NETWORK_BIND_PORT),
- default of KAUTH_NETWORK_SOCKET that would never enter as all of
the possible requests were handled,
- relying on sysctl's permission check for KAUTH_SYSTEM_SETIDCORE and
KAUTH_NETWORK_FORWSRCRT.
I've fixed all of them in the new version of the diff.
Other than that, there are four more "sweeping" ALLOWs:
- KAUTH_REQ_NETWORK_FIREWALL_FW and KAUTH_REQ_NETWORK_FIREWALL_NAT:
subject to device permissions and securelevel. For now should be
left as-is.
- KAUTH_REQ_SYSTEM_DEBUG_IPKDB: originally, subject to securelevel
only. Maybe should have its own listener plugged to return ALLOW
(#ifndef IPKDBSECURE)? can we even rely on kauth(9) here,
especially given in the future the listeners might be able to sleep?
(btw, I have a feeling nobody used, let alone compiled, IPKDB in a
long time). Left as-is for now.
- KAUTH_SYSTEM_CHSYSFLAGS: only called from root context (in ufs,
ext2fs, tmpfs, and rump -- according to the comment), but I've
added a root check "just in case". Ideally, this should be done
differently, but I'm leaving it as-is for now.
Finally, I noticed I forgot to include kern/kern_sysctl.c in the diff.
While looking at it, I came across something that looks wrong: it's
unclear to me how the "custom" sysctl helpers work wrt/access control.
The following is part of kern/kern_sysctl.c:sysctl_lookup():
/*
* if a node wants to be writable according to different rules
* other than "only root can write to stuff unless a flag is
* set", then it needs its own function which should have been
* called and not us.
*/
if (l != NULL && newp != NULL &&
!(rnode->sysctl_flags & CTLFLAG_ANYWRITE) &&
(error = kauth_authorize_generic(l->l_cred,
KAUTH_GENERIC_ISSUSER, NULL)) != 0)
return (error);
None of the sysctl helpers I've seen actually check (or checked) for
permission. They all call sysctl_lookup() themselves, implicitly using
the kauth(9) call there to limit access. That's why, at the moment,
setting net.inet.ip.forwsrcrt works "as expected". I'll take a look at
addressing this some other time...
Attached is the latest version of the diff, including Andy's input.
Thanks,
-e.
Attachment:
pass1.diff
Description: Binary data