Re: [PATCH -v3 1/3] Capabilities: move cap_file_mmap to commoncap.c

Quoting Stephen Smalley (sds@xxxxxxxxxxxxx):
On Thu, 2009-07-30 at 13:50 -0400, Eric Paris wrote:
On Thu, 2009-07-30 at 11:58 -0400, Stephen Smalley wrote:
On Thu, 2009-07-30 at 10:54 -0500, Serge E. Hallyn wrote:
Quoting Eric Paris (eparis@xxxxxxxxxx):
On Thu, 2009-07-30 at 00:14 -0500, Serge E. Hallyn wrote:
Quoting Eric Paris (eparis@xxxxxxxxxx):
Currently we duplicate the mmap_min_addr test in cap_file_mmap and in
security_file_mmap if !CONFIG_SECURITY. This patch moves cap_file_mmap
into commoncap.c and then calls that function directly from
security_file_mmap ifndef CONFIG_SECURITY like all of the other capability
checks are done.

It also

1. changes the return value in error case from -EACCES to
-EPERM
2. no onger sets PF_SUPERPRIV in t->flags if the capability
is used.

Do we care about these?

Personally, not really, but I'll gladly put them back if you care. #2
seems more interesting to me than number 1. I actually kinda like
getting EPERM from caps rather than EACCES since them I know if I was
denied by selinux or by caps.....

-Eric

Yup, I asked bc I didn't particularly care myself.

I think I agree with you about -EPERM being better anyway. However I
(now) think in this case PF_SUPERPRIV definately should be set, as this
is a clear use of a capability to do something that couldn't have been
done without it.

On a related but different note, we should consider all current uses of
cap_capable(), as they represent capability checks that will not be
subject to a further restrictive check by other security modules. In
this case and in the vm_enough_memory case, that is intentional, but not
so clear for other uses in commoncap.c.

Most of commoncap.c is called either as a secondary hook from the active
lsm (aka selinux calls the commoncap.c functions) or in the !
CONFIG_SECURITY case.

I'll audit this afternoon to see which of them might not fit these
rules....

That isn't what I meant. Most of the commoncap functions call capable()
rather than directly calling cap_capable(), thereby causing:
- PF_SUPERPRIV to be set, and
- The primary security module (e.g. SELinux) to apply its own
restrictive check.

That is useful as it allows SELinux or AppArmor or TOMOYO to veto e.g.
CAP_SYS_PTRACE without replicating the same logic within its own hook.

The current exceptions are:
cap_inh_is_capped() called from cap_capset(),
cap_task_prctl() in the PR_SET_SECUREBITS case,
cap_vm_enough_memory(),
cap_file_mmap() after your patch.

The latter two are indeed cases where we made a conscious choice that
SELinux would not apply its capability check against policy. But the
first two are unclear to me.

cap_inh_is_capped:

I'm not sure why it's cap_capable() instead of capable(). However, if we
switch to using capable(), then we should switch the conditions in the caller
around, since at the moment just because the capable() check returned true
doesn't mean that we actually end up needing it.

(CC:ing Andrew Morgan as I believe he wrote this and may have had a reason)

cap_task_prctl: I don't see any reason why that shouldn't be capable().

cap_vm_enough_memory(): I seem to recall we explicitly decided that we
did not want PF_SUPERPRIV set in this case.

cap_file_mmap(): well now that you mention it, it does seem like SELinux
would want a say in whether the task gets CAP_SYS_RAWIO here, so maybe
it should be capable() after all?

-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages