NETFILTER_CFG record generation was inconsistent for x_tables and ebtables configuration changes. The call was needlessly messy and there were supporting records missing at times while they were produced when not requested. Simplify the logging call into a new audit_log_nfcfg call. Honour the audit_enabled setting while more consistently recording information including supporting records by tidying up dummy checks. Add an op= field that indicates the operation being performed (register or replace). Here is the enhanced sample record: type=NETFILTER_CFG msg=audit(1580905834.919:82970): table=filter family=2 entries=83 op=replace Generate audit NETFILTER_CFG records on ebtables table registration. Previously this was being done for x_tables registration and replacement operations and ebtables table replacement only. See: https://github.com/linux-audit/audit-kernel/issues/25 See: https://github.com/linux-audit/audit-kernel/issues/35 See: https://github.com/linux-audit/audit-kernel/issues/43 Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; --- include/linux/audit.h | 21 +++++++++++++++++++++ kernel/auditsc.c | 24 ++++++++++++++++++++++++ net/bridge/netfilter/ebtables.c | 12 ++++-------- net/netfilter/x_tables.c | 12 +++--------- 4 files changed, 52 insertions(+), 17 deletions(-) diff --git a/include/linux/audit.h b/include/linux/audit.h index f9ceae57ca8d..5c7f07a9776d 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -94,6 +94,11 @@ struct audit_ntp_data { struct audit_ntp_data {}; #endif +enum audit_nfcfgop { + AUDIT_XT_OP_REGISTER, + AUDIT_XT_OP_REPLACE, +}; + extern int is_audit_feature_set(int which); extern int __init audit_register_class(int class, unsigned *list); @@ -379,6 +384,8 @@ extern int __audit_log_bprm_fcaps(struct linux_binprm *bprm, extern void __audit_fanotify(unsigned int response); extern void __audit_tk_injoffset(struct timespec64 offset); extern void __audit_ntp_log(const struct audit_ntp_data *ad); +extern void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries, + enum audit_nfcfgop op); static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp) { @@ -514,6 +521,14 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad) __audit_ntp_log(ad); } +static inline void audit_log_nfcfg(const char *name, u8 af, + unsigned int nentries, + enum audit_nfcfgop op) +{ + if (audit_enabled) + __audit_log_nfcfg(name, af, nentries, op); +} + extern int audit_n_rules; extern int audit_signals; #else /* CONFIG_AUDITSYSCALL */ @@ -646,6 +661,12 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad) static inline void audit_ptrace(struct task_struct *t) { } + +static inline void audit_log_nfcfg(const char *name, u8 af, + unsigned int nentries, + enum audit_nfcfgop op) +{ } + #define audit_n_rules 0 #define audit_signals 0 #endif /* CONFIG_AUDITSYSCALL */ diff --git a/kernel/auditsc.c b/kernel/auditsc.c index 814406a35db1..705beac0ce29 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -130,6 +130,16 @@ struct audit_tree_refs { struct audit_chunk *c[31]; }; +struct audit_nfcfgop_tab { + enum audit_nfcfgop op; + const char *s; +}; + +const struct audit_nfcfgop_tab audit_nfcfgs[] = { + { AUDIT_XT_OP_REGISTER, "register" }, + { AUDIT_XT_OP_REPLACE, "replace" }, +}; + static int audit_match_perm(struct audit_context *ctx, int mask) { unsigned n; @@ -2542,6 +2552,20 @@ void __audit_ntp_log(const struct audit_ntp_data *ad) audit_log_ntp_val(ad, "adjust", AUDIT_NTP_ADJUST); } +void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries, + enum audit_nfcfgop op) +{ + struct audit_buffer *ab; + + ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_NETFILTER_CFG); + if (!ab) + return; + audit_log_format(ab, "table=%s family=%u entries=%u op=%s", + name, af, nentries, audit_nfcfgs[op].s); + audit_log_end(ab); +} +EXPORT_SYMBOL_GPL(__audit_log_nfcfg); + static void audit_log_task(struct audit_buffer *ab) { kuid_t auid, uid; diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c index 78db58c7aec2..0a148e68b6e1 100644 --- a/net/bridge/netfilter/ebtables.c +++ b/net/bridge/netfilter/ebtables.c @@ -1046,14 +1046,8 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl, vfree(table); vfree(counterstmp); -#ifdef CONFIG_AUDIT - if (audit_enabled) { - audit_log(audit_context(), GFP_KERNEL, - AUDIT_NETFILTER_CFG, - "table=%s family=%u entries=%u", - repl->name, AF_BRIDGE, repl->nentries); - } -#endif + audit_log_nfcfg(repl->name, AF_BRIDGE, repl->nentries, + AUDIT_XT_OP_REPLACE); return ret; free_unlock: @@ -1223,6 +1217,8 @@ int ebt_register_table(struct net *net, const struct ebt_table *input_table, *res = NULL; } + audit_log_nfcfg(repl->name, AF_BRIDGE, repl->nentries, + AUDIT_XT_OP_REGISTER); return ret; free_unlock: mutex_unlock(&ebt_mutex); diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c index cd2b034eef59..8f8c5dbf603d 100644 --- a/net/netfilter/x_tables.c +++ b/net/netfilter/x_tables.c @@ -1408,15 +1408,9 @@ struct xt_table_info * } } -#ifdef CONFIG_AUDIT - if (audit_enabled) { - audit_log(audit_context(), GFP_KERNEL, - AUDIT_NETFILTER_CFG, - "table=%s family=%u entries=%u", - table->name, table->af, private->number); - } -#endif - + audit_log_nfcfg(table->name, table->af, private->number, + !private->number ? AUDIT_XT_OP_REGISTER : + AUDIT_XT_OP_REPLACE); return private; } EXPORT_SYMBOL_GPL(xt_replace_table); -- 1.8.3.1

Audit the action of unregistering ebtables and x_tables. See: https://github.com/linux-audit/audit-kernel/issues/44 Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; --- include/linux/audit.h | 1 + kernel/auditsc.c | 5 +++-- net/bridge/netfilter/ebtables.c | 2 ++ net/netfilter/x_tables.c | 2 ++ 4 files changed, 8 insertions(+), 2 deletions(-) diff --git a/include/linux/audit.h b/include/linux/audit.h index 5c7f07a9776d..1a2351508211 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -97,6 +97,7 @@ struct audit_ntp_data { enum audit_nfcfgop { AUDIT_XT_OP_REGISTER, AUDIT_XT_OP_REPLACE, + AUDIT_XT_OP_UNREGISTER, }; extern int is_audit_feature_set(int which); diff --git a/kernel/auditsc.c b/kernel/auditsc.c index 705beac0ce29..d281c18d1771 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -136,8 +136,9 @@ struct audit_nfcfgop_tab { }; const struct audit_nfcfgop_tab audit_nfcfgs[] = { - { AUDIT_XT_OP_REGISTER, "register" }, - { AUDIT_XT_OP_REPLACE, "replace" }, + { AUDIT_XT_OP_REGISTER, "register" }, + { AUDIT_XT_OP_REPLACE, "replace" }, + { AUDIT_XT_OP_UNREGISTER, "unregister" }, }; static int audit_match_perm(struct audit_context *ctx, int mask) diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c index 0a148e68b6e1..4778db5601b0 100644 --- a/net/bridge/netfilter/ebtables.c +++ b/net/bridge/netfilter/ebtables.c @@ -1124,6 +1124,8 @@ static void __ebt_unregister_table(struct net *net, struct ebt_table *table) mutex_lock(&ebt_mutex); list_del(&table->list); mutex_unlock(&ebt_mutex); + audit_log_nfcfg(table->name, AF_BRIDGE, table->private->nentries, + AUDIT_XT_OP_UNREGISTER); EBT_ENTRY_ITERATE(table->private->entries, table->private->entries_size, ebt_cleanup_entry, net, NULL); if (table->private->nentries) diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c index 8f8c5dbf603d..99a468be4a59 100644 --- a/net/netfilter/x_tables.c +++ b/net/netfilter/x_tables.c @@ -1472,6 +1472,8 @@ void *xt_unregister_table(struct xt_table *table) private = table->private; list_del(&table->list); mutex_unlock(&xt[table->af].mutex); + audit_log_nfcfg(table->name, table->af, private->number, + AUDIT_XT_OP_UNREGISTER); kfree(table); return private; -- 1.8.3.1

Some table unregister actions seem to be initiated by the kernel to garbage collect unused tables that are not initiated by any userspace actions. It was found to be necessary to add the subject credentials to cover this case to reveal the source of these actions. A sample record: type=NETFILTER_CFG msg=audit(2020-03-11 21:25:21.491:269) : table=nat family=bridge entries=0 op=unregister pid=153 uid=root auid=unset tty=(none) ses=unset subj=system_u:system_r:kernel_t:s0 comm=kworker/u4:2 exe=(null) Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; --- kernel/auditsc.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/kernel/auditsc.c b/kernel/auditsc.c index d281c18d1771..d7a45b181be0 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -2557,12 +2557,30 @@ void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries, enum audit_nfcfgop op) { struct audit_buffer *ab; + const struct cred *cred; + struct tty_struct *tty; + char comm[sizeof(current->comm)]; ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_NETFILTER_CFG); if (!ab) return; audit_log_format(ab, "table=%s family=%u entries=%u op=%s", name, af, nentries, audit_nfcfgs[op].s); + + cred = current_cred(); + tty = audit_get_tty(); + audit_log_format(ab, " pid=%u uid=%u auid=%u tty=%s ses=%u", + task_pid_nr(current), + from_kuid(&init_user_ns, cred->uid), + from_kuid(&init_user_ns, audit_get_loginuid(current)), + tty ? tty_name(tty) : "(none)", + audit_get_sessionid(current)); + audit_put_tty(tty); + audit_log_task_context(ab); /* subj= */ + audit_log_format(ab, " comm="); + audit_log_untrustedstring(ab, get_task_comm(comm, current)); + audit_log_d_path_exe(ab, current->mm); /* exe= */ + audit_log_end(ab); } EXPORT_SYMBOL_GPL(__audit_log_nfcfg); -- 1.8.3.1

On 2020-04-28 18:25, Paul Moore wrote: Steve's preference for the "kernel" record convention, so deviating from this will create yet a new field list. I'll defer to Steve on this. It also has to do with the searchability of fields if they are missing. I do agree that some fields will be superfluous in the kernel case. The most important field would be "subj", but then "pid" and "comm", I would think. Based on this contents of the "subj" field, I'd think that "uid", "auid", "tty", "ses" and "exe" are not needed. If we keep auid, I'd say keep ses, since they usually go together, though they are separated by another field in this "kernel" record field ordering. I expect this orphan record to occur so infrequently that I don't think bandwidth or space are a serious concern. - RGB -- Richard Guy Briggs <rgb(a)redhat.com&gt; 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

On 2020-04-29 14:47, Steve Grubb wrote: I don't think the intent was to have fields swing in and out depending on trigger. The idea is to potentially permanently not include them in this record type only. The justification is that where they aren't needed for the kernel trigger situation it made sense to delete them because if it is a user context event it will be accompanied by a syscall record that already has that information and there would be no sense in duplicating it. - RGB -- Richard Guy Briggs <rgb(a)redhat.com&gt; 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

On 2020-04-28 18:25, Paul Moore wrote: Can you explain why auid falls under this criteria but ses does not if both are unset? If auid is unset then we know ses is unset? If subj contains *:kernel_t:* then uid can also be dropped even though it is set, no? I figure if we are going to start dropping fields, might as well drop enough to make it worth the effort, even though this is a rare event. As for searchability, I have solved that easily in the parser. - RGB -- Richard Guy Briggs <rgb(a)redhat.com&gt; 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