On 6/1/2016 1:38 PM, Stephen Smalley wrote:
> On 06/01/2016 04:30 PM, Casey Schaufler wrote:
>> On 6/1/2016 1:06 PM, Stephen Smalley wrote:
>>> On 06/01/2016 03:27 PM, Casey Schaufler wrote:
>>>> Subject: [PATCH] LSM: Reorder security_capset to do access checks properly
>>>>
>>>> The security module hooks that check whether a process should
>>>> be able to set a new capset are currently called after the new
>>>> values are set in cap_capset(). This change reverses the order.
>>>> The capability module no longer adds cap_capset to the module list.
>>>> Instead, it is invoked directly by the LSM infrastructure.
>>>> This isn't an approach that generalizes well.
>>> Is this change necessary? The fact that cap_capset() modifies new
>>> before the other hooks are called does no harm, because if any hook
>>> returns an error, then the caller returns that error and never commits
>>> the new cred. It is actually possibly beneficial for the other security
>>> hooks to be called after cap_capset() so that they can adjust the new
>>> values if desired (e.g. to reduce them) before they are finally committed.
>> The existing code will set the new credential values before the
>> security modules do their checks. Even if it's harmless, it's sloppy.
>> Currently there's only one caller, but with Serge's work on ns_capabilities
>> I'm looking to make this safer.
> It's intentional. cap_capset() does two things: it validates the
> proposed capability sets (a permission check, returning -EPERM on
> failure) and if valid under its own logic, it then updates new. But the
> update does not take effect until the caller of security_capset() calls
> commit_creds() and that only happens if all of the hooks pass. By
> moving cap_capset() to the end, you are reversing the order of checks
> from the norm (DAC before MAC) and you aren't allowing other security
> modules to vet and possibly reduce new further. Also, it is obvious
> from the patch below that doing so requires a massive hack to what was
> otherwise working fine for stacking.
>
> If you are going to insist on reversing the order, then I think you need
> to split security_capset into two hooks, one which only validates and
> one which sets, and only use your alternative ordering for the latter.
> But that's a lot of work for no apparent gain...
Point. I'll drop this. We'll worry about it if it ever actually
becomes an issue. Thanks for the comments.
>
>>>> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
>>>> ---
>>>> security/commoncap.c | 2 +-
>>>> security/security.c | 24 ++++++++++++++++++++++--
>>>> 2 files changed, 23 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/security/commoncap.c b/security/commoncap.c
>>>> index 48071ed..f5bce18 100644
>>>> --- a/security/commoncap.c
>>>> +++ b/security/commoncap.c
>>>> @@ -1073,7 +1073,7 @@ struct security_hook_list capability_hooks[] = {
>>>> LSM_HOOK_INIT(ptrace_access_check, cap_ptrace_access_check),
>>>> LSM_HOOK_INIT(ptrace_traceme, cap_ptrace_traceme),
>>>> LSM_HOOK_INIT(capget, cap_capget),
>>>> - LSM_HOOK_INIT(capset, cap_capset),
>>>> + /* Carefull! Do not include cap_capset! */
>>>> LSM_HOOK_INIT(bprm_set_creds, cap_bprm_set_creds),
>>>> LSM_HOOK_INIT(bprm_secureexec, cap_bprm_secureexec),
>>>> LSM_HOOK_INIT(inode_need_killpriv, cap_inode_need_killpriv),
>>>> diff --git a/security/security.c b/security/security.c
>>>> index 92cd1d8..1610be8 100644
>>>> --- a/security/security.c
>>>> +++ b/security/security.c
>>>> @@ -177,8 +177,28 @@ int security_capset(struct cred *new, const struct cred *old,
>>>> const kernel_cap_t *inheritable,
>>>> const kernel_cap_t *permitted)
>>>> {
>>>> - return call_int_hook(capset, 0, new, old,
>>>> - effective, inheritable, permitted);
>>>> + struct security_hook_list *hp;
>>>> + int rc;
>>>> +
>>>> + /*
>>>> + * Special case handling because the "new" capabilities
>>>> + * should not be set until it has been determined that
>>>> + * all modules approve of the change. Passing NULL pointers
>>>> + * to all modules except the capabilty module as it is
>>>> + * expected that only the capability modules needs the
>>>> + * result pointers.
>>>> + *
>>>> + * cap_capset() must not be in the capability module hook list!
>>>> + */
>>>> + list_for_each_entry(hp, &security_hook_heads.capset, list) {
>>>> + rc = hp->hook.capset(new, old, NULL, NULL, NULL);
>>>> + if (rc != 0)
>>>> + return rc;
>>>> + }
>>>> + /*
>>>> + * Call cap_capset now to update the new capset.
>>>> + */
>>>> + return cap_capset(new, old, effective, inheritable, permitted);
>>>> }
>>>>
>>>> int security_capable(const struct cred *cred, struct user_namespace *ns,
>>>>
>>>> _______________________________________________
>>>> Selinux mailing list
>>>> Selinux@xxxxxxxxxxxxx
>>>> To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx.
>>>> To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.
>>>>
>
_______________________________________________
Selinux mailing list
Selinux@xxxxxxxxxxxxx
To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx.
To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.
