Re: CONFIG_CHANGE record formats

On Fri, Mar 30, 2018 at 5:26 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > On 2018-03-23 17:48, Paul Moore wrote: >> On Fri, Mar 23, 2018 at 4:20 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; 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