| Message ID | 20230923015351.15707-3-phil@nwl.cc (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Review nf_tables audit logging | expand |
Hi Phil, On Sat, Sep 23, 2023 at 03:53:50AM +0200, Phil Sutter wrote: > When adding/updating an object, the transaction handler emits suitable > audit log entries already, the one in nft_obj_notify() is redundant. To > fix that (and retain the audit logging from objects' 'update' callback), > Introduce an "audit log free" variant for internal use. > > Fixes: c520292f29b80 ("audit: log nftables configuration change events once per table") > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > net/netfilter/nf_tables_api.c | 44 ++++++++++++------- > .../testing/selftests/netfilter/nft_audit.sh | 20 +++++++++ > 2 files changed, 48 insertions(+), 16 deletions(-) > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > index 0e5d9bdba82b8..48d50df950a18 100644 > --- a/net/netfilter/nf_tables_api.c > +++ b/net/netfilter/nf_tables_api.c > @@ -8046,24 +8046,14 @@ static int nf_tables_delobj(struct sk_buff *skb, const struct nfnl_info *info, > return nft_delobj(&ctx, obj); > } > > -void nft_obj_notify(struct net *net, const struct nft_table *table, > - struct nft_object *obj, u32 portid, u32 seq, int event, > - u16 flags, int family, int report, gfp_t gfp) > +static void > +__nft_obj_notify(struct net *net, const struct nft_table *table, > + struct nft_object *obj, u32 portid, u32 seq, int event, > + u16 flags, int family, int report, gfp_t gfp) > { > struct nftables_pernet *nft_net = nft_pernet(net); > struct sk_buff *skb; > int err; > - char *buf = kasprintf(gfp, "%s:%u", > - table->name, nft_net->base_seq); > - > - audit_log_nfcfg(buf, > - family, > - obj->handle, > - event == NFT_MSG_NEWOBJ ? > - AUDIT_NFT_OP_OBJ_REGISTER : > - AUDIT_NFT_OP_OBJ_UNREGISTER, > - gfp); > - kfree(buf); > > if (!report && > !nfnetlink_has_listeners(net, NFNLGRP_NFTABLES)) > @@ -8086,13 +8076,35 @@ void nft_obj_notify(struct net *net, const struct nft_table *table, > err: > nfnetlink_set_err(net, portid, NFNLGRP_NFTABLES, -ENOBUFS); > } > + > +void nft_obj_notify(struct net *net, const struct nft_table *table, > + struct nft_object *obj, u32 portid, u32 seq, int event, > + u16 flags, int family, int report, gfp_t gfp) > +{ > + struct nftables_pernet *nft_net = nft_pernet(net); > + char *buf = kasprintf(gfp, "%s:%u", > + table->name, nft_net->base_seq); > + > + audit_log_nfcfg(buf, > + family, > + obj->handle, > + event == NFT_MSG_NEWOBJ ? > + AUDIT_NFT_OP_OBJ_REGISTER : > + AUDIT_NFT_OP_OBJ_UNREGISTER, > + gfp); > + kfree(buf); > + > + __nft_obj_notify(net, table, obj, portid, seq, event, > + flags, family, report, gfp); > +} > EXPORT_SYMBOL_GPL(nft_obj_notify); OK, so nft_obj_notify() is called from nft_quota to notify that the quota is depleted and the audit log is still there in this case. > static void nf_tables_obj_notify(const struct nft_ctx *ctx, > struct nft_object *obj, int event) > { > - nft_obj_notify(ctx->net, ctx->table, obj, ctx->portid, ctx->seq, event, > - ctx->flags, ctx->family, ctx->report, GFP_KERNEL); > + __nft_obj_notify(ctx->net, ctx->table, obj, ctx->portid, > + ctx->seq, event, ctx->flags, ctx->family, > + ctx->report, GFP_KERNEL); > } This function is called from the commit path to send the event notification, and it should send the audit log? Is this nf_tables_commit_audit_log() that provides the redundant log, right? > /* > diff --git a/tools/testing/selftests/netfilter/nft_audit.sh b/tools/testing/selftests/netfilter/nft_audit.sh > index 0b3255e7b3538..bb34329e02a7f 100755 > --- a/tools/testing/selftests/netfilter/nft_audit.sh > +++ b/tools/testing/selftests/netfilter/nft_audit.sh > @@ -85,6 +85,26 @@ do_test "nft add set t1 s2 $setblock; add set t1 s3 { $settype; }" \ > do_test "nft add element t1 s3 $setelem" \ > "table=t1 family=2 entries=3 op=nft_register_setelem" > > +# adding counters > + > +do_test 'nft add counter t1 c1' \ > +'table=t1 family=2 entries=1 op=nft_register_obj' > + > +do_test 'nft add counter t2 c1; add counter t2 c2' \ > +'table=t2 family=2 entries=2 op=nft_register_obj' > + > +# adding/updating quotas > + > +do_test 'nft add quota t1 q1 { 10 bytes }' \ > +'table=t1 family=2 entries=1 op=nft_register_obj' > + > +do_test 'nft add quota t2 q1 { 10 bytes }; add quota t2 q2 { 10 bytes }' \ > +'table=t2 family=2 entries=2 op=nft_register_obj' > + > +# changing the quota value triggers obj update path > +do_test 'nft add quota t1 q1 { 20 bytes }' \ > +'table=t1 family=2 entries=1 op=nft_register_obj' > + > # resetting rules > > do_test 'nft reset rules t1 c2' \ > -- > 2.41.0 >
On 2023-09-23 03:53, Phil Sutter wrote: > When adding/updating an object, the transaction handler emits suitable > audit log entries already, the one in nft_obj_notify() is redundant. To > fix that (and retain the audit logging from objects' 'update' callback), > Introduce an "audit log free" variant for internal use. > > Fixes: c520292f29b80 ("audit: log nftables configuration change events once per table") > Signed-off-by: Phil Sutter <phil@nwl.cc> Reviewed-by: Richard Guy Briggs <rgb@redhat.com> > --- > net/netfilter/nf_tables_api.c | 44 ++++++++++++------- > .../testing/selftests/netfilter/nft_audit.sh | 20 +++++++++ > 2 files changed, 48 insertions(+), 16 deletions(-) > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > index 0e5d9bdba82b8..48d50df950a18 100644 > --- a/net/netfilter/nf_tables_api.c > +++ b/net/netfilter/nf_tables_api.c > @@ -8046,24 +8046,14 @@ static int nf_tables_delobj(struct sk_buff *skb, const struct nfnl_info *info, > return nft_delobj(&ctx, obj); > } > > -void nft_obj_notify(struct net *net, const struct nft_table *table, > - struct nft_object *obj, u32 portid, u32 seq, int event, > - u16 flags, int family, int report, gfp_t gfp) > +static void > +__nft_obj_notify(struct net *net, const struct nft_table *table, > + struct nft_object *obj, u32 portid, u32 seq, int event, > + u16 flags, int family, int report, gfp_t gfp) > { > struct nftables_pernet *nft_net = nft_pernet(net); > struct sk_buff *skb; > int err; > - char *buf = kasprintf(gfp, "%s:%u", > - table->name, nft_net->base_seq); > - > - audit_log_nfcfg(buf, > - family, > - obj->handle, > - event == NFT_MSG_NEWOBJ ? > - AUDIT_NFT_OP_OBJ_REGISTER : > - AUDIT_NFT_OP_OBJ_UNREGISTER, > - gfp); > - kfree(buf); > > if (!report && > !nfnetlink_has_listeners(net, NFNLGRP_NFTABLES)) > @@ -8086,13 +8076,35 @@ void nft_obj_notify(struct net *net, const struct nft_table *table, > err: > nfnetlink_set_err(net, portid, NFNLGRP_NFTABLES, -ENOBUFS); > } > + > +void nft_obj_notify(struct net *net, const struct nft_table *table, > + struct nft_object *obj, u32 portid, u32 seq, int event, > + u16 flags, int family, int report, gfp_t gfp) > +{ > + struct nftables_pernet *nft_net = nft_pernet(net); > + char *buf = kasprintf(gfp, "%s:%u", > + table->name, nft_net->base_seq); > + > + audit_log_nfcfg(buf, > + family, > + obj->handle, > + event == NFT_MSG_NEWOBJ ? > + AUDIT_NFT_OP_OBJ_REGISTER : > + AUDIT_NFT_OP_OBJ_UNREGISTER, > + gfp); > + kfree(buf); > + > + __nft_obj_notify(net, table, obj, portid, seq, event, > + flags, family, report, gfp); > +} > EXPORT_SYMBOL_GPL(nft_obj_notify); > > static void nf_tables_obj_notify(const struct nft_ctx *ctx, > struct nft_object *obj, int event) > { > - nft_obj_notify(ctx->net, ctx->table, obj, ctx->portid, ctx->seq, event, > - ctx->flags, ctx->family, ctx->report, GFP_KERNEL); > + __nft_obj_notify(ctx->net, ctx->table, obj, ctx->portid, > + ctx->seq, event, ctx->flags, ctx->family, > + ctx->report, GFP_KERNEL); > } > > /* > diff --git a/tools/testing/selftests/netfilter/nft_audit.sh b/tools/testing/selftests/netfilter/nft_audit.sh > index 0b3255e7b3538..bb34329e02a7f 100755 > --- a/tools/testing/selftests/netfilter/nft_audit.sh > +++ b/tools/testing/selftests/netfilter/nft_audit.sh > @@ -85,6 +85,26 @@ do_test "nft add set t1 s2 $setblock; add set t1 s3 { $settype; }" \ > do_test "nft add element t1 s3 $setelem" \ > "table=t1 family=2 entries=3 op=nft_register_setelem" > > +# adding counters > + > +do_test 'nft add counter t1 c1' \ > +'table=t1 family=2 entries=1 op=nft_register_obj' > + > +do_test 'nft add counter t2 c1; add counter t2 c2' \ > +'table=t2 family=2 entries=2 op=nft_register_obj' > + > +# adding/updating quotas > + > +do_test 'nft add quota t1 q1 { 10 bytes }' \ > +'table=t1 family=2 entries=1 op=nft_register_obj' > + > +do_test 'nft add quota t2 q1 { 10 bytes }; add quota t2 q2 { 10 bytes }' \ > +'table=t2 family=2 entries=2 op=nft_register_obj' > + > +# changing the quota value triggers obj update path > +do_test 'nft add quota t1 q1 { 20 bytes }' \ > +'table=t1 family=2 entries=1 op=nft_register_obj' > + > # resetting rules > > do_test 'nft reset rules t1 c2' \ > -- > 2.41.0 > - RGB -- Richard Guy Briggs <rgb@redhat.com> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada Upstream IRC: SunRaycer Voice: +1.613.860 2354 SMS: +1.613.518.6570
Hi Pablo, On Wed, Sep 27, 2023 at 09:41:53PM +0200, Pablo Neira Ayuso wrote: > On Sat, Sep 23, 2023 at 03:53:50AM +0200, Phil Sutter wrote: > > When adding/updating an object, the transaction handler emits suitable > > audit log entries already, the one in nft_obj_notify() is redundant. To > > fix that (and retain the audit logging from objects' 'update' callback), > > Introduce an "audit log free" variant for internal use. > > > > Fixes: c520292f29b80 ("audit: log nftables configuration change events once per table") > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > --- > > net/netfilter/nf_tables_api.c | 44 ++++++++++++------- > > .../testing/selftests/netfilter/nft_audit.sh | 20 +++++++++ > > 2 files changed, 48 insertions(+), 16 deletions(-) > > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > > index 0e5d9bdba82b8..48d50df950a18 100644 > > --- a/net/netfilter/nf_tables_api.c > > +++ b/net/netfilter/nf_tables_api.c > > @@ -8046,24 +8046,14 @@ static int nf_tables_delobj(struct sk_buff *skb, const struct nfnl_info *info, > > return nft_delobj(&ctx, obj); > > } > > > > -void nft_obj_notify(struct net *net, const struct nft_table *table, > > - struct nft_object *obj, u32 portid, u32 seq, int event, > > - u16 flags, int family, int report, gfp_t gfp) > > +static void > > +__nft_obj_notify(struct net *net, const struct nft_table *table, > > + struct nft_object *obj, u32 portid, u32 seq, int event, > > + u16 flags, int family, int report, gfp_t gfp) > > { > > struct nftables_pernet *nft_net = nft_pernet(net); > > struct sk_buff *skb; > > int err; > > - char *buf = kasprintf(gfp, "%s:%u", > > - table->name, nft_net->base_seq); > > - > > - audit_log_nfcfg(buf, > > - family, > > - obj->handle, > > - event == NFT_MSG_NEWOBJ ? > > - AUDIT_NFT_OP_OBJ_REGISTER : > > - AUDIT_NFT_OP_OBJ_UNREGISTER, > > - gfp); > > - kfree(buf); > > > > if (!report && > > !nfnetlink_has_listeners(net, NFNLGRP_NFTABLES)) > > @@ -8086,13 +8076,35 @@ void nft_obj_notify(struct net *net, const struct nft_table *table, > > err: > > nfnetlink_set_err(net, portid, NFNLGRP_NFTABLES, -ENOBUFS); > > } > > + > > +void nft_obj_notify(struct net *net, const struct nft_table *table, > > + struct nft_object *obj, u32 portid, u32 seq, int event, > > + u16 flags, int family, int report, gfp_t gfp) > > +{ > > + struct nftables_pernet *nft_net = nft_pernet(net); > > + char *buf = kasprintf(gfp, "%s:%u", > > + table->name, nft_net->base_seq); > > + > > + audit_log_nfcfg(buf, > > + family, > > + obj->handle, > > + event == NFT_MSG_NEWOBJ ? > > + AUDIT_NFT_OP_OBJ_REGISTER : > > + AUDIT_NFT_OP_OBJ_UNREGISTER, > > + gfp); > > + kfree(buf); > > + > > + __nft_obj_notify(net, table, obj, portid, seq, event, > > + flags, family, report, gfp); > > +} > > EXPORT_SYMBOL_GPL(nft_obj_notify); > > OK, so nft_obj_notify() is called from nft_quota to notify that the > quota is depleted and the audit log is still there in this case. Exactly. I needed an internal __nft_obj_notify() which just serves notify_list but not audit. > > static void nf_tables_obj_notify(const struct nft_ctx *ctx, > > struct nft_object *obj, int event) > > { > > - nft_obj_notify(ctx->net, ctx->table, obj, ctx->portid, ctx->seq, event, > > - ctx->flags, ctx->family, ctx->report, GFP_KERNEL); > > + __nft_obj_notify(ctx->net, ctx->table, obj, ctx->portid, > > + ctx->seq, event, ctx->flags, ctx->family, > > + ctx->report, GFP_KERNEL); > > } > > This function is called from the commit path to send the event > notification, and it should send the audit log? > > Is this nf_tables_commit_audit_log() that provides the redundant log, > right? It's easier to to make nf_tables_obj_notify() skip the audit log and not introduce special casing in nf_tables_commit_audit_log() or call nf_tables_commit_audit_collect() for all transaction entries but NEWOBJ and DELOBJ. Cheers, Phil
On Thu, Sep 28, 2023 at 04:48:11PM +0200, Phil Sutter wrote: > On Wed, Sep 27, 2023 at 09:41:53PM +0200, Pablo Neira Ayuso wrote: > > OK, so nft_obj_notify() is called from nft_quota to notify that the > > quota is depleted and the audit log is still there in this case. > > Exactly. I needed an internal __nft_obj_notify() which just serves > notify_list but not audit. Patch LGTM, thanks for explaining!
On Fri, Sep 22, 2023 at 9:53 PM Phil Sutter <phil@nwl.cc> wrote: > > When adding/updating an object, the transaction handler emits suitable > audit log entries already, the one in nft_obj_notify() is redundant. To > fix that (and retain the audit logging from objects' 'update' callback), > Introduce an "audit log free" variant for internal use. > > Fixes: c520292f29b80 ("audit: log nftables configuration change events once per table") > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > net/netfilter/nf_tables_api.c | 44 ++++++++++++------- > .../testing/selftests/netfilter/nft_audit.sh | 20 +++++++++ > 2 files changed, 48 insertions(+), 16 deletions(-) Thanks for working on this Phil, it looks good to me from an audit perspective. Acked-by: Paul Moore <paul@paul-moore.com> (Audit)
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index 0e5d9bdba82b8..48d50df950a18 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -8046,24 +8046,14 @@ static int nf_tables_delobj(struct sk_buff *skb, const struct nfnl_info *info, return nft_delobj(&ctx, obj); } -void nft_obj_notify(struct net *net, const struct nft_table *table, - struct nft_object *obj, u32 portid, u32 seq, int event, - u16 flags, int family, int report, gfp_t gfp) +static void +__nft_obj_notify(struct net *net, const struct nft_table *table, + struct nft_object *obj, u32 portid, u32 seq, int event, + u16 flags, int family, int report, gfp_t gfp) { struct nftables_pernet *nft_net = nft_pernet(net); struct sk_buff *skb; int err; - char *buf = kasprintf(gfp, "%s:%u", - table->name, nft_net->base_seq); - - audit_log_nfcfg(buf, - family, - obj->handle, - event == NFT_MSG_NEWOBJ ? - AUDIT_NFT_OP_OBJ_REGISTER : - AUDIT_NFT_OP_OBJ_UNREGISTER, - gfp); - kfree(buf); if (!report && !nfnetlink_has_listeners(net, NFNLGRP_NFTABLES)) @@ -8086,13 +8076,35 @@ void nft_obj_notify(struct net *net, const struct nft_table *table, err: nfnetlink_set_err(net, portid, NFNLGRP_NFTABLES, -ENOBUFS); } + +void nft_obj_notify(struct net *net, const struct nft_table *table, + struct nft_object *obj, u32 portid, u32 seq, int event, + u16 flags, int family, int report, gfp_t gfp) +{ + struct nftables_pernet *nft_net = nft_pernet(net); + char *buf = kasprintf(gfp, "%s:%u", + table->name, nft_net->base_seq); + + audit_log_nfcfg(buf, + family, + obj->handle, + event == NFT_MSG_NEWOBJ ? + AUDIT_NFT_OP_OBJ_REGISTER : + AUDIT_NFT_OP_OBJ_UNREGISTER, + gfp); + kfree(buf); + + __nft_obj_notify(net, table, obj, portid, seq, event, + flags, family, report, gfp); +} EXPORT_SYMBOL_GPL(nft_obj_notify); static void nf_tables_obj_notify(const struct nft_ctx *ctx, struct nft_object *obj, int event) { - nft_obj_notify(ctx->net, ctx->table, obj, ctx->portid, ctx->seq, event, - ctx->flags, ctx->family, ctx->report, GFP_KERNEL); + __nft_obj_notify(ctx->net, ctx->table, obj, ctx->portid, + ctx->seq, event, ctx->flags, ctx->family, + ctx->report, GFP_KERNEL); } /* diff --git a/tools/testing/selftests/netfilter/nft_audit.sh b/tools/testing/selftests/netfilter/nft_audit.sh index 0b3255e7b3538..bb34329e02a7f 100755 --- a/tools/testing/selftests/netfilter/nft_audit.sh +++ b/tools/testing/selftests/netfilter/nft_audit.sh @@ -85,6 +85,26 @@ do_test "nft add set t1 s2 $setblock; add set t1 s3 { $settype; }" \ do_test "nft add element t1 s3 $setelem" \ "table=t1 family=2 entries=3 op=nft_register_setelem" +# adding counters + +do_test 'nft add counter t1 c1' \ +'table=t1 family=2 entries=1 op=nft_register_obj' + +do_test 'nft add counter t2 c1; add counter t2 c2' \ +'table=t2 family=2 entries=2 op=nft_register_obj' + +# adding/updating quotas + +do_test 'nft add quota t1 q1 { 10 bytes }' \ +'table=t1 family=2 entries=1 op=nft_register_obj' + +do_test 'nft add quota t2 q1 { 10 bytes }; add quota t2 q2 { 10 bytes }' \ +'table=t2 family=2 entries=2 op=nft_register_obj' + +# changing the quota value triggers obj update path +do_test 'nft add quota t1 q1 { 20 bytes }' \ +'table=t1 family=2 entries=1 op=nft_register_obj' + # resetting rules do_test 'nft reset rules t1 c2' \
When adding/updating an object, the transaction handler emits suitable audit log entries already, the one in nft_obj_notify() is redundant. To fix that (and retain the audit logging from objects' 'update' callback), Introduce an "audit log free" variant for internal use. Fixes: c520292f29b80 ("audit: log nftables configuration change events once per table") Signed-off-by: Phil Sutter <phil@nwl.cc> --- net/netfilter/nf_tables_api.c | 44 ++++++++++++------- .../testing/selftests/netfilter/nft_audit.sh | 20 +++++++++ 2 files changed, 48 insertions(+), 16 deletions(-)