CONFIG_CHANGE record formats

CONFIG_CHANGE record formats

[Richard Guy Briggs]

Hi Steve, Paul,

Looking at some AUDIT_CONFIG_CHANGE record formats, a couple of things
stand out as potential problems:

For ADD_RULE and DEL_RULE case when audit_enabled is in the AUDIT_LOCKED
state, it just outputs "audit_enabled=2 res=0" to indicate locked and
failure, but doesn't appear to actually give the normal "op=<mumble>" to
indicate a rule change was attempted and refused due to immutability of
the rule set.  Will this be a problem for the parser, and should an
attempted rule change be logged as such?

The other is AUDIT_TTY_SET that has non-standard old-* and new-* fields,
but since there are two, I think it is unavoidable and can't be fixed.

Another is that other than a change to the enabled status and maybe
auditd PID changes, every other config change should not be logged if
audit is disabled.  Furthermore, if CONFFIG_CHANGE records are to be
accompanied by syscall records, they should obey audit_dummy_context()
to avoid unaccompanied records.  Does this reasoning make sense?


- 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

Re: CONFIG_CHANGE record formats

[Richard Guy Briggs]

On 2018-03-22 04:42, Richard Guy Briggs wrote:
> Hi Steve, Paul,
> 
> Looking at some AUDIT_CONFIG_CHANGE record formats, a couple of things
> stand out as potential problems:
> 
> For ADD_RULE and DEL_RULE case when audit_enabled is in the AUDIT_LOCKED
> state, it just outputs "audit_enabled=2 res=0" to indicate locked and
> failure, but doesn't appear to actually give the normal "op=<mumble>" to
> indicate a rule change was attempted and refused due to immutability of
> the rule set.  Will this be a problem for the parser, and should an
> attempted rule change be logged as such?
> 
> The other is AUDIT_TTY_SET that has non-standard old-* and new-* fields,
> but since there are two, I think it is unavoidable and can't be fixed.
> 
> Another is that other than a change to the enabled status and maybe
> auditd PID changes, every other config change should not be logged if
> audit is disabled.  Furthermore, if CONFIG_CHANGE records are to be
> accompanied by syscall records, they should obey audit_dummy_context()
> to avoid unaccompanied records.  Does this reasoning make sense?

I'll backtrack on audit_dummy_context since config changes should not
depend on syscall existing rules, and I can't really think of rules that
make sense to catch them.  Stick with the audit_enabled check.

> - RGB

- 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

Re: CONFIG_CHANGE record formats

[Paul Moore]

On Fri, Mar 23, 2018 at 4:20 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-03-22 04:42, Richard Guy Briggs wrote:
>> Hi Steve, Paul,
>>
>> Looking at some AUDIT_CONFIG_CHANGE record formats, a couple of things
>> stand out as potential problems:
>>
>> For ADD_RULE and DEL_RULE case when audit_enabled is in the AUDIT_LOCKED
>> state, it just outputs "audit_enabled=2 res=0" to indicate locked and
>> failure, but doesn't appear to actually give the normal "op=<mumble>" to
>> indicate a rule change was attempted and refused due to immutability of
>> the rule set.  Will this be a problem for the parser, and should an
>> attempted rule change be logged as such?

Since this falls squarely on the audit devs, I would really hope we're
doing the right thing here.  If not, we've only got ourselves, or
actually probably our predecessors, to blame ... and I think most of
them have moved on from audit.  One wonders why ...

Looking quickly at the AUDIT_CONFIG_CHANGE records in kernel/audit.c,
it looks like there is no well defined, single record format so I
think we are probably okay from Steve's point of view.  If not, we've
likely been broken for a long enough time that it isn't really so much
broken as it is "the way it's done".

>> The other is AUDIT_TTY_SET that has non-standard old-* and new-* fields,
>> but since there are two, I think it is unavoidable and can't be fixed.

Similar to the above.  That code has been that way since at least
2014, maybe longer.

>> Another is that other than a change to the enabled status and maybe
>> auditd PID changes, every other config change should not be logged if
>> audit is disabled.

At this point I'm beginning to think we just need to add a quick
audit_enabled check near the top of audit_log_start() and returns NULL
if audit is disabled.  It's clearly not as efficient as adding an
explicit check to the caller, but it should catch all of these
miscellaneous little events that are not checking the enabled/disabled
status.

If we want to add checks to the callers properly we could always add a
WARN, but only for development purposes.

However, as you point out we may need to bypass that check for things
like status and audit PID changes, but we could always have the public
audit_log_start() function and a private __audit_log_start()
accessible only within kernel/audit.c.  The private function would
actually be what audit_log_start() looks like now, and the new public
function would be an audit_enabled() check followed by a call to
__audit_log_start().

-- 
paul moore
www.paul-moore.com

Re: CONFIG_CHANGE record formats

[Richard Guy Briggs]

On 2018-03-23 17:48, Paul Moore wrote:
> On Fri, Mar 23, 2018 at 4:20 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-03-22 04:42, Richard Guy Briggs wrote:
> >> Hi Steve, Paul,
> >>
> >> Looking at some AUDIT_CONFIG_CHANGE record formats, a couple of things
> >> stand out as potential problems:
> >>
> >> For ADD_RULE and DEL_RULE case when audit_enabled is in the AUDIT_LOCKED
> >> state, it just outputs "audit_enabled=2 res=0" to indicate locked and
> >> failure, but doesn't appear to actually give the normal "op=<mumble>" to
> >> indicate a rule change was attempted and refused due to immutability of
> >> the rule set.  Will this be a problem for the parser, and should an
> >> attempted rule change be logged as such?
> 
> Since this falls squarely on the audit devs, I would really hope we're
> doing the right thing here.  If not, we've only got ourselves, or
> actually probably our predecessors, to blame ... and I think most of
> them have moved on from audit.  One wonders why ...
> 
> Looking quickly at the AUDIT_CONFIG_CHANGE records in kernel/audit.c,
> it looks like there is no well defined, single record format so I
> think we are probably okay from Steve's point of view.  If not, we've
> likely been broken for a long enough time that it isn't really so much
> broken as it is "the way it's done".
> 
> >> The other is AUDIT_TTY_SET that has non-standard old-* and new-* fields,
> >> but since there are two, I think it is unavoidable and can't be fixed.
> 
> Similar to the above.  That code has been that way since at least
> 2014, maybe longer.
> 
> >> Another is that other than a change to the enabled status and maybe
> >> auditd PID changes, every other config change should not be logged if
> >> audit is disabled.
> 
> At this point I'm beginning to think we just need to add a quick
> audit_enabled check near the top of audit_log_start() and returns NULL
> if audit is disabled.  It's clearly not as efficient as adding an
> explicit check to the caller, but it should catch all of these
> miscellaneous little events that are not checking the enabled/disabled
> status.
> 
> If we want to add checks to the callers properly we could always add a
> WARN, but only for development purposes.
> 
> However, as you point out we may need to bypass that check for things
> like status and audit PID changes, but we could always have the public
> audit_log_start() function and a private __audit_log_start()
> accessible only within kernel/audit.c.  The private function would
> actually be what audit_log_start() looks like now, and the new public
> function would be an audit_enabled() check followed by a call to
> __audit_log_start().

The following usage not available to the private function is already protected
by audit_enable:
- drivers/tty/tty_audit.c:        ab = audit_log_start(context, GFP_KERNEL, AUDIT_TTY);
- include/net/xfrm.h:        audit_buf = audit_log_start(current->audit_context, GFP_ATOMIC,
- net/bridge/netfilter/ebtables.c:                audit_log(current->audit_context, GFP_KERNEL,
- net/core/dev.c                        audit_log(current->audit_context, GFP_ATOMIC,
- net/netfilter/x_tables.c                audit_log(current->audit_context, GFP_KERNEL,
- net/netfilter/xt_AUDIT.c:       ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
- net/netlabel/netlabel_user.c:   audit_buf = audit_log_start(current->audit_context, GFP_ATOMIC, type);

But these are not, so adding an audit_enable check to audit_log_start() is
going to change their behaviour:
- security/integrity/ima/ima_api.c:       ab = audit_log_start(current->audit_context, GFP_KERNEL,
- security/integrity/ima/ima_policy.c:    ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_RULE);
- security/integrity/integrity_audit.c:   ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno);
- security/lsm_audit.c:   ab = audit_log_start(current->audit_context, GFP_ATOMIC | __GFP_NOWARN,
  - aa_audit_msg() which calls common_lsm_audit()
  - slow_avc_audit()
  - smack_log()
- security/selinux/hooks.c:                       ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
- security/selinux/hooks.c:                               ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
- security/selinux/selinuxfs.c:           audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_STAT
- security/selinux/selinuxfs.c:           audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_STAT
- security/selinux/selinuxfs.c:   audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
- security/selinux/ss/services.c: ab = audit_log_start(current->audit_context,
- security/selinux/ss/services.c: audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR,
- security/selinux/ss/services.c:                 audit_log(current->audit_context,
- security/selinux/ss/services.c: audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR,
- security/selinux/ss/services.c:                 audit_log(current->audit_context, GFP_ATOMIC,
- security/selinux/ss/services.c:                         audit_log(current->audit_context,

There is already an exception for USER AVC messages.  Which other messages
are important enough to ignore audit_enabled?

> 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

Re: CONFIG_CHANGE record formats

[Paul Moore]

On Fri, Mar 30, 2018 at 5:26 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-03-23 17:48, Paul Moore wrote:
>> On Fri, Mar 23, 2018 at 4:20 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
>> > On 2018-03-22 04:42, Richard Guy Briggs wrote:
>> >> Hi Steve, Paul,
>> >>
>> >> Looking at some AUDIT_CONFIG_CHANGE record formats, a couple of things
>> >> stand out as potential problems:
>> >>
>> >> For ADD_RULE and DEL_RULE case when audit_enabled is in the AUDIT_LOCKED
>> >> state, it just outputs "audit_enabled=2 res=0" to indicate locked and
>> >> failure, but doesn't appear to actually give the normal "op=<mumble>" to
>> >> indicate a rule change was attempted and refused due to immutability of
>> >> the rule set.  Will this be a problem for the parser, and should an
>> >> attempted rule change be logged as such?
>>
>> Since this falls squarely on the audit devs, I would really hope we're
>> doing the right thing here.  If not, we've only got ourselves, or
>> actually probably our predecessors, to blame ... and I think most of
>> them have moved on from audit.  One wonders why ...
>>
>> Looking quickly at the AUDIT_CONFIG_CHANGE records in kernel/audit.c,
>> it looks like there is no well defined, single record format so I
>> think we are probably okay from Steve's point of view.  If not, we've
>> likely been broken for a long enough time that it isn't really so much
>> broken as it is "the way it's done".
>>
>> >> The other is AUDIT_TTY_SET that has non-standard old-* and new-* fields,
>> >> but since there are two, I think it is unavoidable and can't be fixed.
>>
>> Similar to the above.  That code has been that way since at least
>> 2014, maybe longer.
>>
>> >> Another is that other than a change to the enabled status and maybe
>> >> auditd PID changes, every other config change should not be logged if
>> >> audit is disabled.
>>
>> At this point I'm beginning to think we just need to add a quick
>> audit_enabled check near the top of audit_log_start() and returns NULL
>> if audit is disabled.  It's clearly not as efficient as adding an
>> explicit check to the caller, but it should catch all of these
>> miscellaneous little events that are not checking the enabled/disabled
>> status.
>>
>> If we want to add checks to the callers properly we could always add a
>> WARN, but only for development purposes.
>>
>> However, as you point out we may need to bypass that check for things
>> like status and audit PID changes, but we could always have the public
>> audit_log_start() function and a private __audit_log_start()
>> accessible only within kernel/audit.c.  The private function would
>> actually be what audit_log_start() looks like now, and the new public
>> function would be an audit_enabled() check followed by a call to
>> __audit_log_start().
>
> The following usage not available to the private function is already protected
> by audit_enable:
> - drivers/tty/tty_audit.c:        ab = audit_log_start(context, GFP_KERNEL, AUDIT_TTY);
> - include/net/xfrm.h:        audit_buf = audit_log_start(current->audit_context, GFP_ATOMIC,
> - net/bridge/netfilter/ebtables.c:                audit_log(current->audit_context, GFP_KERNEL,
> - net/core/dev.c                        audit_log(current->audit_context, GFP_ATOMIC,
> - net/netfilter/x_tables.c                audit_log(current->audit_context, GFP_KERNEL,
> - net/netfilter/xt_AUDIT.c:       ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> - net/netlabel/netlabel_user.c:   audit_buf = audit_log_start(current->audit_context, GFP_ATOMIC, type);
>
> But these are not, so adding an audit_enable check to audit_log_start() is
> going to change their behaviour:
> - security/integrity/ima/ima_api.c:       ab = audit_log_start(current->audit_context, GFP_KERNEL,
> - security/integrity/ima/ima_policy.c:    ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_RULE);
> - security/integrity/integrity_audit.c:   ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno);
> - security/lsm_audit.c:   ab = audit_log_start(current->audit_context, GFP_ATOMIC | __GFP_NOWARN,
>   - aa_audit_msg() which calls common_lsm_audit()
>   - slow_avc_audit()
>   - smack_log()
> - security/selinux/hooks.c:                       ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
> - security/selinux/hooks.c:                               ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
> - security/selinux/selinuxfs.c:           audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_STAT
> - security/selinux/selinuxfs.c:           audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_STAT
> - security/selinux/selinuxfs.c:   audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
> - security/selinux/ss/services.c: ab = audit_log_start(current->audit_context,
> - security/selinux/ss/services.c: audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR,
> - security/selinux/ss/services.c:                 audit_log(current->audit_context,
> - security/selinux/ss/services.c: audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR,
> - security/selinux/ss/services.c:                 audit_log(current->audit_context, GFP_ATOMIC,
> - security/selinux/ss/services.c:                         audit_log(current->audit_context,
>
> There is already an exception for USER AVC messages.  Which other messages
> are important enough to ignore audit_enabled?

Based on comments we've gotten from users on this list over the years,
I tend to think that we really should squelch all audit records if
audit is disabled.  However, I do understand your point that there are
likely some callers which rely on audit records always being printed,
at least to the ring buffer if not the audit log.

Perhaps a solution is to make this behavior explicit.  Building on the
idea of adding an audit_enabled check into audit_log_start(), perhaps
we also introduce an audit_log_always()/audit_log_start_always() for
those callers that must emit records, regardless of the admin's
audit_enabled setting?  At the very least this would help us
instrument the kernel code so we would have a clear understanding on
who requires this behavior, and who is blindly ignoring the
audit_enabled state.

-- 
paul moore
www.paul-moore.com

Re: CONFIG_CHANGE record formats

[Richard Guy Briggs]

On 2018-03-30 08:35, Paul Moore wrote:
> On Fri, Mar 30, 2018 at 5:26 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-03-23 17:48, Paul Moore wrote:
> >> On Fri, Mar 23, 2018 at 4:20 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> >> > On 2018-03-22 04:42, Richard Guy Briggs wrote:
> >> >> Hi Steve, Paul,
> >> >>
> >> >> Looking at some AUDIT_CONFIG_CHANGE record formats, a couple of things
> >> >> stand out as potential problems:
> >> >>
> >> >> For ADD_RULE and DEL_RULE case when audit_enabled is in the AUDIT_LOCKED
> >> >> state, it just outputs "audit_enabled=2 res=0" to indicate locked and
> >> >> failure, but doesn't appear to actually give the normal "op=<mumble>" to
> >> >> indicate a rule change was attempted and refused due to immutability of
> >> >> the rule set.  Will this be a problem for the parser, and should an
> >> >> attempted rule change be logged as such?
> >>
> >> Since this falls squarely on the audit devs, I would really hope we're
> >> doing the right thing here.  If not, we've only got ourselves, or
> >> actually probably our predecessors, to blame ... and I think most of
> >> them have moved on from audit.  One wonders why ...
> >>
> >> Looking quickly at the AUDIT_CONFIG_CHANGE records in kernel/audit.c,
> >> it looks like there is no well defined, single record format so I
> >> think we are probably okay from Steve's point of view.  If not, we've
> >> likely been broken for a long enough time that it isn't really so much
> >> broken as it is "the way it's done".
> >>
> >> >> The other is AUDIT_TTY_SET that has non-standard old-* and new-* fields,
> >> >> but since there are two, I think it is unavoidable and can't be fixed.
> >>
> >> Similar to the above.  That code has been that way since at least
> >> 2014, maybe longer.
> >>
> >> >> Another is that other than a change to the enabled status and maybe
> >> >> auditd PID changes, every other config change should not be logged if
> >> >> audit is disabled.
> >>
> >> At this point I'm beginning to think we just need to add a quick
> >> audit_enabled check near the top of audit_log_start() and returns NULL
> >> if audit is disabled.  It's clearly not as efficient as adding an
> >> explicit check to the caller, but it should catch all of these
> >> miscellaneous little events that are not checking the enabled/disabled
> >> status.
> >>
> >> If we want to add checks to the callers properly we could always add a
> >> WARN, but only for development purposes.
> >>
> >> However, as you point out we may need to bypass that check for things
> >> like status and audit PID changes, but we could always have the public
> >> audit_log_start() function and a private __audit_log_start()
> >> accessible only within kernel/audit.c.  The private function would
> >> actually be what audit_log_start() looks like now, and the new public
> >> function would be an audit_enabled() check followed by a call to
> >> __audit_log_start().
> >
> > The following usage not available to the private function is already protected
> > by audit_enable:
> > - drivers/tty/tty_audit.c:        ab = audit_log_start(context, GFP_KERNEL, AUDIT_TTY);
> > - include/net/xfrm.h:        audit_buf = audit_log_start(current->audit_context, GFP_ATOMIC,
> > - net/bridge/netfilter/ebtables.c:                audit_log(current->audit_context, GFP_KERNEL,
> > - net/core/dev.c                        audit_log(current->audit_context, GFP_ATOMIC,
> > - net/netfilter/x_tables.c                audit_log(current->audit_context, GFP_KERNEL,
> > - net/netfilter/xt_AUDIT.c:       ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> > - net/netlabel/netlabel_user.c:   audit_buf = audit_log_start(current->audit_context, GFP_ATOMIC, type);
> >
> > But these are not, so adding an audit_enable check to audit_log_start() is
> > going to change their behaviour:
> > - security/integrity/ima/ima_api.c:       ab = audit_log_start(current->audit_context, GFP_KERNEL,
> > - security/integrity/ima/ima_policy.c:    ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_INTEGRITY_RULE);
> > - security/integrity/integrity_audit.c:   ab = audit_log_start(current->audit_context, GFP_KERNEL, audit_msgno);
> > - security/lsm_audit.c:   ab = audit_log_start(current->audit_context, GFP_ATOMIC | __GFP_NOWARN,
> >   - aa_audit_msg() which calls common_lsm_audit()
> >   - slow_avc_audit()
> >   - smack_log()
> > - security/selinux/hooks.c:                       ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
> > - security/selinux/hooks.c:                               ab = audit_log_start(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR);
> > - security/selinux/selinuxfs.c:           audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_STAT
> > - security/selinux/selinuxfs.c:           audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_STAT
> > - security/selinux/selinuxfs.c:   audit_log(current->audit_context, GFP_KERNEL, AUDIT_MAC_POLICY_LOAD,
> > - security/selinux/ss/services.c: ab = audit_log_start(current->audit_context,
> > - security/selinux/ss/services.c: audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR,
> > - security/selinux/ss/services.c:                 audit_log(current->audit_context,
> > - security/selinux/ss/services.c: audit_log(current->audit_context, GFP_ATOMIC, AUDIT_SELINUX_ERR,
> > - security/selinux/ss/services.c:                 audit_log(current->audit_context, GFP_ATOMIC,
> > - security/selinux/ss/services.c:                         audit_log(current->audit_context,
> >
> > There is already an exception for USER AVC messages.  Which other messages
> > are important enough to ignore audit_enabled?
> 
> Based on comments we've gotten from users on this list over the years,
> I tend to think that we really should squelch all audit records if
> audit is disabled.  However, I do understand your point that there are
> likely some callers which rely on audit records always being printed,
> at least to the ring buffer if not the audit log.

There's a seccomp audit call (audit_seccomp() vs __audit_seccomp()) from
seccomp_log() that has both that is quite intentional.  Interesting
to note that all the ones that check are non-security-tree calls while
all the ones that log regardless are from the security tree.

> Perhaps a solution is to make this behavior explicit.  Building on the
> idea of adding an audit_enabled check into audit_log_start(), perhaps
> we also introduce an audit_log_always()/audit_log_start_always() for
> those callers that must emit records, regardless of the admin's
> audit_enabled setting?  At the very least this would help us
> instrument the kernel code so we would have a clear understanding on
> who requires this behavior, and who is blindly ignoring the
> audit_enabled state.

I like this idea of making it more evident.  I was even thinking
audit_log_start_forced()

> 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

Re: CONFIG_CHANGE record formats

[Steve Grubb]

On Thursday, March 22, 2018 4:42:46 AM EDT Richard Guy Briggs wrote:
> Hi Steve, Paul,
> 
> Looking at some AUDIT_CONFIG_CHANGE record formats, a couple of things
> stand out as potential problems:
> 
> For ADD_RULE and DEL_RULE case when audit_enabled is in the AUDIT_LOCKED
> state, it just outputs "audit_enabled=2 res=0" to indicate locked and
> failure, but doesn't appear to actually give the normal "op=<mumble>" to
> indicate a rule change was attempted and refused due to immutability of
> the rule set.  Will this be a problem for the parser, and should an
> attempted rule change be logged as such?

If its the only rule change event that does not have an op= field, then make 
it have one.


> The other is AUDIT_TTY_SET that has non-standard old-* and new-* fields,
> but since there are two, I think it is unavoidable and can't be fixed.

We actually have to have old and new values for any configuration change that 
is not a rule add/delete. For example, if we enable audit, we need the old 
and new values. This can be expressed either as old- new-  or old- and the 
item without a new prefix.
 
> Another is that other than a change to the enabled status and maybe
> auditd PID changes, every other config change should not be logged if
> audit is disabled.

True.

> Furthermore, if CONFIG_CHANGE records are to be accompanied by syscall
> records, they should obey audit_dummy_context() to avoid unaccompanied
> records.  Does this reasoning make sense?

CONFIG_CHANGE records are simple events and not compound events. They should 
contain all the pertinent information in their one record.

-Steve