[PATCH 0/6 RFC] Address NETFILTER_CFG issues

[PATCH 0/6 RFC] 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.
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.

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) to the record
to make this obvious.

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.

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


Richard Guy Briggs (6):
  netfilter: normalize x_table function declarations
  netfilter: normalize ebtables function declarations
  netfilter: audit only on xtables and ebtables syscall rule or
    standalone
  netfilter: ebtables: audit table registration
  netfilter: add audit operation field
  netfilter: add audit netns ID

 include/linux/audit.h              |    4 +-
 include/linux/netfilter/x_tables.h |    1 +
 include/uapi/linux/audit.h         |    1 +
 kernel/auditsc.c                   |    3 +-
 net/bridge/netfilter/ebtables.c    |  148 +++++++++++++++++++++++-------------
 net/ipv4/netfilter/arp_tables.c    |    2 +-
 net/ipv4/netfilter/ip_tables.c     |    2 +-
 net/ipv6/netfilter/ip6_tables.c    |    2 +-
 net/netfilter/x_tables.c           |   76 +++++++++++--------
 9 files changed, 149 insertions(+), 90 deletions(-)

[PATCH 1/6 RFC] 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 files changed, 18 insertions(+), 25 deletions(-)

diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 14857af..99c27ed 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -81,8 +81,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;
 
@@ -92,8 +91,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;
@@ -112,8 +110,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]);
@@ -131,8 +128,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;
 
@@ -142,8 +138,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;
@@ -162,8 +157,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]);
@@ -205,8 +199,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(uint8_t nfproto, const char *name,
+				       uint8_t revision)
 {
 	struct xt_match *match;
 
@@ -382,8 +376,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",
@@ -1154,11 +1148,10 @@ static int xt_jumpstack_alloc(struct xt_table_info *i)
 	return 0;
 }
 
-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;
 	int ret;
@@ -1367,7 +1360,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,
@@ -1407,7 +1400,7 @@ enum {
 }
 
 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;
@@ -1553,8 +1546,8 @@ static int xt_target_open(struct inode *inode, struct file *file)
  * 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.7.1

Re: [PATCH 1/6 RFC] netfilter: normalize x_table function declarations

[Pablo Neira Ayuso]

On Thu, May 18, 2017 at 01:21:47PM -0400, Richard Guy Briggs 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.

I know of people that usually complain on this cleanups, since they
just add extra work to backports.

But I have no major objection to this if you want to push it forward.

Re: [PATCH 1/6 RFC] netfilter: normalize x_table function declarations

[Richard Guy Briggs]

On 2017-05-24 19:37, Pablo Neira Ayuso wrote:
> On Thu, May 18, 2017 at 01:21:47PM -0400, Richard Guy Briggs 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.
> 
> I know of people that usually complain on this cleanups, since they
> just add extra work to backports.

Arguably all cleanups add work on backports, including spelling fixes.  ;-)

> But I have no major objection to this if you want to push it forward.

Thanks.

- 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

[PATCH 2/6 RFC] 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 |   92 +++++++++++++++++++-------------------
 1 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 79b6991..13d7fe2 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -84,9 +84,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;
@@ -95,17 +95,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) ? EBT_MATCH : EBT_NOMATCH;
 }
 
-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;
@@ -122,9 +122,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;
@@ -171,8 +172,7 @@ 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;
 }
@@ -313,9 +313,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;
@@ -332,26 +332,26 @@ 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);
 }
 
-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;
@@ -386,9 +386,9 @@ unsigned int ebt_do_table(struct sk_buff *skb,
 	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;
@@ -489,8 +489,7 @@ 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,
+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)
@@ -558,9 +557,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;
 
@@ -584,8 +584,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;
 
@@ -602,8 +602,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;
 
@@ -620,8 +620,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;
@@ -645,11 +645,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;
@@ -1157,8 +1157,8 @@ static int do_replace(struct net *net, const void __user *user,
 	return ret;
 }
 
-struct ebt_table *
-ebt_register_table(struct net *net, const struct ebt_table *input_table)
+struct ebt_table * ebt_register_table(struct net *net,
+				      const struct ebt_table *input_table)
 {
 	struct ebt_table_info *newinfo;
 	struct ebt_table *t, *table;
-- 
1.7.1

[PATCH 3/6 RFC] netfilter: audit only on xtables and ebtables syscall rule or standalone

[Richard Guy Briggs]

There were syscall events unsolicited by any audit rule caused by a missing
!audit_dummy_context() check before creating an
iptables/ip6tables/arptables/ebtables NETFILTER_CFG record.  Check
!audit_dummy_context() before creating the NETFILTER_CFG record.

The vast majority of observed 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.  Audit only when there is an existing syscall audit rule, otherwise issue
a standalone record only on table modification rather than empty table
creation.

Add subject attributes to the new standalone NETFILTER_CFGSOLO record using
a newly exported audit_log_task().

Since the record format will change anyways, this seemed like the right time to
change the order of the fields to put the protocol family before the table
name.

Here is a new sample accompanied record:
  type=NETFILTER_CFG msg=audit(1494977049.375:9418): family=2 table=filter entries=84

and unaccompanied case:
  type=UNKNOWN[1331] msg=audit(1494815998.168:163): auid=4294967295 uid=0 gid=0 ses=4294967295 subj=system_u:system_r:iptables_t:s0 pid=597 comm="iptables-restor" exe="/usr/sbin/xtables-multi" family=2 table=filter entries=4

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>
---
 include/linux/audit.h           |    4 +++-
 include/uapi/linux/audit.h      |    1 +
 kernel/auditsc.c                |    3 ++-
 net/bridge/netfilter/ebtables.c |   25 +++++++++++++++++++------
 net/netfilter/x_tables.c        |   26 +++++++++++++++++++-------
 5 files changed, 44 insertions(+), 15 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 2150bdc..b6fcab1 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -155,7 +155,7 @@ extern void		    audit_log_link_denied(const char *operation,
 static inline void	    audit_log_secctx(struct audit_buffer *ab, u32 secid)
 { }
 #endif
-
+extern void audit_log_task(struct audit_buffer *ab);
 extern int audit_log_task_context(struct audit_buffer *ab);
 extern void audit_log_task_info(struct audit_buffer *ab,
 				struct task_struct *tsk);
@@ -205,6 +205,8 @@ static inline void audit_log_link_denied(const char *string,
 { }
 static inline void audit_log_secctx(struct audit_buffer *ab, u32 secid)
 { }
+static inline void audit_log_task(struct audit_buffer *ab)
+{ }
 static inline int audit_log_task_context(struct audit_buffer *ab)
 {
 	return 0;
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 0714a66..8bee3f5 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -112,6 +112,7 @@
 #define AUDIT_FEATURE_CHANGE	1328	/* audit log listing feature changes */
 #define AUDIT_REPLACE		1329	/* Replace auditd if this packet unanswerd */
 #define AUDIT_KERN_MODULE	1330	/* Kernel Module events */
+#define AUDIT_NETFILTER_CFGSOLO	1331	/* Netfilter chain modifications standalone */
 
 #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
 #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index b2dcbe6..8ac38e6 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2383,7 +2383,7 @@ void __audit_log_kern_module(char *name)
 	context->type = AUDIT_KERN_MODULE;
 }
 
-static void audit_log_task(struct audit_buffer *ab)
+void audit_log_task(struct audit_buffer *ab)
 {
 	kuid_t auid, uid;
 	kgid_t gid;
@@ -2404,6 +2404,7 @@ static void audit_log_task(struct audit_buffer *ab)
 	audit_log_untrustedstring(ab, get_task_comm(comm, current));
 	audit_log_d_path_exe(ab, current->mm);
 }
+EXPORT_SYMBOL_GPL(audit_log_task);
 
 /**
  * audit_core_dumps - record information about processes that end abnormally
diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 13d7fe2..743f9e6 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1071,12 +1071,25 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
 	if (audit_enabled) {
 		struct audit_buffer *ab;
 
-		ab = audit_log_start(current->audit_context, GFP_KERNEL,
-				     AUDIT_NETFILTER_CFG);
-		if (ab) {
-			audit_log_format(ab, "table=%s family=%u entries=%u",
-					 repl->name, AF_BRIDGE, repl->nentries);
-			audit_log_end(ab);
+		if(!audit_dummy_context()) {
+			ab = audit_log_start(current->audit_context, GFP_KERNEL,
+					     AUDIT_NETFILTER_CFG);
+			if (ab) {
+				audit_log_format(ab, "family=%u table=%s entries=%u",
+						 AF_BRIDGE, repl->name,
+						 repl->nentries);
+				audit_log_end(ab);
+			}
+		} else if(repl->nentries) {
+			ab = audit_log_start(NULL, GFP_KERNEL,
+					     AUDIT_NETFILTER_CFGSOLO);
+			if (ab) {
+				audit_log_task(ab);
+				audit_log_format(ab, " family=%u table=%s entries=%u",
+						 AF_BRIDGE, repl->name,
+						 repl->nentries);
+				audit_log_end(ab);
+			}
 		}
 	}
 #endif
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 99c27ed..8d28fff 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1195,13 +1195,25 @@ struct xt_table_info *xt_replace_table(struct xt_table *table,
 	if (audit_enabled) {
 		struct audit_buffer *ab;
 
-		ab = audit_log_start(current->audit_context, GFP_KERNEL,
-				     AUDIT_NETFILTER_CFG);
-		if (ab) {
-			audit_log_format(ab, "table=%s family=%u entries=%u",
-					 table->name, table->af,
-					 private->number);
-			audit_log_end(ab);
+		if(!audit_dummy_context()) {
+			ab = audit_log_start(current->audit_context, GFP_KERNEL,
+					     AUDIT_NETFILTER_CFG);
+			if (ab) {
+				audit_log_format(ab, "family=%u table=%s entries=%u",
+						 table->af, table->name,
+						 private->number);
+				audit_log_end(ab);
+			}
+		} else if(private->stacksize || private->number || private->initial_entries) {
+			ab = audit_log_start(NULL, GFP_KERNEL,
+					     AUDIT_NETFILTER_CFGSOLO);
+			if (ab) {
+				audit_log_task(ab);
+				audit_log_format(ab, " family=%u table=%s entries=%u",
+						 table->af, table->name,
+						 private->number);
+				audit_log_end(ab);
+			}
 		}
 	}
 #endif
-- 
1.7.1

Re: [PATCH 3/6 RFC] netfilter: audit only on xtables and ebtables syscall rule or standalone

[Pablo Neira Ayuso]

On Thu, May 18, 2017 at 01:21:49PM -0400, Richard Guy Briggs wrote:
> There were syscall events unsolicited by any audit rule caused by a missing
> !audit_dummy_context() check before creating an
> iptables/ip6tables/arptables/ebtables NETFILTER_CFG record.  Check
> !audit_dummy_context() before creating the NETFILTER_CFG record.
> 
> The vast majority of observed 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.  Audit only when there is an existing syscall audit rule, otherwise issue
> a standalone record only on table modification rather than empty table
> creation.
> 
> Add subject attributes to the new standalone NETFILTER_CFGSOLO record using
> a newly exported audit_log_task().

This new NETFILTER_CFGSOLO looks like audit infra is missing some way
to export a revision / context to userspace? It's duplicating quite a
bit of the code from what I can see in this patch.

> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 0714a66..8bee3f5 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -112,6 +112,7 @@
>  #define AUDIT_FEATURE_CHANGE	1328	/* audit log listing feature changes */
>  #define AUDIT_REPLACE		1329	/* Replace auditd if this packet unanswerd */
>  #define AUDIT_KERN_MODULE	1330	/* Kernel Module events */
> +#define AUDIT_NETFILTER_CFGSOLO	1331	/* Netfilter chain modifications standalone */
>  
>  #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
>  #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index b2dcbe6..8ac38e6 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2383,7 +2383,7 @@ void __audit_log_kern_module(char *name)
>  	context->type = AUDIT_KERN_MODULE;
>  }
>  
> -static void audit_log_task(struct audit_buffer *ab)
> +void audit_log_task(struct audit_buffer *ab)
>  {
>  	kuid_t auid, uid;
>  	kgid_t gid;
> @@ -2404,6 +2404,7 @@ static void audit_log_task(struct audit_buffer *ab)
>  	audit_log_untrustedstring(ab, get_task_comm(comm, current));
>  	audit_log_d_path_exe(ab, current->mm);
>  }
> +EXPORT_SYMBOL_GPL(audit_log_task);
>  
>  /**
>   * audit_core_dumps - record information about processes that end abnormally
> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> index 13d7fe2..743f9e6 100644
> --- a/net/bridge/netfilter/ebtables.c
> +++ b/net/bridge/netfilter/ebtables.c
> @@ -1071,12 +1071,25 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
>  	if (audit_enabled) {
>  		struct audit_buffer *ab;
>  
> -		ab = audit_log_start(current->audit_context, GFP_KERNEL,
> -				     AUDIT_NETFILTER_CFG);
> -		if (ab) {
> -			audit_log_format(ab, "table=%s family=%u entries=%u",
> -					 repl->name, AF_BRIDGE, repl->nentries);
> -			audit_log_end(ab);
> +		if(!audit_dummy_context()) {
> +			ab = audit_log_start(current->audit_context, GFP_KERNEL,
> +					     AUDIT_NETFILTER_CFG);
> +			if (ab) {
> +				audit_log_format(ab, "family=%u table=%s entries=%u",
> +						 AF_BRIDGE, repl->name,
> +						 repl->nentries);
> +				audit_log_end(ab);
> +			}
> +		} else if(repl->nentries) {
> +			ab = audit_log_start(NULL, GFP_KERNEL,
> +					     AUDIT_NETFILTER_CFGSOLO);
> +			if (ab) {
> +				audit_log_task(ab);
> +				audit_log_format(ab, " family=%u table=%s entries=%u",
> +						 AF_BRIDGE, repl->name,
> +						 repl->nentries);
> +				audit_log_end(ab);
> +			}

If you have no other way, probably it is a good idea to wrap this code 
around a function, eg. ebt_audit_replace_finish() ?

Re: [PATCH 3/6 RFC] netfilter: audit only on xtables and ebtables syscall rule or standalone

[Richard Guy Briggs]

On 2017-05-24 19:36, Pablo Neira Ayuso wrote:
> On Thu, May 18, 2017 at 01:21:49PM -0400, Richard Guy Briggs wrote:
> > There were syscall events unsolicited by any audit rule caused by a missing
> > !audit_dummy_context() check before creating an
> > iptables/ip6tables/arptables/ebtables NETFILTER_CFG record.  Check
> > !audit_dummy_context() before creating the NETFILTER_CFG record.
> > 
> > The vast majority of observed 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.  Audit only when there is an existing syscall audit rule, otherwise issue
> > a standalone record only on table modification rather than empty table
> > creation.
> > 
> > Add subject attributes to the new standalone NETFILTER_CFGSOLO record using
> > a newly exported audit_log_task().
> 
> This new NETFILTER_CFGSOLO looks like audit infra is missing some way
> to export a revision / context to userspace? It's duplicating quite a
> bit of the code from what I can see in this patch.

Interesting you brought that up.  I did another revision that stores
this information in a struct audit_context and greatly simplifies the
code in netfilter and re-uses code in audit itself, which may be a
better way to go, but that idea needed to settle a bit more before
seeing peer review.
> 
I'm also having doubts about two record types.

> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 0714a66..8bee3f5 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -112,6 +112,7 @@
> >  #define AUDIT_FEATURE_CHANGE	1328	/* audit log listing feature changes */
> >  #define AUDIT_REPLACE		1329	/* Replace auditd if this packet unanswerd */
> >  #define AUDIT_KERN_MODULE	1330	/* Kernel Module events */
> > +#define AUDIT_NETFILTER_CFGSOLO	1331	/* Netfilter chain modifications standalone */
> >  
> >  #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
> >  #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index b2dcbe6..8ac38e6 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2383,7 +2383,7 @@ void __audit_log_kern_module(char *name)
> >  	context->type = AUDIT_KERN_MODULE;
> >  }
> >  
> > -static void audit_log_task(struct audit_buffer *ab)
> > +void audit_log_task(struct audit_buffer *ab)
> >  {
> >  	kuid_t auid, uid;
> >  	kgid_t gid;
> > @@ -2404,6 +2404,7 @@ static void audit_log_task(struct audit_buffer *ab)
> >  	audit_log_untrustedstring(ab, get_task_comm(comm, current));
> >  	audit_log_d_path_exe(ab, current->mm);
> >  }
> > +EXPORT_SYMBOL_GPL(audit_log_task);
> >  
> >  /**
> >   * audit_core_dumps - record information about processes that end abnormally
> > diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> > index 13d7fe2..743f9e6 100644
> > --- a/net/bridge/netfilter/ebtables.c
> > +++ b/net/bridge/netfilter/ebtables.c
> > @@ -1071,12 +1071,25 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
> >  	if (audit_enabled) {
> >  		struct audit_buffer *ab;
> >  
> > -		ab = audit_log_start(current->audit_context, GFP_KERNEL,
> > -				     AUDIT_NETFILTER_CFG);
> > -		if (ab) {
> > -			audit_log_format(ab, "table=%s family=%u entries=%u",
> > -					 repl->name, AF_BRIDGE, repl->nentries);
> > -			audit_log_end(ab);
> > +		if(!audit_dummy_context()) {
> > +			ab = audit_log_start(current->audit_context, GFP_KERNEL,
> > +					     AUDIT_NETFILTER_CFG);
> > +			if (ab) {
> > +				audit_log_format(ab, "family=%u table=%s entries=%u",
> > +						 AF_BRIDGE, repl->name,
> > +						 repl->nentries);
> > +				audit_log_end(ab);
> > +			}
> > +		} else if(repl->nentries) {
> > +			ab = audit_log_start(NULL, GFP_KERNEL,
> > +					     AUDIT_NETFILTER_CFGSOLO);
> > +			if (ab) {
> > +				audit_log_task(ab);
> > +				audit_log_format(ab, " family=%u table=%s entries=%u",
> > +						 AF_BRIDGE, repl->name,
> > +						 repl->nentries);
> > +				audit_log_end(ab);
> > +			}
> 
> If you have no other way, probably it is a good idea to wrap this code 
> around a function, eg. ebt_audit_replace_finish() ?

Yes, sending the values I need into the audit subsystem to avoid
exposing some bits of audit did come to mind.  And it would bring 6
lines here down to one instead of adding 19.

- 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 3/6 RFC] netfilter: audit only on xtables and ebtables syscall rule or standalone

[Paul Moore]

On Wed, May 24, 2017 at 2:09 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2017-05-24 19:36, Pablo Neira Ayuso wrote:
>> On Thu, May 18, 2017 at 01:21:49PM -0400, Richard Guy Briggs wrote:
>> > There were syscall events unsolicited by any audit rule caused by a missing
>> > !audit_dummy_context() check before creating an
>> > iptables/ip6tables/arptables/ebtables NETFILTER_CFG record.  Check
>> > !audit_dummy_context() before creating the NETFILTER_CFG record.
>> >
>> > The vast majority of observed 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.  Audit only when there is an existing syscall audit rule, otherwise issue
>> > a standalone record only on table modification rather than empty table
>> > creation.
>> >
>> > Add subject attributes to the new standalone NETFILTER_CFGSOLO record using
>> > a newly exported audit_log_task().
>>
>> This new NETFILTER_CFGSOLO looks like audit infra is missing some way
>> to export a revision / context to userspace? It's duplicating quite a
>> bit of the code from what I can see in this patch.
>
> Interesting you brought that up.  I did another revision that stores
> this information in a struct audit_context and greatly simplifies the
> code in netfilter and re-uses code in audit itself, which may be a
> better way to go, but that idea needed to settle a bit more before
> seeing peer review.
>
> I'm also having doubts about two record types.

Richard and I had a discussion about this a week (or two?) ago and I'm
currently of the opinion that two record types are a mistake.  I agree
that we need to add the audit_dummy_context() check but the other
changes in this patch I'm less excited about.

-- 
paul moore
www.paul-moore.com

[PATCH 4/6 RFC] 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.

Audit only when there is an existing syscall audit rule, otherwise issue a
standalone record only on table modification rather than empty table creation.
Include subject attributes to the new standalone NETFILTER_CFGSOLO record using
audit_log_task().

Here is a sample accompanied record:
  type=NETFILTER_CFG msg=audit(1494907217.558:5403): family=7 table=filter entries=0

and unaccompanied case:
  type=UNKNOWN[1331] msg=audit(1494723394.832:111): auid=4294967295 uid=0 gid=0 ses=4294967295 subj=system_u:system_r:iptables_t:s0 pid=556 comm="ebtables-restor" exe="/usr/sbin/ebtables-restore" family=7 table=broute entries=1

See: https://github.com/linux-audit/audit-kernel/issues/43

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

diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 743f9e6..7499232 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1251,6 +1251,32 @@ struct ebt_table * ebt_register_table(struct net *net,
 	}
 	list_add(&table->list, &net->xt.tables[NFPROTO_BRIDGE]);
 	mutex_unlock(&ebt_mutex);
+#ifdef CONFIG_AUDIT
+	if (audit_enabled) {
+		struct audit_buffer *ab;
+
+		if(!audit_dummy_context()) {
+			ab = audit_log_start(current->audit_context, GFP_KERNEL,
+					     AUDIT_NETFILTER_CFG);
+			if (ab) {
+				audit_log_format(ab, "family=%u table=%s entries=%u",
+						 AF_BRIDGE, repl->name,
+						 repl->nentries);
+				audit_log_end(ab);
+			}
+		} else if(repl->nentries) {
+			ab = audit_log_start(NULL, GFP_KERNEL,
+					     AUDIT_NETFILTER_CFGSOLO);
+			if (ab) {
+				audit_log_task(ab);
+				audit_log_format(ab, " family=%u table=%s entries=%u",
+						 AF_BRIDGE, repl->name,
+						 repl->nentries);
+				audit_log_end(ab);
+			}
+		}
+	}
+#endif
 	return table;
 free_unlock:
 	mutex_unlock(&ebt_mutex);
-- 
1.7.1

Re: [PATCH 4/6 RFC] netfilter: ebtables: audit table registration

[Paul Moore]

On Thu, May 18, 2017 at 1:21 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.
>
> Audit only when there is an existing syscall audit rule, otherwise issue a
> standalone record only on table modification rather than empty table creation.
> Include subject attributes to the new standalone NETFILTER_CFGSOLO record using
> audit_log_task().
>
> Here is a sample accompanied record:
>   type=NETFILTER_CFG msg=audit(1494907217.558:5403): family=7 table=filter entries=0
>
> and unaccompanied case:
>   type=UNKNOWN[1331] msg=audit(1494723394.832:111): auid=4294967295 uid=0 gid=0 ses=4294967295 subj=system_u:system_r:iptables_t:s0 pid=556 comm="ebtables-restor" exe="/usr/sbin/ebtables-restore" family=7 table=broute entries=1
>
> See: https://github.com/linux-audit/audit-kernel/issues/43
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  net/bridge/netfilter/ebtables.c |   26 ++++++++++++++++++++++++++
>  1 files changed, 26 insertions(+), 0 deletions(-)
>
> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> index 743f9e6..7499232 100644
> --- a/net/bridge/netfilter/ebtables.c
> +++ b/net/bridge/netfilter/ebtables.c
> @@ -1251,6 +1251,32 @@ struct ebt_table * ebt_register_table(struct net *net,
>         }
>         list_add(&table->list, &net->xt.tables[NFPROTO_BRIDGE]);
>         mutex_unlock(&ebt_mutex);
> +#ifdef CONFIG_AUDIT
> +       if (audit_enabled) {
> +               struct audit_buffer *ab;
> +
> +               if(!audit_dummy_context()) {
> +                       ab = audit_log_start(current->audit_context, GFP_KERNEL,
> +                                            AUDIT_NETFILTER_CFG);
> +                       if (ab) {
> +                               audit_log_format(ab, "family=%u table=%s entries=%u",
> +                                                AF_BRIDGE, repl->name,
> +                                                repl->nentries);
> +                               audit_log_end(ab);
> +                       }
> +               } else if(repl->nentries) {
> +                       ab = audit_log_start(NULL, GFP_KERNEL,
> +                                            AUDIT_NETFILTER_CFGSOLO);
> +                       if (ab) {
> +                               audit_log_task(ab);
> +                               audit_log_format(ab, " family=%u table=%s entries=%u",
> +                                                AF_BRIDGE, repl->name,
> +                                                repl->nentries);
> +                               audit_log_end(ab);
> +                       }
> +               }
> +       }
> +#endif

Similar comments from patch 3/6 apply here, let's stick with a single
audit record type.

-- 
paul moore
www.paul-moore.com

[PATCH 5/6 RFC] netfilter: add audit operation field

[Richard Guy Briggs]

Add the operation performed (register or replace) to the NETFILTER_CFG and
NETFILTER_CFGSOLO records.

Here are sample records for accompanied:
  type=NETFILTER_CFG msg=audit(1494981627.248:9764): op=replace family=7 table=broute entries=0

and unaccompanied cases:
  type=UNKNOWN[1331] msg=audit(1494815998.178:167): auid=4294967295 uid=0 gid=0 ses=4294967295 subj=system_u:system_r:iptables_t:s0 pid=598 comm="ip6tables-resto" exe="/usr/sbin/xtables-multi" op=replace family=10 table=filter entries=4

See: https://github.com/linux-audit/audit-kernel/issues/25

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 net/bridge/netfilter/ebtables.c |    8 ++++----
 net/netfilter/x_tables.c        |    5 +++--
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 7499232..59b63a8 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1075,7 +1075,7 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
 			ab = audit_log_start(current->audit_context, GFP_KERNEL,
 					     AUDIT_NETFILTER_CFG);
 			if (ab) {
-				audit_log_format(ab, "family=%u table=%s entries=%u",
+				audit_log_format(ab, "op=replace family=%u table=%s entries=%u",
 						 AF_BRIDGE, repl->name,
 						 repl->nentries);
 				audit_log_end(ab);
@@ -1085,7 +1085,7 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
 					     AUDIT_NETFILTER_CFGSOLO);
 			if (ab) {
 				audit_log_task(ab);
-				audit_log_format(ab, " family=%u table=%s entries=%u",
+				audit_log_format(ab, " op=replace family=%u table=%s entries=%u",
 						 AF_BRIDGE, repl->name,
 						 repl->nentries);
 				audit_log_end(ab);
@@ -1259,7 +1259,7 @@ struct ebt_table * ebt_register_table(struct net *net,
 			ab = audit_log_start(current->audit_context, GFP_KERNEL,
 					     AUDIT_NETFILTER_CFG);
 			if (ab) {
-				audit_log_format(ab, "family=%u table=%s entries=%u",
+				audit_log_format(ab, "op=register family=%u table=%s entries=%u",
 						 AF_BRIDGE, repl->name,
 						 repl->nentries);
 				audit_log_end(ab);
@@ -1269,7 +1269,7 @@ struct ebt_table * ebt_register_table(struct net *net,
 					     AUDIT_NETFILTER_CFGSOLO);
 			if (ab) {
 				audit_log_task(ab);
-				audit_log_format(ab, " family=%u table=%s entries=%u",
+				audit_log_format(ab, " op=register family=%u table=%s entries=%u",
 						 AF_BRIDGE, repl->name,
 						 repl->nentries);
 				audit_log_end(ab);
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 8d28fff..395ebd3 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -1199,7 +1199,8 @@ struct xt_table_info *xt_replace_table(struct xt_table *table,
 			ab = audit_log_start(current->audit_context, GFP_KERNEL,
 					     AUDIT_NETFILTER_CFG);
 			if (ab) {
-				audit_log_format(ab, "family=%u table=%s entries=%u",
+				audit_log_format(ab, "op=%s family=%u table=%s entries=%u",
+						 private->number ? "replace" : "register",
 						 table->af, table->name,
 						 private->number);
 				audit_log_end(ab);
@@ -1209,7 +1210,7 @@ struct xt_table_info *xt_replace_table(struct xt_table *table,
 					     AUDIT_NETFILTER_CFGSOLO);
 			if (ab) {
 				audit_log_task(ab);
-				audit_log_format(ab, " family=%u table=%s entries=%u",
+				audit_log_format(ab, " op=replace family=%u table=%s entries=%u",
 						 table->af, table->name,
 						 private->number);
 				audit_log_end(ab);
-- 
1.7.1

Re: [PATCH 5/6 RFC] netfilter: add audit operation field

[Paul Moore]

On Thu, May 18, 2017 at 1:21 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Add the operation performed (register or replace) to the NETFILTER_CFG and
> NETFILTER_CFGSOLO records.
>
> Here are sample records for accompanied:
>   type=NETFILTER_CFG msg=audit(1494981627.248:9764): op=replace family=7 table=broute entries=0
>
> and unaccompanied cases:
>   type=UNKNOWN[1331] msg=audit(1494815998.178:167): auid=4294967295 uid=0 gid=0 ses=4294967295 subj=system_u:system_r:iptables_t:s0 pid=598 comm="ip6tables-resto" exe="/usr/sbin/xtables-multi" op=replace family=10 table=filter entries=4

A reminder to add new fields to the end of the record.

> See: https://github.com/linux-audit/audit-kernel/issues/25
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  net/bridge/netfilter/ebtables.c |    8 ++++----
>  net/netfilter/x_tables.c        |    5 +++--
>  2 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> index 7499232..59b63a8 100644
> --- a/net/bridge/netfilter/ebtables.c
> +++ b/net/bridge/netfilter/ebtables.c
> @@ -1075,7 +1075,7 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
>                         ab = audit_log_start(current->audit_context, GFP_KERNEL,
>                                              AUDIT_NETFILTER_CFG);
>                         if (ab) {
> -                               audit_log_format(ab, "family=%u table=%s entries=%u",
> +                               audit_log_format(ab, "op=replace family=%u table=%s entries=%u",
>                                                  AF_BRIDGE, repl->name,
>                                                  repl->nentries);
>                                 audit_log_end(ab);
> @@ -1085,7 +1085,7 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
>                                              AUDIT_NETFILTER_CFGSOLO);
>                         if (ab) {
>                                 audit_log_task(ab);
> -                               audit_log_format(ab, " family=%u table=%s entries=%u",
> +                               audit_log_format(ab, " op=replace family=%u table=%s entries=%u",
>                                                  AF_BRIDGE, repl->name,
>                                                  repl->nentries);
>                                 audit_log_end(ab);
> @@ -1259,7 +1259,7 @@ struct ebt_table * ebt_register_table(struct net *net,
>                         ab = audit_log_start(current->audit_context, GFP_KERNEL,
>                                              AUDIT_NETFILTER_CFG);
>                         if (ab) {
> -                               audit_log_format(ab, "family=%u table=%s entries=%u",
> +                               audit_log_format(ab, "op=register family=%u table=%s entries=%u",
>                                                  AF_BRIDGE, repl->name,
>                                                  repl->nentries);
>                                 audit_log_end(ab);
> @@ -1269,7 +1269,7 @@ struct ebt_table * ebt_register_table(struct net *net,
>                                              AUDIT_NETFILTER_CFGSOLO);
>                         if (ab) {
>                                 audit_log_task(ab);
> -                               audit_log_format(ab, " family=%u table=%s entries=%u",
> +                               audit_log_format(ab, " op=register family=%u table=%s entries=%u",
>                                                  AF_BRIDGE, repl->name,
>                                                  repl->nentries);
>                                 audit_log_end(ab);
> diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
> index 8d28fff..395ebd3 100644
> --- a/net/netfilter/x_tables.c
> +++ b/net/netfilter/x_tables.c
> @@ -1199,7 +1199,8 @@ struct xt_table_info *xt_replace_table(struct xt_table *table,
>                         ab = audit_log_start(current->audit_context, GFP_KERNEL,
>                                              AUDIT_NETFILTER_CFG);
>                         if (ab) {
> -                               audit_log_format(ab, "family=%u table=%s entries=%u",
> +                               audit_log_format(ab, "op=%s family=%u table=%s entries=%u",
> +                                                private->number ? "replace" : "register",
>                                                  table->af, table->name,
>                                                  private->number);
>                                 audit_log_end(ab);
> @@ -1209,7 +1210,7 @@ struct xt_table_info *xt_replace_table(struct xt_table *table,
>                                              AUDIT_NETFILTER_CFGSOLO);
>                         if (ab) {
>                                 audit_log_task(ab);
> -                               audit_log_format(ab, " family=%u table=%s entries=%u",
> +                               audit_log_format(ab, " op=replace family=%u table=%s entries=%u",
>                                                  table->af, table->name,
>                                                  private->number);
>                                 audit_log_end(ab);
> --
> 1.7.1

-- 
paul moore
www.paul-moore.com

[PATCH 6/6 RFC] netfilter: add audit netns ID

[Richard Guy Briggs]

Seemingly duplicate NETFILTER_CFG records are not actually exact duplicates
that are caused by netfilter table initialization in different network
namespaces from the same syscall.  To differentiate the NETFILTER_CFG records,
the network namespace ID (proc inode) was added to the record to make this
source evident.

Here is a sample event with accompanied records:
  time->Sun May 14 22:40:26 2017
  type=PROCTITLE msg=audit(1494816026.072:248): proctitle=2F7362696E2F6D6F6470726F6265002D71002D2D0065627461626C655F6E6174
  type=KERN_MODULE msg=audit(1494816026.072:248): name="ebtable_nat"
  type=SYSCALL msg=audit(1494816026.072:248): arch=c000003e syscall=313 success=yes exit=0 a0=0 a1=55c4648d4106 a2=0 a3=0 items=0 ppid=84 pid=431 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=4294967295 comm="modprobe" exe="/usr/bin/kmod" subj=system_u:system_r:insmod_t:s0 key="mod-load"
  type=NETFILTER_CFG msg=audit(1494816026.072:248): op=register net=324 family=7 table=nat entries=0
  type=NETFILTER_CFG msg=audit(1494816026.072:248): op=register net=121 family=7 table=nat entries=0

and unaccompanied cases:
  type=UNKNOWN[1331] msg=audit(1494815998.178:167): auid=4294967295 uid=0 gid=0 ses=4294967295 subj=system_u:system_r:iptables_t:s0 pid=598 comm="ip6tables-resto" exe="/usr/sbin/xtables-multi" op=replace net=121 family=10 table=filter entries=4

See: https://github.com/linux-audit/audit-kernel/issues/25

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/linux/netfilter/x_tables.h |    1 +
 net/bridge/netfilter/ebtables.c    |   13 +++++++++----
 net/ipv4/netfilter/arp_tables.c    |    2 +-
 net/ipv4/netfilter/ip_tables.c     |    2 +-
 net/ipv6/netfilter/ip6_tables.c    |    2 +-
 net/netfilter/x_tables.c           |   10 +++++++---
 6 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/include/linux/netfilter/x_tables.h b/include/linux/netfilter/x_tables.h
index be378cf..6be4a04 100644
--- a/include/linux/netfilter/x_tables.h
+++ b/include/linux/netfilter/x_tables.h
@@ -308,6 +308,7 @@ struct xt_table *xt_register_table(struct net *net,
 struct xt_table_info *xt_replace_table(struct xt_table *table,
 				       unsigned int num_counters,
 				       struct xt_table_info *newinfo,
+				       struct net *net,
 				       int *error);
 
 struct xt_match *xt_find_match(u8 af, const char *name, u8 revision);
diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 59b63a8..0f77b2a 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -27,6 +27,7 @@
 #include <linux/smp.h>
 #include <linux/cpumask.h>
 #include <linux/audit.h>
+#define PROC_DYNAMIC_FIRST 0xF0000000U
 #include <net/sock.h>
 /* needed for logical [in,out]-dev filtering */
 #include "../br_private.h"
@@ -1075,7 +1076,8 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
 			ab = audit_log_start(current->audit_context, GFP_KERNEL,
 					     AUDIT_NETFILTER_CFG);
 			if (ab) {
-				audit_log_format(ab, "op=replace family=%u table=%s entries=%u",
+				audit_log_format(ab, "op=replace net=%u family=%u table=%s entries=%u",
+						 net->ns.inum - PROC_DYNAMIC_FIRST,
 						 AF_BRIDGE, repl->name,
 						 repl->nentries);
 				audit_log_end(ab);
@@ -1085,7 +1087,8 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
 					     AUDIT_NETFILTER_CFGSOLO);
 			if (ab) {
 				audit_log_task(ab);
-				audit_log_format(ab, " op=replace family=%u table=%s entries=%u",
+				audit_log_format(ab, " op=replace net=%u family=%u table=%s entries=%u",
+						 net->ns.inum - PROC_DYNAMIC_FIRST,
 						 AF_BRIDGE, repl->name,
 						 repl->nentries);
 				audit_log_end(ab);
@@ -1259,7 +1262,8 @@ struct ebt_table * ebt_register_table(struct net *net,
 			ab = audit_log_start(current->audit_context, GFP_KERNEL,
 					     AUDIT_NETFILTER_CFG);
 			if (ab) {
-				audit_log_format(ab, "op=register family=%u table=%s entries=%u",
+				audit_log_format(ab, "op=register net=%u family=%u table=%s entries=%u",
+						 net->ns.inum - PROC_DYNAMIC_FIRST,
 						 AF_BRIDGE, repl->name,
 						 repl->nentries);
 				audit_log_end(ab);
@@ -1269,7 +1273,8 @@ struct ebt_table * ebt_register_table(struct net *net,
 					     AUDIT_NETFILTER_CFGSOLO);
 			if (ab) {
 				audit_log_task(ab);
-				audit_log_format(ab, " op=register family=%u table=%s entries=%u",
+				audit_log_format(ab, " op=register net=%u family=%u table=%s entries=%u",
+						 net->ns.inum - PROC_DYNAMIC_FIRST,
 						 AF_BRIDGE, repl->name,
 						 repl->nentries);
 				audit_log_end(ab);
diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index 6241a81..4933a5a 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -904,7 +904,7 @@ static int __do_replace(struct net *net, const char *name,
 		goto put_module;
 	}
 
-	oldinfo = xt_replace_table(t, num_counters, newinfo, &ret);
+	oldinfo = xt_replace_table(t, num_counters, newinfo, net, &ret);
 	if (!oldinfo)
 		goto put_module;
 
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 384b857..c638607 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -1066,7 +1066,7 @@ static int get_info(struct net *net, void __user *user,
 		goto put_module;
 	}
 
-	oldinfo = xt_replace_table(t, num_counters, newinfo, &ret);
+	oldinfo = xt_replace_table(t, num_counters, newinfo, net, &ret);
 	if (!oldinfo)
 		goto put_module;
 
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 1e15c54..87ef83b 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -1096,7 +1096,7 @@ static int get_info(struct net *net, void __user *user,
 		goto put_module;
 	}
 
-	oldinfo = xt_replace_table(t, num_counters, newinfo, &ret);
+	oldinfo = xt_replace_table(t, num_counters, newinfo, net, &ret);
 	if (!oldinfo)
 		goto put_module;
 
diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
index 395ebd3..35533f1 100644
--- a/net/netfilter/x_tables.c
+++ b/net/netfilter/x_tables.c
@@ -26,6 +26,7 @@
 #include <linux/mm.h>
 #include <linux/slab.h>
 #include <linux/audit.h>
+#define PROC_DYNAMIC_FIRST 0xF0000000U
 #include <linux/user_namespace.h>
 #include <net/net_namespace.h>
 
@@ -1151,6 +1152,7 @@ static int xt_jumpstack_alloc(struct xt_table_info *i)
 struct xt_table_info *xt_replace_table(struct xt_table *table,
 				       unsigned int num_counters,
 				       struct xt_table_info *newinfo,
+				       struct net *net,
 				       int *error)
 {
 	struct xt_table_info *private;
@@ -1199,8 +1201,9 @@ struct xt_table_info *xt_replace_table(struct xt_table *table,
 			ab = audit_log_start(current->audit_context, GFP_KERNEL,
 					     AUDIT_NETFILTER_CFG);
 			if (ab) {
-				audit_log_format(ab, "op=%s family=%u table=%s entries=%u",
+				audit_log_format(ab, "op=%s net=%u family=%u table=%s entries=%u",
 						 private->number ? "replace" : "register",
+						 net->ns.inum - PROC_DYNAMIC_FIRST,
 						 table->af, table->name,
 						 private->number);
 				audit_log_end(ab);
@@ -1210,7 +1213,8 @@ struct xt_table_info *xt_replace_table(struct xt_table *table,
 					     AUDIT_NETFILTER_CFGSOLO);
 			if (ab) {
 				audit_log_task(ab);
-				audit_log_format(ab, " op=replace family=%u table=%s entries=%u",
+				audit_log_format(ab, " op=replace net=%u family=%u table=%s entries=%u",
+						 net->ns.inum - PROC_DYNAMIC_FIRST,
 						 table->af, table->name,
 						 private->number);
 				audit_log_end(ab);
@@ -1251,7 +1255,7 @@ struct xt_table *xt_register_table(struct net *net,
 	/* Simplifies replace_table code. */
 	table->private = bootstrap;
 
-	if (!xt_replace_table(table, 0, newinfo, &ret))
+	if (!xt_replace_table(table, 0, newinfo, net, &ret))
 		goto unlock;
 
 	private = table->private;
-- 
1.7.1

Re: [PATCH 6/6 RFC] netfilter: add audit netns ID

[Pablo Neira Ayuso]

Cc'ing Eric Biederman.

On Thu, May 18, 2017 at 01:21:52PM -0400, Richard Guy Briggs wrote:
> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> index 59b63a8..0f77b2a 100644
> --- a/net/bridge/netfilter/ebtables.c
> +++ b/net/bridge/netfilter/ebtables.c
> @@ -27,6 +27,7 @@
>  #include <linux/smp.h>
>  #include <linux/cpumask.h>
>  #include <linux/audit.h>
> +#define PROC_DYNAMIC_FIRST 0xF0000000U
>  #include <net/sock.h>
>  /* needed for logical [in,out]-dev filtering */
>  #include "../br_private.h"
> @@ -1075,7 +1076,8 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
>  			ab = audit_log_start(current->audit_context, GFP_KERNEL,
>  					     AUDIT_NETFILTER_CFG);
>  			if (ab) {
> -				audit_log_format(ab, "op=replace family=%u table=%s entries=%u",
> +				audit_log_format(ab, "op=replace net=%u family=%u table=%s entries=%u",
> +						 net->ns.inum - PROC_DYNAMIC_FIRST,

IIRC, there was a discussion on exposing netns i-node number to
userspace time ago on netdev and Eric Biederman was not happy about
this?

Re: [PATCH 6/6 RFC] netfilter: add audit netns ID

[Richard Guy Briggs]

On 2017-05-24 19:31, Pablo Neira Ayuso wrote:
> Cc'ing Eric Biederman.
> 
> On Thu, May 18, 2017 at 01:21:52PM -0400, Richard Guy Briggs wrote:
> > diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> > index 59b63a8..0f77b2a 100644
> > --- a/net/bridge/netfilter/ebtables.c
> > +++ b/net/bridge/netfilter/ebtables.c
> > @@ -27,6 +27,7 @@
> >  #include <linux/smp.h>
> >  #include <linux/cpumask.h>
> >  #include <linux/audit.h>
> > +#define PROC_DYNAMIC_FIRST 0xF0000000U
> >  #include <net/sock.h>
> >  /* needed for logical [in,out]-dev filtering */
> >  #include "../br_private.h"
> > @@ -1075,7 +1076,8 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
> >  			ab = audit_log_start(current->audit_context, GFP_KERNEL,
> >  					     AUDIT_NETFILTER_CFG);
> >  			if (ab) {
> > -				audit_log_format(ab, "op=replace family=%u table=%s entries=%u",
> > +				audit_log_format(ab, "op=replace net=%u family=%u table=%s entries=%u",
> > +						 net->ns.inum - PROC_DYNAMIC_FIRST,
> 
> IIRC, there was a discussion on exposing netns i-node number to
> userspace time ago on netdev and Eric Biederman was not happy about
> this?

He was not happy about it being exposed in the /proc filesystem.  We've
been talking since then and while we've not come to a definitive
conclusion there is a communication channel open.

This is more of an RFC patch than the rest of this set and I didn't
seriously expect this one to be accepted, I did want to present the idea
to see if there were concerns or better ideas generated how to
differentiate this record from a seemingly identical one.  The only
other ID would be the network namespace' struct pointer.

At this stage, one thing that is missing is a device number to qualify
this namespace ID.

Once I started printing the namespace proc inode number (minus the
starting offset) in decimal, it was very clear what was happenning and
seemed worth sharing that debugging tool patch.

- 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 6/6 RFC] netfilter: add audit netns ID

[Eric W. Biederman]

Richard Guy Briggs <rgb@redhat.com> writes:

> On 2017-05-24 19:31, Pablo Neira Ayuso wrote:
>> Cc'ing Eric Biederman.
>> 
>> On Thu, May 18, 2017 at 01:21:52PM -0400, Richard Guy Briggs wrote:
>> > diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
>> > index 59b63a8..0f77b2a 100644
>> > --- a/net/bridge/netfilter/ebtables.c
>> > +++ b/net/bridge/netfilter/ebtables.c
>> > @@ -27,6 +27,7 @@
>> >  #include <linux/smp.h>
>> >  #include <linux/cpumask.h>
>> >  #include <linux/audit.h>
>> > +#define PROC_DYNAMIC_FIRST 0xF0000000U
>> >  #include <net/sock.h>
>> >  /* needed for logical [in,out]-dev filtering */
>> >  #include "../br_private.h"
>> > @@ -1075,7 +1076,8 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
>> >  			ab = audit_log_start(current->audit_context, GFP_KERNEL,
>> >  					     AUDIT_NETFILTER_CFG);
>> >  			if (ab) {
>> > -				audit_log_format(ab, "op=replace family=%u table=%s entries=%u",
>> > +				audit_log_format(ab, "op=replace net=%u family=%u table=%s entries=%u",
>> > +						 net->ns.inum - PROC_DYNAMIC_FIRST,
>> 
>> IIRC, there was a discussion on exposing netns i-node number to
>> userspace time ago on netdev and Eric Biederman was not happy about
>> this?
>
> He was not happy about it being exposed in the /proc filesystem.  We've
> been talking since then and while we've not come to a definitive
> conclusion there is a communication channel open.
>
> This is more of an RFC patch than the rest of this set and I didn't
> seriously expect this one to be accepted, I did want to present the idea
> to see if there were concerns or better ideas generated how to
> differentiate this record from a seemingly identical one.  The only
> other ID would be the network namespace' struct pointer.
>
> At this stage, one thing that is missing is a device number to qualify
> this namespace ID.
>
> Once I started printing the namespace proc inode number (minus the
> starting offset) in decimal, it was very clear what was happenning and
> seemed worth sharing that debugging tool patch.

If the appropriate device number and full inode number is included I
don't have any deep problems with the idea.  I don't like the bare inode
number as we have had in the past and may in the future have these inode
numbers in multiple filesystems so the inode number by itself is not
unique.

Eric

Re: [PATCH 6/6 RFC] netfilter: add audit netns ID

[Paul Moore]

On Wed, May 24, 2017 at 3:44 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Richard Guy Briggs <rgb@redhat.com> writes:
>
>> On 2017-05-24 19:31, Pablo Neira Ayuso wrote:
>>> Cc'ing Eric Biederman.
>>>
>>> On Thu, May 18, 2017 at 01:21:52PM -0400, Richard Guy Briggs wrote:
>>> > diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
>>> > index 59b63a8..0f77b2a 100644
>>> > --- a/net/bridge/netfilter/ebtables.c
>>> > +++ b/net/bridge/netfilter/ebtables.c
>>> > @@ -27,6 +27,7 @@
>>> >  #include <linux/smp.h>
>>> >  #include <linux/cpumask.h>
>>> >  #include <linux/audit.h>
>>> > +#define PROC_DYNAMIC_FIRST 0xF0000000U
>>> >  #include <net/sock.h>
>>> >  /* needed for logical [in,out]-dev filtering */
>>> >  #include "../br_private.h"
>>> > @@ -1075,7 +1076,8 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
>>> >                    ab = audit_log_start(current->audit_context, GFP_KERNEL,
>>> >                                         AUDIT_NETFILTER_CFG);
>>> >                    if (ab) {
>>> > -                          audit_log_format(ab, "op=replace family=%u table=%s entries=%u",
>>> > +                          audit_log_format(ab, "op=replace net=%u family=%u table=%s entries=%u",
>>> > +                                           net->ns.inum - PROC_DYNAMIC_FIRST,
>>>
>>> IIRC, there was a discussion on exposing netns i-node number to
>>> userspace time ago on netdev and Eric Biederman was not happy about
>>> this?
>>
>> He was not happy about it being exposed in the /proc filesystem.  We've
>> been talking since then and while we've not come to a definitive
>> conclusion there is a communication channel open.
>>
>> This is more of an RFC patch than the rest of this set and I didn't
>> seriously expect this one to be accepted, I did want to present the idea
>> to see if there were concerns or better ideas generated how to
>> differentiate this record from a seemingly identical one.  The only
>> other ID would be the network namespace' struct pointer.
>>
>> At this stage, one thing that is missing is a device number to qualify
>> this namespace ID.
>>
>> Once I started printing the namespace proc inode number (minus the
>> starting offset) in decimal, it was very clear what was happenning and
>> seemed worth sharing that debugging tool patch.
>
> If the appropriate device number and full inode number is included I
> don't have any deep problems with the idea.  I don't like the bare inode
> number as we have had in the past and may in the future have these inode
> numbers in multiple filesystems so the inode number by itself is not
> unique.

Let's punt on this netfilter record specific patch until we sort out
the general problem of recording namespace/container information in
the audit records.

-- 
paul moore
www.paul-moore.com