[PATCH] selinux: conditional expression type validation was off-by-one

[PATCH] selinux: conditional expression type validation was off-by-one

[Vesa-Matti Kari]

expr_isvalid() in conditional.c was off-by-one and allowed
invalid expression type COND_LAST. However, it is this header file
that needs to be fixed. That way the if-statement's disjunction's
second component reads more naturally, "if expr type is greater than
the last allowed value" ( rather than using ">=" in conditional.c):

  if (expr->expr_type <= 0 || expr->expr_type > COND_LAST)

Signed-off-by: Vesa-Matti Kari <vmkari@cc.helsinki.fi>
---
 security/selinux/ss/conditional.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
index 65b9f83..53ddb01 100644
--- a/security/selinux/ss/conditional.h
+++ b/security/selinux/ss/conditional.h
@@ -28,7 +28,7 @@ struct cond_expr {
 #define COND_XOR	5 /* bool ^ bool */
 #define COND_EQ		6 /* bool == bool */
 #define COND_NEQ	7 /* bool != bool */
-#define COND_LAST	8
+#define COND_LAST	COND_NEQ
 	__u32 expr_type;
 	__u32 bool;
 	struct cond_expr *next;
-- 
1.5.4.1


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

[PATCH] selinux: conditional expression type validation was off-by-one

[Vesa-Matti Kari]

expr_isvalid() in conditional.c was off-by-one and allowed
invalid expression type COND_LAST. However, it is this header file
that needs to be fixed. That way the if-statement's disjunction's
second component reads more naturally, "if expr type is greater than
the last allowed value" ( rather than using ">=" in conditional.c):

  if (expr->expr_type <= 0 || expr->expr_type > COND_LAST)

Signed-off-by: Vesa-Matti Kari <vmkari@cc.helsinki.fi>
---
 security/selinux/ss/conditional.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
index 65b9f83..53ddb01 100644
--- a/security/selinux/ss/conditional.h
+++ b/security/selinux/ss/conditional.h
@@ -28,7 +28,7 @@ struct cond_expr {
 #define COND_XOR	5 /* bool ^ bool */
 #define COND_EQ		6 /* bool == bool */
 #define COND_NEQ	7 /* bool != bool */
-#define COND_LAST	8
+#define COND_LAST	COND_NEQ
 	__u32 expr_type;
 	__u32 bool;
 	struct cond_expr *next;
-- 
1.5.4.1

Re: [PATCH] selinux: conditional expression type validation was off-by-one

[James Morris]

On Wed, 6 Aug 2008, Vesa-Matti Kari wrote:

> expr_isvalid() in conditional.c was off-by-one and allowed
> invalid expression type COND_LAST. However, it is this header file
> that needs to be fixed. That way the if-statement's disjunction's
> second component reads more naturally, "if expr type is greater than
> the last allowed value" ( rather than using ">=" in conditional.c):
> 
>   if (expr->expr_type <= 0 || expr->expr_type > COND_LAST)
> 
> Signed-off-by: Vesa-Matti Kari <vmkari@cc.helsinki.fi>

Thanks, nice catch.

Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next



- James
-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: conditional expression type validation was off-by-one

[James Morris]

On Wed, 6 Aug 2008, Vesa-Matti Kari wrote:

> expr_isvalid() in conditional.c was off-by-one and allowed
> invalid expression type COND_LAST. However, it is this header file
> that needs to be fixed. That way the if-statement's disjunction's
> second component reads more naturally, "if expr type is greater than
> the last allowed value" ( rather than using ">=" in conditional.c):
> 
>   if (expr->expr_type <= 0 || expr->expr_type > COND_LAST)
> 
> Signed-off-by: Vesa-Matti Kari <vmkari@cc.helsinki.fi>

Thanks, nice catch.

Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next



- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] selinux: conditional expression type validation was off-by-one

[Vesa-Matti J Kari]

Hello,

On Thu, 7 Aug 2008, James Morris wrote:

> On Wed, 6 Aug 2008, Vesa-Matti Kari wrote:
>
> > expr_isvalid() in conditional.c was off-by-one and allowed
> > invalid expression type COND_LAST. However, it is this header file
> > that needs to be fixed. That way the if-statement's disjunction's
> > second component reads more naturally, "if expr type is greater than
> > the last allowed value" ( rather than using ">=" in conditional.c):
> >
> >   if (expr->expr_type <= 0 || expr->expr_type > COND_LAST)
> >
> > Signed-off-by: Vesa-Matti Kari <vmkari@cc.helsinki.fi>
>
> Thanks, nice catch.
>
> Applied to
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next

I also checked out the userspace and the same bug exists in libsepol.

Best regards,
vmk
-- 
************************************************************************
               Tietotekniikkaosasto / Helsingin yliopisto
                 IT Department / University of Helsinki
************************************************************************

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: conditional expression type validation was off-by-one

[Vesa-Matti J Kari]

Hello,

On Thu, 7 Aug 2008, James Morris wrote:

> On Wed, 6 Aug 2008, Vesa-Matti Kari wrote:
>
> > expr_isvalid() in conditional.c was off-by-one and allowed
> > invalid expression type COND_LAST. However, it is this header file
> > that needs to be fixed. That way the if-statement's disjunction's
> > second component reads more naturally, "if expr type is greater than
> > the last allowed value" ( rather than using ">=" in conditional.c):
> >
> >   if (expr->expr_type <= 0 || expr->expr_type > COND_LAST)
> >
> > Signed-off-by: Vesa-Matti Kari <vmkari@cc.helsinki.fi>
>
> Thanks, nice catch.
>
> Applied to
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next

I also checked out the userspace and the same bug exists in libsepol.

Best regards,
vmk
-- 
************************************************************************
               Tietotekniikkaosasto / Helsingin yliopisto
                 IT Department / University of Helsinki
************************************************************************

Re: [PATCH] selinux: conditional expression type validation was off-by-one

[Vesa-Matti J Kari]

This is the same off-by-one bug that was already fixed in the kernel.
(According to my understanding neither of these bugs has security
implications)

Signed-off-by: Vesa-Matti Kari <vmkari@cc.helsinki.fi>
---

 include/sepol/policydb/conditional.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Index: libsepol/include/sepol/policydb/conditional.h
===================================================================
--- libsepol/include/sepol/policydb/conditional.h	(revision 2950)
+++ libsepol/include/sepol/policydb/conditional.h	(working copy)
@@ -48,7 +48,7 @@
 #define COND_XOR	5	/* bool ^ bool */
 #define COND_EQ		6	/* bool == bool */
 #define COND_NEQ	7	/* bool != bool */
-#define COND_LAST	8
+#define COND_LAST	COND_NEQ
 	uint32_t expr_type;
 	uint32_t bool;
 	struct cond_expr *next;

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH] selinux: conditional expression type validation was off-by-one

[Joshua Brindle]

Vesa-Matti J Kari wrote:
> This is the same off-by-one bug that was already fixed in the kernel.
> (According to my understanding neither of these bugs has security
> implications)
> 
> Signed-off-by: Vesa-Matti Kari <vmkari@cc.helsinki.fi>
> ---
> 
>  include/sepol/policydb/conditional.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> Index: libsepol/include/sepol/policydb/conditional.h
> ===================================================================
> --- libsepol/include/sepol/policydb/conditional.h	(revision 2950)
> +++ libsepol/include/sepol/policydb/conditional.h	(working copy)
> @@ -48,7 +48,7 @@
>  #define COND_XOR	5	/* bool ^ bool */
>  #define COND_EQ		6	/* bool == bool */
>  #define COND_NEQ	7	/* bool != bool */
> -#define COND_LAST	8
> +#define COND_LAST	COND_NEQ
>  	uint32_t expr_type;
>  	uint32_t bool;
>  	struct cond_expr *next;
> 

Acked-by: Joshua Brindle <method@manicmethod.com>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.