Re: CONFIG_CHANGE record formats

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?