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

[Richard Guy Briggs <rgb@redhat.com>]

On 14/05/29, Steve Grubb wrote:
> On Wednesday, May 28, 2014 10:43:57 PM Eric Paris 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.
> 
> So, what is the effect of this patch? Does it hide the syscall from the audit 
> system? Does it fail the syscall?

Mostly.  No.

If there is a rule to log any specific syscall on exit, it spits out a
record with a bogus (well, negative) syscall number.  This should really
be blocked too.

> > 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....
> 
> We need absolute guarantees. Either its auditable or prevented - always.

This could be done in __audit_syscall_entry() calling a new function to
log bogus syscalls.  What precise fields would you like to see logged in
this situation?

> -Steve

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545