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

[Steve Grubb <sgrubb@redhat.com>]

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?


> 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.

-Steve