[PATCH ghak25 v2 0/9] Address NETFILTER_CFG issues

[PATCH ghak25 v2 0/9] 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
	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

[PATCH ghak25 v2 1/9] netfilter: normalize x_table function declarations

[Richard Guy Briggs]

Git context diffs were being produced with unhelpful declaration types
in the place of function names to help identify the funciton in which
changes were made.

Normalize x_table function declarations so that git context diff
function labels work as expected.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 net/netfilter/x_tables.c | 43 ++++++++++++++++++-------------------------
 1 file changed, 18 insertions(+), 25 deletions(-)

diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index ce70c2576bb2..0094853ab42a 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -77,8 +77,7 @@ int xt_register_target(struct xt_target *target)
 }
 EXPORT_SYMBOL(xt_register_target);
 
-void
-xt_unregister_target(struct xt_target *target)
+void xt_unregister_target(struct xt_target *target)
 {
 	u_int8_t af = target->family;
 
@@ -88,8 +87,7 @@ int xt_register_target(struct xt_target *target)
 }
 EXPORT_SYMBOL(xt_unregister_target);
 
-int
-xt_register_targets(struct xt_target *target, unsigned int n)
+int xt_register_targets(struct xt_target *target, unsigned int n)
 {
 	unsigned int i;
 	int err = 0;
@@ -108,8 +106,7 @@ int xt_register_target(struct xt_target *target)
 }
 EXPORT_SYMBOL(xt_register_targets);
 
-void
-xt_unregister_targets(struct xt_target *target, unsigned int n)
+void xt_unregister_targets(struct xt_target *target, unsigned int n)
 {
 	while (n-- > 0)
 		xt_unregister_target(&target[n]);
@@ -127,8 +124,7 @@ int xt_register_match(struct xt_match *match)
 }
 EXPORT_SYMBOL(xt_register_match);
 
-void
-xt_unregister_match(struct xt_match *match)
+void xt_unregister_match(struct xt_match *match)
 {
 	u_int8_t af = match->family;
 
@@ -138,8 +134,7 @@ int xt_register_match(struct xt_match *match)
 }
 EXPORT_SYMBOL(xt_unregister_match);
 
-int
-xt_register_matches(struct xt_match *match, unsigned int n)
+int xt_register_matches(struct xt_match *match, unsigned int n)
 {
 	unsigned int i;
 	int err = 0;
@@ -158,8 +153,7 @@ int xt_register_match(struct xt_match *match)
 }
 EXPORT_SYMBOL(xt_register_matches);
 
-void
-xt_unregister_matches(struct xt_match *match, unsigned int n)
+void xt_unregister_matches(struct xt_match *match, unsigned int n)
 {
 	while (n-- > 0)
 		xt_unregister_match(&match[n]);
@@ -204,8 +198,8 @@ struct xt_match *xt_find_match(u8 af, const char *name, u8 revision)
 }
 EXPORT_SYMBOL(xt_find_match);
 
-struct xt_match *
-xt_request_find_match(uint8_t nfproto, const char *name, uint8_t revision)
+struct xt_match *xt_request_find_match(u8 nfproto, const char *name,
+				       u8 revision)
 {
 	struct xt_match *match;
 
@@ -391,8 +385,8 @@ int xt_find_revision(u8 af, const char *name, u8 revision, int target,
 }
 EXPORT_SYMBOL_GPL(xt_find_revision);
 
-static char *
-textify_hooks(char *buf, size_t size, unsigned int mask, uint8_t nfproto)
+static char *textify_hooks(char *buf, size_t size, unsigned int mask,
+			   uint8_t nfproto)
 {
 	static const char *const inetbr_names[] = {
 		"PREROUTING", "INPUT", "FORWARD",
@@ -1349,11 +1343,10 @@ struct xt_counters *xt_counters_alloc(unsigned int counters)
 }
 EXPORT_SYMBOL(xt_counters_alloc);
 
-struct xt_table_info *
-xt_replace_table(struct xt_table *table,
-	      unsigned int num_counters,
-	      struct xt_table_info *newinfo,
-	      int *error)
+struct xt_table_info *xt_replace_table(struct xt_table *table,
+				       unsigned int num_counters,
+				       struct xt_table_info *newinfo,
+				       int *error)
 {
 	struct xt_table_info *private;
 	unsigned int cpu;
@@ -1542,7 +1535,7 @@ enum {
 };
 
 static void *xt_mttg_seq_next(struct seq_file *seq, void *v, loff_t *ppos,
-    bool is_target)
+			      bool is_target)
 {
 	static const uint8_t next_class[] = {
 		[MTTG_TRAV_NFP_UNSPEC] = MTTG_TRAV_NFP_SPEC,
@@ -1583,7 +1576,7 @@ static void *xt_mttg_seq_next(struct seq_file *seq, void *v, loff_t *ppos,
 }
 
 static void *xt_mttg_seq_start(struct seq_file *seq, loff_t *pos,
-    bool is_target)
+			       bool is_target)
 {
 	struct nf_mttg_trav *trav = seq->private;
 	unsigned int j;
@@ -1692,8 +1685,8 @@ static int xt_target_seq_show(struct seq_file *seq, void *v)
  * This function will create the nf_hook_ops that the x_table needs
  * to hand to xt_hook_link_net().
  */
-struct nf_hook_ops *
-xt_hook_ops_alloc(const struct xt_table *table, nf_hookfn *fn)
+struct nf_hook_ops *xt_hook_ops_alloc(const struct xt_table *table,
+				      nf_hookfn *fn)
 {
 	unsigned int hook_mask = table->valid_hooks;
 	uint8_t i, num_hooks = hweight32(hook_mask);
-- 
1.8.3.1

[PATCH ghak25 v2 2/9] netfilter: normalize ebtables function declarations

[Richard Guy Briggs]

Git context diffs were being produced with unhelpful declaration types
in the place of function names to help identify the funciton in which
changes were made.

Normalize ebtables function declarations so that git context diff
function labels work as expected.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 net/bridge/netfilter/ebtables.c | 100 ++++++++++++++++++++--------------------
 1 file changed, 51 insertions(+), 49 deletions(-)

diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 4096d8a74a2b..c9dff9e11ddb 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -76,9 +76,9 @@ static int ebt_standard_compat_to_user(void __user *dst, const void *src)
 #endif
 };
 
-static inline int
-ebt_do_watcher(const struct ebt_entry_watcher *w, struct sk_buff *skb,
-	       struct xt_action_param *par)
+static inline int ebt_do_watcher(const struct ebt_entry_watcher *w,
+				 struct sk_buff *skb,
+				 struct xt_action_param *par)
 {
 	par->target   = w->u.watcher;
 	par->targinfo = w->data;
@@ -87,17 +87,17 @@ static int ebt_standard_compat_to_user(void __user *dst, const void *src)
 	return 0;
 }
 
-static inline int
-ebt_do_match(struct ebt_entry_match *m, const struct sk_buff *skb,
-	     struct xt_action_param *par)
+static inline int ebt_do_match(struct ebt_entry_match *m,
+			       const struct sk_buff *skb,
+			       struct xt_action_param *par)
 {
 	par->match     = m->u.match;
 	par->matchinfo = m->data;
 	return !m->u.match->match(skb, par);
 }
 
-static inline int
-ebt_dev_check(const char *entry, const struct net_device *device)
+static inline int ebt_dev_check(const char *entry,
+				const struct net_device *device)
 {
 	int i = 0;
 	const char *devname;
@@ -114,9 +114,10 @@ static int ebt_standard_compat_to_user(void __user *dst, const void *src)
 }
 
 /* process standard matches */
-static inline int
-ebt_basic_match(const struct ebt_entry *e, const struct sk_buff *skb,
-		const struct net_device *in, const struct net_device *out)
+static inline int ebt_basic_match(const struct ebt_entry *e,
+				  const struct sk_buff *skb,
+				  const struct net_device *in,
+				  const struct net_device *out)
 {
 	const struct ethhdr *h = eth_hdr(skb);
 	const struct net_bridge_port *p;
@@ -163,14 +164,12 @@ static int ebt_standard_compat_to_user(void __user *dst, const void *src)
 	return 0;
 }
 
-static inline
-struct ebt_entry *ebt_next_entry(const struct ebt_entry *entry)
+static inline struct ebt_entry *ebt_next_entry(const struct ebt_entry *entry)
 {
 	return (void *)entry + entry->next_offset;
 }
 
-static inline const struct ebt_entry_target *
-ebt_get_target_c(const struct ebt_entry *e)
+static inline const struct ebt_entry_target *ebt_get_target_c(const struct ebt_entry *e)
 {
 	return ebt_get_target((struct ebt_entry *)e);
 }
@@ -304,9 +303,9 @@ unsigned int ebt_do_table(struct sk_buff *skb,
 }
 
 /* If it succeeds, returns element and locks mutex */
-static inline void *
-find_inlist_lock_noload(struct list_head *head, const char *name, int *error,
-			struct mutex *mutex)
+static inline void *find_inlist_lock_noload(struct list_head *head,
+					    const char *name, int *error,
+					    struct mutex *mutex)
 {
 	struct {
 		struct list_head list;
@@ -323,18 +322,18 @@ unsigned int ebt_do_table(struct sk_buff *skb,
 	return NULL;
 }
 
-static void *
-find_inlist_lock(struct list_head *head, const char *name, const char *prefix,
-		 int *error, struct mutex *mutex)
+static void *find_inlist_lock(struct list_head *head, const char *name,
+			      const char *prefix, int *error,
+			      struct mutex *mutex)
 {
 	return try_then_request_module(
 			find_inlist_lock_noload(head, name, error, mutex),
 			"%s%s", prefix, name);
 }
 
-static inline struct ebt_table *
-find_table_lock(struct net *net, const char *name, int *error,
-		struct mutex *mutex)
+static inline struct ebt_table *find_table_lock(struct net *net,
+						const char *name, int *error,
+						struct mutex *mutex)
 {
 	return find_inlist_lock(&net->xt.tables[NFPROTO_BRIDGE], name,
 				"ebtable_", error, mutex);
@@ -350,9 +349,10 @@ static inline void ebt_free_table_info(struct ebt_table_info *info)
 		vfree(info->chainstack);
 	}
 }
-static inline int
-ebt_check_match(struct ebt_entry_match *m, struct xt_mtchk_param *par,
-		unsigned int *cnt)
+
+static inline int ebt_check_match(struct ebt_entry_match *m,
+				  struct xt_mtchk_param *par,
+				  unsigned int *cnt)
 {
 	const struct ebt_entry *e = par->entryinfo;
 	struct xt_match *match;
@@ -387,9 +387,9 @@ static inline void ebt_free_table_info(struct ebt_table_info *info)
 	return 0;
 }
 
-static inline int
-ebt_check_watcher(struct ebt_entry_watcher *w, struct xt_tgchk_param *par,
-		  unsigned int *cnt)
+static inline int ebt_check_watcher(struct ebt_entry_watcher *w,
+				    struct xt_tgchk_param *par,
+				    unsigned int *cnt)
 {
 	const struct ebt_entry *e = par->entryinfo;
 	struct xt_target *watcher;
@@ -490,11 +490,12 @@ static int ebt_verify_pointers(const struct ebt_replace *repl,
 /* this one is very careful, as it is the first function
  * to parse the userspace data
  */
-static inline int
-ebt_check_entry_size_and_hooks(const struct ebt_entry *e,
-			       const struct ebt_table_info *newinfo,
-			       unsigned int *n, unsigned int *cnt,
-			       unsigned int *totalcnt, unsigned int *udc_cnt)
+static inline int ebt_check_entry_size_and_hooks(const struct ebt_entry *e,
+						 const struct ebt_table_info *newinfo,
+						 unsigned int *n,
+						 unsigned int *cnt,
+						 unsigned int *totalcnt,
+						 unsigned int *udc_cnt)
 {
 	int i;
 
@@ -551,9 +552,10 @@ struct ebt_cl_stack {
 /* We need these positions to check that the jumps to a different part of the
  * entries is a jump to the beginning of a new chain.
  */
-static inline int
-ebt_get_udc_positions(struct ebt_entry *e, struct ebt_table_info *newinfo,
-		      unsigned int *n, struct ebt_cl_stack *udc)
+static inline int ebt_get_udc_positions(struct ebt_entry *e,
+					struct ebt_table_info *newinfo,
+					unsigned int *n,
+					struct ebt_cl_stack *udc)
 {
 	int i;
 
@@ -577,8 +579,8 @@ struct ebt_cl_stack {
 	return 0;
 }
 
-static inline int
-ebt_cleanup_match(struct ebt_entry_match *m, struct net *net, unsigned int *i)
+static inline int ebt_cleanup_match(struct ebt_entry_match *m,
+				    struct net *net, unsigned int *i)
 {
 	struct xt_mtdtor_param par;
 
@@ -595,8 +597,8 @@ struct ebt_cl_stack {
 	return 0;
 }
 
-static inline int
-ebt_cleanup_watcher(struct ebt_entry_watcher *w, struct net *net, unsigned int *i)
+static inline int ebt_cleanup_watcher(struct ebt_entry_watcher *w,
+				      struct net *net, unsigned int *i)
 {
 	struct xt_tgdtor_param par;
 
@@ -613,8 +615,8 @@ struct ebt_cl_stack {
 	return 0;
 }
 
-static inline int
-ebt_cleanup_entry(struct ebt_entry *e, struct net *net, unsigned int *cnt)
+static inline int ebt_cleanup_entry(struct ebt_entry *e, struct net *net,
+				    unsigned int *cnt)
 {
 	struct xt_tgdtor_param par;
 	struct ebt_entry_target *t;
@@ -638,11 +640,11 @@ struct ebt_cl_stack {
 	return 0;
 }
 
-static inline int
-ebt_check_entry(struct ebt_entry *e, struct net *net,
-		const struct ebt_table_info *newinfo,
-		const char *name, unsigned int *cnt,
-		struct ebt_cl_stack *cl_s, unsigned int udc_cnt)
+static inline int ebt_check_entry(struct ebt_entry *e, struct net *net,
+				  const struct ebt_table_info *newinfo,
+				  const char *name, unsigned int *cnt,
+				  struct ebt_cl_stack *cl_s,
+				  unsigned int udc_cnt)
 {
 	struct ebt_entry_target *t;
 	struct xt_target *target;
-- 
1.8.3.1

[PATCH ghak25 v2 3/9] netfilter: normalize ebtables function declarations II

[Richard Guy Briggs]

Align all function declaration parameters with open parenthesis.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 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

[PATCH ghak25 v2 4/9] audit: record nfcfg params

[Richard Guy Briggs]

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 <rgb@redhat.com>
---
 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

[PATCH ghak25 v2 5/9] netfilter: x_tables audit only on syscall rule

[Richard Guy Briggs]

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 <rgb@redhat.com>
---
 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

[PATCH ghak25 v2 6/9] netfilter: ebtables audit only on syscall rule

[Richard Guy Briggs]

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 <rgb@redhat.com>
---
 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

[PATCH ghak25 v2 7/9] netfilter: ebtables audit table registration

[Richard Guy Briggs]

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 <rgb@redhat.com>
---
 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

[PATCH ghak25 v2 8/9] netfilter: add audit operation field

[Richard Guy Briggs]

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 <rgb@redhat.com>
---
 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

[PATCH ghak25 v2 9/9] netfilter: audit table unregister actions

[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 <rgb@redhat.com>
---
 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

Re: [PATCH ghak25 v2 8/9] netfilter: add audit operation field

[Florian Westphal]

Richard Guy Briggs <rgb@redhat.com> 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.

Re: [PATCH ghak25 v2 1/9] netfilter: normalize x_table function declarations

[Florian Westphal]

Richard Guy Briggs <rgb@redhat.com> wrote:
> Git context diffs were being produced with unhelpful declaration types
> in the place of function names to help identify the funciton in which
> changes were made.
> 
> Normalize x_table function declarations so that git context diff
> function labels work as expected.

Acked-by: Florian Westphal <fw@strlen.de>

Re: [PATCH ghak25 v2 2/9] netfilter: normalize ebtables function declarations

[Florian Westphal]

Richard Guy Briggs <rgb@redhat.com> wrote:
> Git context diffs were being produced with unhelpful declaration types
> in the place of function names to help identify the funciton in which
> changes were made.
> 
> Normalize ebtables function declarations so that git context diff
> function labels work as expected.
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

I suggest that you also drop the inline keyword for all functions
that are called from control path, for example

> -static inline void *
> -find_inlist_lock_noload(struct list_head *head, const char *name, int *error,
> -			struct mutex *mutex)
> +static inline void *find_inlist_lock_noload(struct list_head *head,
> +					    const char *name, int *error,
> +					    struct mutex *mutex)

> -static inline struct ebt_table *
> -find_table_lock(struct net *net, const char *name, int *error,
> -		struct mutex *mutex)
> +static inline struct ebt_table *find_table_lock(struct net *net,

> -static inline int
> -ebt_check_match(struct ebt_entry_match *m, struct xt_mtchk_param *par,
> -		unsigned int *cnt)
> +
> +static inline int ebt_check_match(struct ebt_entry_match *m,

> -static inline int
> -ebt_check_watcher(struct ebt_entry_watcher *w, struct xt_tgchk_param *par,
> -		  unsigned int *cnt)

> -static inline int
> -ebt_check_entry_size_and_hooks(const struct ebt_entry *e,

> -static inline int
> -ebt_get_udc_positions(struct ebt_entry *e, struct ebt_table_info *newinfo,
> -		      unsigned int *n, struct ebt_cl_stack *udc)

> -static inline int
> -ebt_cleanup_match(struct ebt_entry_match *m, struct net *net, unsigned int *i)

> -static inline int
> -ebt_cleanup_watcher(struct ebt_entry_watcher *w, struct net *net, unsigned int *i)

> -static inline int
> -ebt_cleanup_entry(struct ebt_entry *e, struct net *net, unsigned int *cnt)

> -static inline int
> -ebt_check_entry(struct ebt_entry *e, struct net *net,
> -		const struct ebt_table_info *newinfo,
> -		const char *name, unsigned int *cnt,
> -		struct ebt_cl_stack *cl_s, unsigned int udc_cnt)

.. can all have 'inline' removed.  Other than that this looks good to me.

Re: [PATCH ghak25 v2 3/9] netfilter: normalize ebtables function declarations II

[Florian Westphal]

Richard Guy Briggs <rgb@redhat.com> wrote:
> Align all function declaration parameters with open parenthesis.

Acked-by: Florian Westphal <fw@strlen.de>

Re: [PATCH ghak25 v2 1/9] netfilter: normalize x_table function declarations

[Nicolas Dichtel]

Le 06/01/2020 à 19:54, Richard Guy Briggs a écrit :
> Git context diffs were being produced with unhelpful declaration types
> in the place of function names to help identify the funciton in which
> changes were made.
Just for my information, how do you reproduce that? With a 'git diff'?

> 
> Normalize x_table function declarations so that git context diff
> function labels work as expected.
> 
[snip]
> 
> -- 
> 1.8.3.1
git v1.8.3.1 is seven years old:
https://github.com/git/git/releases/tag/v1.8.3.1

I don't see any problems with git v2.24. Not sure that the patch brings any
helpful value except complicating backports.

Regards,
Nicolas

Re: [PATCH ghak25 v2 0/9] Address NETFILTER_CFG issues

[Pablo Neira Ayuso]

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.

Re: [PATCH ghak25 v2 0/9] Address NETFILTER_CFG issues

[Paul Moore]

On Thu, Jan 16, 2020 at 10:05 AM Pablo Neira Ayuso <pablo@netfilter.org> 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
www.paul-moore.com

Re: [PATCH ghak25 v2 0/9] Address NETFILTER_CFG issues

[Richard Guy Briggs]

On 2020-01-16 14:07, Paul Moore wrote:
> On Thu, Jan 16, 2020 at 10:05 AM Pablo Neira Ayuso <pablo@netfilter.org> 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 <rgb@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

Re: [PATCH ghak25 v2 1/9] netfilter: normalize x_table function declarations

[Richard Guy Briggs]

On 2020-01-08 17:47, Nicolas Dichtel wrote:
> Le 06/01/2020 à 19:54, Richard Guy Briggs a écrit :
> > Git context diffs were being produced with unhelpful declaration types
> > in the place of function names to help identify the funciton in which
> > changes were made.
> Just for my information, how do you reproduce that? With a 'git diff'?

git format-patch is how it is presenting as a problem, which I assume
would also be git diff.

> > Normalize x_table function declarations so that git context diff
> > function labels work as expected.
> > 
> [snip]
> > 
> > -- 
> > 1.8.3.1
> git v1.8.3.1 is seven years old:
> https://github.com/git/git/releases/tag/v1.8.3.1
> 
> I don't see any problems with git v2.24. Not sure that the patch brings any
> helpful value except complicating backports.

It brings value to anyone who is on a distro that is stable and only
slightly behind.  There are other features of git 2.x that I'd like to
start using (git worktrees) but I'll have to wait until I can afford to
upgrade.

> Nicolas

- RGB

--
Richard Guy Briggs <rgb@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

Re: [PATCH ghak25 v2 1/9] netfilter: normalize x_table function declarations

[Paul Moore]

On Mon, Jan 6, 2020 at 1:54 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Git context diffs were being produced with unhelpful declaration types
> in the place of function names to help identify the funciton in which
                                                      ^^^^^^^^
                                                      function
> changes were made.
>
> Normalize x_table function declarations so that git context diff
> function labels work as expected.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  net/netfilter/x_tables.c | 43 ++++++++++++++++++-------------------------
>  1 file changed, 18 insertions(+), 25 deletions(-)

Considering that this patch is a style change in code outside of
audit, and we want to merge this via the audit tree, I think it is
best if you drop the style changes from this patchset.  You can always
submit them later to the netfilter developers.

--
paul moore
www.paul-moore.com

Re: [PATCH ghak25 v2 2/9] netfilter: normalize ebtables function declarations

[Paul Moore]

On Mon, Jan 6, 2020 at 1:55 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> Git context diffs were being produced with unhelpful declaration types
> in the place of function names to help identify the funciton in which
> changes were made.
>
> Normalize ebtables function declarations so that git context diff
> function labels work as expected.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  net/bridge/netfilter/ebtables.c | 100 ++++++++++++++++++++--------------------
>  1 file changed, 51 insertions(+), 49 deletions(-)

My comments from the first patch regarding style chanes also applies here.

--
paul moore
www.paul-moore.com

Re: [PATCH ghak25 v2 3/9] netfilter: normalize ebtables function declarations II

[Paul Moore]

On Mon, Jan 6, 2020 at 1:55 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> Align all function declaration parameters with open parenthesis.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  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
www.paul-moore.com

Re: [PATCH ghak25 v2 4/9] audit: record nfcfg params

[Paul Moore]

On Mon, Jan 6, 2020 at 1:55 PM Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
> ---
>  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
www.paul-moore.com

Re: [PATCH ghak25 v2 7/9] netfilter: ebtables audit table registration

[Paul Moore]

On Mon, Jan 6, 2020 at 1:56 PM Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
> ---
>  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
www.paul-moore.com

Re: [PATCH ghak25 v2 8/9] netfilter: add audit operation field

[Paul Moore]

On Mon, Jan 6, 2020 at 3:23 PM Florian Westphal <fw@strlen.de> wrote:
> Richard Guy Briggs <rgb@redhat.com> 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
www.paul-moore.com

Re: [PATCH ghak25 v2 9/9] netfilter: audit table unregister actions

[Paul Moore]

On Mon, Jan 6, 2020 at 1:56 PM Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
> ---
>  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
www.paul-moore.com

Re: [PATCH ghak25 v2 1/9] netfilter: normalize x_table function declarations

[Richard Guy Briggs]

On 2020-01-30 22:17, Paul Moore wrote:
> On Mon, Jan 6, 2020 at 1:54 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > Git context diffs were being produced with unhelpful declaration types
> > in the place of function names to help identify the funciton in which
>                                                       ^^^^^^^^
>                                                       function
> > changes were made.
> >
> > Normalize x_table function declarations so that git context diff
> > function labels work as expected.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  net/netfilter/x_tables.c | 43 ++++++++++++++++++-------------------------
> >  1 file changed, 18 insertions(+), 25 deletions(-)
> 
> Considering that this patch is a style change in code outside of
> audit, and we want to merge this via the audit tree, I think it is
> best if you drop the style changes from this patchset.  You can always
> submit them later to the netfilter developers.

Fair enough.  They were intended to help make the audit patches cleaner
by giving proper function name context in the diff, but I'll address the
issues and re-submit via netfilter-devel.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@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

Re: [PATCH ghak25 v2 8/9] netfilter: add audit operation field

[Richard Guy Briggs]

On 2020-01-06 21:23, Florian Westphal wrote:
> Richard Guy Briggs <rgb@redhat.com> 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 <rgb@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

Re: [PATCH ghak25 v2 8/9] netfilter: add audit operation field

[Florian Westphal]

Richard Guy Briggs <rgb@redhat.com> 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.

Re: [PATCH ghak25 v2 8/9] netfilter: add audit operation field

[Richard Guy Briggs]

On 2020-02-13 13:34, Florian Westphal wrote:
> Richard Guy Briggs <rgb@redhat.com> 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 <rgb@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

Re: [PATCH ghak25 v2 7/9] netfilter: ebtables audit table registration

[Richard Guy Briggs]

On 2020-01-30 22:18, Paul Moore wrote:
> On Mon, Jan 6, 2020 at 1:56 PM Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
> > ---
> >  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
> www.paul-moore.com
> 

- RGB

--
Richard Guy Briggs <rgb@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

Re: [PATCH ghak25 v2 4/9] audit: record nfcfg params

[Richard Guy Briggs]

On 2020-01-30 22:18, Paul Moore wrote:
> On Mon, Jan 6, 2020 at 1:55 PM Richard Guy Briggs <rgb@redhat.com> 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 <rgb@redhat.com>
> > ---
> >  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
> www.paul-moore.com
> 

- RGB

--
Richard Guy Briggs <rgb@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