Address NETFILTER_CFG issues

RFC PATCH In review Richard Guy Briggs 4 participants 18 messages May 18, 2017 – Jun 2, 2017

Subsystems (4)

Networking netfilter Ethernet bridge Audit

lore

Share & tools

lore

Richard Guy Briggs

author

2017-05-18 17:21 UTC #0 lore raw

cover Address NETFILTER_CFG issues

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(-)

Richard Guy Briggs

author

2017-05-18 17:21 UTC #1 lore raw

RFC PATCH 1/6 netfilter: normalize x_table function declarations

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 <[email protected]>

---
 net/netfilter/x_tables.c |   43 ++++++++++++++++++-------------------------
 1 files changed, 18 insertions(+), 25 deletions(-)

net/netfilter/x_tables.c +18 ~~-25~~

index 14857af..99c27ed 100644
@@ -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);

Signature · 1 line

1.7.1

Richard Guy Briggs

author

2017-05-18 17:21 UTC #4 lore raw

RFC PATCH 2/6 netfilter: normalize ebtables function declarations

In reply to 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 <[email protected]>

---
 net/bridge/netfilter/ebtables.c |   92 +++++++++++++++++++-------------------
 1 files changed, 46 insertions(+), 46 deletions(-)

net/bridge/netfilter/ebtables.c +46 ~~-46~~

index 79b6991..13d7fe2 100644
@@ -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;

Signature · 1 line

1.7.1

Richard Guy Briggs

author

2017-05-18 17:21 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 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(-)

include/linux/audit.h +3 -1

index 2150bdc..b6fcab1 100644
@@ -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;

include/uapi/linux/audit.h +1 -0

index 0714a66..8bee3f5 100644
@@ -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 */

kernel/auditsc.c +2 -1

index b2dcbe6..8ac38e6 100644
@@ -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

net/bridge/netfilter/ebtables.c +19 -6

index 13d7fe2..743f9e6 100644
@@ -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

net/netfilter/x_tables.c +19 -7

index 99c27ed..8d28fff 100644
@@ -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

Signature · 1 line

1.7.1

Richard Guy Briggs

author

2017-05-18 17:21 UTC #9 lore raw

RFC PATCH 4/6 netfilter: ebtables: audit table registration

In reply to 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 <[email protected]>

---
 net/bridge/netfilter/ebtables.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

net/bridge/netfilter/ebtables.c +26 -0

index 743f9e6..7499232 100644
@@ -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);

Signature · 1 line

1.7.1

Richard Guy Briggs

author

2017-05-18 17:21 UTC #11 lore raw

RFC PATCH 5/6 netfilter: add audit operation field

In reply to 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 <[email protected]>

---
 net/bridge/netfilter/ebtables.c |    8 ++++----
 net/netfilter/x_tables.c        |    5 +++--
 2 files changed, 7 insertions(+), 6 deletions(-)

net/bridge/netfilter/ebtables.c +4 -4

index 7499232..59b63a8 100644
@@ -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);

net/netfilter/x_tables.c +3 -2

index 8d28fff..395ebd3 100644
@@ -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);

Signature · 1 line

1.7.1

Richard Guy Briggs

author

2017-05-18 17:21 UTC #13 lore raw

RFC PATCH 6/6 netfilter: add audit netns ID

In reply to 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

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

Signed-off-by Richard Guy Briggs <[email protected]>

---
 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(-)

include/linux/netfilter/x_tables.h +1 -0

index be378cf..6be4a04 100644
@@ -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);

net/bridge/netfilter/ebtables.c +9 -4

index 59b63a8..0f77b2a 100644
@@ -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);

net/ipv4/netfilter/arp_tables.c +1 -1

index 6241a81..4933a5a 100644
@@ -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;

net/ipv4/netfilter/ip_tables.c +1 -1

index 384b857..c638607 100644
@@ -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;

net/ipv6/netfilter/ip6_tables.c +1 -1

index 1e15c54..87ef83b 100644
@@ -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;

net/netfilter/x_tables.c +7 -3

index 395ebd3..35533f1 100644
@@ -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;

Signature · 1 line

1.7.1

Pablo Neira Ayuso

maintainer

2017-05-24 17:31 UTC #14 lore raw

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

Cc'ing Eric Biederman.

Quoted 13 lines · Richard Guy Briggs

net/bridge/netfilter/ebtables.c +1 -0

index 59b63a8..0f77b2a 100644
@@ -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"

net/bridge/netfilter/ebtables.c static int do_replace_finish(struct net *net, struct ebt_replace *repl, · line 1080

Quoted 6 lines

net/bridge/netfilter/ebtables.c +2 -1

@@ -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?

Pablo Neira Ayuso

maintainer

2017-05-24 17:36 UTC #6 lore raw

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

In reply to Richard Guy Briggs

Quoted 13 lines · 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.

Quoted 2 lines

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.

Quoted 56 lines

include/uapi/linux/audit.h +1 -0

index 0714a66..8bee3f5 100644
@@ -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 */

kernel/auditsc.c +2 -1

index b2dcbe6..8ac38e6 100644
@@ -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

net/bridge/netfilter/ebtables.c +9 -6

index 13d7fe2..743f9e6 100644
@@ -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);
+			}

net/bridge/netfilter/ebtables.c static int do_replace_finish(struct net *net, struct ebt_replace *repl, · line 1092

Quoted 10 lines

net/bridge/netfilter/ebtables.c +10 -0

+		} 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() ?

Pablo Neira Ayuso

maintainer

2017-05-24 17:37 UTC #2 lore raw

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

In reply to Richard Guy Briggs

Quoted 6 lines · 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.

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.

Richard Guy Briggs

author

2017-05-24 18:04 UTC #15 lore raw

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

In reply to Pablo Neira Ayuso

Quoted 18 lines · Pablo Neira Ayuso

Cc'ing Eric Biederman.

Quoted 15 lines · Richard Guy Briggs

net/bridge/netfilter/ebtables.c +1 -0

index 59b63a8..0f77b2a 100644
@@ -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);

Quoted 8 lines

net/bridge/netfilter/ebtables.c static int do_replace_finish(struct net *net, struct ebt_replace *repl, · line 1080
Quoted 4 lines
net/bridge/netfilter/ebtables.c +2 -1
 			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 <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Richard Guy Briggs

author

2017-05-24 18:09 UTC #7 lore raw

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

In reply to Pablo Neira Ayuso

Quoted 14 lines · Pablo Neira Ayuso

Quoted 13 lines · 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.

Quoted 6 lines

Quoted 2 lines
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.

Quoted 1 line

I'm also having doubts about two record types.

Quoted 61 lines

include/uapi/linux/audit.h +1 -0

index 0714a66..8bee3f5 100644
@@ -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 */

kernel/auditsc.c +2 -1

index b2dcbe6..8ac38e6 100644
@@ -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

net/bridge/netfilter/ebtables.c +14 -6

index 13d7fe2..743f9e6 100644
@@ -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);

Quoted 8 lines

net/bridge/netfilter/ebtables.c static int do_replace_finish(struct net *net, struct ebt_replace *repl, · line 1092
Quoted 5 lines
net/bridge/netfilter/ebtables.c +5 -0
+				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 <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Eric W. Biederman

2017-05-24 19:44 UTC #16 lore raw

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

In reply to Richard Guy Briggs

Quoted 38 lines · Richard Guy Briggs

Quoted 18 lines · Pablo Neira Ayuso
Cc'ing Eric Biederman.
Quoted 15 lines · Richard Guy Briggs
net/bridge/netfilter/ebtables.c +1 -0
index 59b63a8..0f77b2a 100644
@@ -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);
Quoted 8 lines
net/bridge/netfilter/ebtables.c static int do_replace_finish(struct net *net, struct ebt_replace *repl, · line 1080
Quoted 4 lines
net/bridge/netfilter/ebtables.c +2 -1
 			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.

Quoted 6 lines

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

Richard Guy Briggs

author

2017-05-24 22:30 UTC #3 lore raw

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

In reply to Pablo Neira Ayuso

Quoted 10 lines · Pablo Neira Ayuso

Quoted 6 lines · 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.
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. ;-)

Quoted 1 line

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

Thanks.

- RGB

--
Richard Guy Briggs <[email protected]>
Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Paul Moore

maintainer

2017-06-02 15:25 UTC #8 lore raw

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

In reply to Richard Guy Briggs

Quoted 22 lines · Richard Guy Briggs

Quoted 14 lines · Pablo Neira Ayuso
Quoted 13 lines · 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.
Quoted 6 lines
Quoted 2 lines
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.

Quoted 7 lines

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.

Signature · 2 lines

paul moore
www.paul-moore.com

Paul Moore

maintainer

2017-06-02 15:27 UTC #10 lore raw

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

In reply to Richard Guy Briggs

Quoted 47 lines · 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 <[email protected]>
---
 net/bridge/netfilter/ebtables.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)
net/bridge/netfilter/ebtables.c +16 -0
index 743f9e6..7499232 100644
@@ -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);

net/bridge/netfilter/ebtables.c struct ebt_table * ebt_register_table(struct net *net, · line 1279

Quoted 10 lines

net/bridge/netfilter/ebtables.c +10 -0

+                       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.

Signature · 2 lines

paul moore
www.paul-moore.com

Paul Moore

maintainer

2017-06-02 15:28 UTC #12 lore raw

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

In reply to Richard Guy Briggs

Quoted 8 lines · 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

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

Quoted 73 lines

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

Signed-off-by Richard Guy Briggs <[email protected]>

---
 net/bridge/netfilter/ebtables.c |    8 ++++----
 net/netfilter/x_tables.c        |    5 +++--
 2 files changed, 7 insertions(+), 6 deletions(-)

net/bridge/netfilter/ebtables.c +4 -4

index 7499232..59b63a8 100644
@@ -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);

net/netfilter/x_tables.c +3 -3

index 8d28fff..395ebd3 100644
@@ -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

Signature · 2 lines

paul moore
www.paul-moore.com

Paul Moore

maintainer

2017-06-02 15:32 UTC #17 lore raw

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

In reply to Eric W. Biederman

Quoted 47 lines · Eric W. Biederman

Quoted 38 lines · Richard Guy Briggs
Quoted 18 lines · Pablo Neira Ayuso
Cc'ing Eric Biederman.
Quoted 15 lines · Richard Guy Briggs
net/bridge/netfilter/ebtables.c +1 -0
index 59b63a8..0f77b2a 100644
@@ -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);
Quoted 8 lines
net/bridge/netfilter/ebtables.c static int do_replace_finish(struct net *net, struct ebt_replace *repl, · line 1080
Quoted 4 lines
net/bridge/netfilter/ebtables.c +2 -1
                   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.
Quoted 6 lines
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.

Quoted 5 lines

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.

Signature · 2 lines

paul moore
www.paul-moore.com