[nft PATCH] monitor: Sanitize startup race condition

[nft PATCH] monitor: Sanitize startup race condition

[Phil Sutter]

During startup, 'nft monitor' first fetches the current ruleset and then
keeps this cache up to date based on received events. This is racey, as
any ruleset changes in between the initial fetch and the socket opening
are not recognized.

This script demonstrates the problem:

| #!/bin/bash
|
| while true; do
| 	nft flush ruleset
| 	iptables-nft -A FORWARD
| done &
| maniploop=$!
|
| trap "kill $maniploop; kill \$!; wait" EXIT
|
| while true; do
| 	nft monitor rules >/dev/null &
| 	sleep 0.2
| 	kill $!
| done

If the table add event is missed, the rule add event callback fails to
deserialize the rule and calls abort().

Avoid the inconvenient program exit by returning NULL from
netlink_delinearize_rule() instead of aborting and make callers check
the return value.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/cache.c               | 1 +
 src/monitor.c             | 5 +++++
 src/netlink_delinearize.c | 5 ++++-
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/cache.c b/src/cache.c
index f790f995e07b0..85de970f76448 100644
--- a/src/cache.c
+++ b/src/cache.c
@@ -598,6 +598,7 @@ static int list_rule_cb(struct nftnl_rule *nlr, void *data)
 
 	netlink_dump_rule(nlr, ctx);
 	rule = netlink_delinearize_rule(ctx, nlr);
+	assert(rule);
 	list_add_tail(&rule->list, &ctx->list);
 
 	return 0;
diff --git a/src/monitor.c b/src/monitor.c
index 7fa92ebfb0f3a..a6b30a18cfd25 100644
--- a/src/monitor.c
+++ b/src/monitor.c
@@ -551,6 +551,10 @@ static int netlink_events_rule_cb(const struct nlmsghdr *nlh, int type,
 
 	nlr = netlink_rule_alloc(nlh);
 	r = netlink_delinearize_rule(monh->ctx, nlr);
+	if (!r) {
+		fprintf(stderr, "W: Received event for an unknown table.\n");
+		goto out_free_nlr;
+	}
 	nlr_for_each_set(nlr, rule_map_decompose_cb, NULL,
 			 &monh->ctx->nft->cache);
 	cmd = netlink_msg2cmd(type, nlh->nlmsg_flags);
@@ -587,6 +591,7 @@ static int netlink_events_rule_cb(const struct nlmsghdr *nlh, int type,
 		break;
 	}
 	rule_free(r);
+out_free_nlr:
 	nftnl_rule_free(nlr);
 	return MNL_CB_OK;
 }
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index 0da6cc78f94fa..e8b9724cbac94 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -3195,7 +3195,10 @@ struct rule *netlink_delinearize_rule(struct netlink_ctx *ctx,
 	pctx->rule = rule_alloc(&netlink_location, &h);
 	pctx->table = table_cache_find(&ctx->nft->cache.table_cache,
 				       h.table.name, h.family);
-	assert(pctx->table != NULL);
+	if (!pctx->table) {
+		errno = ENOENT;
+		return NULL;
+	}
 
 	pctx->rule->comment = nftnl_rule_get_comment(nlr);
 
-- 
2.34.1

Re: [nft PATCH] monitor: Sanitize startup race condition

[Pablo Neira Ayuso]

On Thu, Sep 29, 2022 at 12:32:48AM +0200, Phil Sutter wrote:
> During startup, 'nft monitor' first fetches the current ruleset and then
> keeps this cache up to date based on received events. This is racey, as
> any ruleset changes in between the initial fetch and the socket opening
> are not recognized.
> 
> This script demonstrates the problem:
> 
> | #!/bin/bash
> |
> | while true; do
> | 	nft flush ruleset
> | 	iptables-nft -A FORWARD
> | done &
> | maniploop=$!
> |
> | trap "kill $maniploop; kill \$!; wait" EXIT
> |
> | while true; do
> | 	nft monitor rules >/dev/null &
> | 	sleep 0.2
> | 	kill $!
> | done
> 
> If the table add event is missed, the rule add event callback fails to
> deserialize the rule and calls abort().
> 
> Avoid the inconvenient program exit by returning NULL from
> netlink_delinearize_rule() instead of aborting and make callers check
> the return value.

Fine to apply this meanwhile.

I wanted to fix this, but I found a few kernel bugs at that time, such as:

commit 6fb721cf781808ee2ca5e737fb0592cc68de3381
Author: Pablo Neira Ayuso <pablo@netfilter.org>
Date:   Sun Sep 26 09:59:35 2021 +0200

    netfilter: nf_tables: honor NLM_F_CREATE and NLM_F_EXCL in event notification

which were not allowing me to infer the location accordingly, for
incrementally updating the cache.

So I stopped for a while until these fixes propagate to the kernel.

It's been 1 year even since, times flies...

Re: [nft PATCH] monitor: Sanitize startup race condition

[Phil Sutter]

Hi,

On Fri, Sep 30, 2022 at 03:15:06PM +0200, Pablo Neira Ayuso wrote:
[...]
> Fine to apply this meanwhile.

Thanks. I tried to find a better solution, but failed. IMO it should be
enough to just refresh cache from scratch once the first event is
received, but it seems the reproducer script is too aggressive even for
that.

> I wanted to fix this, but I found a few kernel bugs at that time, such as:
> 
> commit 6fb721cf781808ee2ca5e737fb0592cc68de3381
> Author: Pablo Neira Ayuso <pablo@netfilter.org>
> Date:   Sun Sep 26 09:59:35 2021 +0200
> 
>     netfilter: nf_tables: honor NLM_F_CREATE and NLM_F_EXCL in event notification
> 
> which were not allowing me to infer the location accordingly, for
> incrementally updating the cache.
> 
> So I stopped for a while until these fixes propagate to the kernel.
> 
> It's been 1 year even since, times flies...

Same here. My backlog just keeps growing and with it the number of
side-projects "to get back to later".

Cheers, Phil