[PATCH ghak124 v1] audit: log nftables configuration change events

[PATCH ghak124 v1] audit: log nftables configuration change events

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

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

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
This is an RFC patch.
Note: I have questions about the "entries" count.  Is there a more
appropriate or relevant item to report here?
Note: It might make sense to differentiate in the op= field that this
was a legacy call vs an nft call.  At the moment, legacy calls overlap
with nft table calls, which are similar calls.

 include/linux/audit.h         |  7 +++++++
 kernel/auditsc.c              | 12 +++++++++---
 net/netfilter/nf_tables_api.c | 14 ++++++++++++++
 3 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 3fcd9ee49734..b10f54103a82 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,12 @@ enum audit_nfcfgop {
 	AUDIT_XT_OP_REGISTER,
 	AUDIT_XT_OP_REPLACE,
 	AUDIT_XT_OP_UNREGISTER,
+	AUDIT_XT_OP_CHAIN_REGISTER	= NFT_MSG_NEWCHAIN,
+	AUDIT_XT_OP_CHAIN_NOOP		= NFT_MSG_GETCHAIN,
+	AUDIT_XT_OP_CHAIN_UNREGISTER	= NFT_MSG_DELCHAIN,
+	AUDIT_XT_OP_RULE_REGISTER	= NFT_MSG_NEWRULE,
+	AUDIT_XT_OP_RULE_NOOP		= NFT_MSG_GETRULE,
+	AUDIT_XT_OP_RULE_UNREGISTER	= NFT_MSG_DELRULE,
 };
 
 extern int is_audit_feature_set(int which);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 468a23390457..eedce8fa4067 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -136,9 +136,15 @@ 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,		"register"		},
+	{ AUDIT_XT_OP_REPLACE,		"replace"		},
+	{ AUDIT_XT_OP_UNREGISTER,	"unregister"		},
+	{ AUDIT_XT_OP_CHAIN_REGISTER,	"register_chain"	},
+	{ AUDIT_XT_OP_CHAIN_NOOP,	"noop_chain"		},
+	{ AUDIT_XT_OP_CHAIN_UNREGISTER,	"unregister_chain"	},
+	{ AUDIT_XT_OP_RULE_REGISTER,	"register_rule"		},
+	{ AUDIT_XT_OP_RULE_NOOP,	"noop_rule"		},
+	{ AUDIT_XT_OP_RULE_UNREGISTER,	"unregister_rule"	},
 };
 
 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 4471393da6d8..c8dc954685f2 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>
@@ -7344,6 +7345,19 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 	list_for_each_entry_safe(trans, next, &net->nft.commit_list, list) {
 		switch (trans->msg_type) {
 		case NFT_MSG_NEWTABLE:
+		case NFT_MSG_DELTABLE:
+		case NFT_MSG_NEWCHAIN:
+		case NFT_MSG_DELCHAIN:
+		case NFT_MSG_NEWRULE:
+		case NFT_MSG_DELRULE:
+			audit_log_nfcfg(trans->ctx.table->name,
+					trans->ctx.family,
+					atomic_read(&trans->ctx.table->chains_ht.ht.nelems),
+					trans->msg_type);
+			break;
+		}
+		switch (trans->msg_type) {
+		case NFT_MSG_NEWTABLE:
 			if (nft_trans_table_update(trans)) {
 				if (!nft_trans_table_enable(trans)) {
 					nf_tables_table_disable(net,
-- 
1.8.3.1

[PATCH ghak124 v1] audit: log nftables configuration change events

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

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

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
This is an RFC patch.
Note: I have questions about the "entries" count.  Is there a more
appropriate or relevant item to report here?
Note: It might make sense to differentiate in the op= field that this
was a legacy call vs an nft call.  At the moment, legacy calls overlap
with nft table calls, which are similar calls.

 include/linux/audit.h         |  7 +++++++
 kernel/auditsc.c              | 12 +++++++++---
 net/netfilter/nf_tables_api.c | 14 ++++++++++++++
 3 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 3fcd9ee49734..b10f54103a82 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,12 @@ enum audit_nfcfgop {
 	AUDIT_XT_OP_REGISTER,
 	AUDIT_XT_OP_REPLACE,
 	AUDIT_XT_OP_UNREGISTER,
+	AUDIT_XT_OP_CHAIN_REGISTER	= NFT_MSG_NEWCHAIN,
+	AUDIT_XT_OP_CHAIN_NOOP		= NFT_MSG_GETCHAIN,
+	AUDIT_XT_OP_CHAIN_UNREGISTER	= NFT_MSG_DELCHAIN,
+	AUDIT_XT_OP_RULE_REGISTER	= NFT_MSG_NEWRULE,
+	AUDIT_XT_OP_RULE_NOOP		= NFT_MSG_GETRULE,
+	AUDIT_XT_OP_RULE_UNREGISTER	= NFT_MSG_DELRULE,
 };
 
 extern int is_audit_feature_set(int which);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 468a23390457..eedce8fa4067 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -136,9 +136,15 @@ 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,		"register"		},
+	{ AUDIT_XT_OP_REPLACE,		"replace"		},
+	{ AUDIT_XT_OP_UNREGISTER,	"unregister"		},
+	{ AUDIT_XT_OP_CHAIN_REGISTER,	"register_chain"	},
+	{ AUDIT_XT_OP_CHAIN_NOOP,	"noop_chain"		},
+	{ AUDIT_XT_OP_CHAIN_UNREGISTER,	"unregister_chain"	},
+	{ AUDIT_XT_OP_RULE_REGISTER,	"register_rule"		},
+	{ AUDIT_XT_OP_RULE_NOOP,	"noop_rule"		},
+	{ AUDIT_XT_OP_RULE_UNREGISTER,	"unregister_rule"	},
 };
 
 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 4471393da6d8..c8dc954685f2 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>
@@ -7344,6 +7345,19 @@ static int nf_tables_commit(struct net *net, struct sk_buff *skb)
 	list_for_each_entry_safe(trans, next, &net->nft.commit_list, list) {
 		switch (trans->msg_type) {
 		case NFT_MSG_NEWTABLE:
+		case NFT_MSG_DELTABLE:
+		case NFT_MSG_NEWCHAIN:
+		case NFT_MSG_DELCHAIN:
+		case NFT_MSG_NEWRULE:
+		case NFT_MSG_DELRULE:
+			audit_log_nfcfg(trans->ctx.table->name,
+					trans->ctx.family,
+					atomic_read(&trans->ctx.table->chains_ht.ht.nelems),
+					trans->msg_type);
+			break;
+		}
+		switch (trans->msg_type) {
+		case NFT_MSG_NEWTABLE:
 			if (nft_trans_table_update(trans)) {
 				if (!nft_trans_table_enable(trans)) {
 					nf_tables_table_disable(net,
-- 
1.8.3.1

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH ghak124 v1] audit: log nftables configuration change events

[Florian Westphal]

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.

I know little about audit records.  Does this allow the user to figure
out that this record is created via nf_tables/netlink rather than xtables?

> For further information please see issue
> https://github.com/linux-audit/audit-kernel/issues/124
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> This is an RFC patch.
> Note: I have questions about the "entries" count.  Is there a more
> appropriate or relevant item to report here?
> Note: It might make sense to differentiate in the op= field that this
> was a legacy call vs an nft call.  At the moment, legacy calls overlap
> with nft table calls, which are similar calls.
> 
>  include/linux/audit.h         |  7 +++++++
>  kernel/auditsc.c              | 12 +++++++++---
>  net/netfilter/nf_tables_api.c | 14 ++++++++++++++
>  3 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 3fcd9ee49734..b10f54103a82 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,12 @@ enum audit_nfcfgop {
>  	AUDIT_XT_OP_REGISTER,
>  	AUDIT_XT_OP_REPLACE,
>  	AUDIT_XT_OP_UNREGISTER,
> +	AUDIT_XT_OP_CHAIN_REGISTER	= NFT_MSG_NEWCHAIN,

Hmm, this means AUDIT_XT_OP_CHAIN_REGISTER overlaps with the 4th
audit_nfcfgop value...?

> +	AUDIT_XT_OP_CHAIN_NOOP		= NFT_MSG_GETCHAIN,

GETCHAIN can't appear in the commit path (its not changing anything).
Same for all other NFT_MSG_FOO that use ".call_rcu" action.

Futhermore, I wonder what is to be logged by audit.

The fact that there was 'some change'? If so, its enough to log
the increment of the generation count during the commit phase.

(After that, kernel can't back down anymore, i.e. all errors are
 caught/handled beforehand).

If its 'any config change', then you also need to handle adds
or delete from sets/maps, since that may allow something that wasn't
allowed before, e.g. consider

ip saddr @trused accept

and then, later on,
nft add element ip filter @trusted { 10.0.0.0/8, 192.168.0.1 }

This would not add a table, or chain, or set, but it does implicitly
alter the ruleset.

> +		case NFT_MSG_DELRULE:
> +			audit_log_nfcfg(trans->ctx.table->name,
> +					trans->ctx.family,
> +					atomic_read(&trans->ctx.table->chains_ht.ht.nelems),
> +					trans->msg_type);
> +			break;

Is that record format expected to emit the current number of chains?
I'm not sure if that info is meaningful.

Since table names can be anything in nf_tables (they have no special
properties anymore), the table name is interesting from a informational
pov, but not super interesting.

This will also emit the same message/record multiple times, with only
difference being the msg_type.  I'm not sure thats interesting.

Consider a batch update that commits 100 new rules in chain x,
this would result in 100 audit_log_nfcfg() calls, each with the
same information.

There are test cases in nftables.git, you could run them to see what
kind of audit events are generated and how redundant they might be.

Re: [PATCH ghak124 v1] audit: log nftables configuration change events

[Florian Westphal]

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.

I know little about audit records.  Does this allow the user to figure
out that this record is created via nf_tables/netlink rather than xtables?

> For further information please see issue
> https://github.com/linux-audit/audit-kernel/issues/124
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> This is an RFC patch.
> Note: I have questions about the "entries" count.  Is there a more
> appropriate or relevant item to report here?
> Note: It might make sense to differentiate in the op= field that this
> was a legacy call vs an nft call.  At the moment, legacy calls overlap
> with nft table calls, which are similar calls.
> 
>  include/linux/audit.h         |  7 +++++++
>  kernel/auditsc.c              | 12 +++++++++---
>  net/netfilter/nf_tables_api.c | 14 ++++++++++++++
>  3 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 3fcd9ee49734..b10f54103a82 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,12 @@ enum audit_nfcfgop {
>  	AUDIT_XT_OP_REGISTER,
>  	AUDIT_XT_OP_REPLACE,
>  	AUDIT_XT_OP_UNREGISTER,
> +	AUDIT_XT_OP_CHAIN_REGISTER	= NFT_MSG_NEWCHAIN,

Hmm, this means AUDIT_XT_OP_CHAIN_REGISTER overlaps with the 4th
audit_nfcfgop value...?

> +	AUDIT_XT_OP_CHAIN_NOOP		= NFT_MSG_GETCHAIN,

GETCHAIN can't appear in the commit path (its not changing anything).
Same for all other NFT_MSG_FOO that use ".call_rcu" action.

Futhermore, I wonder what is to be logged by audit.

The fact that there was 'some change'? If so, its enough to log
the increment of the generation count during the commit phase.

(After that, kernel can't back down anymore, i.e. all errors are
 caught/handled beforehand).

If its 'any config change', then you also need to handle adds
or delete from sets/maps, since that may allow something that wasn't
allowed before, e.g. consider

ip saddr @trused accept

and then, later on,
nft add element ip filter @trusted { 10.0.0.0/8, 192.168.0.1 }

This would not add a table, or chain, or set, but it does implicitly
alter the ruleset.

> +		case NFT_MSG_DELRULE:
> +			audit_log_nfcfg(trans->ctx.table->name,
> +					trans->ctx.family,
> +					atomic_read(&trans->ctx.table->chains_ht.ht.nelems),
> +					trans->msg_type);
> +			break;

Is that record format expected to emit the current number of chains?
I'm not sure if that info is meaningful.

Since table names can be anything in nf_tables (they have no special
properties anymore), the table name is interesting from a informational
pov, but not super interesting.

This will also emit the same message/record multiple times, with only
difference being the msg_type.  I'm not sure thats interesting.

Consider a batch update that commits 100 new rules in chain x,
this would result in 100 audit_log_nfcfg() calls, each with the
same information.

There are test cases in nftables.git, you could run them to see what
kind of audit events are generated and how redundant they might be.

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH ghak124 v1] audit: log nftables configuration change events

[Richard Guy Briggs]

On 2020-05-27 16:53, Florian Westphal wrote:
> 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.
> 
> I know little about audit records.  Does this allow the user to figure
> out that this record is created via nf_tables/netlink rather than xtables?

No, which is why I added that note below.  It shouldn't be hard to
change but I took the easy way to program it and now that I reflect on
it more, it sounds like it should be a basic requriement.

> > For further information please see issue
> > https://github.com/linux-audit/audit-kernel/issues/124
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > This is an RFC patch.
> > Note: I have questions about the "entries" count.  Is there a more
> > appropriate or relevant item to report here?
> > Note: It might make sense to differentiate in the op= field that this
> > was a legacy call vs an nft call.  At the moment, legacy calls overlap
> > with nft table calls, which are similar calls.
> > 
> >  include/linux/audit.h         |  7 +++++++
> >  kernel/auditsc.c              | 12 +++++++++---
> >  net/netfilter/nf_tables_api.c | 14 ++++++++++++++
> >  3 files changed, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 3fcd9ee49734..b10f54103a82 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,12 @@ enum audit_nfcfgop {
> >  	AUDIT_XT_OP_REGISTER,
> >  	AUDIT_XT_OP_REPLACE,
> >  	AUDIT_XT_OP_UNREGISTER,
> > +	AUDIT_XT_OP_CHAIN_REGISTER	= NFT_MSG_NEWCHAIN,
> 
> Hmm, this means AUDIT_XT_OP_CHAIN_REGISTER overlaps with the 4th
> audit_nfcfgop value...?

There was no 4th value, so the overlap is just the first three, which
are all table operations that more or less line up.

> > +	AUDIT_XT_OP_CHAIN_NOOP		= NFT_MSG_GETCHAIN,
> 
> GETCHAIN can't appear in the commit path (its not changing anything).
> Same for all other NFT_MSG_FOO that use ".call_rcu" action.

Again, I was a bit lazy in selecting the actions, and the GET actions
are of no interest since they don't change the configuration.

> Futhermore, I wonder what is to be logged by audit.

NEW and DEL of TABLEs, CHAINs and RULEs.

> The fact that there was 'some change'? If so, its enough to log
> the increment of the generation count during the commit phase.

Well, we are only logging "some change", so is it necessary to log the
generation count to show that?  Is the generation count of specific
interest?

> (After that, kernel can't back down anymore, i.e. all errors are
>  caught/handled beforehand).

I did think of recording all failed attempts too, but coding that would
be more effort.  It is worth doing if it is deemed important,
particularly for permission issues (as opposed to resource limits or
packet format errors.  This would be more of interest to a security
officer rather than a network technician, but the latter may find it
useful for debugging.

> If its 'any config change', then you also need to handle adds
> or delete from sets/maps, since that may allow something that wasn't
> allowed before, e.g. consider
> 
> ip saddr @trused accept
> 
> and then, later on,
> nft add element ip filter @trusted { 10.0.0.0/8, 192.168.0.1 }
> 
> This would not add a table, or chain, or set, but it does implicitly
> alter the ruleset.

Ah, ok, so yes, we would need that too.  I see family and table in
there, op is evident.  Is there a useful value we can use in the
"entries" field?

> > +		case NFT_MSG_DELRULE:
> > +			audit_log_nfcfg(trans->ctx.table->name,
> > +					trans->ctx.family,
> > +					atomic_read(&trans->ctx.table->chains_ht.ht.nelems),
> > +					trans->msg_type);
> > +			break;
> 
> Is that record format expected to emit the current number of chains?

I was aiming for a relevant value such as perhaps the new rule number or
the rule number being deleted.

> I'm not sure if that info is meaningful.

Can you suggest something meaningful?  This field may need to get
different information for each operation.

> Since table names can be anything in nf_tables (they have no special
> properties anymore), the table name is interesting from a informational
> pov, but not super interesting.

I don't think we need to be able to completely reconstruct the
tables/chains/rules from the information in the audit log, but be aware
of who is changing what when.

> This will also emit the same message/record multiple times, with only
> difference being the msg_type.  I'm not sure thats interesting.

I do think it is interesting.

> Consider a batch update that commits 100 new rules in chain x,
> this would result in 100 audit_log_nfcfg() calls, each with the
> same information.

So rule number would be a useful differentiator here.

> There are test cases in nftables.git, you could run them to see what
> kind of audit events are generated and how redundant they might be.

As a first pass, simply booting a test system and running the
audit-testsuite has provided some useful fodder.

Florian, thanks for your review and input.

- RGB

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

Re: [PATCH ghak124 v1] audit: log nftables configuration change events

[Richard Guy Briggs]

On 2020-05-27 16:53, Florian Westphal wrote:
> 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.
> 
> I know little about audit records.  Does this allow the user to figure
> out that this record is created via nf_tables/netlink rather than xtables?

No, which is why I added that note below.  It shouldn't be hard to
change but I took the easy way to program it and now that I reflect on
it more, it sounds like it should be a basic requriement.

> > For further information please see issue
> > https://github.com/linux-audit/audit-kernel/issues/124
> > 
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > This is an RFC patch.
> > Note: I have questions about the "entries" count.  Is there a more
> > appropriate or relevant item to report here?
> > Note: It might make sense to differentiate in the op= field that this
> > was a legacy call vs an nft call.  At the moment, legacy calls overlap
> > with nft table calls, which are similar calls.
> > 
> >  include/linux/audit.h         |  7 +++++++
> >  kernel/auditsc.c              | 12 +++++++++---
> >  net/netfilter/nf_tables_api.c | 14 ++++++++++++++
> >  3 files changed, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 3fcd9ee49734..b10f54103a82 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,12 @@ enum audit_nfcfgop {
> >  	AUDIT_XT_OP_REGISTER,
> >  	AUDIT_XT_OP_REPLACE,
> >  	AUDIT_XT_OP_UNREGISTER,
> > +	AUDIT_XT_OP_CHAIN_REGISTER	= NFT_MSG_NEWCHAIN,
> 
> Hmm, this means AUDIT_XT_OP_CHAIN_REGISTER overlaps with the 4th
> audit_nfcfgop value...?

There was no 4th value, so the overlap is just the first three, which
are all table operations that more or less line up.

> > +	AUDIT_XT_OP_CHAIN_NOOP		= NFT_MSG_GETCHAIN,
> 
> GETCHAIN can't appear in the commit path (its not changing anything).
> Same for all other NFT_MSG_FOO that use ".call_rcu" action.

Again, I was a bit lazy in selecting the actions, and the GET actions
are of no interest since they don't change the configuration.

> Futhermore, I wonder what is to be logged by audit.

NEW and DEL of TABLEs, CHAINs and RULEs.

> The fact that there was 'some change'? If so, its enough to log
> the increment of the generation count during the commit phase.

Well, we are only logging "some change", so is it necessary to log the
generation count to show that?  Is the generation count of specific
interest?

> (After that, kernel can't back down anymore, i.e. all errors are
>  caught/handled beforehand).

I did think of recording all failed attempts too, but coding that would
be more effort.  It is worth doing if it is deemed important,
particularly for permission issues (as opposed to resource limits or
packet format errors.  This would be more of interest to a security
officer rather than a network technician, but the latter may find it
useful for debugging.

> If its 'any config change', then you also need to handle adds
> or delete from sets/maps, since that may allow something that wasn't
> allowed before, e.g. consider
> 
> ip saddr @trused accept
> 
> and then, later on,
> nft add element ip filter @trusted { 10.0.0.0/8, 192.168.0.1 }
> 
> This would not add a table, or chain, or set, but it does implicitly
> alter the ruleset.

Ah, ok, so yes, we would need that too.  I see family and table in
there, op is evident.  Is there a useful value we can use in the
"entries" field?

> > +		case NFT_MSG_DELRULE:
> > +			audit_log_nfcfg(trans->ctx.table->name,
> > +					trans->ctx.family,
> > +					atomic_read(&trans->ctx.table->chains_ht.ht.nelems),
> > +					trans->msg_type);
> > +			break;
> 
> Is that record format expected to emit the current number of chains?

I was aiming for a relevant value such as perhaps the new rule number or
the rule number being deleted.

> I'm not sure if that info is meaningful.

Can you suggest something meaningful?  This field may need to get
different information for each operation.

> Since table names can be anything in nf_tables (they have no special
> properties anymore), the table name is interesting from a informational
> pov, but not super interesting.

I don't think we need to be able to completely reconstruct the
tables/chains/rules from the information in the audit log, but be aware
of who is changing what when.

> This will also emit the same message/record multiple times, with only
> difference being the msg_type.  I'm not sure thats interesting.

I do think it is interesting.

> Consider a batch update that commits 100 new rules in chain x,
> this would result in 100 audit_log_nfcfg() calls, each with the
> same information.

So rule number would be a useful differentiator here.

> There are test cases in nftables.git, you could run them to see what
> kind of audit events are generated and how redundant they might be.

As a first pass, simply booting a test system and running the
audit-testsuite has provided some useful fodder.

Florian, thanks for your review and input.

- 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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH ghak124 v1] audit: log nftables configuration change events

[Florian Westphal]

Richard Guy Briggs <rgb@redhat.com> wrote:
> Well, we are only logging "some change", so is it necessary to log the
> generation count to show that?  Is the generation count of specific
> interest?

No, its of no specific interest.  I just worded this poorly.
If the generation id increments, then something has been changed by the
batch, thats all.

> > (After that, kernel can't back down anymore, i.e. all errors are
> >  caught/handled beforehand).
> 
> I did think of recording all failed attempts too, but coding that would
> be more effort.  It is worth doing if it is deemed important,
> particularly for permission issues (as opposed to resource limits or
> packet format errors.  This would be more of interest to a security
> officer rather than a network technician, but the latter may find it
> useful for debugging.

The permission check is done early, in nfnetlink_rcv() (search for
EPERM), you would need to add an audit call there if thats relevant
for audit purposes.

> > If its 'any config change', then you also need to handle adds
> > or delete from sets/maps, since that may allow something that wasn't
> > allowed before, e.g. consider
> > 
> > ip saddr @trused accept
> > 
> > and then, later on,
> > nft add element ip filter @trusted { 10.0.0.0/8, 192.168.0.1 }
> > 
> > This would not add a table, or chain, or set, but it does implicitly
> > alter the ruleset.
> 
> Ah, ok, so yes, we would need that too.  I see family and table in
> there, op is evident.  Is there a useful value we can use in the
> "entries" field?

Maybe the handle of the set that the element was added to.
Each set, rule, chain, ... has a kernel-assigned number that
serves as a unique identifier.

> > Is that record format expected to emit the current number of chains?
> 
> I was aiming for a relevant value such as perhaps the new rule number or
> the rule number being deleted.

In that case, use the handle, which is a u64 with a unique value (for a
given table).

> > Since table names can be anything in nf_tables (they have no special
> > properties anymore), the table name is interesting from a informational
> > pov, but not super interesting.
> 
> I don't think we need to be able to completely reconstruct the
> tables/chains/rules from the information in the audit log, but be aware
> of who is changing what when.

Ok.  Have a look at nf_tables_fill_gen_info() in that case, you probably
want to emit at least the pid and task info, unless audit doesn't add
that already anyway.

> > Consider a batch update that commits 100 new rules in chain x,
> > this would result in 100 audit_log_nfcfg() calls, each with the
> > same information.
> 
> So rule number would be a useful differentiator here.

Ok.  Yes, that is available (rule->handle).

Re: [PATCH ghak124 v1] audit: log nftables configuration change events

[Florian Westphal]

Richard Guy Briggs <rgb@redhat.com> wrote:
> Well, we are only logging "some change", so is it necessary to log the
> generation count to show that?  Is the generation count of specific
> interest?

No, its of no specific interest.  I just worded this poorly.
If the generation id increments, then something has been changed by the
batch, thats all.

> > (After that, kernel can't back down anymore, i.e. all errors are
> >  caught/handled beforehand).
> 
> I did think of recording all failed attempts too, but coding that would
> be more effort.  It is worth doing if it is deemed important,
> particularly for permission issues (as opposed to resource limits or
> packet format errors.  This would be more of interest to a security
> officer rather than a network technician, but the latter may find it
> useful for debugging.

The permission check is done early, in nfnetlink_rcv() (search for
EPERM), you would need to add an audit call there if thats relevant
for audit purposes.

> > If its 'any config change', then you also need to handle adds
> > or delete from sets/maps, since that may allow something that wasn't
> > allowed before, e.g. consider
> > 
> > ip saddr @trused accept
> > 
> > and then, later on,
> > nft add element ip filter @trusted { 10.0.0.0/8, 192.168.0.1 }
> > 
> > This would not add a table, or chain, or set, but it does implicitly
> > alter the ruleset.
> 
> Ah, ok, so yes, we would need that too.  I see family and table in
> there, op is evident.  Is there a useful value we can use in the
> "entries" field?

Maybe the handle of the set that the element was added to.
Each set, rule, chain, ... has a kernel-assigned number that
serves as a unique identifier.

> > Is that record format expected to emit the current number of chains?
> 
> I was aiming for a relevant value such as perhaps the new rule number or
> the rule number being deleted.

In that case, use the handle, which is a u64 with a unique value (for a
given table).

> > Since table names can be anything in nf_tables (they have no special
> > properties anymore), the table name is interesting from a informational
> > pov, but not super interesting.
> 
> I don't think we need to be able to completely reconstruct the
> tables/chains/rules from the information in the audit log, but be aware
> of who is changing what when.

Ok.  Have a look at nf_tables_fill_gen_info() in that case, you probably
want to emit at least the pid and task info, unless audit doesn't add
that already anyway.

> > Consider a batch update that commits 100 new rules in chain x,
> > this would result in 100 audit_log_nfcfg() calls, each with the
> > same information.
> 
> So rule number would be a useful differentiator here.

Ok.  Yes, that is available (rule->handle).

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit