CONFIG_CHANGE record formats

[Paul Moore]

On Fri, Mar 23, 2018 at 4:20 AM, Richard Guy Briggs <rgb at 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