CONFIG_CHANGE record formats

[Richard Guy Briggs]

On 2018-03-30 08:35, Paul Moore wrote:
> 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.

There's a seccomp audit call (audit_seccomp() vs __audit_seccomp()) from
seccomp_log() that has both that is quite intentional.  Interesting
to note that all the ones that check are non-security-tree calls while
all the ones that log regardless are from the security tree.

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

I like this idea of making it more evident.  I was even thinking
audit_log_start_forced()

> 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