CONFIG_CHANGE record formats

[Paul Moore]

On Fri, Mar 30, 2018 at 5:26 AM, Richard Guy Briggs <rgb at 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 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().
>
> 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