[PATCH ghak120] audit: trigger accompanying records when no rules present

[PATCH ghak120] audit: trigger accompanying records when no rules present

[Richard Guy Briggs]

When there are no audit rules registered, mandatory records (config,
etc.) are missing their accompanying records (syscall, proctitle, etc.).

This is due to audit context dummy set on syscall entry based on absence
of rules that signals that no other records are to be printed.

Clear the dummy bit in auditsc_set_stamp() when the first record of an
event is generated.

Please see upstream github issue
https://github.com/linux-audit/audit-kernel/issues/120

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

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 4effe01ebbe2..31195d122344 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2176,6 +2176,8 @@ int auditsc_get_stamp(struct audit_context *ctx,
 	t->tv_sec  = ctx->ctime.tv_sec;
 	t->tv_nsec = ctx->ctime.tv_nsec;
 	*serial    = ctx->serial;
+	if (ctx->dummy)
+		ctx->dummy = 0;
 	if (!ctx->prio) {
 		ctx->prio = 1;
 		ctx->current_state = AUDIT_RECORD_CONTEXT;
-- 
1.8.3.1

[PATCH ghak120] audit: trigger accompanying records when no rules present

[Richard Guy Briggs]

When there are no audit rules registered, mandatory records (config,
etc.) are missing their accompanying records (syscall, proctitle, etc.).

This is due to audit context dummy set on syscall entry based on absence
of rules that signals that no other records are to be printed.

Clear the dummy bit in auditsc_set_stamp() when the first record of an
event is generated.

Please see upstream github issue
https://github.com/linux-audit/audit-kernel/issues/120

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

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 4effe01ebbe2..31195d122344 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2176,6 +2176,8 @@ int auditsc_get_stamp(struct audit_context *ctx,
 	t->tv_sec  = ctx->ctime.tv_sec;
 	t->tv_nsec = ctx->ctime.tv_nsec;
 	*serial    = ctx->serial;
+	if (ctx->dummy)
+		ctx->dummy = 0;
 	if (!ctx->prio) {
 		ctx->prio = 1;
 		ctx->current_state = AUDIT_RECORD_CONTEXT;
-- 
1.8.3.1

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

Re: [PATCH ghak120] audit: trigger accompanying records when no rules present

[Paul Moore]

On Tue, Feb 18, 2020 at 4:01 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> When there are no audit rules registered, mandatory records (config,
> etc.) are missing their accompanying records (syscall, proctitle, etc.).
>
> This is due to audit context dummy set on syscall entry based on absence
> of rules that signals that no other records are to be printed.
>
> Clear the dummy bit in auditsc_set_stamp() when the first record of an
> event is generated.
>
> Please see upstream github issue
> https://github.com/linux-audit/audit-kernel/issues/120
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/auditsc.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 4effe01ebbe2..31195d122344 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2176,6 +2176,8 @@ int auditsc_get_stamp(struct audit_context *ctx,
>         t->tv_sec  = ctx->ctime.tv_sec;
>         t->tv_nsec = ctx->ctime.tv_nsec;
>         *serial    = ctx->serial;
> +       if (ctx->dummy)
> +               ctx->dummy = 0;

Two comments:

* Why even bother checking to see if ctx->dummy is true?  If it is
true you set it to false/0; if it is already false you leave it alone.
Either way ctx->dummy is going to be set to false when you are past
these two lines, might as well just always set ctx->dummy to false/0.

* Why are you setting ->dummy to false in auditsc_get_stamp() and not
someplace a bit more obvious like audit_log_start()?  Is it because
auditsc_get_stamp() only gets called once per event?  I'm willing to
take the "hit" of one extra assignment in audit_log_start() to keep
this in a more obvious place and not buried in auditsc_get_stamp().

-- 
paul moore
www.paul-moore.com

Re: [PATCH ghak120] audit: trigger accompanying records when no rules present

[Paul Moore]

On Tue, Feb 18, 2020 at 4:01 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> When there are no audit rules registered, mandatory records (config,
> etc.) are missing their accompanying records (syscall, proctitle, etc.).
>
> This is due to audit context dummy set on syscall entry based on absence
> of rules that signals that no other records are to be printed.
>
> Clear the dummy bit in auditsc_set_stamp() when the first record of an
> event is generated.
>
> Please see upstream github issue
> https://github.com/linux-audit/audit-kernel/issues/120
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  kernel/auditsc.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 4effe01ebbe2..31195d122344 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2176,6 +2176,8 @@ int auditsc_get_stamp(struct audit_context *ctx,
>         t->tv_sec  = ctx->ctime.tv_sec;
>         t->tv_nsec = ctx->ctime.tv_nsec;
>         *serial    = ctx->serial;
> +       if (ctx->dummy)
> +               ctx->dummy = 0;

Two comments:

* Why even bother checking to see if ctx->dummy is true?  If it is
true you set it to false/0; if it is already false you leave it alone.
Either way ctx->dummy is going to be set to false when you are past
these two lines, might as well just always set ctx->dummy to false/0.

* Why are you setting ->dummy to false in auditsc_get_stamp() and not
someplace a bit more obvious like audit_log_start()?  Is it because
auditsc_get_stamp() only gets called once per event?  I'm willing to
take the "hit" of one extra assignment in audit_log_start() to keep
this in a more obvious place and not buried in auditsc_get_stamp().

-- 
paul moore
www.paul-moore.com


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

Re: [PATCH ghak120] audit: trigger accompanying records when no rules present

[Richard Guy Briggs]

On 2020-02-27 20:02, Paul Moore wrote:
> On Tue, Feb 18, 2020 at 4:01 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >
> > When there are no audit rules registered, mandatory records (config,
> > etc.) are missing their accompanying records (syscall, proctitle, etc.).
> >
> > This is due to audit context dummy set on syscall entry based on absence
> > of rules that signals that no other records are to be printed.
> >
> > Clear the dummy bit in auditsc_set_stamp() when the first record of an
> > event is generated.
> >
> > Please see upstream github issue
> > https://github.com/linux-audit/audit-kernel/issues/120
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  kernel/auditsc.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 4effe01ebbe2..31195d122344 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2176,6 +2176,8 @@ int auditsc_get_stamp(struct audit_context *ctx,
> >         t->tv_sec  = ctx->ctime.tv_sec;
> >         t->tv_nsec = ctx->ctime.tv_nsec;
> >         *serial    = ctx->serial;
> > +       if (ctx->dummy)
> > +               ctx->dummy = 0;
> 
> Two comments:
> 
> * Why even bother checking to see if ctx->dummy is true?  If it is
> true you set it to false/0; if it is already false you leave it alone.
> Either way ctx->dummy is going to be set to false when you are past
> these two lines, might as well just always set ctx->dummy to false/0.

Ok, no problem.

> * Why are you setting ->dummy to false in auditsc_get_stamp() and not
> someplace a bit more obvious like audit_log_start()?  Is it because
> auditsc_get_stamp() only gets called once per event?  I'm willing to
> take the "hit" of one extra assignment in audit_log_start() to keep
> this in a more obvious place and not buried in auditsc_get_stamp().

It is because the context is only available when syscall logging is
enabled (which is on most platforms and hopefully eventually all) and
makes for cleaner code and lack of need to check existance of the
context.

> paul moore

- 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

Re: [PATCH ghak120] audit: trigger accompanying records when no rules present

[Paul Moore]

On Mon, Mar 9, 2020 at 4:31 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2020-02-27 20:02, Paul Moore wrote:
> > On Tue, Feb 18, 2020 at 4:01 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > >
> > > When there are no audit rules registered, mandatory records (config,
> > > etc.) are missing their accompanying records (syscall, proctitle, etc.).
> > >
> > > This is due to audit context dummy set on syscall entry based on absence
> > > of rules that signals that no other records are to be printed.
> > >
> > > Clear the dummy bit in auditsc_set_stamp() when the first record of an
> > > event is generated.
> > >
> > > Please see upstream github issue
> > > https://github.com/linux-audit/audit-kernel/issues/120
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > >  kernel/auditsc.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index 4effe01ebbe2..31195d122344 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -2176,6 +2176,8 @@ int auditsc_get_stamp(struct audit_context *ctx,
> > >         t->tv_sec  = ctx->ctime.tv_sec;
> > >         t->tv_nsec = ctx->ctime.tv_nsec;
> > >         *serial    = ctx->serial;
> > > +       if (ctx->dummy)
> > > +               ctx->dummy = 0;
> >
> > Two comments:
> >
> > * Why even bother checking to see if ctx->dummy is true?  If it is
> > true you set it to false/0; if it is already false you leave it alone.
> > Either way ctx->dummy is going to be set to false when you are past
> > these two lines, might as well just always set ctx->dummy to false/0.
>
> Ok, no problem.
>
> > * Why are you setting ->dummy to false in auditsc_get_stamp() and not
> > someplace a bit more obvious like audit_log_start()?  Is it because
> > auditsc_get_stamp() only gets called once per event?  I'm willing to
> > take the "hit" of one extra assignment in audit_log_start() to keep
> > this in a more obvious place and not buried in auditsc_get_stamp().
>
> It is because the context is only available when syscall logging is
> enabled (which is on most platforms and hopefully eventually all) and
> makes for cleaner code and lack of need to check existance of the
> context.

At the very least let's create some sort of accessor function for
dummy then, hiding this in auditsc_get_stamp() seems very wrong to me.

-- 
paul moore
www.paul-moore.com


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

Re: [PATCH ghak120] audit: trigger accompanying records when no rules present

[Richard Guy Briggs]

On 2020-03-09 19:55, Paul Moore wrote:
> On Mon, Mar 9, 2020 at 4:31 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2020-02-27 20:02, Paul Moore wrote:
> > > On Tue, Feb 18, 2020 at 4:01 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > >
> > > > When there are no audit rules registered, mandatory records (config,
> > > > etc.) are missing their accompanying records (syscall, proctitle, etc.).
> > > >
> > > > This is due to audit context dummy set on syscall entry based on absence
> > > > of rules that signals that no other records are to be printed.
> > > >
> > > > Clear the dummy bit in auditsc_set_stamp() when the first record of an
> > > > event is generated.
> > > >
> > > > Please see upstream github issue
> > > > https://github.com/linux-audit/audit-kernel/issues/120
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > >  kernel/auditsc.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > index 4effe01ebbe2..31195d122344 100644
> > > > --- a/kernel/auditsc.c
> > > > +++ b/kernel/auditsc.c
> > > > @@ -2176,6 +2176,8 @@ int auditsc_get_stamp(struct audit_context *ctx,
> > > >         t->tv_sec  = ctx->ctime.tv_sec;
> > > >         t->tv_nsec = ctx->ctime.tv_nsec;
> > > >         *serial    = ctx->serial;
> > > > +       if (ctx->dummy)
> > > > +               ctx->dummy = 0;
> > >
> > > Two comments:
> > >
> > > * Why even bother checking to see if ctx->dummy is true?  If it is
> > > true you set it to false/0; if it is already false you leave it alone.
> > > Either way ctx->dummy is going to be set to false when you are past
> > > these two lines, might as well just always set ctx->dummy to false/0.
> >
> > Ok, no problem.
> >
> > > * Why are you setting ->dummy to false in auditsc_get_stamp() and not
> > > someplace a bit more obvious like audit_log_start()?  Is it because
> > > auditsc_get_stamp() only gets called once per event?  I'm willing to
> > > take the "hit" of one extra assignment in audit_log_start() to keep
> > > this in a more obvious place and not buried in auditsc_get_stamp().
> >
> > It is because the context is only available when syscall logging is
> > enabled (which is on most platforms and hopefully eventually all) and
> > makes for cleaner code and lack of need to check existance of the
> > context.
> 
> At the very least let's create some sort of accessor function for
> dummy then, hiding this in auditsc_get_stamp() seems very wrong to me.

Ok.  Anything else?

I also found useless context and dummy checks in audit_log_proctitle
and removed them since it can't be called if the context doesn't exist.

> paul moore

- 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

Re: [PATCH ghak120] audit: trigger accompanying records when no rules present

[Paul Moore]

On Mon, Mar 9, 2020 at 8:59 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> On 2020-03-09 19:55, Paul Moore wrote:
> > On Mon, Mar 9, 2020 at 4:31 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2020-02-27 20:02, Paul Moore wrote:
> > > > On Tue, Feb 18, 2020 at 4:01 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > >
> > > > > When there are no audit rules registered, mandatory records (config,
> > > > > etc.) are missing their accompanying records (syscall, proctitle, etc.).
> > > > >
> > > > > This is due to audit context dummy set on syscall entry based on absence
> > > > > of rules that signals that no other records are to be printed.
> > > > >
> > > > > Clear the dummy bit in auditsc_set_stamp() when the first record of an
> > > > > event is generated.
> > > > >
> > > > > Please see upstream github issue
> > > > > https://github.com/linux-audit/audit-kernel/issues/120
> > > > >
> > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > ---
> > > > >  kernel/auditsc.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > index 4effe01ebbe2..31195d122344 100644
> > > > > --- a/kernel/auditsc.c
> > > > > +++ b/kernel/auditsc.c
> > > > > @@ -2176,6 +2176,8 @@ int auditsc_get_stamp(struct audit_context *ctx,
> > > > >         t->tv_sec  = ctx->ctime.tv_sec;
> > > > >         t->tv_nsec = ctx->ctime.tv_nsec;
> > > > >         *serial    = ctx->serial;
> > > > > +       if (ctx->dummy)
> > > > > +               ctx->dummy = 0;
> > > >
> > > > Two comments:
> > > >
> > > > * Why even bother checking to see if ctx->dummy is true?  If it is
> > > > true you set it to false/0; if it is already false you leave it alone.
> > > > Either way ctx->dummy is going to be set to false when you are past
> > > > these two lines, might as well just always set ctx->dummy to false/0.
> > >
> > > Ok, no problem.
> > >
> > > > * Why are you setting ->dummy to false in auditsc_get_stamp() and not
> > > > someplace a bit more obvious like audit_log_start()?  Is it because
> > > > auditsc_get_stamp() only gets called once per event?  I'm willing to
> > > > take the "hit" of one extra assignment in audit_log_start() to keep
> > > > this in a more obvious place and not buried in auditsc_get_stamp().
> > >
> > > It is because the context is only available when syscall logging is
> > > enabled (which is on most platforms and hopefully eventually all) and
> > > makes for cleaner code and lack of need to check existance of the
> > > context.
> >
> > At the very least let's create some sort of accessor function for
> > dummy then, hiding this in auditsc_get_stamp() seems very wrong to me.
>
> Ok.  Anything else?

I'm not sure how many more words we can spill over a two line patch :)

Work up the v2, post it, and we can go from there.

-- 
paul moore
www.paul-moore.com


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