[PATCH v2 1/2] auditsc: audit_krule mask accesses need bounds checking

[PATCH v2 1/2] auditsc: audit_krule mask accesses need bounds checking

[Andy Lutomirski]

Fixes an easy DoS and possible information disclosure.

This does nothing about the broken state of x32 auditing.

Cc: stable@vger.kernel.org
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 kernel/auditsc.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index f251a5e..7ccd9db 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -728,6 +728,22 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
 	return AUDIT_BUILD_CONTEXT;
 }
 
+static bool audit_in_mask(const struct audit_krule *rule, unsigned long val)
+{
+	int word, bit;
+
+	if (val > 0xffffffff)
+		return false;
+
+	word = AUDIT_WORD(val);
+	if (word >= AUDIT_BITMASK_SIZE)
+		return false;
+
+	bit = AUDIT_BIT(val);
+
+	return rule->mask[word] & bit;
+}
+
 /* At syscall entry and exit time, this filter is called if the
  * audit_state is not low enough that auditing cannot take place, but is
  * also not high enough that we already know we have to write an audit
@@ -745,11 +761,8 @@ static enum audit_state audit_filter_syscall(struct task_struct *tsk,
 
 	rcu_read_lock();
 	if (!list_empty(list)) {
-		int word = AUDIT_WORD(ctx->major);
-		int bit  = AUDIT_BIT(ctx->major);
-
 		list_for_each_entry_rcu(e, list, list) {
-			if ((e->rule.mask[word] & bit) == bit &&
+			if (audit_in_mask(&e->rule, ctx->major) &&
 			    audit_filter_rules(tsk, &e->rule, ctx, NULL,
 					       &state, false)) {
 				rcu_read_unlock();
@@ -769,20 +782,16 @@ static enum audit_state audit_filter_syscall(struct task_struct *tsk,
 static int audit_filter_inode_name(struct task_struct *tsk,
 				   struct audit_names *n,
 				   struct audit_context *ctx) {
-	int word, bit;
 	int h = audit_hash_ino((u32)n->ino);
 	struct list_head *list = &audit_inode_hash[h];
 	struct audit_entry *e;
 	enum audit_state state;
 
-	word = AUDIT_WORD(ctx->major);
-	bit  = AUDIT_BIT(ctx->major);
-
 	if (list_empty(list))
 		return 0;
 
 	list_for_each_entry_rcu(e, list, list) {
-		if ((e->rule.mask[word] & bit) == bit &&
+		if (audit_in_mask(&e->rule, ctx->major) &&
 		    audit_filter_rules(tsk, &e->rule, ctx, n, &state, false)) {
 			ctx->current_state = state;
 			return 1;
-- 
1.9.3

Re: [PATCH v2 1/2] auditsc: audit_krule mask accesses need bounds checking

[Eric Paris]

On Wed, 2014-05-28 at 18:44 -0700, Andy Lutomirski wrote:
> Fixes an easy DoS and possible information disclosure.
> 
> This does nothing about the broken state of x32 auditing.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  kernel/auditsc.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index f251a5e..7ccd9db 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -728,6 +728,22 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
>  	return AUDIT_BUILD_CONTEXT;
>  }
>  
> +static bool audit_in_mask(const struct audit_krule *rule, unsigned long val)
> +{
> +	int word, bit;
> +
> +	if (val > 0xffffffff)
> +		return false;

Why is this necessary?  

> +
> +	word = AUDIT_WORD(val);
> +	if (word >= AUDIT_BITMASK_SIZE)
> +		return false;

Since this covers it and it extensible...

> +
> +	bit = AUDIT_BIT(val);
> +
> +	return rule->mask[word] & bit;

Returning an int as a bool creates worse code than just returning the
int.  (although in this case if the compiler chooses to inline it might
be smart enough not to actually convert this int to a bool and make
worse assembly...)   I'd suggest the function here return an int.  bools
usually make the assembly worse...

Otherwise I'd give it an ACK...

> +}
> +
>  /* At syscall entry and exit time, this filter is called if the
>   * audit_state is not low enough that auditing cannot take place, but is
>   * also not high enough that we already know we have to write an audit
> @@ -745,11 +761,8 @@ static enum audit_state audit_filter_syscall(struct task_struct *tsk,
>  
>  	rcu_read_lock();
>  	if (!list_empty(list)) {
> -		int word = AUDIT_WORD(ctx->major);
> -		int bit  = AUDIT_BIT(ctx->major);
> -
>  		list_for_each_entry_rcu(e, list, list) {
> -			if ((e->rule.mask[word] & bit) == bit &&
> +			if (audit_in_mask(&e->rule, ctx->major) &&
>  			    audit_filter_rules(tsk, &e->rule, ctx, NULL,
>  					       &state, false)) {
>  				rcu_read_unlock();
> @@ -769,20 +782,16 @@ static enum audit_state audit_filter_syscall(struct task_struct *tsk,
>  static int audit_filter_inode_name(struct task_struct *tsk,
>  				   struct audit_names *n,
>  				   struct audit_context *ctx) {
> -	int word, bit;
>  	int h = audit_hash_ino((u32)n->ino);
>  	struct list_head *list = &audit_inode_hash[h];
>  	struct audit_entry *e;
>  	enum audit_state state;
>  
> -	word = AUDIT_WORD(ctx->major);
> -	bit  = AUDIT_BIT(ctx->major);
> -
>  	if (list_empty(list))
>  		return 0;
>  
>  	list_for_each_entry_rcu(e, list, list) {
> -		if ((e->rule.mask[word] & bit) == bit &&
> +		if (audit_in_mask(&e->rule, ctx->major) &&
>  		    audit_filter_rules(tsk, &e->rule, ctx, n, &state, false)) {
>  			ctx->current_state = state;
>  			return 1;

Re: [PATCH v2 1/2] auditsc: audit_krule mask accesses need bounds checking

[Andy Lutomirski]

On Wed, May 28, 2014 at 7:23 PM, Eric Paris <eparis@redhat.com> wrote:
> On Wed, 2014-05-28 at 18:44 -0700, Andy Lutomirski wrote:
>> Fixes an easy DoS and possible information disclosure.
>>
>> This does nothing about the broken state of x32 auditing.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>> ---
>>  kernel/auditsc.c | 27 ++++++++++++++++++---------
>>  1 file changed, 18 insertions(+), 9 deletions(-)
>>
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index f251a5e..7ccd9db 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -728,6 +728,22 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
>>       return AUDIT_BUILD_CONTEXT;
>>  }
>>
>> +static bool audit_in_mask(const struct audit_krule *rule, unsigned long val)
>> +{
>> +     int word, bit;
>> +
>> +     if (val > 0xffffffff)
>> +             return false;
>
> Why is this necessary?

To avoid an integer overflow.  Admittedly, this particular overflow
won't cause a crash, but it will cause incorrect results.

>
>> +
>> +     word = AUDIT_WORD(val);
>> +     if (word >= AUDIT_BITMASK_SIZE)
>> +             return false;
>
> Since this covers it and it extensible...
>
>> +
>> +     bit = AUDIT_BIT(val);
>> +
>> +     return rule->mask[word] & bit;
>
> Returning an int as a bool creates worse code than just returning the
> int.  (although in this case if the compiler chooses to inline it might
> be smart enough not to actually convert this int to a bool and make
> worse assembly...)   I'd suggest the function here return an int.  bools
> usually make the assembly worse...

I'm ambivalent.  The right assembly would use flags on x86, not an int
or a bool, and I don't know what the compiler will do.

--Andy

Re: [PATCH v2 1/2] auditsc: audit_krule mask accesses need bounds checking

[Eric Paris]

On Wed, 2014-05-28 at 19:27 -0700, Andy Lutomirski wrote:
> On Wed, May 28, 2014 at 7:23 PM, Eric Paris <eparis@redhat.com> wrote:
> > On Wed, 2014-05-28 at 18:44 -0700, Andy Lutomirski wrote:
> >> Fixes an easy DoS and possible information disclosure.
> >>
> >> This does nothing about the broken state of x32 auditing.
> >>
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> >> ---
> >>  kernel/auditsc.c | 27 ++++++++++++++++++---------
> >>  1 file changed, 18 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >> index f251a5e..7ccd9db 100644
> >> --- a/kernel/auditsc.c
> >> +++ b/kernel/auditsc.c
> >> @@ -728,6 +728,22 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
> >>       return AUDIT_BUILD_CONTEXT;
> >>  }
> >>
> >> +static bool audit_in_mask(const struct audit_krule *rule, unsigned long val)
> >> +{
> >> +     int word, bit;
> >> +
> >> +     if (val > 0xffffffff)
> >> +             return false;
> >
> > Why is this necessary?
> 
> To avoid an integer overflow.  Admittedly, this particular overflow
> won't cause a crash, but it will cause incorrect results.

You know this code pre-dates git?  I admit, I'm shocked no one ever
noticed it before!  This is ANCIENT.  And clearly broken.

I'll likely ask Richard to add a WARN_ONCE() in both this place, and
below in word > AUDIT_BITMASK_SIZE so we might know if we ever need a
larger bitmask to store syscall numbers....

It'd be nice if lib had a efficient bitmask implementation...

> >
> >> +
> >> +     word = AUDIT_WORD(val);
> >> +     if (word >= AUDIT_BITMASK_SIZE)
> >> +             return false;
> >
> > Since this covers it and it extensible...
> >
> >> +
> >> +     bit = AUDIT_BIT(val);
> >> +
> >> +     return rule->mask[word] & bit;
> >
> > Returning an int as a bool creates worse code than just returning the
> > int.  (although in this case if the compiler chooses to inline it might
> > be smart enough not to actually convert this int to a bool and make
> > worse assembly...)   I'd suggest the function here return an int.  bools
> > usually make the assembly worse...
> 
> I'm ambivalent.  The right assembly would use flags on x86, not an int
> or a bool, and I don't know what the compiler will do.

Also, clearly X86_X32 was implemented without audit support, so we
shouldn't config it in.  What do you think of this?

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 25d2c6f..fa852e2 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -129,7 +129,7 @@ config X86
 	select HAVE_IRQ_EXIT_ON_IRQ_STACK if X86_64
 	select HAVE_CC_STACKPROTECTOR
 	select GENERIC_CPU_AUTOPROBE
-	select HAVE_ARCH_AUDITSYSCALL
+	select HAVE_ARCH_AUDITSYSCALL if !X86_X32
 
 config INSTRUCTION_DECODER
 	def_bool y

Re: [PATCH v2 1/2] auditsc: audit_krule mask accesses need bounds checking

[Andy Lutomirski]

On Wed, May 28, 2014 at 7:43 PM, Eric Paris <eparis@redhat.com> wrote:
> On Wed, 2014-05-28 at 19:27 -0700, Andy Lutomirski wrote:
>> On Wed, May 28, 2014 at 7:23 PM, Eric Paris <eparis@redhat.com> wrote:
>> > On Wed, 2014-05-28 at 18:44 -0700, Andy Lutomirski wrote:
>> >> Fixes an easy DoS and possible information disclosure.
>> >>
>> >> This does nothing about the broken state of x32 auditing.
>> >>
>> >> Cc: stable@vger.kernel.org
>> >> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>> >> ---
>> >>  kernel/auditsc.c | 27 ++++++++++++++++++---------
>> >>  1 file changed, 18 insertions(+), 9 deletions(-)
>> >>
>> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> >> index f251a5e..7ccd9db 100644
>> >> --- a/kernel/auditsc.c
>> >> +++ b/kernel/auditsc.c
>> >> @@ -728,6 +728,22 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
>> >>       return AUDIT_BUILD_CONTEXT;
>> >>  }
>> >>
>> >> +static bool audit_in_mask(const struct audit_krule *rule, unsigned long val)
>> >> +{
>> >> +     int word, bit;
>> >> +
>> >> +     if (val > 0xffffffff)
>> >> +             return false;
>> >
>> > Why is this necessary?
>>
>> To avoid an integer overflow.  Admittedly, this particular overflow
>> won't cause a crash, but it will cause incorrect results.
>
> You know this code pre-dates git?  I admit, I'm shocked no one ever
> noticed it before!  This is ANCIENT.  And clearly broken.
>
> I'll likely ask Richard to add a WARN_ONCE() in both this place, and
> below in word > AUDIT_BITMASK_SIZE so we might know if we ever need a
> larger bitmask to store syscall numbers....
>

Not there, please!  It would make sense to check when rules are
*added*, but any user can trivially invoke the filter with pretty much
any syscall nr.

> It'd be nice if lib had a efficient bitmask implementation...
>
>> >
>> >> +
>> >> +     word = AUDIT_WORD(val);
>> >> +     if (word >= AUDIT_BITMASK_SIZE)
>> >> +             return false;
>> >
>> > Since this covers it and it extensible...
>> >
>> >> +
>> >> +     bit = AUDIT_BIT(val);
>> >> +
>> >> +     return rule->mask[word] & bit;
>> >
>> > Returning an int as a bool creates worse code than just returning the
>> > int.  (although in this case if the compiler chooses to inline it might
>> > be smart enough not to actually convert this int to a bool and make
>> > worse assembly...)   I'd suggest the function here return an int.  bools
>> > usually make the assembly worse...
>>
>> I'm ambivalent.  The right assembly would use flags on x86, not an int
>> or a bool, and I don't know what the compiler will do.
>
> Also, clearly X86_X32 was implemented without audit support, so we
> shouldn't config it in.  What do you think of this?
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 25d2c6f..fa852e2 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -129,7 +129,7 @@ config X86
>         select HAVE_IRQ_EXIT_ON_IRQ_STACK if X86_64
>         select HAVE_CC_STACKPROTECTOR
>         select GENERIC_CPU_AUTOPROBE
> -       select HAVE_ARCH_AUDITSYSCALL
> +       select HAVE_ARCH_AUDITSYSCALL if !X86_X32

I'm fine with that, but I hope that distros will choose x32 over
auditsc, at least until someone makes enabling auditsc be less
intrusive.

Or someone could fix it, modulo the syscall nr 2048 thing.  x32
syscall nrs are *huge*.  So are lots of other architectures', a few of
which probably claim to support auditsc.

--Andy