Re: [RFC PATCH ghak59 V1 1/6] audit: give a clue what CONFIG_CHANGE op was involved

On Thu, Jun 14, 2018 at 4:23 PM Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > The failure to add an audit rule due to audit locked gives no clue > what CONFIG_CHANGE operation failed. > Similarly the set operation is the only other operation that doesn't > give the "op=" field to indicate the action. > All other CONFIG_CHANGE records include an op= field to give a clue as > to what sort of configuration change is being executed. > > Since these are the only CONFIG_CHANGE records that that do not have an > op= field, add them to bring them in line with the rest. Normally this would be an immediate reject because this patch inserts a field into an existing record, but the CONFIG_CHANGE record is so variable (supposedly bad in its own right) that I don't this really matters. With that out of the way, I think this patch is fine, but I don't think it is complete. At the very least there is another CONFIG_CHANGE record in audit_watch_log_rule_change() that doesn't appear to include an "op" field. If we want to make sure we have an "op" field in every CONFIG_CHANGE record, let's actually add them all :)

There appears to be another one in audit_mark_log_rule_change() ...

and one more in audit_receive_msg(). There may be more.

> Old records: > type=CONFIG_CHANGE msg=audit(1519812997.781:374): pid=610 uid=0 auid=0 ses=1 subj=... audit_enabled=2 res=0 > type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : audit_enabled=1 old=1 auid=unset ses=unset subj=... res=yes > > New records: > type=CONFIG_CHANGE msg=audit(1520958477.855:100): pid=610 uid=0 auid=0 ses=1 subj=... op=add_rule audit_enabled=2 res=0 > > type=CONFIG_CHANGE msg=audit(2018-06-14 14:55:04.507:47) : op=set audit_enabled=1 old=1 auid=unset ses=unset subj=... res=yes > > See: https://github.com/linux-audit/audit-kernel/issues/59 > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; > --- > kernel/audit.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/kernel/audit.c b/kernel/audit.c > index e7478cb..ad54339 100644 > --- a/kernel/audit.c > +++ b/kernel/audit.c > @@ -403,7 +403,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old, > ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE); > if (unlikely(!ab)) > return rc; > - audit_log_format(ab, "%s=%u old=%u", function_name, new, old); > + audit_log_format(ab, "op=set %s=%u old=%u", function_name, new, old); > audit_log_session_info(ab); > rc = audit_log_task_context(ab); > if (rc) > @@ -1365,7 +1365,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) > return -EINVAL; > if (audit_enabled == AUDIT_LOCKED) { > audit_log_common_recv_msg(&ab, AUDIT_CONFIG_CHANGE); > - audit_log_format(ab, " audit_enabled=%d res=0", audit_enabled); > + audit_log_format(ab, " op=%s_rule audit_enabled=%d res=0", > + msg_type == AUDIT_ADD_RULE ? "add" : "remove", > + audit_enabled); > audit_log_end(ab); > return -EPERM; > } > -- > 1.8.3.1 -- paul moore www.paul-moore.com