2008-08-06 16:02:56

by Vesa-Matti J Kari

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

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 <[email protected]>
---
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

2008-08-07 00:04:52

by James Morris

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

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 <[email protected]>

Thanks, nice catch.

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



- James
--
James Morris
<[email protected]>

2008-08-08 10:42:53

by Vesa-Matti J Kari

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

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 <[email protected]>
>
> 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
************************************************************************