diff mbox

selinux: fix bug in conditional rules handling

Message ID 1448312861-3574-1-git-send-email-sds@tycho.nsa.gov
State Accepted
Headers show

Commit Message

Stephen Smalley Nov. 23, 2015, 9:07 p.m. UTC
commit fa1aa143ac4a ("selinux: extended permissions for ioctls") introduced
a bug into the handling of conditional rules, skipping the processing
entirely when the caller does not provide an extended permissions (xperms)
structure.  Access checks from userspace using /sys/fs/selinux/access
do not include such a structure since that interface does not presently
expose extended permission information.  As a result, conditional rules
were being ignored entirely on userspace access requests, producing
denials when access was allowed by conditional rules in the policy.
Fix the bug by only skipping computation of extended permissions
in this situation, not the entire conditional rules processing.

Reported-by: Laurent Bigonville <bigon@debian.org>
Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
---
 security/selinux/ss/conditional.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Paul Moore Nov. 23, 2015, 9:23 p.m. UTC | #1
On Mon, Nov 23, 2015 at 4:07 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> commit fa1aa143ac4a ("selinux: extended permissions for ioctls") introduced
> a bug into the handling of conditional rules, skipping the processing
> entirely when the caller does not provide an extended permissions (xperms)
> structure.  Access checks from userspace using /sys/fs/selinux/access
> do not include such a structure since that interface does not presently
> expose extended permission information.  As a result, conditional rules
> were being ignored entirely on userspace access requests, producing
> denials when access was allowed by conditional rules in the policy.
> Fix the bug by only skipping computation of extended permissions
> in this situation, not the entire conditional rules processing.
>
> Reported-by: Laurent Bigonville <bigon@debian.org>
> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
>  security/selinux/ss/conditional.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Looks fine to me, thanks for the fix guys.  I'm going to mark this for
stable unless I hear a strong objection from anyone.

> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index 18643bf..456e1a9 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -638,7 +638,7 @@ void cond_compute_av(struct avtab *ctab, struct avtab_key *key,
>  {
>         struct avtab_node *node;
>
> -       if (!ctab || !key || !avd || !xperms)
> +       if (!ctab || !key || !avd)
>                 return;
>
>         for (node = avtab_search_node(ctab, key); node;
> @@ -657,7 +657,7 @@ void cond_compute_av(struct avtab *ctab, struct avtab_key *key,
>                 if ((u16)(AVTAB_AUDITALLOW|AVTAB_ENABLED) ==
>                     (node->key.specified & (AVTAB_AUDITALLOW|AVTAB_ENABLED)))
>                         avd->auditallow |= node->datum.u.data;
> -               if ((node->key.specified & AVTAB_ENABLED) &&
> +               if (xperms && (node->key.specified & AVTAB_ENABLED) &&
>                                 (node->key.specified & AVTAB_XPERMS))
>                         services_compute_xperms_drivers(xperms, node);
>         }
> --
> 2.4.3
>
Stephen Smalley Nov. 23, 2015, 9:30 p.m. UTC | #2
On 11/23/2015 04:23 PM, Paul Moore wrote:
> On Mon, Nov 23, 2015 at 4:07 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
>> commit fa1aa143ac4a ("selinux: extended permissions for ioctls") introduced
>> a bug into the handling of conditional rules, skipping the processing
>> entirely when the caller does not provide an extended permissions (xperms)
>> structure.  Access checks from userspace using /sys/fs/selinux/access
>> do not include such a structure since that interface does not presently
>> expose extended permission information.  As a result, conditional rules
>> were being ignored entirely on userspace access requests, producing
>> denials when access was allowed by conditional rules in the policy.
>> Fix the bug by only skipping computation of extended permissions
>> in this situation, not the entire conditional rules processing.
>>
>> Reported-by: Laurent Bigonville <bigon@debian.org>
>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
>> ---
>>   security/selinux/ss/conditional.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> Looks fine to me, thanks for the fix guys.  I'm going to mark this for
> stable unless I hear a strong objection from anyone.

Yes, definitely should go to stable.

>
>> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
>> index 18643bf..456e1a9 100644
>> --- a/security/selinux/ss/conditional.c
>> +++ b/security/selinux/ss/conditional.c
>> @@ -638,7 +638,7 @@ void cond_compute_av(struct avtab *ctab, struct avtab_key *key,
>>   {
>>          struct avtab_node *node;
>>
>> -       if (!ctab || !key || !avd || !xperms)
>> +       if (!ctab || !key || !avd)
>>                  return;
>>
>>          for (node = avtab_search_node(ctab, key); node;
>> @@ -657,7 +657,7 @@ void cond_compute_av(struct avtab *ctab, struct avtab_key *key,
>>                  if ((u16)(AVTAB_AUDITALLOW|AVTAB_ENABLED) ==
>>                      (node->key.specified & (AVTAB_AUDITALLOW|AVTAB_ENABLED)))
>>                          avd->auditallow |= node->datum.u.data;
>> -               if ((node->key.specified & AVTAB_ENABLED) &&
>> +               if (xperms && (node->key.specified & AVTAB_ENABLED) &&
>>                                  (node->key.specified & AVTAB_XPERMS))
>>                          services_compute_xperms_drivers(xperms, node);
>>          }
>> --
>> 2.4.3
>>
>
>
>
Paul Moore Nov. 24, 2015, 6:51 p.m. UTC | #3
On Monday, November 23, 2015 04:07:41 PM Stephen Smalley wrote:
> commit fa1aa143ac4a ("selinux: extended permissions for ioctls") introduced
> a bug into the handling of conditional rules, skipping the processing
> entirely when the caller does not provide an extended permissions (xperms)
> structure.  Access checks from userspace using /sys/fs/selinux/access
> do not include such a structure since that interface does not presently
> expose extended permission information.  As a result, conditional rules
> were being ignored entirely on userspace access requests, producing
> denials when access was allowed by conditional rules in the policy.
> Fix the bug by only skipping computation of extended permissions
> in this situation, not the entire conditional rules processing.
> 
> Reported-by: Laurent Bigonville <bigon@debian.org>
> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
>  security/selinux/ss/conditional.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Merged into the SELinux stable branch, I'll push it to James tomorrow.

> diff --git a/security/selinux/ss/conditional.c
> b/security/selinux/ss/conditional.c index 18643bf..456e1a9 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -638,7 +638,7 @@ void cond_compute_av(struct avtab *ctab, struct
> avtab_key *key, {
>  	struct avtab_node *node;
> 
> -	if (!ctab || !key || !avd || !xperms)
> +	if (!ctab || !key || !avd)
>  		return;
> 
>  	for (node = avtab_search_node(ctab, key); node;
> @@ -657,7 +657,7 @@ void cond_compute_av(struct avtab *ctab, struct
> avtab_key *key, if ((u16)(AVTAB_AUDITALLOW|AVTAB_ENABLED) ==
>  		    (node->key.specified & (AVTAB_AUDITALLOW|AVTAB_ENABLED)))
>  			avd->auditallow |= node->datum.u.data;
> -		if ((node->key.specified & AVTAB_ENABLED) &&
> +		if (xperms && (node->key.specified & AVTAB_ENABLED) &&
>  				(node->key.specified & AVTAB_XPERMS))
>  			services_compute_xperms_drivers(xperms, node);
>  	}
diff mbox

Patch

diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index 18643bf..456e1a9 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -638,7 +638,7 @@  void cond_compute_av(struct avtab *ctab, struct avtab_key *key,
 {
 	struct avtab_node *node;
 
-	if (!ctab || !key || !avd || !xperms)
+	if (!ctab || !key || !avd)
 		return;
 
 	for (node = avtab_search_node(ctab, key); node;
@@ -657,7 +657,7 @@  void cond_compute_av(struct avtab *ctab, struct avtab_key *key,
 		if ((u16)(AVTAB_AUDITALLOW|AVTAB_ENABLED) ==
 		    (node->key.specified & (AVTAB_AUDITALLOW|AVTAB_ENABLED)))
 			avd->auditallow |= node->datum.u.data;
-		if ((node->key.specified & AVTAB_ENABLED) &&
+		if (xperms && (node->key.specified & AVTAB_ENABLED) &&
 				(node->key.specified & AVTAB_XPERMS))
 			services_compute_xperms_drivers(xperms, node);
 	}