linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH ghak124 v3] audit: log nftables configuration change events
@ 2020-06-04 13:20 Richard Guy Briggs
  2020-06-04 17:03 ` Steve Grubb
                   ` (3 more replies)
  0 siblings, 4 replies; 27+ messages in thread
From: Richard Guy Briggs @ 2020-06-04 13:20 UTC (permalink / raw)
  To: Linux-Audit Mailing List, LKML, netfilter-devel
  Cc: Paul Moore, sgrubb, omosnace, fw, twoerner, eparis, tgraf,
	Richard Guy Briggs

iptables, ip6tables, arptables and ebtables table registration,
replacement and unregistration configuration events are logged for the
native (legacy) iptables setsockopt api, but not for the
nftables netlink api which is used by the nft-variant of iptables in
addition to nftables itself.

Add calls to log the configuration actions in the nftables netlink api.

This uses the same NETFILTER_CFG record format but overloads the table
field.

  type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.878:162) : table=?:0;?:0 family=unspecified entries=2 op=nft_register_gen pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
  ...
  type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.878:162) : table=firewalld:1;?:0 family=inet entries=0 op=nft_register_table pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
  ...
  type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : table=firewalld:1;filter_FORWARD:85 family=inet entries=8 op=nft_register_chain pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
  ...
  type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : table=firewalld:1;filter_FORWARD:85 family=inet entries=101 op=nft_register_rule pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
  ...
  type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : table=firewalld:1;__set0:87 family=inet entries=87 op=nft_register_setelem pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
  ...
  type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : table=firewalld:1;__set0:87 family=inet entries=0 op=nft_register_set pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld

For further information please see issue
https://github.com/linux-audit/audit-kernel/issues/124

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
Changelog:
v3:
- inline message type rather than table

v2:
- differentiate between xtables and nftables
- add set, setelem, obj, flowtable, gen
- use nentries field as appropriate per type
- overload the "tables" field with table handle and chain/set/flowtable

 include/linux/audit.h         |  18 ++++++++
 kernel/auditsc.c              |  24 ++++++++--
 net/netfilter/nf_tables_api.c | 103 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 142 insertions(+), 3 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 3fcd9ee49734..604ede630580 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -12,6 +12,7 @@
 #include <linux/sched.h>
 #include <linux/ptrace.h>
 #include <uapi/linux/audit.h>
+#include <uapi/linux/netfilter/nf_tables.h>
 
 #define AUDIT_INO_UNSET ((unsigned long)-1)
 #define AUDIT_DEV_UNSET ((dev_t)-1)
@@ -98,6 +99,23 @@ enum audit_nfcfgop {
 	AUDIT_XT_OP_REGISTER,
 	AUDIT_XT_OP_REPLACE,
 	AUDIT_XT_OP_UNREGISTER,
+	AUDIT_NFT_OP_TABLE_REGISTER,
+	AUDIT_NFT_OP_TABLE_UNREGISTER,
+	AUDIT_NFT_OP_CHAIN_REGISTER,
+	AUDIT_NFT_OP_CHAIN_UNREGISTER,
+	AUDIT_NFT_OP_RULE_REGISTER,
+	AUDIT_NFT_OP_RULE_UNREGISTER,
+	AUDIT_NFT_OP_SET_REGISTER,
+	AUDIT_NFT_OP_SET_UNREGISTER,
+	AUDIT_NFT_OP_SETELEM_REGISTER,
+	AUDIT_NFT_OP_SETELEM_UNREGISTER,
+	AUDIT_NFT_OP_GEN_REGISTER,
+	AUDIT_NFT_OP_OBJ_REGISTER,
+	AUDIT_NFT_OP_OBJ_UNREGISTER,
+	AUDIT_NFT_OP_OBJ_RESET,
+	AUDIT_NFT_OP_FLOWTABLE_REGISTER,
+	AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,
+	AUDIT_NFT_OP_INVALID,
 };
 
 extern int is_audit_feature_set(int which);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 468a23390457..3a9100e95fda 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -75,6 +75,7 @@
 #include <linux/uaccess.h>
 #include <linux/fsnotify_backend.h>
 #include <uapi/linux/limits.h>
+#include <uapi/linux/netfilter/nf_tables.h>
 
 #include "audit.h"
 
@@ -136,9 +137,26 @@ struct audit_nfcfgop_tab {
 };
 
 static const struct audit_nfcfgop_tab audit_nfcfgs[] = {
-	{ AUDIT_XT_OP_REGISTER,		"register"	},
-	{ AUDIT_XT_OP_REPLACE,		"replace"	},
-	{ AUDIT_XT_OP_UNREGISTER,	"unregister"	},
+	{ AUDIT_XT_OP_REGISTER,			"xt_register"		   },
+	{ AUDIT_XT_OP_REPLACE,			"xt_replace"		   },
+	{ AUDIT_XT_OP_UNREGISTER,		"xt_unregister"		   },
+	{ AUDIT_NFT_OP_TABLE_REGISTER,		"nft_register_table"	   },
+	{ AUDIT_NFT_OP_TABLE_UNREGISTER,	"nft_unregister_table"	   },
+	{ AUDIT_NFT_OP_CHAIN_REGISTER,		"nft_register_chain"	   },
+	{ AUDIT_NFT_OP_CHAIN_UNREGISTER,	"nft_unregister_chain"	   },
+	{ AUDIT_NFT_OP_RULE_REGISTER,		"nft_register_rule"	   },
+	{ AUDIT_NFT_OP_RULE_UNREGISTER,		"nft_unregister_rule"	   },
+	{ AUDIT_NFT_OP_SET_REGISTER,		"nft_register_set"	   },
+	{ AUDIT_NFT_OP_SET_UNREGISTER,		"nft_unregister_set"	   },
+	{ AUDIT_NFT_OP_SETELEM_REGISTER,	"nft_register_setelem"	   },
+	{ AUDIT_NFT_OP_SETELEM_UNREGISTER,	"nft_unregister_setelem"   },
+	{ AUDIT_NFT_OP_GEN_REGISTER,		"nft_register_gen"	   },
+	{ AUDIT_NFT_OP_OBJ_REGISTER,		"nft_register_obj"	   },
+	{ AUDIT_NFT_OP_OBJ_UNREGISTER,		"nft_unregister_obj"	   },
+	{ AUDIT_NFT_OP_OBJ_RESET,		"nft_reset_obj"		   },
+	{ AUDIT_NFT_OP_FLOWTABLE_REGISTER,	"nft_register_flowtable"   },
+	{ AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,	"nft_unregister_flowtable" },
+	{ AUDIT_NFT_OP_INVALID,			"nft_invalid"		   },
 };
 
 static int audit_match_perm(struct audit_context *ctx, int mask)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 3558e76e2733..b9e7440cc87d 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -12,6 +12,7 @@
 #include <linux/netlink.h>
 #include <linux/vmalloc.h>
 #include <linux/rhashtable.h>
+#include <linux/audit.h>
 #include <linux/netfilter.h>
 #include <linux/netfilter/nfnetlink.h>
 #include <linux/netfilter/nf_tables.h>
@@ -693,6 +694,16 @@ static void nf_tables_table_notify(const struct nft_ctx *ctx, int event)
 {
 	struct sk_buff *skb;
 	int err;
+	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;?:0",
+			      ctx->table->name, ctx->table->handle);
+
+	audit_log_nfcfg(buf,
+			ctx->family,
+			ctx->table->use,
+			event == NFT_MSG_NEWTABLE ?
+				AUDIT_NFT_OP_TABLE_REGISTER :
+				AUDIT_NFT_OP_TABLE_UNREGISTER);
+	kfree(buf);
 
 	if (!ctx->report &&
 	    !nfnetlink_has_listeners(ctx->net, NFNLGRP_NFTABLES))
@@ -1428,6 +1439,17 @@ static void nf_tables_chain_notify(const struct nft_ctx *ctx, int event)
 {
 	struct sk_buff *skb;
 	int err;
+	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;%s:%llu",
+			      ctx->table->name, ctx->table->handle,
+			      ctx->chain->name, ctx->chain->handle);
+
+	audit_log_nfcfg(buf,
+			ctx->family,
+			ctx->chain->use,
+			event == NFT_MSG_NEWCHAIN ?
+				AUDIT_NFT_OP_CHAIN_REGISTER :
+				AUDIT_NFT_OP_CHAIN_UNREGISTER);
+	kfree(buf);
 
 	if (!ctx->report &&
 	    !nfnetlink_has_listeners(ctx->net, NFNLGRP_NFTABLES))
@@ -2691,6 +2713,17 @@ static void nf_tables_rule_notify(const struct nft_ctx *ctx,
 {
 	struct sk_buff *skb;
 	int err;
+	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;%s:%llu",
+			      ctx->table->name, ctx->table->handle,
+			      ctx->chain->name, ctx->chain->handle);
+
+	audit_log_nfcfg(buf,
+			ctx->family,
+			rule->handle,
+			event == NFT_MSG_NEWRULE ?
+				AUDIT_NFT_OP_RULE_REGISTER :
+				AUDIT_NFT_OP_RULE_UNREGISTER);
+	kfree(buf);
 
 	if (!ctx->report &&
 	    !nfnetlink_has_listeners(ctx->net, NFNLGRP_NFTABLES))
@@ -3693,6 +3726,17 @@ static void nf_tables_set_notify(const struct nft_ctx *ctx,
 	struct sk_buff *skb;
 	u32 portid = ctx->portid;
 	int err;
+	char *buf = kasprintf(gfp_flags, "%s:%llu;%s:%llu",
+			      ctx->table->name, ctx->table->handle,
+			      set->name, set->handle);
+
+	audit_log_nfcfg(buf,
+			ctx->family,
+			set->field_count,
+			event == NFT_MSG_NEWSET ?
+				AUDIT_NFT_OP_SET_REGISTER :
+				AUDIT_NFT_OP_SET_UNREGISTER);
+	kfree(buf);
 
 	if (!ctx->report &&
 	    !nfnetlink_has_listeners(ctx->net, NFNLGRP_NFTABLES))
@@ -4809,6 +4853,17 @@ static void nf_tables_setelem_notify(const struct nft_ctx *ctx,
 	u32 portid = ctx->portid;
 	struct sk_buff *skb;
 	int err;
+	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;%s:%llu",
+			      ctx->table->name, ctx->table->handle,
+			      set->name, set->handle);
+
+	audit_log_nfcfg(buf,
+			ctx->family,
+			set->handle,
+			event == NFT_MSG_NEWSETELEM ?
+				AUDIT_NFT_OP_SETELEM_REGISTER :
+				AUDIT_NFT_OP_SETELEM_UNREGISTER);
+	kfree(buf);
 
 	if (!ctx->report && !nfnetlink_has_listeners(net, NFNLGRP_NFTABLES))
 		return;
@@ -5890,6 +5945,19 @@ static int nf_tables_dump_obj(struct sk_buff *skb, struct netlink_callback *cb)
 			    obj->ops->type->type != filter->type)
 				goto cont;
 
+			if (reset) {
+				char *buf = kasprintf(GFP_KERNEL,
+						      "%s:%llu;?:0",
+						      table->name,
+						      table->handle);
+
+				audit_log_nfcfg(buf,
+						family,
+						obj->handle,
+						AUDIT_NFT_OP_OBJ_RESET);
+				kfree(buf);
+			}
+
 			if (nf_tables_fill_obj_info(skb, net, NETLINK_CB(cb->skb).portid,
 						    cb->nlh->nlmsg_seq,
 						    NFT_MSG_NEWOBJ,
@@ -6000,6 +6068,17 @@ static int nf_tables_getobj(struct net *net, struct sock *nlsk,
 	if (NFNL_MSG_TYPE(nlh->nlmsg_type) == NFT_MSG_GETOBJ_RESET)
 		reset = true;
 
+	if (reset) {
+		char *buf = kasprintf(GFP_KERNEL, "%s:%llu;?:0",
+				      table->name, table->handle);
+
+		audit_log_nfcfg(buf,
+				family,
+				obj->handle,
+				AUDIT_NFT_OP_OBJ_RESET);
+		kfree(buf);
+	}
+
 	err = nf_tables_fill_obj_info(skb2, net, NETLINK_CB(skb).portid,
 				      nlh->nlmsg_seq, NFT_MSG_NEWOBJ, 0,
 				      family, table, obj, reset);
@@ -6075,6 +6154,16 @@ void nft_obj_notify(struct net *net, const struct nft_table *table,
 {
 	struct sk_buff *skb;
 	int err;
+	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;?:0",
+			      table->name, table->handle);
+
+	audit_log_nfcfg(buf,
+			family,
+			obj->handle,
+			event == NFT_MSG_NEWOBJ ?
+				AUDIT_NFT_OP_OBJ_REGISTER :
+				AUDIT_NFT_OP_OBJ_UNREGISTER);
+	kfree(buf);
 
 	if (!report &&
 	    !nfnetlink_has_listeners(net, NFNLGRP_NFTABLES))
@@ -6701,6 +6790,17 @@ static void nf_tables_flowtable_notify(struct nft_ctx *ctx,
 {
 	struct sk_buff *skb;
 	int err;
+	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;%s:%llu",
+			      flowtable->table->name, flowtable->table->handle,
+			      flowtable->name, flowtable->handle);
+
+	audit_log_nfcfg(buf,
+			ctx->family,
+			flowtable->hooknum,
+			event == NFT_MSG_NEWFLOWTABLE ?
+				AUDIT_NFT_OP_FLOWTABLE_REGISTER :
+				AUDIT_NFT_OP_FLOWTABLE_UNREGISTER);
+	kfree(buf);
 
 	if (ctx->report &&
 	    !nfnetlink_has_listeners(ctx->net, NFNLGRP_NFTABLES))
@@ -6822,6 +6922,9 @@ static void nf_tables_gen_notify(struct net *net, struct sk_buff *skb,
 	struct sk_buff *skb2;
 	int err;
 
+	audit_log_nfcfg("?:0;?:0", 0, net->nft.base_seq,
+			AUDIT_NFT_OP_GEN_REGISTER);
+
 	if (nlmsg_report(nlh) &&
 	    !nfnetlink_has_listeners(net, NFNLGRP_NFTABLES))
 		return;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2020-06-04 13:20 [PATCH ghak124 v3] audit: log nftables configuration change events Richard Guy Briggs
@ 2020-06-04 17:03 ` Steve Grubb
  2020-06-04 17:57   ` Richard Guy Briggs
  2020-06-24  0:34 ` Paul Moore
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 27+ messages in thread
From: Steve Grubb @ 2020-06-04 17:03 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Linux-Audit Mailing List, LKML, netfilter-devel, Paul Moore,
	omosnace, fw, twoerner, eparis, tgraf

On Thursday, June 4, 2020 9:20:49 AM EDT Richard Guy Briggs wrote:
> iptables, ip6tables, arptables and ebtables table registration,
> replacement and unregistration configuration events are logged for the
> native (legacy) iptables setsockopt api, but not for the
> nftables netlink api which is used by the nft-variant of iptables in
> addition to nftables itself.
> 
> Add calls to log the configuration actions in the nftables netlink api.
> 
> This uses the same NETFILTER_CFG record format but overloads the table
> field.
> 
>   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.878:162) : table=?:0;?:0
> family=unspecified entries=2 op=nft_register_gen pid=396
> subj=system_u:system_r:firewalld_t:s0 comm=firewalld ...
>   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.878:162) :
> table=firewalld:1;?:0 family=inet entries=0 op=nft_register_table pid=396
> subj=system_u:system_r:firewalld_t:s0 comm=firewalld ...
>   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) :
> table=firewalld:1;filter_FORWARD:85 family=inet entries=8
> op=nft_register_chain pid=396 subj=system_u:system_r:firewalld_t:s0
> comm=firewalld ...
>   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) :
> table=firewalld:1;filter_FORWARD:85 family=inet entries=101
> op=nft_register_rule pid=396 subj=system_u:system_r:firewalld_t:s0
> comm=firewalld ...
>   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) :
> table=firewalld:1;__set0:87 family=inet entries=87 op=nft_register_setelem
> pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld ...
>   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) :
> table=firewalld:1;__set0:87 family=inet entries=0 op=nft_register_set
> pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
> 
> For further information please see issue
> https://github.com/linux-audit/audit-kernel/issues/124
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> Changelog:
> v3:
> - inline message type rather than table
> 
> v2:
> - differentiate between xtables and nftables
> - add set, setelem, obj, flowtable, gen
> - use nentries field as appropriate per type
> - overload the "tables" field with table handle and chain/set/flowtable
> 
>  include/linux/audit.h         |  18 ++++++++
>  kernel/auditsc.c              |  24 ++++++++--
>  net/netfilter/nf_tables_api.c | 103
> ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 142
> insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 3fcd9ee49734..604ede630580 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -12,6 +12,7 @@
>  #include <linux/sched.h>
>  #include <linux/ptrace.h>
>  #include <uapi/linux/audit.h>
> +#include <uapi/linux/netfilter/nf_tables.h>
> 
>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> @@ -98,6 +99,23 @@ enum audit_nfcfgop {
>  	AUDIT_XT_OP_REGISTER,
>  	AUDIT_XT_OP_REPLACE,
>  	AUDIT_XT_OP_UNREGISTER,
> +	AUDIT_NFT_OP_TABLE_REGISTER,
> +	AUDIT_NFT_OP_TABLE_UNREGISTER,
> +	AUDIT_NFT_OP_CHAIN_REGISTER,
> +	AUDIT_NFT_OP_CHAIN_UNREGISTER,
> +	AUDIT_NFT_OP_RULE_REGISTER,
> +	AUDIT_NFT_OP_RULE_UNREGISTER,
> +	AUDIT_NFT_OP_SET_REGISTER,
> +	AUDIT_NFT_OP_SET_UNREGISTER,
> +	AUDIT_NFT_OP_SETELEM_REGISTER,
> +	AUDIT_NFT_OP_SETELEM_UNREGISTER,
> +	AUDIT_NFT_OP_GEN_REGISTER,
> +	AUDIT_NFT_OP_OBJ_REGISTER,
> +	AUDIT_NFT_OP_OBJ_UNREGISTER,
> +	AUDIT_NFT_OP_OBJ_RESET,
> +	AUDIT_NFT_OP_FLOWTABLE_REGISTER,
> +	AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,
> +	AUDIT_NFT_OP_INVALID,
>  };
> 
>  extern int is_audit_feature_set(int which);
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 468a23390457..3a9100e95fda 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -75,6 +75,7 @@
>  #include <linux/uaccess.h>
>  #include <linux/fsnotify_backend.h>
>  #include <uapi/linux/limits.h>
> +#include <uapi/linux/netfilter/nf_tables.h>
> 
>  #include "audit.h"
> 
> @@ -136,9 +137,26 @@ struct audit_nfcfgop_tab {
>  };
> 
>  static const struct audit_nfcfgop_tab audit_nfcfgs[] = {
> -	{ AUDIT_XT_OP_REGISTER,		"register"	},
> -	{ AUDIT_XT_OP_REPLACE,		"replace"	},
> -	{ AUDIT_XT_OP_UNREGISTER,	"unregister"	},
> +	{ AUDIT_XT_OP_REGISTER,			"xt_register"		   
},
> +	{ AUDIT_XT_OP_REPLACE,			"xt_replace"		   },
> +	{ AUDIT_XT_OP_UNREGISTER,		"xt_unregister"		   },
> +	{ AUDIT_NFT_OP_TABLE_REGISTER,		"nft_register_table"	   
},
> +	{ AUDIT_NFT_OP_TABLE_UNREGISTER,	"nft_unregister_table"	   },
> +	{ AUDIT_NFT_OP_CHAIN_REGISTER,		"nft_register_chain"	   
},
> +	{ AUDIT_NFT_OP_CHAIN_UNREGISTER,	"nft_unregister_chain"	   },
> +	{ AUDIT_NFT_OP_RULE_REGISTER,		"nft_register_rule"	   
},
> +	{ AUDIT_NFT_OP_RULE_UNREGISTER,		"nft_unregister_rule"	   
},
> +	{ AUDIT_NFT_OP_SET_REGISTER,		"nft_register_set"	   
},
> +	{ AUDIT_NFT_OP_SET_UNREGISTER,		"nft_unregister_set"	   
},
> +	{ AUDIT_NFT_OP_SETELEM_REGISTER,	"nft_register_setelem"	   },
> +	{ AUDIT_NFT_OP_SETELEM_UNREGISTER,	"nft_unregister_setelem"   },
> +	{ AUDIT_NFT_OP_GEN_REGISTER,		"nft_register_gen"	   },
> +	{ AUDIT_NFT_OP_OBJ_REGISTER,		"nft_register_obj"	   },
> +	{ AUDIT_NFT_OP_OBJ_UNREGISTER,		"nft_unregister_obj"	   },
> +	{ AUDIT_NFT_OP_OBJ_RESET,		"nft_reset_obj"		   },
> +	{ AUDIT_NFT_OP_FLOWTABLE_REGISTER,	"nft_register_flowtable"   },
> +	{ AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,	"nft_unregister_flowtable" },
> +	{ AUDIT_NFT_OP_INVALID,			"nft_invalid"		   
},
>  };

I still don't like the event format because it doesn't give complete subject 
information. However, I thought I'd comment on this string table. Usually 
it's sufficient to log the number and then have the string table in user space 
which looks it up during interpretation.

-Steve

>  static int audit_match_perm(struct audit_context *ctx, int mask)
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 3558e76e2733..b9e7440cc87d 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -12,6 +12,7 @@
>  #include <linux/netlink.h>
>  #include <linux/vmalloc.h>
>  #include <linux/rhashtable.h>
> +#include <linux/audit.h>
>  #include <linux/netfilter.h>
>  #include <linux/netfilter/nfnetlink.h>
>  #include <linux/netfilter/nf_tables.h>
> @@ -693,6 +694,16 @@ static void nf_tables_table_notify(const struct
> nft_ctx *ctx, int event) {
>  	struct sk_buff *skb;
>  	int err;
> +	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;?:0",
> +			      ctx->table->name, ctx->table->handle);
> +
> +	audit_log_nfcfg(buf,
> +			ctx->family,
> +			ctx->table->use,
> +			event == NFT_MSG_NEWTABLE ?
> +				AUDIT_NFT_OP_TABLE_REGISTER :
> +				AUDIT_NFT_OP_TABLE_UNREGISTER);
> +	kfree(buf);
> 
>  	if (!ctx->report &&
>  	    !nfnetlink_has_listeners(ctx->net, NFNLGRP_NFTABLES))
> @@ -1428,6 +1439,17 @@ static void nf_tables_chain_notify(const struct
> nft_ctx *ctx, int event) {
>  	struct sk_buff *skb;
>  	int err;
> +	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;%s:%llu",
> +			      ctx->table->name, ctx->table->handle,
> +			      ctx->chain->name, ctx->chain->handle);
> +
> +	audit_log_nfcfg(buf,
> +			ctx->family,
> +			ctx->chain->use,
> +			event == NFT_MSG_NEWCHAIN ?
> +				AUDIT_NFT_OP_CHAIN_REGISTER :
> +				AUDIT_NFT_OP_CHAIN_UNREGISTER);
> +	kfree(buf);
> 
>  	if (!ctx->report &&
>  	    !nfnetlink_has_listeners(ctx->net, NFNLGRP_NFTABLES))
> @@ -2691,6 +2713,17 @@ static void nf_tables_rule_notify(const struct
> nft_ctx *ctx, {
>  	struct sk_buff *skb;
>  	int err;
> +	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;%s:%llu",
> +			      ctx->table->name, ctx->table->handle,
> +			      ctx->chain->name, ctx->chain->handle);
> +
> +	audit_log_nfcfg(buf,
> +			ctx->family,
> +			rule->handle,
> +			event == NFT_MSG_NEWRULE ?
> +				AUDIT_NFT_OP_RULE_REGISTER :
> +				AUDIT_NFT_OP_RULE_UNREGISTER);
> +	kfree(buf);
> 
>  	if (!ctx->report &&
>  	    !nfnetlink_has_listeners(ctx->net, NFNLGRP_NFTABLES))
> @@ -3693,6 +3726,17 @@ static void nf_tables_set_notify(const struct
> nft_ctx *ctx, struct sk_buff *skb;
>  	u32 portid = ctx->portid;
>  	int err;
> +	char *buf = kasprintf(gfp_flags, "%s:%llu;%s:%llu",
> +			      ctx->table->name, ctx->table->handle,
> +			      set->name, set->handle);
> +
> +	audit_log_nfcfg(buf,
> +			ctx->family,
> +			set->field_count,
> +			event == NFT_MSG_NEWSET ?
> +				AUDIT_NFT_OP_SET_REGISTER :
> +				AUDIT_NFT_OP_SET_UNREGISTER);
> +	kfree(buf);
> 
>  	if (!ctx->report &&
>  	    !nfnetlink_has_listeners(ctx->net, NFNLGRP_NFTABLES))
> @@ -4809,6 +4853,17 @@ static void nf_tables_setelem_notify(const struct
> nft_ctx *ctx, u32 portid = ctx->portid;
>  	struct sk_buff *skb;
>  	int err;
> +	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;%s:%llu",
> +			      ctx->table->name, ctx->table->handle,
> +			      set->name, set->handle);
> +
> +	audit_log_nfcfg(buf,
> +			ctx->family,
> +			set->handle,
> +			event == NFT_MSG_NEWSETELEM ?
> +				AUDIT_NFT_OP_SETELEM_REGISTER :
> +				AUDIT_NFT_OP_SETELEM_UNREGISTER);
> +	kfree(buf);
> 
>  	if (!ctx->report && !nfnetlink_has_listeners(net, NFNLGRP_NFTABLES))
>  		return;
> @@ -5890,6 +5945,19 @@ static int nf_tables_dump_obj(struct sk_buff *skb,
> struct netlink_callback *cb) obj->ops->type->type != filter->type)
>  				goto cont;
> 
> +			if (reset) {
> +				char *buf = kasprintf(GFP_KERNEL,
> +						      "%s:%llu;?:0",
> +						      table->name,
> +						      table->handle);
> +
> +				audit_log_nfcfg(buf,
> +						family,
> +						obj->handle,
> +						AUDIT_NFT_OP_OBJ_RESET);
> +				kfree(buf);
> +			}
> +
>  			if (nf_tables_fill_obj_info(skb, net, NETLINK_CB(cb-
>skb).portid,
>  						    cb->nlh->nlmsg_seq,
>  						    NFT_MSG_NEWOBJ,
> @@ -6000,6 +6068,17 @@ static int nf_tables_getobj(struct net *net, struct
> sock *nlsk, if (NFNL_MSG_TYPE(nlh->nlmsg_type) == NFT_MSG_GETOBJ_RESET)
>  		reset = true;
> 
> +	if (reset) {
> +		char *buf = kasprintf(GFP_KERNEL, "%s:%llu;?:0",
> +				      table->name, table->handle);
> +
> +		audit_log_nfcfg(buf,
> +				family,
> +				obj->handle,
> +				AUDIT_NFT_OP_OBJ_RESET);
> +		kfree(buf);
> +	}
> +
>  	err = nf_tables_fill_obj_info(skb2, net, NETLINK_CB(skb).portid,
>  				      nlh->nlmsg_seq, NFT_MSG_NEWOBJ, 0,
>  				      family, table, obj, reset);
> @@ -6075,6 +6154,16 @@ void nft_obj_notify(struct net *net, const struct
> nft_table *table, {
>  	struct sk_buff *skb;
>  	int err;
> +	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;?:0",
> +			      table->name, table->handle);
> +
> +	audit_log_nfcfg(buf,
> +			family,
> +			obj->handle,
> +			event == NFT_MSG_NEWOBJ ?
> +				AUDIT_NFT_OP_OBJ_REGISTER :
> +				AUDIT_NFT_OP_OBJ_UNREGISTER);
> +	kfree(buf);
> 
>  	if (!report &&
>  	    !nfnetlink_has_listeners(net, NFNLGRP_NFTABLES))
> @@ -6701,6 +6790,17 @@ static void nf_tables_flowtable_notify(struct
> nft_ctx *ctx, {
>  	struct sk_buff *skb;
>  	int err;
> +	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;%s:%llu",
> +			      flowtable->table->name, flowtable->table->handle,
> +			      flowtable->name, flowtable->handle);
> +
> +	audit_log_nfcfg(buf,
> +			ctx->family,
> +			flowtable->hooknum,
> +			event == NFT_MSG_NEWFLOWTABLE ?
> +				AUDIT_NFT_OP_FLOWTABLE_REGISTER :
> +				AUDIT_NFT_OP_FLOWTABLE_UNREGISTER);
> +	kfree(buf);
> 
>  	if (ctx->report &&
>  	    !nfnetlink_has_listeners(ctx->net, NFNLGRP_NFTABLES))
> @@ -6822,6 +6922,9 @@ static void nf_tables_gen_notify(struct net *net,
> struct sk_buff *skb, struct sk_buff *skb2;
>  	int err;
> 
> +	audit_log_nfcfg("?:0;?:0", 0, net->nft.base_seq,
> +			AUDIT_NFT_OP_GEN_REGISTER);
> +
>  	if (nlmsg_report(nlh) &&
>  	    !nfnetlink_has_listeners(net, NFNLGRP_NFTABLES))
>  		return;





^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2020-06-04 17:03 ` Steve Grubb
@ 2020-06-04 17:57   ` Richard Guy Briggs
  2020-06-04 18:51     ` Steve Grubb
  0 siblings, 1 reply; 27+ messages in thread
From: Richard Guy Briggs @ 2020-06-04 17:57 UTC (permalink / raw)
  To: Steve Grubb
  Cc: Linux-Audit Mailing List, LKML, netfilter-devel, Paul Moore,
	omosnace, fw, twoerner, eparis, tgraf

On 2020-06-04 13:03, Steve Grubb wrote:
> On Thursday, June 4, 2020 9:20:49 AM EDT Richard Guy Briggs wrote:
> > iptables, ip6tables, arptables and ebtables table registration,
> > replacement and unregistration configuration events are logged for the
> > native (legacy) iptables setsockopt api, but not for the
> > nftables netlink api which is used by the nft-variant of iptables in
> > addition to nftables itself.
> > 
> > Add calls to log the configuration actions in the nftables netlink api.
> > 
> > This uses the same NETFILTER_CFG record format but overloads the table
> > field.
> > 
> >   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.878:162) : table=?:0;?:0
> > family=unspecified entries=2 op=nft_register_gen pid=396
> > subj=system_u:system_r:firewalld_t:s0 comm=firewalld ...
> >   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.878:162) :
> > table=firewalld:1;?:0 family=inet entries=0 op=nft_register_table pid=396
> > subj=system_u:system_r:firewalld_t:s0 comm=firewalld ...
> >   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) :
> > table=firewalld:1;filter_FORWARD:85 family=inet entries=8
> > op=nft_register_chain pid=396 subj=system_u:system_r:firewalld_t:s0
> > comm=firewalld ...
> >   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) :
> > table=firewalld:1;filter_FORWARD:85 family=inet entries=101
> > op=nft_register_rule pid=396 subj=system_u:system_r:firewalld_t:s0
> > comm=firewalld ...
> >   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) :
> > table=firewalld:1;__set0:87 family=inet entries=87 op=nft_register_setelem
> > pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld ...
> >   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) :
> > table=firewalld:1;__set0:87 family=inet entries=0 op=nft_register_set
> > pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
> > 
> > For further information please see issue
> > https://github.com/linux-audit/audit-kernel/issues/124
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > Changelog:
> > v3:
> > - inline message type rather than table
> > 
> > v2:
> > - differentiate between xtables and nftables
> > - add set, setelem, obj, flowtable, gen
> > - use nentries field as appropriate per type
> > - overload the "tables" field with table handle and chain/set/flowtable
> > 
> >  include/linux/audit.h         |  18 ++++++++
> >  kernel/auditsc.c              |  24 ++++++++--
> >  net/netfilter/nf_tables_api.c | 103
> > ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 142
> > insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 3fcd9ee49734..604ede630580 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -12,6 +12,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/ptrace.h>
> >  #include <uapi/linux/audit.h>
> > +#include <uapi/linux/netfilter/nf_tables.h>
> > 
> >  #define AUDIT_INO_UNSET ((unsigned long)-1)
> >  #define AUDIT_DEV_UNSET ((dev_t)-1)
> > @@ -98,6 +99,23 @@ enum audit_nfcfgop {
> >  	AUDIT_XT_OP_REGISTER,
> >  	AUDIT_XT_OP_REPLACE,
> >  	AUDIT_XT_OP_UNREGISTER,
> > +	AUDIT_NFT_OP_TABLE_REGISTER,
> > +	AUDIT_NFT_OP_TABLE_UNREGISTER,
> > +	AUDIT_NFT_OP_CHAIN_REGISTER,
> > +	AUDIT_NFT_OP_CHAIN_UNREGISTER,
> > +	AUDIT_NFT_OP_RULE_REGISTER,
> > +	AUDIT_NFT_OP_RULE_UNREGISTER,
> > +	AUDIT_NFT_OP_SET_REGISTER,
> > +	AUDIT_NFT_OP_SET_UNREGISTER,
> > +	AUDIT_NFT_OP_SETELEM_REGISTER,
> > +	AUDIT_NFT_OP_SETELEM_UNREGISTER,
> > +	AUDIT_NFT_OP_GEN_REGISTER,
> > +	AUDIT_NFT_OP_OBJ_REGISTER,
> > +	AUDIT_NFT_OP_OBJ_UNREGISTER,
> > +	AUDIT_NFT_OP_OBJ_RESET,
> > +	AUDIT_NFT_OP_FLOWTABLE_REGISTER,
> > +	AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,
> > +	AUDIT_NFT_OP_INVALID,
> >  };
> > 
> >  extern int is_audit_feature_set(int which);
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 468a23390457..3a9100e95fda 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -75,6 +75,7 @@
> >  #include <linux/uaccess.h>
> >  #include <linux/fsnotify_backend.h>
> >  #include <uapi/linux/limits.h>
> > +#include <uapi/linux/netfilter/nf_tables.h>
> > 
> >  #include "audit.h"
> > 
> > @@ -136,9 +137,26 @@ struct audit_nfcfgop_tab {
> >  };
> > 
> >  static const struct audit_nfcfgop_tab audit_nfcfgs[] = {
> > -	{ AUDIT_XT_OP_REGISTER,		"register"	},
> > -	{ AUDIT_XT_OP_REPLACE,		"replace"	},
> > -	{ AUDIT_XT_OP_UNREGISTER,	"unregister"	},
> > +	{ AUDIT_XT_OP_REGISTER,			"xt_register"		   
> },
> > +	{ AUDIT_XT_OP_REPLACE,			"xt_replace"		   },
> > +	{ AUDIT_XT_OP_UNREGISTER,		"xt_unregister"		   },
> > +	{ AUDIT_NFT_OP_TABLE_REGISTER,		"nft_register_table"	   
> },
> > +	{ AUDIT_NFT_OP_TABLE_UNREGISTER,	"nft_unregister_table"	   },
> > +	{ AUDIT_NFT_OP_CHAIN_REGISTER,		"nft_register_chain"	   
> },
> > +	{ AUDIT_NFT_OP_CHAIN_UNREGISTER,	"nft_unregister_chain"	   },
> > +	{ AUDIT_NFT_OP_RULE_REGISTER,		"nft_register_rule"	   
> },
> > +	{ AUDIT_NFT_OP_RULE_UNREGISTER,		"nft_unregister_rule"	   
> },
> > +	{ AUDIT_NFT_OP_SET_REGISTER,		"nft_register_set"	   
> },
> > +	{ AUDIT_NFT_OP_SET_UNREGISTER,		"nft_unregister_set"	   
> },
> > +	{ AUDIT_NFT_OP_SETELEM_REGISTER,	"nft_register_setelem"	   },
> > +	{ AUDIT_NFT_OP_SETELEM_UNREGISTER,	"nft_unregister_setelem"   },
> > +	{ AUDIT_NFT_OP_GEN_REGISTER,		"nft_register_gen"	   },
> > +	{ AUDIT_NFT_OP_OBJ_REGISTER,		"nft_register_obj"	   },
> > +	{ AUDIT_NFT_OP_OBJ_UNREGISTER,		"nft_unregister_obj"	   },
> > +	{ AUDIT_NFT_OP_OBJ_RESET,		"nft_reset_obj"		   },
> > +	{ AUDIT_NFT_OP_FLOWTABLE_REGISTER,	"nft_register_flowtable"   },
> > +	{ AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,	"nft_unregister_flowtable" },
> > +	{ AUDIT_NFT_OP_INVALID,			"nft_invalid"		   
> },
> >  };
> 
> I still don't like the event format because it doesn't give complete subject 
> information. However, I thought I'd comment on this string table. Usually 
> it's sufficient to log the number and then have the string table in user space 
> which looks it up during interpretation.

That is a good idea that would help reduce kernel cycles and netlink
bandwidth, but the format was set in 2011 so it is a bit late to change
that now:
	fbabf31e4d48 ("netfilter: create audit records for x_tables replaces")

> -Steve
> 
> >  static int audit_match_perm(struct audit_context *ctx, int mask)
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index 3558e76e2733..b9e7440cc87d 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/netlink.h>
> >  #include <linux/vmalloc.h>
> >  #include <linux/rhashtable.h>
> > +#include <linux/audit.h>
> >  #include <linux/netfilter.h>
> >  #include <linux/netfilter/nfnetlink.h>
> >  #include <linux/netfilter/nf_tables.h>
> > @@ -693,6 +694,16 @@ static void nf_tables_table_notify(const struct
> > nft_ctx *ctx, int event) {
> >  	struct sk_buff *skb;
> >  	int err;
> > +	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;?:0",
> > +			      ctx->table->name, ctx->table->handle);
> > +
> > +	audit_log_nfcfg(buf,
> > +			ctx->family,
> > +			ctx->table->use,
> > +			event == NFT_MSG_NEWTABLE ?
> > +				AUDIT_NFT_OP_TABLE_REGISTER :
> > +				AUDIT_NFT_OP_TABLE_UNREGISTER);
> > +	kfree(buf);
> > 
> >  	if (!ctx->report &&
> >  	    !nfnetlink_has_listeners(ctx->net, NFNLGRP_NFTABLES))
> > @@ -1428,6 +1439,17 @@ static void nf_tables_chain_notify(const struct
> > nft_ctx *ctx, int event) {
> >  	struct sk_buff *skb;
> >  	int err;
> > +	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;%s:%llu",
> > +			      ctx->table->name, ctx->table->handle,
> > +			      ctx->chain->name, ctx->chain->handle);
> > +
> > +	audit_log_nfcfg(buf,
> > +			ctx->family,
> > +			ctx->chain->use,
> > +			event == NFT_MSG_NEWCHAIN ?
> > +				AUDIT_NFT_OP_CHAIN_REGISTER :
> > +				AUDIT_NFT_OP_CHAIN_UNREGISTER);
> > +	kfree(buf);
> > 
> >  	if (!ctx->report &&
> >  	    !nfnetlink_has_listeners(ctx->net, NFNLGRP_NFTABLES))
> > @@ -2691,6 +2713,17 @@ static void nf_tables_rule_notify(const struct
> > nft_ctx *ctx, {
> >  	struct sk_buff *skb;
> >  	int err;
> > +	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;%s:%llu",
> > +			      ctx->table->name, ctx->table->handle,
> > +			      ctx->chain->name, ctx->chain->handle);
> > +
> > +	audit_log_nfcfg(buf,
> > +			ctx->family,
> > +			rule->handle,
> > +			event == NFT_MSG_NEWRULE ?
> > +				AUDIT_NFT_OP_RULE_REGISTER :
> > +				AUDIT_NFT_OP_RULE_UNREGISTER);
> > +	kfree(buf);
> > 
> >  	if (!ctx->report &&
> >  	    !nfnetlink_has_listeners(ctx->net, NFNLGRP_NFTABLES))
> > @@ -3693,6 +3726,17 @@ static void nf_tables_set_notify(const struct
> > nft_ctx *ctx, struct sk_buff *skb;
> >  	u32 portid = ctx->portid;
> >  	int err;
> > +	char *buf = kasprintf(gfp_flags, "%s:%llu;%s:%llu",
> > +			      ctx->table->name, ctx->table->handle,
> > +			      set->name, set->handle);
> > +
> > +	audit_log_nfcfg(buf,
> > +			ctx->family,
> > +			set->field_count,
> > +			event == NFT_MSG_NEWSET ?
> > +				AUDIT_NFT_OP_SET_REGISTER :
> > +				AUDIT_NFT_OP_SET_UNREGISTER);
> > +	kfree(buf);
> > 
> >  	if (!ctx->report &&
> >  	    !nfnetlink_has_listeners(ctx->net, NFNLGRP_NFTABLES))
> > @@ -4809,6 +4853,17 @@ static void nf_tables_setelem_notify(const struct
> > nft_ctx *ctx, u32 portid = ctx->portid;
> >  	struct sk_buff *skb;
> >  	int err;
> > +	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;%s:%llu",
> > +			      ctx->table->name, ctx->table->handle,
> > +			      set->name, set->handle);
> > +
> > +	audit_log_nfcfg(buf,
> > +			ctx->family,
> > +			set->handle,
> > +			event == NFT_MSG_NEWSETELEM ?
> > +				AUDIT_NFT_OP_SETELEM_REGISTER :
> > +				AUDIT_NFT_OP_SETELEM_UNREGISTER);
> > +	kfree(buf);
> > 
> >  	if (!ctx->report && !nfnetlink_has_listeners(net, NFNLGRP_NFTABLES))
> >  		return;
> > @@ -5890,6 +5945,19 @@ static int nf_tables_dump_obj(struct sk_buff *skb,
> > struct netlink_callback *cb) obj->ops->type->type != filter->type)
> >  				goto cont;
> > 
> > +			if (reset) {
> > +				char *buf = kasprintf(GFP_KERNEL,
> > +						      "%s:%llu;?:0",
> > +						      table->name,
> > +						      table->handle);
> > +
> > +				audit_log_nfcfg(buf,
> > +						family,
> > +						obj->handle,
> > +						AUDIT_NFT_OP_OBJ_RESET);
> > +				kfree(buf);
> > +			}
> > +
> >  			if (nf_tables_fill_obj_info(skb, net, NETLINK_CB(cb-
> >skb).portid,
> >  						    cb->nlh->nlmsg_seq,
> >  						    NFT_MSG_NEWOBJ,
> > @@ -6000,6 +6068,17 @@ static int nf_tables_getobj(struct net *net, struct
> > sock *nlsk, if (NFNL_MSG_TYPE(nlh->nlmsg_type) == NFT_MSG_GETOBJ_RESET)
> >  		reset = true;
> > 
> > +	if (reset) {
> > +		char *buf = kasprintf(GFP_KERNEL, "%s:%llu;?:0",
> > +				      table->name, table->handle);
> > +
> > +		audit_log_nfcfg(buf,
> > +				family,
> > +				obj->handle,
> > +				AUDIT_NFT_OP_OBJ_RESET);
> > +		kfree(buf);
> > +	}
> > +
> >  	err = nf_tables_fill_obj_info(skb2, net, NETLINK_CB(skb).portid,
> >  				      nlh->nlmsg_seq, NFT_MSG_NEWOBJ, 0,
> >  				      family, table, obj, reset);
> > @@ -6075,6 +6154,16 @@ void nft_obj_notify(struct net *net, const struct
> > nft_table *table, {
> >  	struct sk_buff *skb;
> >  	int err;
> > +	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;?:0",
> > +			      table->name, table->handle);
> > +
> > +	audit_log_nfcfg(buf,
> > +			family,
> > +			obj->handle,
> > +			event == NFT_MSG_NEWOBJ ?
> > +				AUDIT_NFT_OP_OBJ_REGISTER :
> > +				AUDIT_NFT_OP_OBJ_UNREGISTER);
> > +	kfree(buf);
> > 
> >  	if (!report &&
> >  	    !nfnetlink_has_listeners(net, NFNLGRP_NFTABLES))
> > @@ -6701,6 +6790,17 @@ static void nf_tables_flowtable_notify(struct
> > nft_ctx *ctx, {
> >  	struct sk_buff *skb;
> >  	int err;
> > +	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;%s:%llu",
> > +			      flowtable->table->name, flowtable->table->handle,
> > +			      flowtable->name, flowtable->handle);
> > +
> > +	audit_log_nfcfg(buf,
> > +			ctx->family,
> > +			flowtable->hooknum,
> > +			event == NFT_MSG_NEWFLOWTABLE ?
> > +				AUDIT_NFT_OP_FLOWTABLE_REGISTER :
> > +				AUDIT_NFT_OP_FLOWTABLE_UNREGISTER);
> > +	kfree(buf);
> > 
> >  	if (ctx->report &&
> >  	    !nfnetlink_has_listeners(ctx->net, NFNLGRP_NFTABLES))
> > @@ -6822,6 +6922,9 @@ static void nf_tables_gen_notify(struct net *net,
> > struct sk_buff *skb, struct sk_buff *skb2;
> >  	int err;
> > 
> > +	audit_log_nfcfg("?:0;?:0", 0, net->nft.base_seq,
> > +			AUDIT_NFT_OP_GEN_REGISTER);
> > +
> >  	if (nlmsg_report(nlh) &&
> >  	    !nfnetlink_has_listeners(net, NFNLGRP_NFTABLES))
> >  		return;
> 
> 
> 
> 

- 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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2020-06-04 17:57   ` Richard Guy Briggs
@ 2020-06-04 18:51     ` Steve Grubb
  0 siblings, 0 replies; 27+ messages in thread
From: Steve Grubb @ 2020-06-04 18:51 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Linux-Audit Mailing List, LKML, netfilter-devel, Paul Moore,
	omosnace, fw, twoerner, eparis, tgraf

On Thursday, June 4, 2020 1:57:56 PM EDT Richard Guy Briggs wrote:
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index 468a23390457..3a9100e95fda 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -75,6 +75,7 @@
> > > #include <linux/uaccess.h>
> > > #include <linux/fsnotify_backend.h>
> > > #include <uapi/linux/limits.h>
> > > +#include <uapi/linux/netfilter/nf_tables.h>
> > > 
> > > #include "audit.h"
> > > 
> > > @@ -136,9 +137,26 @@ struct audit_nfcfgop_tab {
> > > };
> > > 
> > > static const struct audit_nfcfgop_tab audit_nfcfgs[] = {
> > > -       { AUDIT_XT_OP_REGISTER,         "register"      },
> > > -       { AUDIT_XT_OP_REPLACE,          "replace"       },
> > > -       { AUDIT_XT_OP_UNREGISTER,       "unregister"    },
> > > +       { AUDIT_XT_OP_REGISTER,                 "xt_register"
> > 
> > },
> > 
> > > +       { AUDIT_XT_OP_REPLACE,                  "xt_replace"           
> > >    }, +       { AUDIT_XT_OP_UNREGISTER,               "xt_unregister" 
> > >           }, +       { AUDIT_NFT_OP_TABLE_REGISTER,         
> > > "nft_register_table"> 
> > },
> > 
> > > +       { AUDIT_NFT_OP_TABLE_UNREGISTER,        "nft_unregister_table" 
> > >    }, +       { AUDIT_NFT_OP_CHAIN_REGISTER,         
> > > "nft_register_chain"> 
> > },
> > 
> > > +       { AUDIT_NFT_OP_CHAIN_UNREGISTER,        "nft_unregister_chain" 
> > >    }, +       { AUDIT_NFT_OP_RULE_REGISTER,          
> > > "nft_register_rule"> 
> > },
> > 
> > > +       { AUDIT_NFT_OP_RULE_UNREGISTER,         "nft_unregister_rule"
> > 
> > },
> > 
> > > +       { AUDIT_NFT_OP_SET_REGISTER,            "nft_register_set"
> > 
> > },
> > 
> > > +       { AUDIT_NFT_OP_SET_UNREGISTER,          "nft_unregister_set"
> > 
> > },
> > 
> > > +       { AUDIT_NFT_OP_SETELEM_REGISTER,        "nft_register_setelem" 
> > >    }, +       { AUDIT_NFT_OP_SETELEM_UNREGISTER,     
> > > "nft_unregister_setelem"   }, +       { AUDIT_NFT_OP_GEN_REGISTER,    
> > >        "nft_register_gen"         }, +       {
> > > AUDIT_NFT_OP_OBJ_REGISTER,            "nft_register_obj"         }, + 
> > >      { AUDIT_NFT_OP_OBJ_UNREGISTER,          "nft_unregister_obj"     
> > >  }, +       { AUDIT_NFT_OP_OBJ_RESET,               "nft_reset_obj"   
> > >         }, +       { AUDIT_NFT_OP_FLOWTABLE_REGISTER,     
> > > "nft_register_flowtable"   }, +       {
> > > AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,    "nft_unregister_flowtable" }, + 
> > >      { AUDIT_NFT_OP_INVALID,                 "nft_invalid"
> > 
> > },
> > 
> > > };
> > 
> > I still don't like the event format because it doesn't give complete
> > subject information. However, I thought I'd comment on this string
> > table. Usually it's sufficient to log the number and then have the
> > string table in user space which looks it up during interpretation.
> 
> That is a good idea that would help reduce kernel cycles and netlink
> bandwidth, but the format was set in 2011 so it is a bit late to change
> that now:
>         fbabf31e4d48 ("netfilter: create audit records for x_tables
> replaces")

Nothing searches/interprets that field name. So, you can redefine it by 
renaming it. Or just go with what you have. My preference is push that to 
user space. But not a showstopper "as is".

-Steve



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2020-06-04 13:20 [PATCH ghak124 v3] audit: log nftables configuration change events Richard Guy Briggs
  2020-06-04 17:03 ` Steve Grubb
@ 2020-06-24  0:34 ` Paul Moore
  2020-06-24 10:03 ` Pablo Neira Ayuso
  2021-02-11 15:16 ` Phil Sutter
  3 siblings, 0 replies; 27+ messages in thread
From: Paul Moore @ 2020-06-24  0:34 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Linux-Audit Mailing List, LKML, netfilter-devel, sgrubb,
	Ondrej Mosnacek, fw, twoerner, Eric Paris, tgraf

On Thu, Jun 4, 2020 at 9:21 AM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> iptables, ip6tables, arptables and ebtables table registration,
> replacement and unregistration configuration events are logged for the
> native (legacy) iptables setsockopt api, but not for the
> nftables netlink api which is used by the nft-variant of iptables in
> addition to nftables itself.
>
> Add calls to log the configuration actions in the nftables netlink api.
>
> This uses the same NETFILTER_CFG record format but overloads the table
> field.
>
>   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.878:162) : table=?:0;?:0 family=unspecified entries=2 op=nft_register_gen pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
>   ...
>   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.878:162) : table=firewalld:1;?:0 family=inet entries=0 op=nft_register_table pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
>   ...
>   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : table=firewalld:1;filter_FORWARD:85 family=inet entries=8 op=nft_register_chain pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
>   ...
>   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : table=firewalld:1;filter_FORWARD:85 family=inet entries=101 op=nft_register_rule pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
>   ...
>   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : table=firewalld:1;__set0:87 family=inet entries=87 op=nft_register_setelem pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
>   ...
>   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : table=firewalld:1;__set0:87 family=inet entries=0 op=nft_register_set pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
>
> For further information please see issue
> https://github.com/linux-audit/audit-kernel/issues/124
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> Changelog:
> v3:
> - inline message type rather than table
>
> v2:
> - differentiate between xtables and nftables
> - add set, setelem, obj, flowtable, gen
> - use nentries field as appropriate per type
> - overload the "tables" field with table handle and chain/set/flowtable
>
>  include/linux/audit.h         |  18 ++++++++
>  kernel/auditsc.c              |  24 ++++++++--
>  net/netfilter/nf_tables_api.c | 103 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 142 insertions(+), 3 deletions(-)

I'm not seeing any additional comments from the netfilter folks so
I've gone ahead and merged this into audit/next.  Thanks Richard.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2020-06-04 13:20 [PATCH ghak124 v3] audit: log nftables configuration change events Richard Guy Briggs
  2020-06-04 17:03 ` Steve Grubb
  2020-06-24  0:34 ` Paul Moore
@ 2020-06-24 10:03 ` Pablo Neira Ayuso
  2020-06-24 12:34   ` Richard Guy Briggs
  2021-02-11 15:16 ` Phil Sutter
  3 siblings, 1 reply; 27+ messages in thread
From: Pablo Neira Ayuso @ 2020-06-24 10:03 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Linux-Audit Mailing List, LKML, netfilter-devel, Paul Moore,
	sgrubb, omosnace, fw, twoerner, eparis, tgraf

On Thu, Jun 04, 2020 at 09:20:49AM -0400, Richard Guy Briggs wrote:
> iptables, ip6tables, arptables and ebtables table registration,
> replacement and unregistration configuration events are logged for the
> native (legacy) iptables setsockopt api, but not for the
> nftables netlink api which is used by the nft-variant of iptables in
> addition to nftables itself.
> 
> Add calls to log the configuration actions in the nftables netlink api.
> 
> This uses the same NETFILTER_CFG record format but overloads the table
> field.
> 
>   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.878:162) : table=?:0;?:0 family=unspecified entries=2 op=nft_register_gen pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
>   ...
>   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.878:162) : table=firewalld:1;?:0 family=inet entries=0 op=nft_register_table pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
>   ...
>   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : table=firewalld:1;filter_FORWARD:85 family=inet entries=8 op=nft_register_chain pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
>   ...
>   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : table=firewalld:1;filter_FORWARD:85 family=inet entries=101 op=nft_register_rule pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
>   ...
>   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : table=firewalld:1;__set0:87 family=inet entries=87 op=nft_register_setelem pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
>   ...
>   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : table=firewalld:1;__set0:87 family=inet entries=0 op=nft_register_set pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
> 
> For further information please see issue
> https://github.com/linux-audit/audit-kernel/issues/124
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> Changelog:
> v3:
> - inline message type rather than table
> 
> v2:
> - differentiate between xtables and nftables
> - add set, setelem, obj, flowtable, gen
> - use nentries field as appropriate per type
> - overload the "tables" field with table handle and chain/set/flowtable
> 
>  include/linux/audit.h         |  18 ++++++++
>  kernel/auditsc.c              |  24 ++++++++--
>  net/netfilter/nf_tables_api.c | 103 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 142 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 3fcd9ee49734..604ede630580 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -12,6 +12,7 @@
>  #include <linux/sched.h>
>  #include <linux/ptrace.h>
>  #include <uapi/linux/audit.h>
> +#include <uapi/linux/netfilter/nf_tables.h>
>  
>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> @@ -98,6 +99,23 @@ enum audit_nfcfgop {
>  	AUDIT_XT_OP_REGISTER,
>  	AUDIT_XT_OP_REPLACE,
>  	AUDIT_XT_OP_UNREGISTER,
> +	AUDIT_NFT_OP_TABLE_REGISTER,
> +	AUDIT_NFT_OP_TABLE_UNREGISTER,
> +	AUDIT_NFT_OP_CHAIN_REGISTER,
> +	AUDIT_NFT_OP_CHAIN_UNREGISTER,
> +	AUDIT_NFT_OP_RULE_REGISTER,
> +	AUDIT_NFT_OP_RULE_UNREGISTER,
> +	AUDIT_NFT_OP_SET_REGISTER,
> +	AUDIT_NFT_OP_SET_UNREGISTER,
> +	AUDIT_NFT_OP_SETELEM_REGISTER,
> +	AUDIT_NFT_OP_SETELEM_UNREGISTER,
> +	AUDIT_NFT_OP_GEN_REGISTER,
> +	AUDIT_NFT_OP_OBJ_REGISTER,
> +	AUDIT_NFT_OP_OBJ_UNREGISTER,
> +	AUDIT_NFT_OP_OBJ_RESET,
> +	AUDIT_NFT_OP_FLOWTABLE_REGISTER,
> +	AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,
> +	AUDIT_NFT_OP_INVALID,
>  };
>  
>  extern int is_audit_feature_set(int which);
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 468a23390457..3a9100e95fda 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -75,6 +75,7 @@
>  #include <linux/uaccess.h>
>  #include <linux/fsnotify_backend.h>
>  #include <uapi/linux/limits.h>
> +#include <uapi/linux/netfilter/nf_tables.h>
>  
>  #include "audit.h"
>  
> @@ -136,9 +137,26 @@ struct audit_nfcfgop_tab {
>  };
>  
>  static const struct audit_nfcfgop_tab audit_nfcfgs[] = {
> -	{ AUDIT_XT_OP_REGISTER,		"register"	},
> -	{ AUDIT_XT_OP_REPLACE,		"replace"	},
> -	{ AUDIT_XT_OP_UNREGISTER,	"unregister"	},
> +	{ AUDIT_XT_OP_REGISTER,			"xt_register"		   },
> +	{ AUDIT_XT_OP_REPLACE,			"xt_replace"		   },
> +	{ AUDIT_XT_OP_UNREGISTER,		"xt_unregister"		   },
> +	{ AUDIT_NFT_OP_TABLE_REGISTER,		"nft_register_table"	   },
> +	{ AUDIT_NFT_OP_TABLE_UNREGISTER,	"nft_unregister_table"	   },
> +	{ AUDIT_NFT_OP_CHAIN_REGISTER,		"nft_register_chain"	   },
> +	{ AUDIT_NFT_OP_CHAIN_UNREGISTER,	"nft_unregister_chain"	   },
> +	{ AUDIT_NFT_OP_RULE_REGISTER,		"nft_register_rule"	   },
> +	{ AUDIT_NFT_OP_RULE_UNREGISTER,		"nft_unregister_rule"	   },
> +	{ AUDIT_NFT_OP_SET_REGISTER,		"nft_register_set"	   },
> +	{ AUDIT_NFT_OP_SET_UNREGISTER,		"nft_unregister_set"	   },
> +	{ AUDIT_NFT_OP_SETELEM_REGISTER,	"nft_register_setelem"	   },
> +	{ AUDIT_NFT_OP_SETELEM_UNREGISTER,	"nft_unregister_setelem"   },
> +	{ AUDIT_NFT_OP_GEN_REGISTER,		"nft_register_gen"	   },
> +	{ AUDIT_NFT_OP_OBJ_REGISTER,		"nft_register_obj"	   },
> +	{ AUDIT_NFT_OP_OBJ_UNREGISTER,		"nft_unregister_obj"	   },
> +	{ AUDIT_NFT_OP_OBJ_RESET,		"nft_reset_obj"		   },
> +	{ AUDIT_NFT_OP_FLOWTABLE_REGISTER,	"nft_register_flowtable"   },
> +	{ AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,	"nft_unregister_flowtable" },
> +	{ AUDIT_NFT_OP_INVALID,			"nft_invalid"		   },
>  };
>  
>  static int audit_match_perm(struct audit_context *ctx, int mask)
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 3558e76e2733..b9e7440cc87d 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -12,6 +12,7 @@
>  #include <linux/netlink.h>
>  #include <linux/vmalloc.h>
>  #include <linux/rhashtable.h>
> +#include <linux/audit.h>
>  #include <linux/netfilter.h>
>  #include <linux/netfilter/nfnetlink.h>
>  #include <linux/netfilter/nf_tables.h>
> @@ -693,6 +694,16 @@ static void nf_tables_table_notify(const struct nft_ctx *ctx, int event)
>  {
>  	struct sk_buff *skb;
>  	int err;
> +	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;?:0",
> +			      ctx->table->name, ctx->table->handle);
> +
> +	audit_log_nfcfg(buf,
> +			ctx->family,
> +			ctx->table->use,
> +			event == NFT_MSG_NEWTABLE ?
> +				AUDIT_NFT_OP_TABLE_REGISTER :
> +				AUDIT_NFT_OP_TABLE_UNREGISTER);
> +	kfree(buf);

As a follow up: Would you wrap this code into a function?

        nft_table_audit()

Same thing for other pieces of code below.

Thanks.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2020-06-24 10:03 ` Pablo Neira Ayuso
@ 2020-06-24 12:34   ` Richard Guy Briggs
  2020-06-24 13:03     ` Pablo Neira Ayuso
  0 siblings, 1 reply; 27+ messages in thread
From: Richard Guy Briggs @ 2020-06-24 12:34 UTC (permalink / raw)
  To: Pablo Neira Ayuso
  Cc: Linux-Audit Mailing List, LKML, netfilter-devel, Paul Moore,
	sgrubb, omosnace, fw, twoerner, eparis, tgraf

On 2020-06-24 12:03, Pablo Neira Ayuso wrote:
> On Thu, Jun 04, 2020 at 09:20:49AM -0400, Richard Guy Briggs wrote:
> > iptables, ip6tables, arptables and ebtables table registration,
> > replacement and unregistration configuration events are logged for the
> > native (legacy) iptables setsockopt api, but not for the
> > nftables netlink api which is used by the nft-variant of iptables in
> > addition to nftables itself.
> > 
> > Add calls to log the configuration actions in the nftables netlink api.
> > 
> > This uses the same NETFILTER_CFG record format but overloads the table
> > field.
> > 
> >   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.878:162) : table=?:0;?:0 family=unspecified entries=2 op=nft_register_gen pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
> >   ...
> >   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.878:162) : table=firewalld:1;?:0 family=inet entries=0 op=nft_register_table pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
> >   ...
> >   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : table=firewalld:1;filter_FORWARD:85 family=inet entries=8 op=nft_register_chain pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
> >   ...
> >   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : table=firewalld:1;filter_FORWARD:85 family=inet entries=101 op=nft_register_rule pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
> >   ...
> >   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : table=firewalld:1;__set0:87 family=inet entries=87 op=nft_register_setelem pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
> >   ...
> >   type=NETFILTER_CFG msg=audit(2020-05-28 17:46:41.911:163) : table=firewalld:1;__set0:87 family=inet entries=0 op=nft_register_set pid=396 subj=system_u:system_r:firewalld_t:s0 comm=firewalld
> > 
> > For further information please see issue
> > https://github.com/linux-audit/audit-kernel/issues/124
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > Changelog:
> > v3:
> > - inline message type rather than table
> > 
> > v2:
> > - differentiate between xtables and nftables
> > - add set, setelem, obj, flowtable, gen
> > - use nentries field as appropriate per type
> > - overload the "tables" field with table handle and chain/set/flowtable
> > 
> >  include/linux/audit.h         |  18 ++++++++
> >  kernel/auditsc.c              |  24 ++++++++--
> >  net/netfilter/nf_tables_api.c | 103 ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 142 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 3fcd9ee49734..604ede630580 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -12,6 +12,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/ptrace.h>
> >  #include <uapi/linux/audit.h>
> > +#include <uapi/linux/netfilter/nf_tables.h>
> >  
> >  #define AUDIT_INO_UNSET ((unsigned long)-1)
> >  #define AUDIT_DEV_UNSET ((dev_t)-1)
> > @@ -98,6 +99,23 @@ enum audit_nfcfgop {
> >  	AUDIT_XT_OP_REGISTER,
> >  	AUDIT_XT_OP_REPLACE,
> >  	AUDIT_XT_OP_UNREGISTER,
> > +	AUDIT_NFT_OP_TABLE_REGISTER,
> > +	AUDIT_NFT_OP_TABLE_UNREGISTER,
> > +	AUDIT_NFT_OP_CHAIN_REGISTER,
> > +	AUDIT_NFT_OP_CHAIN_UNREGISTER,
> > +	AUDIT_NFT_OP_RULE_REGISTER,
> > +	AUDIT_NFT_OP_RULE_UNREGISTER,
> > +	AUDIT_NFT_OP_SET_REGISTER,
> > +	AUDIT_NFT_OP_SET_UNREGISTER,
> > +	AUDIT_NFT_OP_SETELEM_REGISTER,
> > +	AUDIT_NFT_OP_SETELEM_UNREGISTER,
> > +	AUDIT_NFT_OP_GEN_REGISTER,
> > +	AUDIT_NFT_OP_OBJ_REGISTER,
> > +	AUDIT_NFT_OP_OBJ_UNREGISTER,
> > +	AUDIT_NFT_OP_OBJ_RESET,
> > +	AUDIT_NFT_OP_FLOWTABLE_REGISTER,
> > +	AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,
> > +	AUDIT_NFT_OP_INVALID,
> >  };
> >  
> >  extern int is_audit_feature_set(int which);
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 468a23390457..3a9100e95fda 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -75,6 +75,7 @@
> >  #include <linux/uaccess.h>
> >  #include <linux/fsnotify_backend.h>
> >  #include <uapi/linux/limits.h>
> > +#include <uapi/linux/netfilter/nf_tables.h>
> >  
> >  #include "audit.h"
> >  
> > @@ -136,9 +137,26 @@ struct audit_nfcfgop_tab {
> >  };
> >  
> >  static const struct audit_nfcfgop_tab audit_nfcfgs[] = {
> > -	{ AUDIT_XT_OP_REGISTER,		"register"	},
> > -	{ AUDIT_XT_OP_REPLACE,		"replace"	},
> > -	{ AUDIT_XT_OP_UNREGISTER,	"unregister"	},
> > +	{ AUDIT_XT_OP_REGISTER,			"xt_register"		   },
> > +	{ AUDIT_XT_OP_REPLACE,			"xt_replace"		   },
> > +	{ AUDIT_XT_OP_UNREGISTER,		"xt_unregister"		   },
> > +	{ AUDIT_NFT_OP_TABLE_REGISTER,		"nft_register_table"	   },
> > +	{ AUDIT_NFT_OP_TABLE_UNREGISTER,	"nft_unregister_table"	   },
> > +	{ AUDIT_NFT_OP_CHAIN_REGISTER,		"nft_register_chain"	   },
> > +	{ AUDIT_NFT_OP_CHAIN_UNREGISTER,	"nft_unregister_chain"	   },
> > +	{ AUDIT_NFT_OP_RULE_REGISTER,		"nft_register_rule"	   },
> > +	{ AUDIT_NFT_OP_RULE_UNREGISTER,		"nft_unregister_rule"	   },
> > +	{ AUDIT_NFT_OP_SET_REGISTER,		"nft_register_set"	   },
> > +	{ AUDIT_NFT_OP_SET_UNREGISTER,		"nft_unregister_set"	   },
> > +	{ AUDIT_NFT_OP_SETELEM_REGISTER,	"nft_register_setelem"	   },
> > +	{ AUDIT_NFT_OP_SETELEM_UNREGISTER,	"nft_unregister_setelem"   },
> > +	{ AUDIT_NFT_OP_GEN_REGISTER,		"nft_register_gen"	   },
> > +	{ AUDIT_NFT_OP_OBJ_REGISTER,		"nft_register_obj"	   },
> > +	{ AUDIT_NFT_OP_OBJ_UNREGISTER,		"nft_unregister_obj"	   },
> > +	{ AUDIT_NFT_OP_OBJ_RESET,		"nft_reset_obj"		   },
> > +	{ AUDIT_NFT_OP_FLOWTABLE_REGISTER,	"nft_register_flowtable"   },
> > +	{ AUDIT_NFT_OP_FLOWTABLE_UNREGISTER,	"nft_unregister_flowtable" },
> > +	{ AUDIT_NFT_OP_INVALID,			"nft_invalid"		   },
> >  };
> >  
> >  static int audit_match_perm(struct audit_context *ctx, int mask)
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index 3558e76e2733..b9e7440cc87d 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/netlink.h>
> >  #include <linux/vmalloc.h>
> >  #include <linux/rhashtable.h>
> > +#include <linux/audit.h>
> >  #include <linux/netfilter.h>
> >  #include <linux/netfilter/nfnetlink.h>
> >  #include <linux/netfilter/nf_tables.h>
> > @@ -693,6 +694,16 @@ static void nf_tables_table_notify(const struct nft_ctx *ctx, int event)
> >  {
> >  	struct sk_buff *skb;
> >  	int err;
> > +	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;?:0",
> > +			      ctx->table->name, ctx->table->handle);
> > +
> > +	audit_log_nfcfg(buf,
> > +			ctx->family,
> > +			ctx->table->use,
> > +			event == NFT_MSG_NEWTABLE ?
> > +				AUDIT_NFT_OP_TABLE_REGISTER :
> > +				AUDIT_NFT_OP_TABLE_UNREGISTER);
> > +	kfree(buf);
> 
> As a follow up: Would you wrap this code into a function?
> 
>         nft_table_audit()
> 
> Same thing for other pieces of code below.

If I'm guessing right, you are asking for a supplementary follow-up
cleanup patch to this one (or are you nacking this patch)?

Also, I gather you would like to see the kasprintf and kfree hidden in
nft_table_audit(), handing this function at least 8 parameters?  This
sounds pretty messy given the format of the table field.

> 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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2020-06-24 12:34   ` Richard Guy Briggs
@ 2020-06-24 13:03     ` Pablo Neira Ayuso
  2020-06-24 13:26       ` Richard Guy Briggs
  0 siblings, 1 reply; 27+ messages in thread
From: Pablo Neira Ayuso @ 2020-06-24 13:03 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Linux-Audit Mailing List, LKML, netfilter-devel, Paul Moore,
	sgrubb, omosnace, fw, twoerner, eparis, tgraf

On Wed, Jun 24, 2020 at 08:34:23AM -0400, Richard Guy Briggs wrote:
> On 2020-06-24 12:03, Pablo Neira Ayuso wrote:
> > On Thu, Jun 04, 2020 at 09:20:49AM -0400, Richard Guy Briggs wrote:
[...]
> > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > > index 3558e76e2733..b9e7440cc87d 100644
> > > --- a/net/netfilter/nf_tables_api.c
> > > +++ b/net/netfilter/nf_tables_api.c
> > > @@ -12,6 +12,7 @@
> > >  #include <linux/netlink.h>
> > >  #include <linux/vmalloc.h>
> > >  #include <linux/rhashtable.h>
> > > +#include <linux/audit.h>
> > >  #include <linux/netfilter.h>
> > >  #include <linux/netfilter/nfnetlink.h>
> > >  #include <linux/netfilter/nf_tables.h>
> > > @@ -693,6 +694,16 @@ static void nf_tables_table_notify(const struct nft_ctx *ctx, int event)
> > >  {
> > >  	struct sk_buff *skb;
> > >  	int err;
> > > +	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;?:0",
> > > +			      ctx->table->name, ctx->table->handle);
> > > +
> > > +	audit_log_nfcfg(buf,
> > > +			ctx->family,
> > > +			ctx->table->use,
> > > +			event == NFT_MSG_NEWTABLE ?
> > > +				AUDIT_NFT_OP_TABLE_REGISTER :
> > > +				AUDIT_NFT_OP_TABLE_UNREGISTER);
> > > +	kfree(buf);
> > 
> > As a follow up: Would you wrap this code into a function?
> > 
> >         nft_table_audit()
> > 
> > Same thing for other pieces of code below.
> 
> If I'm guessing right, you are asking for a supplementary follow-up
> cleanup patch to this one (or are you nacking this patch)?

No nack, it's just that I'd prefer to see this wrapped in a function.
I think your patch is already in the audit tree.

> Also, I gather you would like to see the kasprintf and kfree hidden in
> nft_table_audit(), handing this function at least 8 parameters?  This
> sounds pretty messy given the format of the table field.

I think you can pass ctx and the specific object, e.g. table, in most
cases? There is also event and the gfp_flags. That counts 4 here, but
maybe I'm overlooking something.

Thanks.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2020-06-24 13:03     ` Pablo Neira Ayuso
@ 2020-06-24 13:26       ` Richard Guy Briggs
  0 siblings, 0 replies; 27+ messages in thread
From: Richard Guy Briggs @ 2020-06-24 13:26 UTC (permalink / raw)
  To: Pablo Neira Ayuso
  Cc: Linux-Audit Mailing List, LKML, netfilter-devel, Paul Moore,
	sgrubb, omosnace, fw, twoerner, eparis, tgraf

On 2020-06-24 15:03, Pablo Neira Ayuso wrote:
> On Wed, Jun 24, 2020 at 08:34:23AM -0400, Richard Guy Briggs wrote:
> > On 2020-06-24 12:03, Pablo Neira Ayuso wrote:
> > > On Thu, Jun 04, 2020 at 09:20:49AM -0400, Richard Guy Briggs wrote:
> [...]
> > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > > > index 3558e76e2733..b9e7440cc87d 100644
> > > > --- a/net/netfilter/nf_tables_api.c
> > > > +++ b/net/netfilter/nf_tables_api.c
> > > > @@ -12,6 +12,7 @@
> > > >  #include <linux/netlink.h>
> > > >  #include <linux/vmalloc.h>
> > > >  #include <linux/rhashtable.h>
> > > > +#include <linux/audit.h>
> > > >  #include <linux/netfilter.h>
> > > >  #include <linux/netfilter/nfnetlink.h>
> > > >  #include <linux/netfilter/nf_tables.h>
> > > > @@ -693,6 +694,16 @@ static void nf_tables_table_notify(const struct nft_ctx *ctx, int event)
> > > >  {
> > > >  	struct sk_buff *skb;
> > > >  	int err;
> > > > +	char *buf = kasprintf(GFP_KERNEL, "%s:%llu;?:0",
> > > > +			      ctx->table->name, ctx->table->handle);
> > > > +
> > > > +	audit_log_nfcfg(buf,
> > > > +			ctx->family,
> > > > +			ctx->table->use,
> > > > +			event == NFT_MSG_NEWTABLE ?
> > > > +				AUDIT_NFT_OP_TABLE_REGISTER :
> > > > +				AUDIT_NFT_OP_TABLE_UNREGISTER);
> > > > +	kfree(buf);
> > > 
> > > As a follow up: Would you wrap this code into a function?
> > > 
> > >         nft_table_audit()
> > > 
> > > Same thing for other pieces of code below.
> > 
> > If I'm guessing right, you are asking for a supplementary follow-up
> > cleanup patch to this one (or are you nacking this patch)?
> 
> No nack, it's just that I'd prefer to see this wrapped in a function.
> I think your patch is already in the audit tree.
> 
> > Also, I gather you would like to see the kasprintf and kfree hidden in
> > nft_table_audit(), handing this function at least 8 parameters?  This
> > sounds pretty messy given the format of the table field.
> 
> I think you can pass ctx and the specific object, e.g. table, in most
> cases? There is also event and the gfp_flags. That counts 4 here, but
> maybe I'm overlooking something.

Since every event is sufficiently different, it isn't as simple as
passing ctx, unfortunately, and the table field I've overloaded with 4
bits of information for tracking the chain as well, some of which are ?
that would need an in-band representation (such as -1? that might
already be valid).  So 4 right there, family, nentries, event, gfp for 8.

I did try in the first patch to make it just one call keyed on event,
but there was enough variety of information available for each message
type that it became necessary to break it out.

> 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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2020-06-04 13:20 [PATCH ghak124 v3] audit: log nftables configuration change events Richard Guy Briggs
                   ` (2 preceding siblings ...)
  2020-06-24 10:03 ` Pablo Neira Ayuso
@ 2021-02-11 15:16 ` Phil Sutter
  2021-02-11 16:29   ` Paul Moore
  3 siblings, 1 reply; 27+ messages in thread
From: Phil Sutter @ 2021-02-11 15:16 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Linux-Audit Mailing List, LKML, netfilter-devel, Paul Moore,
	sgrubb, omosnace, fw, twoerner, eparis, tgraf

Hi,

On Thu, Jun 04, 2020 at 09:20:49AM -0400, Richard Guy Briggs wrote:
> iptables, ip6tables, arptables and ebtables table registration,
> replacement and unregistration configuration events are logged for the
> native (legacy) iptables setsockopt api, but not for the
> nftables netlink api which is used by the nft-variant of iptables in
> addition to nftables itself.
> 
> Add calls to log the configuration actions in the nftables netlink api.

As discussed offline already, these audit notifications are pretty hefty
performance-wise. In an internal report, 300% restore time of a ruleset
containing 70k set elements is measured.

If I'm not mistaken, iptables emits a single audit log per table, ipset
doesn't support audit at all. So I wonder how much audit logging is
required at all (for certification or whatever reason). How much
granularity is desired?

I personally would notify once per transaction. This is easy and quick.
Once per table or chain should be acceptable, as well. At the very
least, we should not have to notify once per each element. This is the
last resort of fast ruleset adjustments. If we lose it, people are
better off with ipset IMHO.

Unlike nft monitor, auditd is not designed to be disabled "at will". So
turning it off for performance-critical workloads is no option.

Cheers, Phil

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2021-02-11 15:16 ` Phil Sutter
@ 2021-02-11 16:29   ` Paul Moore
  2021-02-11 20:26     ` Richard Guy Briggs
  2021-02-11 21:02     ` Steve Grubb
  0 siblings, 2 replies; 27+ messages in thread
From: Paul Moore @ 2021-02-11 16:29 UTC (permalink / raw)
  To: Phil Sutter, Richard Guy Briggs, Linux-Audit Mailing List, LKML,
	netfilter-devel, Paul Moore, sgrubb, Ondrej Mosnacek, fw,
	twoerner, Eric Paris, tgraf

On Thu, Feb 11, 2021 at 10:16 AM Phil Sutter <phil@nwl.cc> wrote:
> Hi,
>
> On Thu, Jun 04, 2020 at 09:20:49AM -0400, Richard Guy Briggs wrote:
> > iptables, ip6tables, arptables and ebtables table registration,
> > replacement and unregistration configuration events are logged for the
> > native (legacy) iptables setsockopt api, but not for the
> > nftables netlink api which is used by the nft-variant of iptables in
> > addition to nftables itself.
> >
> > Add calls to log the configuration actions in the nftables netlink api.
>
> As discussed offline already, these audit notifications are pretty hefty
> performance-wise. In an internal report, 300% restore time of a ruleset
> containing 70k set elements is measured.

If you're going to reference offline/off-list discussions in a post to
a public list, perhaps the original discussion shouldn't have been
off-list ;)  If you don't involve us in the discussion, we have to
waste a lot of time getting caught up.

> If I'm not mistaken, iptables emits a single audit log per table, ipset
> doesn't support audit at all. So I wonder how much audit logging is
> required at all (for certification or whatever reason). How much
> granularity is desired?

That's a question for the people who track these certification
requirements, which is thankfully not me at the moment.  Unless
somebody else wants to speak up, Steve Grubb is probably the only
person who tracks that sort of stuff and comments here.

I believe the netfilter auditing was mostly a nice-to-have bit of
functionality to help add to the completeness of the audit logs, but I
could very easily be mistaken.  Richard put together those patches, he
can probably provide the background/motivation for the effort.

> I personally would notify once per transaction. This is easy and quick.
> Once per table or chain should be acceptable, as well. At the very
> least, we should not have to notify once per each element. This is the
> last resort of fast ruleset adjustments. If we lose it, people are
> better off with ipset IMHO.
>
> Unlike nft monitor, auditd is not designed to be disabled "at will". So
> turning it off for performance-critical workloads is no option.

Patches are always welcome, but it might be wise to get to the bottom
of the certification requirements first.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2021-02-11 16:29   ` Paul Moore
@ 2021-02-11 20:26     ` Richard Guy Briggs
  2021-02-11 22:09       ` Florian Westphal
  2021-02-12 20:48       ` Richard Guy Briggs
  2021-02-11 21:02     ` Steve Grubb
  1 sibling, 2 replies; 27+ messages in thread
From: Richard Guy Briggs @ 2021-02-11 20:26 UTC (permalink / raw)
  To: Paul Moore
  Cc: Phil Sutter, Linux-Audit Mailing List, LKML, netfilter-devel,
	sgrubb, Ondrej Mosnacek, fw, twoerner, Eric Paris, tgraf

On 2021-02-11 11:29, Paul Moore wrote:
> On Thu, Feb 11, 2021 at 10:16 AM Phil Sutter <phil@nwl.cc> wrote:
> > Hi,
> >
> > On Thu, Jun 04, 2020 at 09:20:49AM -0400, Richard Guy Briggs wrote:
> > > iptables, ip6tables, arptables and ebtables table registration,
> > > replacement and unregistration configuration events are logged for the
> > > native (legacy) iptables setsockopt api, but not for the
> > > nftables netlink api which is used by the nft-variant of iptables in
> > > addition to nftables itself.
> > >
> > > Add calls to log the configuration actions in the nftables netlink api.
> >
> > As discussed offline already, these audit notifications are pretty hefty
> > performance-wise. In an internal report, 300% restore time of a ruleset
> > containing 70k set elements is measured.
> 
> If you're going to reference offline/off-list discussions in a post to
> a public list, perhaps the original discussion shouldn't have been
> off-list ;)  If you don't involve us in the discussion, we have to
> waste a lot of time getting caught up.

Here's part of that discussion:
	https://bugzilla.redhat.com/show_bug.cgi?id=1918013

> > If I'm not mistaken, iptables emits a single audit log per table, ipset
> > doesn't support audit at all. So I wonder how much audit logging is
> > required at all (for certification or whatever reason). How much
> > granularity is desired?
> 
> That's a question for the people who track these certification
> requirements, which is thankfully not me at the moment.  Unless
> somebody else wants to speak up, Steve Grubb is probably the only
> person who tracks that sort of stuff and comments here.
> 
> I believe the netfilter auditing was mostly a nice-to-have bit of
> functionality to help add to the completeness of the audit logs, but I
> could very easily be mistaken.  Richard put together those patches, he
> can probably provide the background/motivation for the effort.

It was added because an audit test that normally produced records from
iptables on one distro stopped producing any records on another.
Investigation led to the fact that on the first it was using
iptables-legacy API and on the other it was using iptables-nft API.

> > I personally would notify once per transaction. This is easy and quick.

This was the goal.  iptables was atomic.  nftables appears to no longer
be so.  If I have this wrong, please show how that works.

> > Once per table or chain should be acceptable, as well. At the very
> > least, we should not have to notify once per each element. This is the
> > last resort of fast ruleset adjustments. If we lose it, people are
> > better off with ipset IMHO.
> >
> > Unlike nft monitor, auditd is not designed to be disabled "at will". So
> > turning it off for performance-critical workloads is no option.

If it were to be disabled "at will" it would defeat the purpose of
audit.  Those records can already be filtered, or audit can be disabled,
but let us look at rationalizing the current nftables records first.

> Patches are always welcome, but it might be wise to get to the bottom
> of the certification requirements first.
> 
> -- 
> paul moore
> www.paul-moore.com
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

- 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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2021-02-11 16:29   ` Paul Moore
  2021-02-11 20:26     ` Richard Guy Briggs
@ 2021-02-11 21:02     ` Steve Grubb
  2021-02-12 12:11       ` Phil Sutter
  1 sibling, 1 reply; 27+ messages in thread
From: Steve Grubb @ 2021-02-11 21:02 UTC (permalink / raw)
  To: Phil Sutter, Richard Guy Briggs, Linux-Audit Mailing List, LKML,
	netfilter-devel, Paul Moore, Ondrej Mosnacek, fw, twoerner,
	Eric Paris, tgraf, Paul Moore

On Thursday, February 11, 2021 11:29:34 AM EST Paul Moore wrote:
> > If I'm not mistaken, iptables emits a single audit log per table, ipset
> > doesn't support audit at all. So I wonder how much audit logging is
> > required at all (for certification or whatever reason). How much
> > granularity is desired?
 
  <snip> 

> I believe the netfilter auditing was mostly a nice-to-have bit of
> functionality to help add to the completeness of the audit logs, but I
> could very easily be mistaken.  Richard put together those patches, he
> can probably provide the background/motivation for the effort.

There are certifications which levy requirements on information flow control. 
The firewall can decide if information should flow or be blocked. Information 
flow decisions need to be auditable - which we have with the audit target. 
That then swings in requirements on the configuration of the information flow 
policy.

The requirements state a need to audit any management activity - meaning the 
creation, modification, and/or deletion of a "firewall ruleset". Because it 
talks constantly about a ruleset and then individual rules, I suspect only 1 
summary event is needed to say something happened, who did it, and the 
outcome. This would be in line with how selinux is treated: we have 1 summary 
event for loading/modifying/unloading selinux policy.

Hope this helps...

-Steve



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2021-02-11 20:26     ` Richard Guy Briggs
@ 2021-02-11 22:09       ` Florian Westphal
  2021-02-17 23:41         ` Richard Guy Briggs
  2021-02-12 20:48       ` Richard Guy Briggs
  1 sibling, 1 reply; 27+ messages in thread
From: Florian Westphal @ 2021-02-11 22:09 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Paul Moore, Phil Sutter, Linux-Audit Mailing List, LKML,
	netfilter-devel, sgrubb, Ondrej Mosnacek, fw, twoerner,
	Eric Paris, tgraf

Richard Guy Briggs <rgb@redhat.com> wrote:
> > > I personally would notify once per transaction. This is easy and quick.
> 
> This was the goal.  iptables was atomic.  nftables appears to no longer
> be so.  If I have this wrong, please show how that works.

nftables transactions are atomic, either the entire batch takes effect or not
at all.

The audit_log_nfcfg() calls got added to the the nft monitor infra which
is designed to allow userspace to follow the entire content of the
transaction log.

So, if its just a 'something was changed' event that is needed all of
them can be removed. ATM the audit_log_nfcfg() looks like this:

        /* step 3. Start new generation, rules_gen_X now in use. */
        net->nft.gencursor = nft_gencursor_next(net);

        list_for_each_entry_safe(trans, next, &net->nft.commit_list, list) {
                switch (trans->msg_type) {
                case NFT_MSG_NEWTABLE:
			audit_log_nfcfg();
			...
		case NFT_MSG_...
			audit_log_nfcfg();
	..
	       	}

which gives an audit_log for every single change in the batch.

So, if just a summary is needed a single audit_log_nfcfg()
after 'step 3' and outside of the list_for_each_entry_safe() is all
that is needed.

If a summary is wanted as well one could fe. count the number of
transaction types in the batch, e.g. table adds, chain adds, rule
adds etc. and then log a summary count instead.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2021-02-11 21:02     ` Steve Grubb
@ 2021-02-12 12:11       ` Phil Sutter
  2021-02-12 20:54         ` Richard Guy Briggs
  0 siblings, 1 reply; 27+ messages in thread
From: Phil Sutter @ 2021-02-12 12:11 UTC (permalink / raw)
  To: Steve Grubb
  Cc: Richard Guy Briggs, Linux-Audit Mailing List, LKML,
	netfilter-devel, Paul Moore, Ondrej Mosnacek, fw, twoerner,
	Eric Paris, tgraf

Hi,

On Thu, Feb 11, 2021 at 04:02:55PM -0500, Steve Grubb wrote:
> On Thursday, February 11, 2021 11:29:34 AM EST Paul Moore wrote:
> > > If I'm not mistaken, iptables emits a single audit log per table, ipset
> > > doesn't support audit at all. So I wonder how much audit logging is
> > > required at all (for certification or whatever reason). How much
> > > granularity is desired?
>  
>   <snip> 
> 
> > I believe the netfilter auditing was mostly a nice-to-have bit of
> > functionality to help add to the completeness of the audit logs, but I
> > could very easily be mistaken.  Richard put together those patches, he
> > can probably provide the background/motivation for the effort.
> 
> There are certifications which levy requirements on information flow control. 
> The firewall can decide if information should flow or be blocked. Information 
> flow decisions need to be auditable - which we have with the audit target. 

In nftables, this is realized via 'log level audit' statement.
Functionality should by all means be identical to that of xtables' AUDIT
target.

> That then swings in requirements on the configuration of the information flow 
> policy.
> 
> The requirements state a need to audit any management activity - meaning the 
> creation, modification, and/or deletion of a "firewall ruleset". Because it 
> talks constantly about a ruleset and then individual rules, I suspect only 1 
> summary event is needed to say something happened, who did it, and the 
> outcome. This would be in line with how selinux is treated: we have 1 summary 
> event for loading/modifying/unloading selinux policy.

So the central element are firewall rules for audit purposes and
NETFILTER_CFG notifications merely serve asserting changes to those
rules are noticed by the auditing system. Looking at xtables again, this
seems coherent: Any change causes the whole table blob to be replaced
(while others stay in place). So table replace/create is the most common
place for a change notification. In nftables, the most common one is
generation dump - all tables are treated as elements of the same
ruleset, not individually like in xtables.

Richard, assuming the above is correct, are you fine with reducing
nftables auditing to a single notification per transaction then? I guess
Florian sufficiently illustrated how this would be implemented.

> Hope this helps...

It does, thanks a lot for the information!

Thanks, Phil

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2021-02-11 20:26     ` Richard Guy Briggs
  2021-02-11 22:09       ` Florian Westphal
@ 2021-02-12 20:48       ` Richard Guy Briggs
  1 sibling, 0 replies; 27+ messages in thread
From: Richard Guy Briggs @ 2021-02-12 20:48 UTC (permalink / raw)
  To: Paul Moore
  Cc: Phil Sutter, Linux-Audit Mailing List, LKML, netfilter-devel,
	sgrubb, Ondrej Mosnacek, fw, twoerner, Eric Paris, tgraf

On 2021-02-11 15:26, Richard Guy Briggs wrote:
> On 2021-02-11 11:29, Paul Moore wrote:
> > On Thu, Feb 11, 2021 at 10:16 AM Phil Sutter <phil@nwl.cc> wrote:
> > > Hi,
> > >
> > > On Thu, Jun 04, 2020 at 09:20:49AM -0400, Richard Guy Briggs wrote:
> > > > iptables, ip6tables, arptables and ebtables table registration,
> > > > replacement and unregistration configuration events are logged for the
> > > > native (legacy) iptables setsockopt api, but not for the
> > > > nftables netlink api which is used by the nft-variant of iptables in
> > > > addition to nftables itself.
> > > >
> > > > Add calls to log the configuration actions in the nftables netlink api.
> > >
> > > As discussed offline already, these audit notifications are pretty hefty
> > > performance-wise. In an internal report, 300% restore time of a ruleset
> > > containing 70k set elements is measured.
> > 
> > If you're going to reference offline/off-list discussions in a post to
> > a public list, perhaps the original discussion shouldn't have been
> > off-list ;)  If you don't involve us in the discussion, we have to
> > waste a lot of time getting caught up.
> 
> Here's part of that discussion:
> 	https://bugzilla.redhat.com/show_bug.cgi?id=1918013

Here's the rest:
	 https://bugzilla.redhat.com/show_bug.cgi?id=1921624

> > > If I'm not mistaken, iptables emits a single audit log per table, ipset
> > > doesn't support audit at all. So I wonder how much audit logging is
> > > required at all (for certification or whatever reason). How much
> > > granularity is desired?
> > 
> > That's a question for the people who track these certification
> > requirements, which is thankfully not me at the moment.  Unless
> > somebody else wants to speak up, Steve Grubb is probably the only
> > person who tracks that sort of stuff and comments here.
> > 
> > I believe the netfilter auditing was mostly a nice-to-have bit of
> > functionality to help add to the completeness of the audit logs, but I
> > could very easily be mistaken.  Richard put together those patches, he
> > can probably provide the background/motivation for the effort.
> 
> It was added because an audit test that normally produced records from
> iptables on one distro stopped producing any records on another.
> Investigation led to the fact that on the first it was using
> iptables-legacy API and on the other it was using iptables-nft API.
> 
> > > I personally would notify once per transaction. This is easy and quick.
> 
> This was the goal.  iptables was atomic.  nftables appears to no longer
> be so.  If I have this wrong, please show how that works.
> 
> > > Once per table or chain should be acceptable, as well. At the very
> > > least, we should not have to notify once per each element. This is the
> > > last resort of fast ruleset adjustments. If we lose it, people are
> > > better off with ipset IMHO.
> > >
> > > Unlike nft monitor, auditd is not designed to be disabled "at will". So
> > > turning it off for performance-critical workloads is no option.
> 
> If it were to be disabled "at will" it would defeat the purpose of
> audit.  Those records can already be filtered, or audit can be disabled,
> but let us look at rationalizing the current nftables records first.
> 
> > Patches are always welcome, but it might be wise to get to the bottom
> > of the certification requirements first.
> > 
> > paul moore
> 
> - RGB

- 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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2021-02-12 12:11       ` Phil Sutter
@ 2021-02-12 20:54         ` Richard Guy Briggs
  0 siblings, 0 replies; 27+ messages in thread
From: Richard Guy Briggs @ 2021-02-12 20:54 UTC (permalink / raw)
  To: Phil Sutter, Steve Grubb, Linux-Audit Mailing List, LKML,
	netfilter-devel, Paul Moore, Ondrej Mosnacek, fw, twoerner,
	Eric Paris, tgraf

On 2021-02-12 13:11, Phil Sutter wrote:
> Hi,
> 
> On Thu, Feb 11, 2021 at 04:02:55PM -0500, Steve Grubb wrote:
> > On Thursday, February 11, 2021 11:29:34 AM EST Paul Moore wrote:
> > > > If I'm not mistaken, iptables emits a single audit log per table, ipset
> > > > doesn't support audit at all. So I wonder how much audit logging is
> > > > required at all (for certification or whatever reason). How much
> > > > granularity is desired?
> >  
> >   <snip> 
> > 
> > > I believe the netfilter auditing was mostly a nice-to-have bit of
> > > functionality to help add to the completeness of the audit logs, but I
> > > could very easily be mistaken.  Richard put together those patches, he
> > > can probably provide the background/motivation for the effort.
> > 
> > There are certifications which levy requirements on information flow control. 
> > The firewall can decide if information should flow or be blocked. Information 
> > flow decisions need to be auditable - which we have with the audit target. 
> 
> In nftables, this is realized via 'log level audit' statement.
> Functionality should by all means be identical to that of xtables' AUDIT
> target.
> 
> > That then swings in requirements on the configuration of the information flow 
> > policy.
> > 
> > The requirements state a need to audit any management activity - meaning the 
> > creation, modification, and/or deletion of a "firewall ruleset". Because it 
> > talks constantly about a ruleset and then individual rules, I suspect only 1 
> > summary event is needed to say something happened, who did it, and the 
> > outcome. This would be in line with how selinux is treated: we have 1 summary 
> > event for loading/modifying/unloading selinux policy.
> 
> So the central element are firewall rules for audit purposes and
> NETFILTER_CFG notifications merely serve asserting changes to those
> rules are noticed by the auditing system. Looking at xtables again, this
> seems coherent: Any change causes the whole table blob to be replaced
> (while others stay in place). So table replace/create is the most common
> place for a change notification. In nftables, the most common one is
> generation dump - all tables are treated as elements of the same
> ruleset, not individually like in xtables.
> 
> Richard, assuming the above is correct, are you fine with reducing
> nftables auditing to a single notification per transaction then? I guess
> Florian sufficiently illustrated how this would be implemented.

Yes, that should be possible.

> > Hope this helps...
> 
> It does, thanks a lot for the information!
> 
> Thanks, Phil

- 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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2021-02-11 22:09       ` Florian Westphal
@ 2021-02-17 23:41         ` Richard Guy Briggs
  2021-02-18  8:22           ` Florian Westphal
  0 siblings, 1 reply; 27+ messages in thread
From: Richard Guy Briggs @ 2021-02-17 23:41 UTC (permalink / raw)
  To: Florian Westphal
  Cc: Phil Sutter, LKML, Linux-Audit Mailing List, netfilter-devel,
	twoerner, Eric Paris, tgraf

On 2021-02-11 23:09, Florian Westphal wrote:
> Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > I personally would notify once per transaction. This is easy and quick.
> > 
> > This was the goal.  iptables was atomic.  nftables appears to no longer
> > be so.  If I have this wrong, please show how that works.
> 
> nftables transactions are atomic, either the entire batch takes effect or not
> at all.
> 
> The audit_log_nfcfg() calls got added to the the nft monitor infra which
> is designed to allow userspace to follow the entire content of the
> transaction log.
> 
> So, if its just a 'something was changed' event that is needed all of
> them can be removed. ATM the audit_log_nfcfg() looks like this:
> 
>         /* step 3. Start new generation, rules_gen_X now in use. */
>         net->nft.gencursor = nft_gencursor_next(net);
> 
>         list_for_each_entry_safe(trans, next, &net->nft.commit_list, list) {
>                 switch (trans->msg_type) {
>                 case NFT_MSG_NEWTABLE:
> 			audit_log_nfcfg();
> 			...
> 		case NFT_MSG_...
> 			audit_log_nfcfg();
> 	..
> 	       	}
> 
> which gives an audit_log for every single change in the batch.
> 
> So, if just a summary is needed a single audit_log_nfcfg()
> after 'step 3' and outside of the list_for_each_entry_safe() is all
> that is needed.

Ok, so it should not matter if it is before or after that
list_for_each_entry_safe(), which could be used to collect that summary.

> If a summary is wanted as well one could fe. count the number of
> transaction types in the batch, e.g. table adds, chain adds, rule
> adds etc. and then log a summary count instead.

The current fields are "table", "family", "entries", "op".

Could one batch change more than one table?  (I think it could?)

It appears it can change more than one family.
"family" is currently a single integer, so that might need to be changed
to a list, or something to indicate multi-family.

A batch can certainly change more than one chain, but that was already a
bonus.

"entries" would be the obvious place for the summary count.

Listing all the ops seems a bit onerous.  Is there a hierarchy to the
ops and if so, are they in that order in a batch or in nf_tables_commit()?
It seems I'd need to filter out the NFT_MSG_GET_* ops.


- 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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2021-02-17 23:41         ` Richard Guy Briggs
@ 2021-02-18  8:22           ` Florian Westphal
  2021-02-18 12:42             ` Richard Guy Briggs
  0 siblings, 1 reply; 27+ messages in thread
From: Florian Westphal @ 2021-02-18  8:22 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Florian Westphal, Phil Sutter, LKML, Linux-Audit Mailing List,
	netfilter-devel, twoerner, Eric Paris, tgraf

Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2021-02-11 23:09, Florian Westphal wrote:
> > So, if just a summary is needed a single audit_log_nfcfg()
> > after 'step 3' and outside of the list_for_each_entry_safe() is all
> > that is needed.
> 
> Ok, so it should not matter if it is before or after that
> list_for_each_entry_safe(), which could be used to collect that summary.

Right, it won't matter.

> > If a summary is wanted as well one could fe. count the number of
> > transaction types in the batch, e.g. table adds, chain adds, rule
> > adds etc. and then log a summary count instead.
> 
> The current fields are "table", "family", "entries", "op".
> 
> Could one batch change more than one table?  (I think it could?)

Yes.

> It appears it can change more than one family.
> "family" is currently a single integer, so that might need to be changed
> to a list, or something to indicate multi-family.

Yes, it can also affect different families.

> Listing all the ops seems a bit onerous.  Is there a hierarchy to the
> ops and if so, are they in that order in a batch or in nf_tables_commit()?

No.  There is a hierarchy, e.g. you can't add a chain without first
adding a table, BUT in case the table was already created by an earlier
transaction it can also be stand-alone.

> It seems I'd need to filter out the NFT_MSG_GET_* ops.

No need, the GET ops do not cause changes and will not trigger a
generation id change.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2021-02-18  8:22           ` Florian Westphal
@ 2021-02-18 12:42             ` Richard Guy Briggs
  2021-02-18 12:52               ` Florian Westphal
  0 siblings, 1 reply; 27+ messages in thread
From: Richard Guy Briggs @ 2021-02-18 12:42 UTC (permalink / raw)
  To: Florian Westphal
  Cc: Phil Sutter, LKML, Linux-Audit Mailing List, netfilter-devel,
	twoerner, Eric Paris, tgraf

On 2021-02-18 09:22, Florian Westphal wrote:
> Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2021-02-11 23:09, Florian Westphal wrote:
> > > So, if just a summary is needed a single audit_log_nfcfg()
> > > after 'step 3' and outside of the list_for_each_entry_safe() is all
> > > that is needed.
> > 
> > Ok, so it should not matter if it is before or after that
> > list_for_each_entry_safe(), which could be used to collect that summary.
> 
> Right, it won't matter.
> 
> > > If a summary is wanted as well one could fe. count the number of
> > > transaction types in the batch, e.g. table adds, chain adds, rule
> > > adds etc. and then log a summary count instead.
> > 
> > The current fields are "table", "family", "entries", "op".
> > 
> > Could one batch change more than one table?  (I think it could?)
> 
> Yes.

Ok, listing all tables involved in one commit with deduplication will be
a bit of a nuisance.

> > It appears it can change more than one family.
> > "family" is currently a single integer, so that might need to be changed
> > to a list, or something to indicate multi-family.
> 
> Yes, it can also affect different families.
> 
> > Listing all the ops seems a bit onerous.  Is there a hierarchy to the
> > ops and if so, are they in that order in a batch or in nf_tables_commit()?
> 
> No.  There is a hierarchy, e.g. you can't add a chain without first
> adding a table, BUT in case the table was already created by an earlier
> transaction it can also be stand-alone.

Ok, so there could be a stand-alone chain mod with one table, then a
table add of a different one with a "higher ranking" op...

> > It seems I'd need to filter out the NFT_MSG_GET_* ops.
> 
> No need, the GET ops do not cause changes and will not trigger a
> generation id change.

Ah, so it could trigger on generation change.  Would GET ops be included
in any other change, such that it would be desirable to filter them out
to reduce noise in that single log line if it is attempted to list all
the change ops?  It almost sounds like it would be better to do one
audit log line for each table for each family, and possibly for each op
to avoid the need to change userspace.  This would already be a
significant improvement picking the highest ranking op.

- 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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2021-02-18 12:42             ` Richard Guy Briggs
@ 2021-02-18 12:52               ` Florian Westphal
  2021-02-18 13:28                 ` Richard Guy Briggs
  2021-02-18 21:20                 ` Richard Guy Briggs
  0 siblings, 2 replies; 27+ messages in thread
From: Florian Westphal @ 2021-02-18 12:52 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Florian Westphal, Phil Sutter, LKML, Linux-Audit Mailing List,
	netfilter-devel, twoerner, Eric Paris, tgraf

Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2021-02-18 09:22, Florian Westphal wrote:
> > No.  There is a hierarchy, e.g. you can't add a chain without first
> > adding a table, BUT in case the table was already created by an earlier
> > transaction it can also be stand-alone.
> 
> Ok, so there could be a stand-alone chain mod with one table, then a
> table add of a different one with a "higher ranking" op...

Yes, that can happen.

> > > It seems I'd need to filter out the NFT_MSG_GET_* ops.
> > 
> > No need, the GET ops do not cause changes and will not trigger a
> > generation id change.
> 
> Ah, so it could trigger on generation change.  Would GET ops be included
> in any other change

No, GET ops are standalone, they cannot be part of a transaction.
If you look at

static const struct nfnl_callback nf_tables_cb[NFT_MSG_MAX] = {

array in nf_tables_api.c, then those ops with a '.call_batch' can
appear in transaction (i.e., can cause modification).

The other ones (.call_rcu) are read-only.

If they appear in a batch tehy will be ignored, if the batch consists of
such non-modifying ops only then nf_tables_commit() returns early
because the transaction list is empty (nothing to do/change).

> such that it would be desirable to filter them out
> to reduce noise in that single log line if it is attempted to list all
> the change ops?  It almost sounds like it would be better to do one
> audit log line for each table for each family, and possibly for each op
> to avoid the need to change userspace.  This would already be a
> significant improvement picking the highest ranking op.

I think i understand what you'd like to do.  Yes, that would reduce
the log output a lot.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2021-02-18 12:52               ` Florian Westphal
@ 2021-02-18 13:28                 ` Richard Guy Briggs
  2021-02-18 13:41                   ` Florian Westphal
  2021-02-18 21:20                 ` Richard Guy Briggs
  1 sibling, 1 reply; 27+ messages in thread
From: Richard Guy Briggs @ 2021-02-18 13:28 UTC (permalink / raw)
  To: Florian Westphal
  Cc: Phil Sutter, LKML, Linux-Audit Mailing List, netfilter-devel,
	twoerner, Eric Paris, tgraf

On 2021-02-18 13:52, Florian Westphal wrote:
> Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2021-02-18 09:22, Florian Westphal wrote:
> > > No.  There is a hierarchy, e.g. you can't add a chain without first
> > > adding a table, BUT in case the table was already created by an earlier
> > > transaction it can also be stand-alone.
> > 
> > Ok, so there could be a stand-alone chain mod with one table, then a
> > table add of a different one with a "higher ranking" op...
> 
> Yes, that can happen.

Ok, can I get one more clarification on this "hierarchy"?  Is it roughly
in the order they appear in nf_tables_commit() after step 3?  It appears
it might be mostly already.  If it isn't already, would it be reasonable
to re-order them?  Would you suggest a different order?

(snip GET bits, that's now clear, thank you)

> > such that it would be desirable to filter them out
> > to reduce noise in that single log line if it is attempted to list all
> > the change ops?  It almost sounds like it would be better to do one
> > audit log line for each table for each family, and possibly for each op
> > to avoid the need to change userspace.  This would already be a
> > significant improvement picking the highest ranking op.
> 
> I think i understand what you'd like to do.  Yes, that would reduce
> the log output a lot.

Would the generation change id be useful outside the kernel?  What
exactly does it look like?  I don't quite understand the genmask purpose.

- 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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2021-02-18 13:28                 ` Richard Guy Briggs
@ 2021-02-18 13:41                   ` Florian Westphal
  0 siblings, 0 replies; 27+ messages in thread
From: Florian Westphal @ 2021-02-18 13:41 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Florian Westphal, Phil Sutter, LKML, Linux-Audit Mailing List,
	netfilter-devel, twoerner, Eric Paris, tgraf

Richard Guy Briggs <rgb@redhat.com> wrote:
> Ok, can I get one more clarification on this "hierarchy"?  Is it roughly
> in the order they appear in nf_tables_commit() after step 3?  It appears
> it might be mostly already.  If it isn't already, would it be reasonable
> to re-order them?  Would you suggest a different order?

For audit purposes I think enum nf_tables_msg_types is already in the
most relevant order, the lower numbers being more imporant.

So e.g. NEWTABLE would be more interesting than DELRULE, if both
are in same batch.

> > > such that it would be desirable to filter them out
> > > to reduce noise in that single log line if it is attempted to list all
> > > the change ops?  It almost sounds like it would be better to do one
> > > audit log line for each table for each family, and possibly for each op
> > > to avoid the need to change userspace.  This would already be a
> > > significant improvement picking the highest ranking op.
> > 
> > I think i understand what you'd like to do.  Yes, that would reduce
> > the log output a lot.
> 
> Would the generation change id be useful outside the kernel?

Yes, we already announce it to interested parties via nfnetlink.

> What
> exactly does it look like?

Its just a u64 counter that gets incremented whenever there is a change.

> I don't quite understand the genmask purpose.

Thats an implementation detail only.  When we process a transaction,
changes to the ruleset are being made but they should not have any
effect until the entire transaction is processed.

So there are two 'generations' at any time:
1. The active ruleset
2. The future ruleset

2) is what is being changed/modified.

When the transaction completes, then the future ruleset becomes
the active ruleset.  If the transaction has to be aborted, the
pending changes are reverted and the genid/genmasks are not changed.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2021-02-18 12:52               ` Florian Westphal
  2021-02-18 13:28                 ` Richard Guy Briggs
@ 2021-02-18 21:20                 ` Richard Guy Briggs
  2021-02-18 22:42                   ` Florian Westphal
  1 sibling, 1 reply; 27+ messages in thread
From: Richard Guy Briggs @ 2021-02-18 21:20 UTC (permalink / raw)
  To: Florian Westphal
  Cc: Phil Sutter, LKML, Linux-Audit Mailing List, netfilter-devel,
	twoerner, Eric Paris, tgraf

On 2021-02-18 13:52, Florian Westphal wrote:
> Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2021-02-18 09:22, Florian Westphal wrote:
> > > > It seems I'd need to filter out the NFT_MSG_GET_* ops.
> > > 
> > > No need, the GET ops do not cause changes and will not trigger a
> > > generation id change.
> > 
> > Ah, so it could trigger on generation change.  Would GET ops be included
> > in any other change
> 
> No, GET ops are standalone, they cannot be part of a transaction.
> If you look at
> 
> static const struct nfnl_callback nf_tables_cb[NFT_MSG_MAX] = {
> 
> array in nf_tables_api.c, then those ops with a '.call_batch' can
> appear in transaction (i.e., can cause modification).
> 
> The other ones (.call_rcu) are read-only.
> 
> If they appear in a batch tehy will be ignored, if the batch consists of
> such non-modifying ops only then nf_tables_commit() returns early
> because the transaction list is empty (nothing to do/change).

Ok, one little inconvenient question: what about GETOBJ_RESET?  That
looks like a hybrid that modifies kernel table counters and reports
synchronously.  That could be a special case call in
nf_tables_dump_obj() and nf_tables_getobj().  Will that cause a storm
per commit?

> > such that it would be desirable to filter them out
> > to reduce noise in that single log line if it is attempted to list all
> > the change ops?  It almost sounds like it would be better to do one
> > audit log line for each table for each family, and possibly for each op
> > to avoid the need to change userspace.  This would already be a
> > significant improvement picking the highest ranking op.
> 
> I think i understand what you'd like to do.  Yes, that would reduce
> the log output a lot.

Coded, testing...

- 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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2021-02-18 21:20                 ` Richard Guy Briggs
@ 2021-02-18 22:42                   ` Florian Westphal
  2021-02-19  6:26                     ` Richard Guy Briggs
  0 siblings, 1 reply; 27+ messages in thread
From: Florian Westphal @ 2021-02-18 22:42 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Florian Westphal, Phil Sutter, LKML, Linux-Audit Mailing List,
	netfilter-devel, twoerner, Eric Paris, tgraf

Richard Guy Briggs <rgb@redhat.com> wrote:
> > If they appear in a batch tehy will be ignored, if the batch consists of
> > such non-modifying ops only then nf_tables_commit() returns early
> > because the transaction list is empty (nothing to do/change).
> 
> Ok, one little inconvenient question: what about GETOBJ_RESET?  That
> looks like a hybrid that modifies kernel table counters and reports
> synchronously.  That could be a special case call in
> nf_tables_dump_obj() and nf_tables_getobj().  Will that cause a storm
> per commit?

No, since they can't be part of a commit (they don't implement the
'call_batch' function).

I'm not sure GETOBJ_RESET should be reported in the first place:
RESET only affects expr internal state, and that state changes all the time
anyway in response to network traffic.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2021-02-18 22:42                   ` Florian Westphal
@ 2021-02-19  6:26                     ` Richard Guy Briggs
  2021-02-19 19:25                       ` Richard Guy Briggs
  0 siblings, 1 reply; 27+ messages in thread
From: Richard Guy Briggs @ 2021-02-19  6:26 UTC (permalink / raw)
  To: Florian Westphal
  Cc: Phil Sutter, LKML, Linux-Audit Mailing List, netfilter-devel,
	twoerner, Eric Paris, tgraf

On 2021-02-18 23:42, Florian Westphal wrote:
> Richard Guy Briggs <rgb@redhat.com> wrote:
> > > If they appear in a batch tehy will be ignored, if the batch consists of
> > > such non-modifying ops only then nf_tables_commit() returns early
> > > because the transaction list is empty (nothing to do/change).
> > 
> > Ok, one little inconvenient question: what about GETOBJ_RESET?  That
> > looks like a hybrid that modifies kernel table counters and reports
> > synchronously.  That could be a special case call in
> > nf_tables_dump_obj() and nf_tables_getobj().  Will that cause a storm
> > per commit?
> 
> No, since they can't be part of a commit (they don't implement the
> 'call_batch' function).

Ok, good, so they should be safe (but still needs the gfp param to
audit_log_nfcfg() for atomic alloc in that obj reset callback).

> I'm not sure GETOBJ_RESET should be reported in the first place:
> RESET only affects expr internal state, and that state changes all the time
> anyway in response to network traffic.

We report audit lost messages reset as a config change since it affects
the view that an admin has about a system.  An unaccounted for reset
could mislead an administrator into thinking things are alright when
some messages were lost and there was nothing to show for it.  I could
see similar situations with network entity counters.

- 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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH ghak124 v3] audit: log nftables configuration change events
  2021-02-19  6:26                     ` Richard Guy Briggs
@ 2021-02-19 19:25                       ` Richard Guy Briggs
  0 siblings, 0 replies; 27+ messages in thread
From: Richard Guy Briggs @ 2021-02-19 19:25 UTC (permalink / raw)
  To: Florian Westphal
  Cc: Phil Sutter, LKML, Linux-Audit Mailing List, netfilter-devel,
	twoerner, Eric Paris, tgraf

On 2021-02-19 01:26, Richard Guy Briggs wrote:
> On 2021-02-18 23:42, Florian Westphal wrote:
> > Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > If they appear in a batch tehy will be ignored, if the batch consists of
> > > > such non-modifying ops only then nf_tables_commit() returns early
> > > > because the transaction list is empty (nothing to do/change).
> > > 
> > > Ok, one little inconvenient question: what about GETOBJ_RESET?  That
> > > looks like a hybrid that modifies kernel table counters and reports
> > > synchronously.  That could be a special case call in
> > > nf_tables_dump_obj() and nf_tables_getobj().  Will that cause a storm
> > > per commit?
> > 
> > No, since they can't be part of a commit (they don't implement the
> > 'call_batch' function).
> 
> Ok, good, so they should be safe (but still needs the gfp param to
> audit_log_nfcfg() for atomic alloc in that obj reset callback).

I just noticed that nft_quota_obj_eval() misses logging NFT_MSG_NEWOBJ
in nf_tables_commit(), so that looks like it should be added.

> - RGB

- 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


^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2021-02-19 19:27 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-04 13:20 [PATCH ghak124 v3] audit: log nftables configuration change events Richard Guy Briggs
2020-06-04 17:03 ` Steve Grubb
2020-06-04 17:57   ` Richard Guy Briggs
2020-06-04 18:51     ` Steve Grubb
2020-06-24  0:34 ` Paul Moore
2020-06-24 10:03 ` Pablo Neira Ayuso
2020-06-24 12:34   ` Richard Guy Briggs
2020-06-24 13:03     ` Pablo Neira Ayuso
2020-06-24 13:26       ` Richard Guy Briggs
2021-02-11 15:16 ` Phil Sutter
2021-02-11 16:29   ` Paul Moore
2021-02-11 20:26     ` Richard Guy Briggs
2021-02-11 22:09       ` Florian Westphal
2021-02-17 23:41         ` Richard Guy Briggs
2021-02-18  8:22           ` Florian Westphal
2021-02-18 12:42             ` Richard Guy Briggs
2021-02-18 12:52               ` Florian Westphal
2021-02-18 13:28                 ` Richard Guy Briggs
2021-02-18 13:41                   ` Florian Westphal
2021-02-18 21:20                 ` Richard Guy Briggs
2021-02-18 22:42                   ` Florian Westphal
2021-02-19  6:26                     ` Richard Guy Briggs
2021-02-19 19:25                       ` Richard Guy Briggs
2021-02-12 20:48       ` Richard Guy Briggs
2021-02-11 21:02     ` Steve Grubb
2021-02-12 12:11       ` Phil Sutter
2021-02-12 20:54         ` Richard Guy Briggs

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).