[RFC,ghak21,1/4] audit: make ANOM_LINK obey audit_enabled and audit_dummy_context
diff mbox

Message ID 3a9542b261d93bc4eaecfaf359affbba152cf965.1518603831.git.rgb@redhat.com
State New
Headers show

Commit Message

Richard Guy Briggs Feb. 14, 2018, 4:18 p.m. UTC
Audit link denied events emit disjointed records when audit is disabled.
No records should be emitted when audit is disabled.

See: https://github.com/linux-audit/audit-kernel/issues/21
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/audit.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Kees Cook Feb. 14, 2018, 5:51 p.m. UTC | #1
On Wed, Feb 14, 2018 at 8:18 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Audit link denied events emit disjointed records when audit is disabled.
> No records should be emitted when audit is disabled.
>
> See: https://github.com/linux-audit/audit-kernel/issues/21
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/audit.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 227db99..4c3fd24 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2261,6 +2261,9 @@ void audit_log_link_denied(const char *operation, const struct path *link)
>         struct audit_buffer *ab;
>         struct audit_names *name;
>
> +       if (!audit_enabled || audit_dummy_context())
> +               return;
> +
>         name = kzalloc(sizeof(*name), GFP_NOFS);
>         if (!name)
>                 return;

Doesn't this means errors here would be silent if audit isn't enabled?
I don't that; sysadmins should see this notification regardless of the
audit state...

-Kees
Richard Guy Briggs Feb. 15, 2018, 2:33 a.m. UTC | #2
On 2018-02-14 09:51, Kees Cook wrote:
> On Wed, Feb 14, 2018 at 8:18 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > Audit link denied events emit disjointed records when audit is disabled.
> > No records should be emitted when audit is disabled.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/21
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  kernel/audit.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 227db99..4c3fd24 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -2261,6 +2261,9 @@ void audit_log_link_denied(const char *operation, const struct path *link)
> >         struct audit_buffer *ab;
> >         struct audit_names *name;
> >
> > +       if (!audit_enabled || audit_dummy_context())
> > +               return;
> > +
> >         name = kzalloc(sizeof(*name), GFP_NOFS);
> >         if (!name)
> >                 return;
> 
> Doesn't this means errors here would be silent if audit isn't enabled?
> I don't that; sysadmins should see this notification regardless of the
> audit state...

This is a user error and not a system error, so I would think if system
auditing is disabled, they don't care about this kind of error.

Steve?

> -Kees

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Kees Cook Feb. 15, 2018, 6:16 a.m. UTC | #3
On Wed, Feb 14, 2018 at 6:33 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-02-14 09:51, Kees Cook wrote:
>> On Wed, Feb 14, 2018 at 8:18 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > Audit link denied events emit disjointed records when audit is disabled.
>> > No records should be emitted when audit is disabled.
>> >
>> > See: https://github.com/linux-audit/audit-kernel/issues/21
>> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>> > ---
>> >  kernel/audit.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/kernel/audit.c b/kernel/audit.c
>> > index 227db99..4c3fd24 100644
>> > --- a/kernel/audit.c
>> > +++ b/kernel/audit.c
>> > @@ -2261,6 +2261,9 @@ void audit_log_link_denied(const char *operation, const struct path *link)
>> >         struct audit_buffer *ab;
>> >         struct audit_names *name;
>> >
>> > +       if (!audit_enabled || audit_dummy_context())
>> > +               return;
>> > +
>> >         name = kzalloc(sizeof(*name), GFP_NOFS);
>> >         if (!name)
>> >                 return;
>>
>> Doesn't this means errors here would be silent if audit isn't enabled?
>> I don't that; sysadmins should see this notification regardless of the
>> audit state...
>
> This is a user error and not a system error, so I would think if system
> auditing is disabled, they don't care about this kind of error.

It could indicate an attack attempt...

-Kees

>
> Steve?
>
>> -Kees
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635
Paul Moore Feb. 15, 2018, 10:51 p.m. UTC | #4
On Thu, Feb 15, 2018 at 1:16 AM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Feb 14, 2018 at 6:33 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> On 2018-02-14 09:51, Kees Cook wrote:
>>> On Wed, Feb 14, 2018 at 8:18 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>>> > Audit link denied events emit disjointed records when audit is disabled.
>>> > No records should be emitted when audit is disabled.
>>> >
>>> > See: https://github.com/linux-audit/audit-kernel/issues/21
>>> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>>> > ---
>>> >  kernel/audit.c | 3 +++
>>> >  1 file changed, 3 insertions(+)
>>> >
>>> > diff --git a/kernel/audit.c b/kernel/audit.c
>>> > index 227db99..4c3fd24 100644
>>> > --- a/kernel/audit.c
>>> > +++ b/kernel/audit.c
>>> > @@ -2261,6 +2261,9 @@ void audit_log_link_denied(const char *operation, const struct path *link)
>>> >         struct audit_buffer *ab;
>>> >         struct audit_names *name;
>>> >
>>> > +       if (!audit_enabled || audit_dummy_context())
>>> > +               return;
>>> > +
>>> >         name = kzalloc(sizeof(*name), GFP_NOFS);
>>> >         if (!name)
>>> >                 return;
>>>
>>> Doesn't this means errors here would be silent if audit isn't enabled?
>>> I don't that; sysadmins should see this notification regardless of the
>>> audit state...
>>
>> This is a user error and not a system error, so I would think if system
>> auditing is disabled, they don't care about this kind of error.
>
> It could indicate an attack attempt...

We get beat up by several folks when we emit audit records with audit
disabled, and they have a very valid point.

I'm not arguing that the information isn't useful, I'm arguing that if
you are interested in the sort of information that audit provides you
should enable audit.  :)
Paul Moore March 9, 2018, 12:23 a.m. UTC | #5
On Thu, Feb 15, 2018 at 5:51 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Feb 15, 2018 at 1:16 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Wed, Feb 14, 2018 at 6:33 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
>>> On 2018-02-14 09:51, Kees Cook wrote:
>>>> On Wed, Feb 14, 2018 at 8:18 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>>>> > Audit link denied events emit disjointed records when audit is disabled.
>>>> > No records should be emitted when audit is disabled.
>>>> >
>>>> > See: https://github.com/linux-audit/audit-kernel/issues/21
>>>> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>>>> > ---
>>>> >  kernel/audit.c | 3 +++
>>>> >  1 file changed, 3 insertions(+)
>>>> >
>>>> > diff --git a/kernel/audit.c b/kernel/audit.c
>>>> > index 227db99..4c3fd24 100644
>>>> > --- a/kernel/audit.c
>>>> > +++ b/kernel/audit.c
>>>> > @@ -2261,6 +2261,9 @@ void audit_log_link_denied(const char *operation, const struct path *link)
>>>> >         struct audit_buffer *ab;
>>>> >         struct audit_names *name;
>>>> >
>>>> > +       if (!audit_enabled || audit_dummy_context())
>>>> > +               return;
>>>> > +
>>>> >         name = kzalloc(sizeof(*name), GFP_NOFS);
>>>> >         if (!name)
>>>> >                 return;
>>>>
>>>> Doesn't this means errors here would be silent if audit isn't enabled?
>>>> I don't that; sysadmins should see this notification regardless of the
>>>> audit state...
>>>
>>> This is a user error and not a system error, so I would think if system
>>> auditing is disabled, they don't care about this kind of error.
>>
>> It could indicate an attack attempt...
>
> We get beat up by several folks when we emit audit records with audit
> disabled, and they have a very valid point.
>
> I'm not arguing that the information isn't useful, I'm arguing that if
> you are interested in the sort of information that audit provides you
> should enable audit.  :)

FYI, merged into audit/next.

Patch
diff mbox

diff --git a/kernel/audit.c b/kernel/audit.c
index 227db99..4c3fd24 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -2261,6 +2261,9 @@  void audit_log_link_denied(const char *operation, const struct path *link)
 	struct audit_buffer *ab;
 	struct audit_names *name;
 
+	if (!audit_enabled || audit_dummy_context())
+		return;
+
 	name = kzalloc(sizeof(*name), GFP_NOFS);
 	if (!name)
 		return;