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@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
>> >> Signed-off-by: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
>> >> ---
>> >>  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/