[PATCH ghak25 v3 0/3] Address NETFILTER_CFG issues

[Richard Guy Briggs]

There were questions about the presence and cause of unsolicited syscall events
in the logs containing NETFILTER_CFG records and sometimes unaccompanied
NETFILTER_CFG records.

During testing at least the following list of events trigger NETFILTER_CFG
records and the syscalls related (There may be more events that will trigger
this message type.):
init_module, finit_module: modprobe
delete_module: rmmod
setsockopt: iptables-restore, ip6tables-restore, ebtables-restore
unshare: (h?)ostnamed, updatedb
clone: libvirtd
kernel threads garbage collecting empty ebtables

The syscall events unsolicited by any audit rule were found to be caused by a
missing !audit_dummy_context() check before issuing a NETFILTER_CFG
record. In fact, since this is a configuration change it is required,
and we always want the accompanying syscall record even with no rules
present, so this has been addressed by ghak120.

The vast majority of unaccompanied records are caused by the fedora default
rule: "-a never,task" and the occasional early startup one is I believe caused
by the iptables filter table module hard linked into the kernel rather than a
loadable module.

A couple of other factors should help eliminate unaccompanied records
which include commit cb74ed278f80 ("audit: always enable syscall
auditing when supported and audit is enabled") which makes sure that
when audit is enabled, so automatically is syscall auditing, and ghak66
which addressed initializing audit before PID 1.

Ebtables module initialization to register tables doesn't generate records
because it was never hooked in to audit. Recommend adding audit hooks to log
this covered by ghak43 covered by patch 1.

Table unregistration was never logged, which is now covered by ghak44 in
patch 2. Unaccompanied records were caused by kernel threads
automatically unregistering empty ebtables, which necessitates adding
subject credentials covered in patch 3.

Seemingly duplicate records are not actually exact duplicates that are caused
by netfilter table initialization in different network namespaces from the same
syscall. Recommend adding the network namespace ID (proc inode and dev)
to the record to make this obvious (address later with ghak79 after nsid
patches).

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
See: https://github.com/linux-audit/audit-kernel/issues/44

Changelog:
v3
- rebase on v5.6-rc1 audit/next
- change audit_nf_cfg to audit_log_nfcfg
- squash 2,3,4,5 to 1 and update patch descriptions
- add subject credentials to cover garbage collecting kernel threads

v2
- Rebase (audit/next 5.5-rc1) to get audit_context access and ebt_register_table ret code
- Split x_tables and ebtables updates
- Check audit_dummy_context
- Store struct audit_nfcfg params in audit_context, abstract to audit_nf_cfg() call
- Restore back to "table, family, entries" from "family, table, entries"
- Log unregistration of tables
- Add "op=" at the end of the AUDIT_NETFILTER_CFG record
- Defer nsid patch (ghak79) to once nsid patchset upstreamed (ghak32)
- Add ghak refs
- Ditch NETFILTER_CFGSOLO record

Richard Guy Briggs (3):
audit: tidy and extend netfilter_cfg x_tables and ebtables logging
netfilter: add audit table unregister actions
audit: add subj creds to NETFILTER_CFG record to cover async
unregister

include/linux/audit.h | 20 +++++++++++++++++++
kernel/auditsc.c | 43 +++++++++++++++++++++++++++++++++++++++++
net/bridge/netfilter/ebtables.c | 14 ++++++--------
net/netfilter/x_tables.c | 14 +++++---------
4 files changed, 74 insertions(+), 17 deletions(-)

[Richard Guy Briggs]

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 <***@redhat.com>
---
include/linux/audit.h | 19 +++++++++++++++++++
kernel/auditsc.c | 24 ++++++++++++++++++++++++
net/bridge/netfilter/ebtables.c | 12 ++++--------
net/netfilter/x_tables.c | 12 +++---------
4 files changed, 50 insertions(+), 17 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index f9ceae57ca8d..f4aed2b9be8d 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,13 @@ 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 +660,11 @@ 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..f4e342125dd9 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 e1256e03a9a8..55f9409c3ee0 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 e27c6c5ba9df..db5cbcf43748 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);

[Richard Guy Briggs]

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 <***@redhat.com>
---
kernel/auditsc.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index dbb056feccb9..6c233076dfb7 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);

[Richard Guy Briggs]

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 <***@redhat.com>
---
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 f4aed2b9be8d..17427c41cc29 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 f4e342125dd9..dbb056feccb9 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 55f9409c3ee0..b3a2e6ea516c 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 db5cbcf43748..e43720a7783b 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;