[v7,22/28] SELinux: Verify LSM display sanity in binder
diff mbox series

Message ID 20190807194410.9762-23-casey@schaufler-ca.com
State New
Headers show
Series
  • LSM: Module stacking for AppArmor
Related show

Commit Message

Casey Schaufler Aug. 7, 2019, 7:44 p.m. UTC
Verify that the tasks on the ends of a binder transaction
use LSM display values that don't cause SELinux contexts
to be interpreted by another LSM or another LSM's context
to be interpreted by SELinux. No judgement is made in cases
that where SELinux contexts are not used in the binder
transaction.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
 security/selinux/hooks.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

Comments

Kees Cook Aug. 8, 2019, 9:55 p.m. UTC | #1
On Wed, Aug 07, 2019 at 12:44:04PM -0700, Casey Schaufler wrote:
> Verify that the tasks on the ends of a binder transaction
> use LSM display values that don't cause SELinux contexts
> to be interpreted by another LSM or another LSM's context
> to be interpreted by SELinux. No judgement is made in cases
> that where SELinux contexts are not used in the binder
> transaction.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
>  security/selinux/hooks.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 352be16a887d..fcad2e3432d2 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -2009,6 +2009,28 @@ static inline u32 open_file_to_av(struct file *file)
>  	return av;
>  }
>  
> +/*
> + * Verify that if the "display" LSM is SELinux for either task
> + * that it is for both tasks.
> + */
> +static inline bool compatible_task_displays(struct task_struct *here,
> +					    struct task_struct *there)
> +{
> +	int h = lsm_task_display(here);
> +	int t = lsm_task_display(there);
> +
> +	if (h == t)
> +		return true;
> +
> +	/* unspecified is only ok if SELinux isn't going to be involved */
> +	if (selinux_lsmid.slot == 0)
> +		return ((h == 0 && t == LSMBLOB_INVALID) ||
> +			(t == 0 && h == LSMBLOB_INVALID));

What is "0" here? Doesn't that just mean the first LSM. I though only -1
had a special meaning (and had a #define name for it).

-Kees

> +
> +	/* it's ok only if neither display is SELinux */
> +	return (h != selinux_lsmid.slot && t != selinux_lsmid.slot);
> +}
> +
>  /* Hook functions begin here. */
>  
>  static int selinux_binder_set_context_mgr(struct task_struct *mgr)
> @@ -2016,6 +2038,9 @@ static int selinux_binder_set_context_mgr(struct task_struct *mgr)
>  	u32 mysid = current_sid();
>  	u32 mgrsid = task_sid(mgr);
>  
> +	if (!compatible_task_displays(current, mgr))
> +		return -EINVAL;
> +
>  	return avc_has_perm(&selinux_state,
>  			    mysid, mgrsid, SECCLASS_BINDER,
>  			    BINDER__SET_CONTEXT_MGR, NULL);
> @@ -2029,6 +2054,9 @@ static int selinux_binder_transaction(struct task_struct *from,
>  	u32 tosid = task_sid(to);
>  	int rc;
>  
> +	if (!compatible_task_displays(from, to))
> +		return -EINVAL;
> +
>  	if (mysid != fromsid) {
>  		rc = avc_has_perm(&selinux_state,
>  				  mysid, fromsid, SECCLASS_BINDER,
> @@ -2048,6 +2076,9 @@ static int selinux_binder_transfer_binder(struct task_struct *from,
>  	u32 fromsid = task_sid(from);
>  	u32 tosid = task_sid(to);
>  
> +	if (!compatible_task_displays(from, to))
> +		return -EINVAL;
> +
>  	return avc_has_perm(&selinux_state,
>  			    fromsid, tosid, SECCLASS_BINDER, BINDER__TRANSFER,
>  			    NULL);
> @@ -2064,6 +2095,9 @@ static int selinux_binder_transfer_file(struct task_struct *from,
>  	struct common_audit_data ad;
>  	int rc;
>  
> +	if (!compatible_task_displays(from, to))
> +		return -EINVAL;
> +
>  	ad.type = LSM_AUDIT_DATA_PATH;
>  	ad.u.path = file->f_path;
>  
> -- 
> 2.20.1
>
Casey Schaufler Aug. 9, 2019, 12:56 a.m. UTC | #2
On 8/8/2019 2:55 PM, Kees Cook wrote:
> On Wed, Aug 07, 2019 at 12:44:04PM -0700, Casey Schaufler wrote:
>> Verify that the tasks on the ends of a binder transaction
>> use LSM display values that don't cause SELinux contexts
>> to be interpreted by another LSM or another LSM's context
>> to be interpreted by SELinux. No judgement is made in cases
>> that where SELinux contexts are not used in the binder
>> transaction.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>>  security/selinux/hooks.c | 34 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 34 insertions(+)
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 352be16a887d..fcad2e3432d2 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -2009,6 +2009,28 @@ static inline u32 open_file_to_av(struct file *file)
>>  	return av;
>>  }
>>  
>> +/*
>> + * Verify that if the "display" LSM is SELinux for either task
>> + * that it is for both tasks.
>> + */
>> +static inline bool compatible_task_displays(struct task_struct *here,
>> +					    struct task_struct *there)
>> +{
>> +	int h = lsm_task_display(here);
>> +	int t = lsm_task_display(there);
>> +
>> +	if (h == t)
>> +		return true;
>> +
>> +	/* unspecified is only ok if SELinux isn't going to be involved */
>> +	if (selinux_lsmid.slot == 0)
>> +		return ((h == 0 && t == LSMBLOB_INVALID) ||
>> +			(t == 0 && h == LSMBLOB_INVALID));
> What is "0" here? Doesn't that just mean the first LSM. I though only -1
> had a special meaning (and had a #define name for it).

I try not to write obscure code, but I seem to have done so here.

The lsm in slot 0 (the first registered "display" lsm) will
get used if the display value is LSMBLOB_INVALID. We've already
checked to see if the display values are the same, and they're not.

If selinux is in slot 0, one of the display values is 0 and the
other is LSMBLOB_INVALID, the displays are compatible. Otherwise,
they're not. If selinux is not in slot 0 and either of the displays
slots is selinux's slot, it's not compatible.

Simple, no?

I'll have a go at making the code more obvious or, failing
that, better documented.

>
> -Kees
>
>> +
>> +	/* it's ok only if neither display is SELinux */
>> +	return (h != selinux_lsmid.slot && t != selinux_lsmid.slot);
>> +}
>> +
>>  /* Hook functions begin here. */
>>  
>>  static int selinux_binder_set_context_mgr(struct task_struct *mgr)
>> @@ -2016,6 +2038,9 @@ static int selinux_binder_set_context_mgr(struct task_struct *mgr)
>>  	u32 mysid = current_sid();
>>  	u32 mgrsid = task_sid(mgr);
>>  
>> +	if (!compatible_task_displays(current, mgr))
>> +		return -EINVAL;
>> +
>>  	return avc_has_perm(&selinux_state,
>>  			    mysid, mgrsid, SECCLASS_BINDER,
>>  			    BINDER__SET_CONTEXT_MGR, NULL);
>> @@ -2029,6 +2054,9 @@ static int selinux_binder_transaction(struct task_struct *from,
>>  	u32 tosid = task_sid(to);
>>  	int rc;
>>  
>> +	if (!compatible_task_displays(from, to))
>> +		return -EINVAL;
>> +
>>  	if (mysid != fromsid) {
>>  		rc = avc_has_perm(&selinux_state,
>>  				  mysid, fromsid, SECCLASS_BINDER,
>> @@ -2048,6 +2076,9 @@ static int selinux_binder_transfer_binder(struct task_struct *from,
>>  	u32 fromsid = task_sid(from);
>>  	u32 tosid = task_sid(to);
>>  
>> +	if (!compatible_task_displays(from, to))
>> +		return -EINVAL;
>> +
>>  	return avc_has_perm(&selinux_state,
>>  			    fromsid, tosid, SECCLASS_BINDER, BINDER__TRANSFER,
>>  			    NULL);
>> @@ -2064,6 +2095,9 @@ static int selinux_binder_transfer_file(struct task_struct *from,
>>  	struct common_audit_data ad;
>>  	int rc;
>>  
>> +	if (!compatible_task_displays(from, to))
>> +		return -EINVAL;
>> +
>>  	ad.type = LSM_AUDIT_DATA_PATH;
>>  	ad.u.path = file->f_path;
>>  
>> -- 
>> 2.20.1
>>

Patch
diff mbox series

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 352be16a887d..fcad2e3432d2 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -2009,6 +2009,28 @@  static inline u32 open_file_to_av(struct file *file)
 	return av;
 }
 
+/*
+ * Verify that if the "display" LSM is SELinux for either task
+ * that it is for both tasks.
+ */
+static inline bool compatible_task_displays(struct task_struct *here,
+					    struct task_struct *there)
+{
+	int h = lsm_task_display(here);
+	int t = lsm_task_display(there);
+
+	if (h == t)
+		return true;
+
+	/* unspecified is only ok if SELinux isn't going to be involved */
+	if (selinux_lsmid.slot == 0)
+		return ((h == 0 && t == LSMBLOB_INVALID) ||
+			(t == 0 && h == LSMBLOB_INVALID));
+
+	/* it's ok only if neither display is SELinux */
+	return (h != selinux_lsmid.slot && t != selinux_lsmid.slot);
+}
+
 /* Hook functions begin here. */
 
 static int selinux_binder_set_context_mgr(struct task_struct *mgr)
@@ -2016,6 +2038,9 @@  static int selinux_binder_set_context_mgr(struct task_struct *mgr)
 	u32 mysid = current_sid();
 	u32 mgrsid = task_sid(mgr);
 
+	if (!compatible_task_displays(current, mgr))
+		return -EINVAL;
+
 	return avc_has_perm(&selinux_state,
 			    mysid, mgrsid, SECCLASS_BINDER,
 			    BINDER__SET_CONTEXT_MGR, NULL);
@@ -2029,6 +2054,9 @@  static int selinux_binder_transaction(struct task_struct *from,
 	u32 tosid = task_sid(to);
 	int rc;
 
+	if (!compatible_task_displays(from, to))
+		return -EINVAL;
+
 	if (mysid != fromsid) {
 		rc = avc_has_perm(&selinux_state,
 				  mysid, fromsid, SECCLASS_BINDER,
@@ -2048,6 +2076,9 @@  static int selinux_binder_transfer_binder(struct task_struct *from,
 	u32 fromsid = task_sid(from);
 	u32 tosid = task_sid(to);
 
+	if (!compatible_task_displays(from, to))
+		return -EINVAL;
+
 	return avc_has_perm(&selinux_state,
 			    fromsid, tosid, SECCLASS_BINDER, BINDER__TRANSFER,
 			    NULL);
@@ -2064,6 +2095,9 @@  static int selinux_binder_transfer_file(struct task_struct *from,
 	struct common_audit_data ad;
 	int rc;
 
+	if (!compatible_task_displays(from, to))
+		return -EINVAL;
+
 	ad.type = LSM_AUDIT_DATA_PATH;
 	ad.u.path = file->f_path;