audit: catch errors from audit_filter_rules field checks
diff mbox

Message ID 6c97e8eec7aee6bade82dd0c7933908098846a90.1465934130.git.rgb@redhat.com
State New
Headers show

Commit Message

Richard Guy Briggs June 14, 2016, 9:03 p.m. UTC
In the case of an error returned from a field check in an audit filter
syscall rule, it is treated as a match and the rule action is honoured.

This could cause a rule with a default of NEVER and an selinux field
check error to avoid logging.

Recommend matching with an action of ALWAYS to catch malicious abuse of
this bug.  The downside of this approach is it could DoS the audit
subsystem.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/auditsc.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

Comments

Paul Moore June 16, 2016, 9:07 p.m. UTC | #1
On Tue, Jun 14, 2016 at 5:03 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> In the case of an error returned from a field check in an audit filter
> syscall rule, it is treated as a match and the rule action is honoured.
>
> This could cause a rule with a default of NEVER and an selinux field
> check error to avoid logging.
>
> Recommend matching with an action of ALWAYS to catch malicious abuse of
> this bug.  The downside of this approach is it could DoS the audit
> subsystem.

I understand your concern about the DoS, but in reality it is no worse
than if no audit filter rules were configured, yes?

> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/auditsc.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 71e14d8..6123672 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -683,6 +683,10 @@ static int audit_filter_rules(struct task_struct *tsk,
>                 }
>                 if (!result)
>                         return 0;
> +               if (result < 0) {
> +                       *state = AUDIT_RECORD_CONTEXT;
> +                       return 1;
> +               }
>         }
>
>         if (ctx) {
Paul Moore June 27, 2016, 7:58 p.m. UTC | #2
On Thu, Jun 16, 2016 at 5:07 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jun 14, 2016 at 5:03 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> In the case of an error returned from a field check in an audit filter
>> syscall rule, it is treated as a match and the rule action is honoured.
>>
>> This could cause a rule with a default of NEVER and an selinux field
>> check error to avoid logging.
>>
>> Recommend matching with an action of ALWAYS to catch malicious abuse of
>> this bug.  The downside of this approach is it could DoS the audit
>> subsystem.
>
> I understand your concern about the DoS, but in reality it is no worse
> than if no audit filter rules were configured, yes?

Just following up on this since I don't recall seeing a response ...

>> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> ---
>>  kernel/auditsc.c |    4 ++++
>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index 71e14d8..6123672 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -683,6 +683,10 @@ static int audit_filter_rules(struct task_struct *tsk,
>>                 }
>>                 if (!result)
>>                         return 0;
>> +               if (result < 0) {
>> +                       *state = AUDIT_RECORD_CONTEXT;
>> +                       return 1;
>> +               }
>>         }
>>
>>         if (ctx) {
>
> --
> paul moore
> www.paul-moore.com
Richard Guy Briggs June 28, 2016, 5:29 p.m. UTC | #3
On 2016-06-16 17:07, Paul Moore wrote:
> On Tue, Jun 14, 2016 at 5:03 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > In the case of an error returned from a field check in an audit filter
> > syscall rule, it is treated as a match and the rule action is honoured.
> >
> > This could cause a rule with a default of NEVER and an selinux field
> > check error to avoid logging.
> >
> > Recommend matching with an action of ALWAYS to catch malicious abuse of
> > this bug.  The downside of this approach is it could DoS the audit
> > subsystem.
> 
> I understand your concern about the DoS, but in reality it is no worse
> than if no audit filter rules were configured, yes?

Are you thinking of audit_filter_type which has now been merged with
audit_filter_user?

This is audit_filter_rules, which is used by syscalls with a much
broader choice of selectors.

If there are no rules set, there are no messages recorded other than
AVCs.  If a rule was configured and an error occurred in one of the
SELinux checks, it would match and not report.  I'd argue it should fail
safe and report.

> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  kernel/auditsc.c |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> >
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 71e14d8..6123672 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -683,6 +683,10 @@ static int audit_filter_rules(struct task_struct *tsk,
> >                 }
> >                 if (!result)
> >                         return 0;
> > +               if (result < 0) {
> > +                       *state = AUDIT_RECORD_CONTEXT;
> > +                       return 1;
> > +               }
> >         }
> >
> >         if (ctx) {
> 
> -- 
> paul moore
> www.paul-moore.com

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Patch
diff mbox

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 71e14d8..6123672 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -683,6 +683,10 @@  static int audit_filter_rules(struct task_struct *tsk,
 		}
 		if (!result)
 			return 0;
+		if (result < 0) {
+			*state = AUDIT_RECORD_CONTEXT;
+			return 1;
+		}
 	}
 
 	if (ctx) {