Control: severity -1 normal
Control: tags -1 confirmed bullseye patch

On 2024-02-11, at 09:50:27 +0100, Einhard Leichtfuß wrote:
> Package: nftables
> Version: 0.9.8-3.1+deb11u2
> Severity: important
> 
> Upon running `nft -f file.nft`, where `file.nft` specifies the same
> table at least twice, and a named set or map is defined in the second
> (or later) table specification, a segmentation fault is caused.
> 
> The specified ruleset appears to be correctly applied regardless.
> 
> Example `file.nft`:
> ---
> table inet t0 {
> }
> 
> table inet t0 {
>         set s0 {
>                 type inet_service
>                 elements = { 42 }
>         }
> }
> ---
> 
> Note that both a named set and a named map definition cause the
> segfault, while a (similarly simple) chain definition does not.
> 
> The only error message printed is "Segmentation fault\n".
> 
> Note that this causes nftables.service to fail if `/etc/nftables.conf`
> contains such configuration (but the ruleset appears to be applied).
> 
> I cannot reproduce the bug with the preceding package version,
> 0.9.8-3.1+deb11u1, nor on Debian 12 Bookworm (nftables 1.0.6-2+deb12u2).

0.9.8-3.1+deb11u2 includes some cherry-picked patches from upstream to a
different bug (#1051592).  Unfortunately, it turns out that they also
introduce the crash that you have observed.

The crash occurs when freeing resources after processing the file, so as
you note the commands in the file are successfully executed.  The crash
can be avoided by making sure that the set definition is in the first
table block.  For example, swapping:

    table inet t0 {
        set s0 {
            type inet_service
            elements = { 42 }
        }
    }
    table inet t0 {
    }

or just merging:

    table inet t0 {
        set s0 {
            type inet_service
            elements = { 42 }
        }
    }

should work around the problem.  For these reasons I am lowering the
severity to "normal".

I have identified the underlying problem and I believe that only a small
code change is needed to fix it (patch attached).  I'm going to get a
second opinion from upstream.

J.
From 45203fff06e15b97037e3c32e4c6256025a23d75 Mon Sep 17 00:00:00 2001
From: Jeremy Sowden <jer...@azazel.net>
Date: Sat, 17 Feb 2024 14:58:23 +0000
Subject: [PATCH] evaluate: don't call `set_add_hash` for sets which are
 already associated with a table

Signed-off-by: Jeremy Sowden <jer...@azazel.net>
---
 src/evaluate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 232ae39020cc..c58e37e14064 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -3759,7 +3759,8 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
 	}
 	ctx->set = NULL;
 
-	if (set_lookup(table, set->handle.set.name) == NULL)
+	if (set_lookup(table, set->handle.set.name) == NULL &&
+	    list_empty(&set->list))
 		set_add_hash(set_get(set), table);
 
 	return 0;
-- 
2.43.0

Attachment: signature.asc
Description: PGP signature

Reply via email to