[PATCH] fix broken class-based syscall audit

[PATCH] fix broken class-based syscall audit

[Klaus Weidner]

Bug description: When I add an audit watch on a file with no arguments, I
get perm=rwxa but on ia64, changes to the mode and context aren't
audited.  I get audit records on i386 and x86_64.

See also: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=239887

The sanity check in audit_match_class() is wrong, AUDIT_BITMASK_SIZE is
64, providing space for 2048 syscalls in 64 * 32bit integers. The
comparison only supports 256 syscalls (sizeof __u32 is 4), and silently
returns "no match" for valid higher-numbered syscalls.

This breaks class-based audit for all syscalls on ia64 since on that
architecture syscall numbers start at 1024. It breaks some syscall audit
on other architectures also, for example __NR_fchmodat is 306 on x86.

I'd suggest adding a printk() in addition to returning 0 - you don't want
to silently ignore unknown or unsupported syscalls when auditing.

Signed-off-by: Klaus Weidner <klaus@atsec.com>

--- linux-2.6.18.i686/kernel/auditfilter.c.lspp.80	2007-05-11 17:06:08.000000000 -0500
+++ linux-2.6.18.i686/kernel/auditfilter.c	2007-05-11 17:09:37.000000000 -0500
@@ -306,7 +306,7 @@
 
 int audit_match_class(int class, unsigned syscall)
 {
-	if (unlikely(syscall >= AUDIT_BITMASK_SIZE * sizeof(__u32)))
+	if (unlikely(syscall >= AUDIT_BITMASK_SIZE * 32))
 		return 0;
 	if (unlikely(class >= AUDIT_SYSCALL_CLASSES || !classes[class]))
 		return 0;

Re: [PATCH] fix broken class-based syscall audit

[Marcus Meissner]

On Mon, May 14, 2007 at 10:46:36AM -0500, Klaus Weidner wrote:
> Bug description: When I add an audit watch on a file with no arguments, I
> get perm=rwxa but on ia64, changes to the mode and context aren't
> audited.  I get audit records on i386 and x86_64.
> 
> See also: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=239887
> 
> The sanity check in audit_match_class() is wrong, AUDIT_BITMASK_SIZE is
> 64, providing space for 2048 syscalls in 64 * 32bit integers. The
> comparison only supports 256 syscalls (sizeof __u32 is 4), and silently
> returns "no match" for valid higher-numbered syscalls.
> 
> This breaks class-based audit for all syscalls on ia64 since on that
> architecture syscall numbers start at 1024. It breaks some syscall audit
> on other architectures also, for example __NR_fchmodat is 306 on x86.
> 
> I'd suggest adding a printk() in addition to returning 0 - you don't want
> to silently ignore unknown or unsupported syscalls when auditing.
> 
> Signed-off-by: Klaus Weidner <klaus@atsec.com>
> 
> --- linux-2.6.18.i686/kernel/auditfilter.c.lspp.80	2007-05-11 17:06:08.000000000 -0500
> +++ linux-2.6.18.i686/kernel/auditfilter.c	2007-05-11 17:09:37.000000000 -0500
> @@ -306,7 +306,7 @@
>  
>  int audit_match_class(int class, unsigned syscall)
>  {
> -	if (unlikely(syscall >= AUDIT_BITMASK_SIZE * sizeof(__u32)))
> +	if (unlikely(syscall >= AUDIT_BITMASK_SIZE * 32))
>  		return 0;
>  	if (unlikely(class >= AUDIT_SYSCALL_CLASSES || !classes[class]))
>  		return 0;

You likely need to fix audit_register_class() if this is true.

Ciao, Marcus

Re: [PATCH] fix broken class-based syscall audit

[Klaus Weidner]

On Mon, May 14, 2007 at 05:51:50PM +0200, Marcus Meissner wrote:
> On Mon, May 14, 2007 at 10:46:36AM -0500, Klaus Weidner wrote:
> > The sanity check in audit_match_class() is wrong, AUDIT_BITMASK_SIZE is
> > 64, providing space for 2048 syscalls in 64 * 32bit integers. The
> > comparison only supports 256 syscalls (sizeof __u32 is 4), and silently
> > returns "no match" for valid higher-numbered syscalls.
[...]
> > --- linux-2.6.18.i686/kernel/auditfilter.c.lspp.80	2007-05-11 17:06:08.000000000 -0500
> > +++ linux-2.6.18.i686/kernel/auditfilter.c	2007-05-11 17:09:37.000000000 -0500
> > @@ -306,7 +306,7 @@
> >  
> >  int audit_match_class(int class, unsigned syscall)
> >  {
> > -	if (unlikely(syscall >= AUDIT_BITMASK_SIZE * sizeof(__u32)))
> > +	if (unlikely(syscall >= AUDIT_BITMASK_SIZE * 32))
> >  		return 0;
> >  	if (unlikely(class >= AUDIT_SYSCALL_CLASSES || !classes[class]))
> >  		return 0;
> 
> You likely need to fix audit_register_class() if this is true.

I don't see a problem in audit_register_class() - it correctly uses
sizeof(__u32) for allocating the memory since that's counted in bytes,
only the comparison needs to count bits.

-Klaus

Re: [PATCH] fix broken class-based syscall audit

[Eric Paris]

On Mon, 2007-05-14 at 10:56 -0500, Klaus Weidner wrote:
> On Mon, May 14, 2007 at 05:51:50PM +0200, Marcus Meissner wrote:
> > On Mon, May 14, 2007 at 10:46:36AM -0500, Klaus Weidner wrote:
> > > The sanity check in audit_match_class() is wrong, AUDIT_BITMASK_SIZE is
> > > 64, providing space for 2048 syscalls in 64 * 32bit integers. The
> > > comparison only supports 256 syscalls (sizeof __u32 is 4), and silently
> > > returns "no match" for valid higher-numbered syscalls.
> [...]
> > > --- linux-2.6.18.i686/kernel/auditfilter.c.lspp.80	2007-05-11 17:06:08.000000000 -0500
> > > +++ linux-2.6.18.i686/kernel/auditfilter.c	2007-05-11 17:09:37.000000000 -0500
> > > @@ -306,7 +306,7 @@
> > >  
> > >  int audit_match_class(int class, unsigned syscall)
> > >  {
> > > -	if (unlikely(syscall >= AUDIT_BITMASK_SIZE * sizeof(__u32)))
> > > +	if (unlikely(syscall >= AUDIT_BITMASK_SIZE * 32))
> > >  		return 0;
> > >  	if (unlikely(class >= AUDIT_SYSCALL_CLASSES || !classes[class]))
> > >  		return 0;
> > 
> > You likely need to fix audit_register_class() if this is true.
> 
> I don't see a problem in audit_register_class() - it correctly uses
> sizeof(__u32) for allocating the memory since that's counted in bytes,
> only the comparison needs to count bits.

If that's the problem wouldn't we be better to use sizeof(__u32) *8
rather than hard code the 32 in there?  (sidebar: is it portable to
assume sizeof() returns bytes and *8 is the right way to go on all
archs?)  That would make it easier to do u32 search/replace in the
future if we ever have to grow this stuff....

-Eric

Re: [PATCH] fix broken class-based syscall audit

[Klaus Weidner]

On Mon, May 14, 2007 at 04:47:29PM -0400, Eric Paris wrote:
> On Mon, 2007-05-14 at 10:56 -0500, Klaus Weidner wrote:
> > On Mon, May 14, 2007 at 05:51:50PM +0200, Marcus Meissner wrote:
> > > On Mon, May 14, 2007 at 10:46:36AM -0500, Klaus Weidner wrote:
> > > > The sanity check in audit_match_class() is wrong, AUDIT_BITMASK_SIZE is
> > > > 64, providing space for 2048 syscalls in 64 * 32bit integers. The
> > > > comparison only supports 256 syscalls (sizeof __u32 is 4), and silently
> > > > returns "no match" for valid higher-numbered syscalls.
> > [...]
> > > > --- linux-2.6.18.i686/kernel/auditfilter.c.lspp.80	2007-05-11 17:06:08.000000000 -0500
> > > > +++ linux-2.6.18.i686/kernel/auditfilter.c	2007-05-11 17:09:37.000000000 -0500
> > > > @@ -306,7 +306,7 @@
> > > >  
> > > >  int audit_match_class(int class, unsigned syscall)
> > > >  {
> > > > -	if (unlikely(syscall >= AUDIT_BITMASK_SIZE * sizeof(__u32)))
> > > > +	if (unlikely(syscall >= AUDIT_BITMASK_SIZE * 32))
> > > >  		return 0;
> > > >  	if (unlikely(class >= AUDIT_SYSCALL_CLASSES || !classes[class]))
> > > >  		return 0;
> > > 
> > > You likely need to fix audit_register_class() if this is true.
> > 
> > I don't see a problem in audit_register_class() - it correctly uses
> > sizeof(__u32) for allocating the memory since that's counted in bytes,
> > only the comparison needs to count bits.
> 
> If that's the problem wouldn't we be better to use sizeof(__u32) *8
> rather than hard code the 32 in there?  (sidebar: is it portable to
> assume sizeof() returns bytes and *8 is the right way to go on all
> archs?)  That would make it easier to do u32 search/replace in the
> future if we ever have to grow this stuff....

The AUDIT_WORD and AUDIT_BIT macros in include/linux/audit.h assume that
they'll fit 32 bits into each __u32:

#define AUDIT_WORD(nr) ((__u32)((nr)/32))
#define AUDIT_BIT(nr)  (1 << ((nr) - AUDIT_WORD(nr)*32))

(If the actual capacity of the __u32 were larger they would continue
using only 32 bits.)

I think the hardcoded 32 is appropriate. I'm not sure about the exact
sizeof() standard semantics, but I remember that the standard doesn't
assume 8-bit bytes.  However, I think it's safe to assume that a __u32
can fit 32 bits, if not a lot of other code would break also.

-Klaus

Re: [PATCH] fix broken class-based syscall audit

[Alexander Viro]

On Mon, May 14, 2007 at 04:32:22PM -0500, Klaus Weidner wrote:
> On Mon, May 14, 2007 at 04:47:29PM -0400, Eric Paris wrote:
> > On Mon, 2007-05-14 at 10:56 -0500, Klaus Weidner wrote:
> > > On Mon, May 14, 2007 at 05:51:50PM +0200, Marcus Meissner wrote:
> > > > On Mon, May 14, 2007 at 10:46:36AM -0500, Klaus Weidner wrote:
> > > > > The sanity check in audit_match_class() is wrong, AUDIT_BITMASK_SIZE is
> > > > > 64, providing space for 2048 syscalls in 64 * 32bit integers. The
> > > > > comparison only supports 256 syscalls (sizeof __u32 is 4), and silently
> > > > > returns "no match" for valid higher-numbered syscalls.
> > > [...]
> > > > > --- linux-2.6.18.i686/kernel/auditfilter.c.lspp.80	2007-05-11 17:06:08.000000000 -0500
> > > > > +++ linux-2.6.18.i686/kernel/auditfilter.c	2007-05-11 17:09:37.000000000 -0500
> > > > > @@ -306,7 +306,7 @@
> > > > >  
> > > > >  int audit_match_class(int class, unsigned syscall)
> > > > >  {
> > > > > -	if (unlikely(syscall >= AUDIT_BITMASK_SIZE * sizeof(__u32)))
> > > > > +	if (unlikely(syscall >= AUDIT_BITMASK_SIZE * 32))
> > > > >  		return 0;
> > > > >  	if (unlikely(class >= AUDIT_SYSCALL_CLASSES || !classes[class]))
> > > > >  		return 0;
> > > > 
> > > > You likely need to fix audit_register_class() if this is true.
> > > 
> > > I don't see a problem in audit_register_class() - it correctly uses
> > > sizeof(__u32) for allocating the memory since that's counted in bytes,
> > > only the comparison needs to count bits.
> > 
> > If that's the problem wouldn't we be better to use sizeof(__u32) *8
> > rather than hard code the 32 in there?  (sidebar: is it portable to
> > assume sizeof() returns bytes and *8 is the right way to go on all
> > archs?)  That would make it easier to do u32 search/replace in the
> > future if we ever have to grow this stuff....
> 
> The AUDIT_WORD and AUDIT_BIT macros in include/linux/audit.h assume that
> they'll fit 32 bits into each __u32:
> 
> #define AUDIT_WORD(nr) ((__u32)((nr)/32))
> #define AUDIT_BIT(nr)  (1 << ((nr) - AUDIT_WORD(nr)*32))
> 
> (If the actual capacity of the __u32 were larger they would continue
> using only 32 bits.)
> 
> I think the hardcoded 32 is appropriate. I'm not sure about the exact
> sizeof() standard semantics, but I remember that the standard doesn't
> assume 8-bit bytes.  However, I think it's safe to assume that a __u32
> can fit 32 bits, if not a lot of other code would break also.

Precisely.  Nice catch and an obvious fix; patch from the beginning of
this thread it can go to Linus right now.

Acked-by: Al Viro <viro@zeniv.linux.org.uk>

(to match the signoff I'm normally using on kernel submits).  Or I could
pass it to Linus myself, if you wish.