LSM: Reorder security_capset to do access checks properly

Commit Message

Casey Schaufler June 1, 2016, 7:27 p.m. UTC

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.

Signed-off-by: Casey Schaufler <[e-mail]>
---
 security/commoncap.c |  2 +-
 security/security.c  | 24 ++++++++++++++++++++++--
 2 files changed, 23 insertions(+), 3 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Stephen Smalley June 1, 2016, 8:06 p.m. UTC | #1

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.

> 
> Signed-off-by: Casey Schaufler <[e-mail]>
> ---
>  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
> [e-mail]
> To unsubscribe, send email to [e-mail].
> To get help, send an email containing "help" to [e-mail].
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Casey Schaufler June 1, 2016, 8:30 p.m. UTC | #2

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.

>
>> Signed-off-by: Casey Schaufler <[e-mail]>
>> ---
>>  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
>> [e-mail]
>> To unsubscribe, send email to [e-mail].
>> To get help, send an email containing "help" to [e-mail].
>>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Smalley June 1, 2016, 8:38 p.m. UTC | #3

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

> 
>>
>>> Signed-off-by: Casey Schaufler <[e-mail]>
>>> ---
>>>  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
>>> [e-mail]
>>> To unsubscribe, send email to [e-mail].
>>> To get help, send an email containing "help" to [e-mail].
>>>
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Casey Schaufler June 1, 2016, 8:45 p.m. UTC | #4

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 <[e-mail]>
>>>> ---
>>>>  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
>>>> [e-mail]
>>>> To unsubscribe, send email to [e-mail].
>>>> To get help, send an email containing "help" to [e-mail].
>>>>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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,