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 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?

> paul moore

- RGB

--
Richard Guy Briggs <rgb at 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