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
setsockopt: iptables-restore, ip6tables-restore, ebtables-restore
unshare: (h?)ostnamed
clone: libvirtd
The syscall events unsolicited by any audit rule were found to be caused by a
missing !audit_dummy_context() check before creating a NETFILTER_CFG
record and issuing the record immediately rather than saving the
information to create the record at syscall exit.
Check !audit_dummy_context() before creating the NETFILTER_CFG record.
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. The !audit_dummy_context() check above should avoid them.
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.
Table unregistration was never logged, which is now covered.
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:
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 (9):
netfilter: normalize x_table function declarations
netfilter: normalize ebtables function declarations
netfilter: normalize ebtables function declarations II
audit: record nfcfg params
netfilter: x_tables audit only on syscall rule
netfilter: ebtables audit only on syscall rule
netfilter: ebtables audit table registration
netfilter: add audit operation field
netfilter: audit table unregister actions
include/linux/audit.h | 11 ++++
kernel/auditsc.c | 18 +++++
net/bridge/netfilter/ebtables.c | 142 ++++++++++++++++++++--------------------
net/netfilter/x_tables.c | 56 +++++++---------
4 files changed, 124 insertions(+), 103 deletions(-)
--
1.8.3.1
Align all function declaration parameters with open parenthesis.
Signed-off-by: Richard Guy Briggs <[email protected]>
---
net/bridge/netfilter/ebtables.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index c9dff9e11ddb..b3c784ae33a0 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1248,9 +1248,9 @@ void ebt_unregister_table(struct net *net, struct ebt_table *table,
/* userspace just supplied us with counters */
static int do_update_counters(struct net *net, const char *name,
- struct ebt_counter __user *counters,
- unsigned int num_counters,
- const void __user *user, unsigned int len)
+ struct ebt_counter __user *counters,
+ unsigned int num_counters,
+ const void __user *user, unsigned int len)
{
int i, ret;
struct ebt_counter *tmp;
@@ -1294,7 +1294,7 @@ static int do_update_counters(struct net *net, const char *name,
}
static int update_counters(struct net *net, const void __user *user,
- unsigned int len)
+ unsigned int len)
{
struct ebt_replace hlp;
@@ -1457,8 +1457,8 @@ static int copy_everything_to_user(struct ebt_table *t, void __user *user,
ebt_entry_to_user, entries, tmp.entries);
}
-static int do_ebt_set_ctl(struct sock *sk,
- int cmd, void __user *user, unsigned int len)
+static int do_ebt_set_ctl(struct sock *sk, int cmd, void __user *user,
+ unsigned int len)
{
int ret;
struct net *net = sock_net(sk);
@@ -1660,7 +1660,7 @@ static int compat_watcher_to_user(struct ebt_entry_watcher *w,
}
static int compat_copy_entry_to_user(struct ebt_entry *e, void __user **dstptr,
- unsigned int *size)
+ unsigned int *size)
{
struct ebt_entry_target *t;
struct ebt_entry __user *ce;
@@ -2149,7 +2149,7 @@ static int size_entry_mwt(struct ebt_entry *entry, const unsigned char *base,
* Called before validation is performed.
*/
static int compat_copy_entries(unsigned char *data, unsigned int size_user,
- struct ebt_entries_buf_state *state)
+ struct ebt_entries_buf_state *state)
{
unsigned int size_remaining = size_user;
int ret;
@@ -2167,7 +2167,8 @@ static int compat_copy_entries(unsigned char *data, unsigned int size_user,
static int compat_copy_ebt_replace_from_user(struct ebt_replace *repl,
- void __user *user, unsigned int len)
+ void __user *user,
+ unsigned int len)
{
struct compat_ebt_replace tmp;
int i;
@@ -2321,8 +2322,8 @@ static int compat_update_counters(struct net *net, void __user *user,
hlp.num_counters, user, len);
}
-static int compat_do_ebt_set_ctl(struct sock *sk,
- int cmd, void __user *user, unsigned int len)
+static int compat_do_ebt_set_ctl(struct sock *sk, int cmd, void __user *user,
+ unsigned int len)
{
int ret;
struct net *net = sock_net(sk);
@@ -2343,8 +2344,8 @@ static int compat_do_ebt_set_ctl(struct sock *sk,
return ret;
}
-static int compat_do_ebt_get_ctl(struct sock *sk, int cmd,
- void __user *user, int *len)
+static int compat_do_ebt_get_ctl(struct sock *sk, int cmd, void __user *user,
+ int *len)
{
int ret;
struct compat_ebt_replace tmp;
--
1.8.3.1
Call new audit_nf_cfg() to store table parameters for later use with
syscall records.
See: https://github.com/linux-audit/audit-kernel/issues/25
See: https://github.com/linux-audit/audit-kernel/issues/35
Signed-off-by: Richard Guy Briggs <[email protected]>
---
net/netfilter/x_tables.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 0094853ab42a..c0416ae52f7f 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1401,14 +1401,8 @@ struct xt_table_info *xt_replace_table(struct xt_table *table,
}
}
-#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
+ if (audit_enabled)
+ audit_nf_cfg(table->name, table->af, private->number);
return private;
}
--
1.8.3.1
Call new audit_nf_cfg() to store table parameters for later use with
syscall records.
See: https://github.com/linux-audit/audit-kernel/issues/25
See: https://github.com/linux-audit/audit-kernel/issues/35
Signed-off-by: Richard Guy Briggs <[email protected]>
---
net/bridge/netfilter/ebtables.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index b3c784ae33a0..57dc11c0f349 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1048,14 +1048,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
+ if (audit_enabled)
+ audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries);
return ret;
free_unlock:
--
1.8.3.1
Generate audit NETFILTER_CFG records on ebtables table registration.
Previously this was only being done for all x_tables operations and
ebtables table replacement.
Call new audit_nf_cfg() to store table parameters for later use with
syscall records.
Here is a sample accompanied record:
type=NETFILTER_CFG msg=audit(1494907217.558:5403): table=filter family=7 entries=0
See: https://github.com/linux-audit/audit-kernel/issues/43
Signed-off-by: Richard Guy Briggs <[email protected]>
---
net/bridge/netfilter/ebtables.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 57dc11c0f349..58126547b175 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1219,6 +1219,8 @@ int ebt_register_table(struct net *net, const struct ebt_table *input_table,
*res = NULL;
}
+ if (audit_enabled)
+ audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries);
return ret;
free_unlock:
mutex_unlock(&ebt_mutex);
--
1.8.3.1
Add the operation performed (register or replace) to the NETFILTER_CFG
record.
Here is the sample record:
type=NETFILTER_CFG msg=audit(1494981627.248:9764): family=7 table=broute entries=0 op=replace
See: https://github.com/linux-audit/audit-kernel/issues/25
Signed-off-by: Richard Guy Briggs <[email protected]>
---
include/linux/audit.h | 8 ++++----
kernel/auditsc.c | 7 ++++---
net/bridge/netfilter/ebtables.c | 4 ++--
net/netfilter/x_tables.c | 3 ++-
4 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 96cabb095eed..5eab4d898c26 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -379,7 +379,7 @@ 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_nf_cfg(const char *name, u8 af, int nentries);
+extern void __audit_nf_cfg(const char *name, u8 af, int nentries, int op);
static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
{
@@ -515,10 +515,10 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad)
__audit_ntp_log(ad);
}
-static inline void audit_nf_cfg(const char *name, u8 af, int nentries)
+static inline void audit_nf_cfg(const char *name, u8 af, int nentries, int op)
{
if (!audit_dummy_context())
- __audit_nf_cfg(name, af, nentries);
+ __audit_nf_cfg(name, af, nentries, op);
}
extern int audit_n_rules;
@@ -654,7 +654,7 @@ 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_nf_cfg(const char *name, u8 af, int nentries)
+static inline void audit_nf_cfg(const char *name, u8 af, int nentries, int op)
{ }
#define audit_n_rules 0
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 4e1df4233cd3..999ac184246b 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2545,7 +2545,7 @@ void __audit_ntp_log(const struct audit_ntp_data *ad)
audit_log_ntp_val(ad, "adjust", AUDIT_NTP_ADJUST);
}
-void __audit_nf_cfg(const char *name, u8 af, int nentries)
+void __audit_nf_cfg(const char *name, u8 af, int nentries, int op)
{
struct audit_buffer *ab;
struct audit_context *context = audit_context();
@@ -2555,8 +2555,9 @@ void __audit_nf_cfg(const char *name, u8 af, int nentries)
ab = audit_log_start(context, GFP_KERNEL, AUDIT_NETFILTER_CFG);
if (!ab)
return; /* audit_panic or being filtered */
- audit_log_format(ab, "table=%s family=%u entries=%u",
- name, af, nentries);
+ audit_log_format(ab, "table=%s family=%u entries=%u op=%s",
+ name, af, nentries,
+ op ? "replace" : "register");
audit_log_end(ab);
}
EXPORT_SYMBOL_GPL(__audit_nf_cfg);
diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 58126547b175..baff2f05af43 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1049,7 +1049,7 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
vfree(counterstmp);
if (audit_enabled)
- audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries);
+ audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries, 1);
return ret;
free_unlock:
@@ -1220,7 +1220,7 @@ int ebt_register_table(struct net *net, const struct ebt_table *input_table,
}
if (audit_enabled)
- audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries);
+ audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries, 0);
return ret;
free_unlock:
mutex_unlock(&ebt_mutex);
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index c0416ae52f7f..4ae4f7bf8946 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1402,7 +1402,8 @@ struct xt_table_info *xt_replace_table(struct xt_table *table,
}
if (audit_enabled)
- audit_nf_cfg(table->name, table->af, private->number);
+ audit_nf_cfg(table->name, table->af, private->number,
+ private->number);
return private;
}
--
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 <[email protected]>
---
kernel/auditsc.c | 3 ++-
net/bridge/netfilter/ebtables.c | 3 +++
net/netfilter/x_tables.c | 4 +++-
3 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 999ac184246b..2644130a9e66 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2557,7 +2557,8 @@ void __audit_nf_cfg(const char *name, u8 af, int nentries, int op)
return; /* audit_panic or being filtered */
audit_log_format(ab, "table=%s family=%u entries=%u op=%s",
name, af, nentries,
- op ? "replace" : "register");
+ op == 1 ? "replace" :
+ (op ? "unregister" : "register"));
audit_log_end(ab);
}
EXPORT_SYMBOL_GPL(__audit_nf_cfg);
diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index baff2f05af43..3dd4eb5b13fd 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1126,6 +1126,9 @@ static void __ebt_unregister_table(struct net *net, struct ebt_table *table)
mutex_lock(&ebt_mutex);
list_del(&table->list);
mutex_unlock(&ebt_mutex);
+ if (audit_enabled)
+ audit_nf_cfg(table->name, AF_BRIDGE, table->private->nentries,
+ 2);
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 4ae4f7bf8946..e4852a0cb62f 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1403,7 +1403,7 @@ struct xt_table_info *xt_replace_table(struct xt_table *table,
if (audit_enabled)
audit_nf_cfg(table->name, table->af, private->number,
- private->number);
+ !!private->number);
return private;
}
@@ -1466,6 +1466,8 @@ void *xt_unregister_table(struct xt_table *table)
private = table->private;
list_del(&table->list);
mutex_unlock(&xt[table->af].mutex);
+ if (audit_enabled)
+ audit_nf_cfg(table->name, table->af, private->number, 2);
kfree(table);
return private;
--
1.8.3.1
Record the auditable parameters of any non-empty netfilter table
configuration change.
See: https://github.com/linux-audit/audit-kernel/issues/25
Signed-off-by: Richard Guy Briggs <[email protected]>
---
include/linux/audit.h | 11 +++++++++++
kernel/auditsc.c | 16 ++++++++++++++++
2 files changed, 27 insertions(+)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index f9ceae57ca8d..96cabb095eed 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -379,6 +379,7 @@ 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_nf_cfg(const char *name, u8 af, int nentries);
static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
{
@@ -514,6 +515,12 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad)
__audit_ntp_log(ad);
}
+static inline void audit_nf_cfg(const char *name, u8 af, int nentries)
+{
+ if (!audit_dummy_context())
+ __audit_nf_cfg(name, af, nentries);
+}
+
extern int audit_n_rules;
extern int audit_signals;
#else /* CONFIG_AUDITSYSCALL */
@@ -646,6 +653,10 @@ 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_nf_cfg(const char *name, u8 af, int nentries)
+{ }
+
#define audit_n_rules 0
#define audit_signals 0
#endif /* CONFIG_AUDITSYSCALL */
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 4effe01ebbe2..4e1df4233cd3 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2545,6 +2545,22 @@ void __audit_ntp_log(const struct audit_ntp_data *ad)
audit_log_ntp_val(ad, "adjust", AUDIT_NTP_ADJUST);
}
+void __audit_nf_cfg(const char *name, u8 af, int nentries)
+{
+ struct audit_buffer *ab;
+ struct audit_context *context = audit_context();
+
+ if (!nentries)
+ return;
+ ab = audit_log_start(context, GFP_KERNEL, AUDIT_NETFILTER_CFG);
+ if (!ab)
+ return; /* audit_panic or being filtered */
+ audit_log_format(ab, "table=%s family=%u entries=%u",
+ name, af, nentries);
+ audit_log_end(ab);
+}
+EXPORT_SYMBOL_GPL(__audit_nf_cfg);
+
static void audit_log_task(struct audit_buffer *ab)
{
kuid_t auid, uid;
--
1.8.3.1
Richard Guy Briggs <[email protected]> wrote:
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 96cabb095eed..5eab4d898c26 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -379,7 +379,7 @@ 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_nf_cfg(const char *name, u8 af, int nentries);
> +extern void __audit_nf_cfg(const char *name, u8 af, int nentries, int op);
Consider adding an enum instead of int op.
> if (audit_enabled)
> - audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries);
> + audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries, 1);
audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries, AUDIT_XT_OP_REPLACE);
... would be a bit more readable than '1'.
The name is just an example, you can pick something else.
Richard Guy Briggs <[email protected]> wrote:
> Align all function declaration parameters with open parenthesis.
Acked-by: Florian Westphal <[email protected]>
On Mon, Jan 06, 2020 at 01:54:01PM -0500, Richard Guy Briggs wrote:
> 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
> setsockopt: iptables-restore, ip6tables-restore, ebtables-restore
> unshare: (h?)ostnamed
> clone: libvirtd
>
> The syscall events unsolicited by any audit rule were found to be caused by a
> missing !audit_dummy_context() check before creating a NETFILTER_CFG
> record and issuing the record immediately rather than saving the
> information to create the record at syscall exit.
> Check !audit_dummy_context() before creating the NETFILTER_CFG record.
>
> 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. The !audit_dummy_context() check above should avoid them.
>
> 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.
>
> Table unregistration was never logged, which is now covered.
>
> 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
What tree is this batch targeted to?
Thanks.
On Thu, Jan 16, 2020 at 10:05 AM Pablo Neira Ayuso <[email protected]> wrote:
> On Mon, Jan 06, 2020 at 01:54:01PM -0500, Richard Guy Briggs wrote:
> > 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
> > setsockopt: iptables-restore, ip6tables-restore, ebtables-restore
> > unshare: (h?)ostnamed
> > clone: libvirtd
> >
> > The syscall events unsolicited by any audit rule were found to be caused by a
> > missing !audit_dummy_context() check before creating a NETFILTER_CFG
> > record and issuing the record immediately rather than saving the
> > information to create the record at syscall exit.
> > Check !audit_dummy_context() before creating the NETFILTER_CFG record.
> >
> > 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. The !audit_dummy_context() check above should avoid them.
> >
> > 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.
> >
> > Table unregistration was never logged, which is now covered.
> >
> > 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
>
> What tree is this batch targeted to?
I believe Richard was targeting this for the audit tree.
--
paul moore
http://www.paul-moore.com
On 2020-01-16 14:07, Paul Moore wrote:
> On Thu, Jan 16, 2020 at 10:05 AM Pablo Neira Ayuso <[email protected]> wrote:
> > On Mon, Jan 06, 2020 at 01:54:01PM -0500, Richard Guy Briggs wrote:
> > > 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
> > > setsockopt: iptables-restore, ip6tables-restore, ebtables-restore
> > > unshare: (h?)ostnamed
> > > clone: libvirtd
> > >
> > > The syscall events unsolicited by any audit rule were found to be caused by a
> > > missing !audit_dummy_context() check before creating a NETFILTER_CFG
> > > record and issuing the record immediately rather than saving the
> > > information to create the record at syscall exit.
> > > Check !audit_dummy_context() before creating the NETFILTER_CFG record.
> > >
> > > 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. The !audit_dummy_context() check above should avoid them.
> > >
> > > 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.
> > >
> > > Table unregistration was never logged, which is now covered.
> > >
> > > 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
> >
> > What tree is this batch targeted to?
>
> I believe Richard was targeting this for the audit tree.
Yes, sorry Pablo, it is against audit/next based on v5.5-rc1
> paul moore
- RGB
--
Richard Guy Briggs <[email protected]>
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 Mon, Jan 6, 2020 at 1:55 PM Richard Guy Briggs <[email protected]> wrote:
> Record the auditable parameters of any non-empty netfilter table
> configuration change.
>
> See: https://github.com/linux-audit/audit-kernel/issues/25
> Signed-off-by: Richard Guy Briggs <[email protected]>
> ---
> include/linux/audit.h | 11 +++++++++++
> kernel/auditsc.c | 16 ++++++++++++++++
> 2 files changed, 27 insertions(+)
I can not see a good reason why this patch is separate from patches 5
and 6, please squash them down into one patch. As it currently stands
the logging function introduced here has no caller so it is pointless
by itself. Strive to make an individual patch have some significance
on its own whenever possible.
This will also help you write a better commit description, right now
the commit description tells me nothing, but if you bring in the other
patches you can talk about consolidating similar code into a common
function.
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index f9ceae57ca8d..96cabb095eed 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -379,6 +379,7 @@ 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_nf_cfg(const char *name, u8 af, int nentries);
>
> static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> {
> @@ -514,6 +515,12 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad)
> __audit_ntp_log(ad);
> }
>
> +static inline void audit_nf_cfg(const char *name, u8 af, int nentries)
> +{
> + if (!audit_dummy_context())
> + __audit_nf_cfg(name, af, nentries);
See my comments below about audit_enabled.
> +}
> +
> extern int audit_n_rules;
> extern int audit_signals;
> #else /* CONFIG_AUDITSYSCALL */
> @@ -646,6 +653,10 @@ 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_nf_cfg(const char *name, u8 af, int nentries)
> +{ }
> +
> #define audit_n_rules 0
> #define audit_signals 0
> #endif /* CONFIG_AUDITSYSCALL */
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 4effe01ebbe2..4e1df4233cd3 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2545,6 +2545,22 @@ void __audit_ntp_log(const struct audit_ntp_data *ad)
> audit_log_ntp_val(ad, "adjust", AUDIT_NTP_ADJUST);
> }
>
> +void __audit_nf_cfg(const char *name, u8 af, int nentries)
Should nentries be an unsigned int?
> +{
> + struct audit_buffer *ab;
> + struct audit_context *context = audit_context();
This is a good example of why the context of a caller matters; taken
alone I would say that we need a check for audit_enabled here, but if
we look at the latter patches we can see that the caller already has
the audit_enabled check.
Considering that the caller is already doing an audit_enabled check,
we might want to consider moving the audit_enabled check into
audit_nf_cfg() where we do the dummy context check. It's a static
inline so there shouldn't be a performance impact and it makes the
caller's code cleaner.
> + if (!nentries)
> + return;
> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_NETFILTER_CFG);
Why do we need the context variable, why not just call audit_context()
here directly?
> + if (!ab)
> + return; /* audit_panic or being filtered */
We generally don't add comments when audit_log_start() fails
elsewhere, please don't do it here.
> + audit_log_format(ab, "table=%s family=%u entries=%u",
> + name, af, nentries);
> + audit_log_end(ab);
> +}
> +EXPORT_SYMBOL_GPL(__audit_nf_cfg);
> +
> static void audit_log_task(struct audit_buffer *ab)
> {
> kuid_t auid, uid;
> --
> 1.8.3.1
--
paul moore
http://www.paul-moore.com
On Mon, Jan 6, 2020 at 1:56 PM Richard Guy Briggs <[email protected]> wrote:
>
> Generate audit NETFILTER_CFG records on ebtables table registration.
>
> Previously this was only being done for all x_tables operations and
> ebtables table replacement.
>
> Call new audit_nf_cfg() to store table parameters for later use with
> syscall records.
>
> Here is a sample accompanied record:
> type=NETFILTER_CFG msg=audit(1494907217.558:5403): table=filter family=7 entries=0
Wait a minute ... in patch 4 you have code that explicitly checks for
"entries=0" and doesn't generate a record in that case; is the example
a lie or is the code a lie? ;)
> See: https://github.com/linux-audit/audit-kernel/issues/43
> Signed-off-by: Richard Guy Briggs <[email protected]>
> ---
> net/bridge/netfilter/ebtables.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> index 57dc11c0f349..58126547b175 100644
> --- a/net/bridge/netfilter/ebtables.c
> +++ b/net/bridge/netfilter/ebtables.c
> @@ -1219,6 +1219,8 @@ int ebt_register_table(struct net *net, const struct ebt_table *input_table,
> *res = NULL;
> }
>
> + if (audit_enabled)
> + audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries);
> return ret;
> free_unlock:
> mutex_unlock(&ebt_mutex);
> --
> 1.8.3.1
--
paul moore
http://www.paul-moore.com
On Mon, Jan 6, 2020 at 1:55 PM Richard Guy Briggs <[email protected]> wrote:
>
> Align all function declaration parameters with open parenthesis.
>
> Signed-off-by: Richard Guy Briggs <[email protected]>
> ---
> net/bridge/netfilter/ebtables.c | 27 ++++++++++++++-------------
> 1 file changed, 14 insertions(+), 13 deletions(-)
My comments from the first patch regarding style changes also applies here.
--
paul moore
http://www.paul-moore.com
On Mon, Jan 6, 2020 at 1:56 PM Richard Guy Briggs <[email protected]> wrote:
>
> 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 <[email protected]>
> ---
> kernel/auditsc.c | 3 ++-
> net/bridge/netfilter/ebtables.c | 3 +++
> net/netfilter/x_tables.c | 4 +++-
> 3 files changed, 8 insertions(+), 2 deletions(-)
... and in keeping with an ongoing theme for this patchset, please
squash this patch too.
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 999ac184246b..2644130a9e66 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2557,7 +2557,8 @@ void __audit_nf_cfg(const char *name, u8 af, int nentries, int op)
> return; /* audit_panic or being filtered */
> audit_log_format(ab, "table=%s family=%u entries=%u op=%s",
> name, af, nentries,
> - op ? "replace" : "register");
> + op == 1 ? "replace" :
> + (op ? "unregister" : "register"));
> audit_log_end(ab);
> }
> EXPORT_SYMBOL_GPL(__audit_nf_cfg);
> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> index baff2f05af43..3dd4eb5b13fd 100644
> --- a/net/bridge/netfilter/ebtables.c
> +++ b/net/bridge/netfilter/ebtables.c
> @@ -1126,6 +1126,9 @@ static void __ebt_unregister_table(struct net *net, struct ebt_table *table)
> mutex_lock(&ebt_mutex);
> list_del(&table->list);
> mutex_unlock(&ebt_mutex);
> + if (audit_enabled)
> + audit_nf_cfg(table->name, AF_BRIDGE, table->private->nentries,
> + 2);
> 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 4ae4f7bf8946..e4852a0cb62f 100644
> --- a/net/netfilter/x_tables.c
> +++ b/net/netfilter/x_tables.c
> @@ -1403,7 +1403,7 @@ struct xt_table_info *xt_replace_table(struct xt_table *table,
>
> if (audit_enabled)
> audit_nf_cfg(table->name, table->af, private->number,
> - private->number);
> + !!private->number);
>
> return private;
> }
> @@ -1466,6 +1466,8 @@ void *xt_unregister_table(struct xt_table *table)
> private = table->private;
> list_del(&table->list);
> mutex_unlock(&xt[table->af].mutex);
> + if (audit_enabled)
> + audit_nf_cfg(table->name, table->af, private->number, 2);
> kfree(table);
>
> return private;
> --
> 1.8.3.1
--
paul moore
http://www.paul-moore.com
On Mon, Jan 6, 2020 at 3:23 PM Florian Westphal <[email protected]> wrote:
> Richard Guy Briggs <[email protected]> wrote:
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 96cabb095eed..5eab4d898c26 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -379,7 +379,7 @@ 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_nf_cfg(const char *name, u8 af, int nentries);
> > +extern void __audit_nf_cfg(const char *name, u8 af, int nentries, int op);
>
> Consider adding an enum instead of int op.
>
> > if (audit_enabled)
> > - audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries);
> > + audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries, 1);
>
> audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries, AUDIT_XT_OP_REPLACE);
>
> ... would be a bit more readable than '1'.
>
> The name is just an example, you can pick something else.
I agree. Also, please just merge this into patch 4; I don't see a
solid reason why it shouldn't be there.
--
paul moore
http://www.paul-moore.com
On 2020-01-06 21:23, Florian Westphal wrote:
> Richard Guy Briggs <[email protected]> wrote:
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 96cabb095eed..5eab4d898c26 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -379,7 +379,7 @@ 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_nf_cfg(const char *name, u8 af, int nentries);
> > +extern void __audit_nf_cfg(const char *name, u8 af, int nentries, int op);
>
> Consider adding an enum instead of int op.
>
> > if (audit_enabled)
> > - audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries);
> > + audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries, 1);
>
> audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries, AUDIT_XT_OP_REPLACE);
>
> ... would be a bit more readable than '1'.
>
> The name is just an example, you can pick something else.
Thanks Florian.
Another question occurs to me about table default policy.
I'd observed previously that if nentries was zero due to an empty table,
then it was due to table registration calls, which resulted from module
loading. The default policy is NF_ACCEPT (because Rusty didn't want
more email, go figure...). It occurred to me later that some table
loads took a command line parameter to be able to change the default
policy verdict from NF_ACCEPT to NF_DROP. In particular, filter FORWARD
hook tables. Is there a straightforward way to be able to detect this
in all the audit_nf_cfg() callers to be able to log it? In particular,
in:
net/bridge/netfilter/ebtables.c: ebt_register_table()
net/bridge/netfilter/ebtables.c: do_replace_finish()
net/bridge/netfilter/ebtables.c: __ebt_unregister_table()
net/netfilter/x_tables.c: xt_replace_table()
net/netfilter/x_tables.c: xt_unregister_table()
It appears to be stored in the second entry of struct ipt_replace and
struct ip6t_replace, of types struct ipt_standard and struct
ip6t_standard in target.verdict, which doesn't appear to be obvious or
easily accessible from xt_replace_table(). In ebtables, it appears to
be in the policy member of struct ebt_entries.
Both potential solutions are awkward, adding a parameter to pass that
value in, but also trying to reach into the protocol-specific entry
table to find that value. Would you have a recommendation? This
assumes that reporting that default policy value is even desired or
required.
- RGB
--
Richard Guy Briggs <[email protected]>
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
Richard Guy Briggs <[email protected]> wrote:
> The default policy is NF_ACCEPT (because Rusty didn't want
> more email, go figure...). It occurred to me later that some table
> loads took a command line parameter to be able to change the default
> policy verdict from NF_ACCEPT to NF_DROP. In particular, filter FORWARD
> hook tables. Is there a straightforward way to be able to detect this
> in all the audit_nf_cfg() callers to be able to log it? In particular,
> in:
> net/bridge/netfilter/ebtables.c: ebt_register_table()
> net/bridge/netfilter/ebtables.c: do_replace_finish()
> net/bridge/netfilter/ebtables.c: __ebt_unregister_table()
> net/netfilter/x_tables.c: xt_replace_table()
> net/netfilter/x_tables.c: xt_unregister_table()
The module parameter or the policy?
The poliy can be changed via the xtables tools.
Given you can have:
*filter
:INPUT ACCEPT [0:0]
:FORWARD DROP [0:0]
:OUTPUT ACCEPT [0:0]
-A FORWARD -j ACCEPT
COMMIT
... which effectily gives a FORWARD ACCEPT policy I'm not sure logging
the policy is useful.
Furthermore, ebtables has polices even for user-defined chains.
> Both potential solutions are awkward, adding a parameter to pass that
> value in, but also trying to reach into the protocol-specific entry
> table to find that value. Would you have a recommendation? This
> assumes that reporting that default policy value is even desired or
> required.
See above, I don't think its useful. If it is needed, its probably best
to define an informational struct containing the policy (accept/drop)
value for the each hook points (prerouting to postrouting), fill
that from the backend specific code (as thats the only place that
exposes the backend specific structs ...) and then pass that to
the audit logging functions.
On 2020-02-13 13:34, Florian Westphal wrote:
> Richard Guy Briggs <[email protected]> wrote:
> > The default policy is NF_ACCEPT (because Rusty didn't want
> > more email, go figure...). It occurred to me later that some table
> > loads took a command line parameter to be able to change the default
> > policy verdict from NF_ACCEPT to NF_DROP. In particular, filter FORWARD
> > hook tables. Is there a straightforward way to be able to detect this
> > in all the audit_nf_cfg() callers to be able to log it? In particular,
> > in:
> > net/bridge/netfilter/ebtables.c: ebt_register_table()
> > net/bridge/netfilter/ebtables.c: do_replace_finish()
> > net/bridge/netfilter/ebtables.c: __ebt_unregister_table()
> > net/netfilter/x_tables.c: xt_replace_table()
> > net/netfilter/x_tables.c: xt_unregister_table()
>
> The module parameter or the policy?
>
> The poliy can be changed via the xtables tools.
> Given you can have:
>
> *filter
> :INPUT ACCEPT [0:0]
> :FORWARD DROP [0:0]
> :OUTPUT ACCEPT [0:0]
> -A FORWARD -j ACCEPT
> COMMIT
>
> ... which effectily gives a FORWARD ACCEPT policy I'm not sure logging
> the policy is useful.
>
> Furthermore, ebtables has polices even for user-defined chains.
>
> > Both potential solutions are awkward, adding a parameter to pass that
> > value in, but also trying to reach into the protocol-specific entry
> > table to find that value. Would you have a recommendation? This
> > assumes that reporting that default policy value is even desired or
> > required.
>
> See above, I don't think its useful. If it is needed, its probably best
> to define an informational struct containing the policy (accept/drop)
> value for the each hook points (prerouting to postrouting), fill
> that from the backend specific code (as thats the only place that
> exposes the backend specific structs ...) and then pass that to
> the audit logging functions.
Ok, for this set, I'll drop the idea. If it becomes apparent later than
it is necessary, it can be added as a field at the end of the record.
Thanks for looking at this.
- RGB
--
Richard Guy Briggs <[email protected]>
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-01-30 22:18, Paul Moore wrote:
> On Mon, Jan 6, 2020 at 1:56 PM Richard Guy Briggs <[email protected]> wrote:
> >
> > Generate audit NETFILTER_CFG records on ebtables table registration.
> >
> > Previously this was only being done for all x_tables operations and
> > ebtables table replacement.
> >
> > Call new audit_nf_cfg() to store table parameters for later use with
> > syscall records.
> >
> > Here is a sample accompanied record:
> > type=NETFILTER_CFG msg=audit(1494907217.558:5403): table=filter family=7 entries=0
>
> Wait a minute ... in patch 4 you have code that explicitly checks for
> "entries=0" and doesn't generate a record in that case; is the example
> a lie or is the code a lie? ;)
The example was stale once the entries check was added. The entries
check has now been removed due to the source of registration records
being orphanned from their syscall record being found and solved in the
ghak120 (norule missing accompanying) issue.
However, there are ebtables nat and filter tables being added that are
being automatically reaped if there are no entries and there is no
syscall accompanying them. I don't yet know if it is being reaped by
userspace with an async drop, or if it is the kernel that is deciding to
garbage collect that table after a period of disuse.
Some quick instrumentation says it is kernel thread [kworker/u4:2-events_unbound]
pid=153 uid=0 auid=4294967295 tty=(none) ses=4294967295 subj=system_u:system_r:kernel_t:s0 comm="kworker/u4:2" exe=(null)
> > See: https://github.com/linux-audit/audit-kernel/issues/43
> > Signed-off-by: Richard Guy Briggs <[email protected]>
> > ---
> > net/bridge/netfilter/ebtables.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> > index 57dc11c0f349..58126547b175 100644
> > --- a/net/bridge/netfilter/ebtables.c
> > +++ b/net/bridge/netfilter/ebtables.c
> > @@ -1219,6 +1219,8 @@ int ebt_register_table(struct net *net, const struct ebt_table *input_table,
> > *res = NULL;
> > }
> >
> > + if (audit_enabled)
> > + audit_nf_cfg(repl->name, AF_BRIDGE, repl->nentries);
> > return ret;
> > free_unlock:
> > mutex_unlock(&ebt_mutex);
> > --
> > 1.8.3.1
>
> --
> paul moore
> http://www.paul-moore.com
>
- RGB
--
Richard Guy Briggs <[email protected]>
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-01-30 22:18, Paul Moore wrote:
> On Mon, Jan 6, 2020 at 1:55 PM Richard Guy Briggs <[email protected]> wrote:
> > Record the auditable parameters of any non-empty netfilter table
> > configuration change.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/25
> > Signed-off-by: Richard Guy Briggs <[email protected]>
> > ---
> > include/linux/audit.h | 11 +++++++++++
> > kernel/auditsc.c | 16 ++++++++++++++++
> > 2 files changed, 27 insertions(+)
>
> I can not see a good reason why this patch is separate from patches 5
> and 6, please squash them down into one patch. As it currently stands
> the logging function introduced here has no caller so it is pointless
> by itself. Strive to make an individual patch have some significance
> on its own whenever possible.
>
> This will also help you write a better commit description, right now
> the commit description tells me nothing, but if you bring in the other
> patches you can talk about consolidating similar code into a common
> function.
Fair enough. I could see squashing some of these, but there are a
number of issues being addressed and would like to see some granularity,
but as you point out, each patch should stand on its own...
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index f9ceae57ca8d..96cabb095eed 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -379,6 +379,7 @@ 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_nf_cfg(const char *name, u8 af, int nentries);
> >
> > static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> > {
> > @@ -514,6 +515,12 @@ static inline void audit_ntp_log(const struct audit_ntp_data *ad)
> > __audit_ntp_log(ad);
> > }
> >
> > +static inline void audit_nf_cfg(const char *name, u8 af, int nentries)
> > +{
> > + if (!audit_dummy_context())
> > + __audit_nf_cfg(name, af, nentries);
>
> See my comments below about audit_enabled.
I've cleaned up audit_enabled and removed dummy due to ghak120.
> > +}
> > +
> > extern int audit_n_rules;
> > extern int audit_signals;
> > #else /* CONFIG_AUDITSYSCALL */
> > @@ -646,6 +653,10 @@ 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_nf_cfg(const char *name, u8 af, int nentries)
> > +{ }
> > +
> > #define audit_n_rules 0
> > #define audit_signals 0
> > #endif /* CONFIG_AUDITSYSCALL */
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 4effe01ebbe2..4e1df4233cd3 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2545,6 +2545,22 @@ void __audit_ntp_log(const struct audit_ntp_data *ad)
> > audit_log_ntp_val(ad, "adjust", AUDIT_NTP_ADJUST);
> > }
> >
> > +void __audit_nf_cfg(const char *name, u8 af, int nentries)
>
> Should nentries be an unsigned int?
Yes, it should, thank you.
> > +{
> > + struct audit_buffer *ab;
> > + struct audit_context *context = audit_context();
>
> This is a good example of why the context of a caller matters; taken
> alone I would say that we need a check for audit_enabled here, but if
> we look at the latter patches we can see that the caller already has
> the audit_enabled check.
>
> Considering that the caller is already doing an audit_enabled check,
> we might want to consider moving the audit_enabled check into
> audit_nf_cfg() where we do the dummy context check. It's a static
> inline so there shouldn't be a performance impact and it makes the
> caller's code cleaner.
>
> > + if (!nentries)
> > + return;
> > + ab = audit_log_start(context, GFP_KERNEL, AUDIT_NETFILTER_CFG);
>
> Why do we need the context variable, why not just call audit_context()
> here directly?
Context has been cleaned up.
> > + if (!ab)
> > + return; /* audit_panic or being filtered */
>
> We generally don't add comments when audit_log_start() fails
> elsewhere, please don't do it here.
Ok.
> > + audit_log_format(ab, "table=%s family=%u entries=%u",
> > + name, af, nentries);
> > + audit_log_end(ab);
> > +}
> > +EXPORT_SYMBOL_GPL(__audit_nf_cfg);
> > +
> > static void audit_log_task(struct audit_buffer *ab)
> > {
> > kuid_t auid, uid;
> > --
> > 1.8.3.1
>
> --
> paul moore
> http://www.paul-moore.com
>
- RGB
--
Richard Guy Briggs <[email protected]>
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