* [PATCH nft] evaluate: reject negative values for unsigned datatypes
@ 2026-08-08 1:33 Avinash Duduskar
2026-08-13 11:21 ` Phil Sutter
0 siblings, 1 reply; 3+ messages in thread
From: Avinash Duduskar @ 2026-08-08 1:33 UTC (permalink / raw)
To: netfilter-devel; +Cc: Pablo Neira Ayuso, Phil Sutter
expr_evaluate_integer() only tests the upper bound, so a negative value
passes the range check and mpz_export() then drops the sign:
# nft add element ip t m { "-1" }
# nft list set ip t m
table ip t {
set m {
type mark
elements = { 0x00000001 }
}
}
The error string has read "Value %s exceeds valid range 0-%s" since the
check was added, so the contract was already unsigned; only the upper
half of it was enforced. The result is not a wrap either: "-1" gives 1
while "-4294967295" gives 0xffffffff.
Reject a negative value, unless the evaluation context is a chain
priority, which is signed.
Fixes: cb7cb885d65e ("evaluate: add expr_evaluate_integer()")
Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Avinash Duduskar <avinash.duduskar@gmail.com>
---
src/evaluate.c | 12 +++++
tests/py/any/meta.t | 2 +
.../parsing/dumps/negative_values_0.nodump | 0
.../shell/testcases/parsing/negative_values_0 | 46 +++++++++++++++++++
4 files changed, 60 insertions(+)
create mode 100644 tests/shell/testcases/parsing/dumps/negative_values_0.nodump
create mode 100755 tests/shell/testcases/parsing/negative_values_0
diff --git a/src/evaluate.c b/src/evaluate.c
index 8bb7b609..f5b88c0b 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -447,6 +447,18 @@ static int expr_evaluate_integer(struct eval_ctx *ctx, struct expr **exprp)
return -1;
}
+ /* chain priorities are signed, everything else is an unsigned key:
+ * mpz_export() drops the sign, so "-1" would silently become 1.
+ */
+ if (mpz_sgn(expr->value) < 0 && ctx->ectx.dtype != &priority_type) {
+ valstr = mpz_get_str(NULL, 10, expr->value);
+ expr_error(ctx->msgs, expr,
+ "Value %s is negative, expecting an unsigned value",
+ valstr);
+ nft_gmp_free(valstr);
+ return -1;
+ }
+
if (ctx->stmt_len > ctx->ectx.len)
masklen = ctx->stmt_len;
else
diff --git a/tests/py/any/meta.t b/tests/py/any/meta.t
index c5ab2ad9..4f486307 100644
--- a/tests/py/any/meta.t
+++ b/tests/py/any/meta.t
@@ -56,6 +56,8 @@ meta mark and 0x03 == 0x01;ok;meta mark & 0x00000003 == 0x00000001
meta mark and 0x03 != 0x01;ok;meta mark & 0x00000003 != 0x00000001
meta mark 0x10;ok;meta mark 0x00000010
meta mark != 0x10;ok;meta mark != 0x00000010
+meta mark "-1";fail
+meta mark "-4294967295";fail
meta mark 0xffffff00/24;ok;meta mark & 0xffffff00 == 0xffffff00
meta mark or 0x03 == 0x01;ok;meta mark | 0x00000003 == 0x00000001
diff --git a/tests/shell/testcases/parsing/dumps/negative_values_0.nodump b/tests/shell/testcases/parsing/dumps/negative_values_0.nodump
new file mode 100644
index 00000000..e69de29b
diff --git a/tests/shell/testcases/parsing/negative_values_0 b/tests/shell/testcases/parsing/negative_values_0
new file mode 100755
index 00000000..663579a0
--- /dev/null
+++ b/tests/shell/testcases/parsing/negative_values_0
@@ -0,0 +1,46 @@
+#!/bin/bash
+
+# mpz_export() drops the sign, so a negative value used to land as its
+# absolute value: "-1" became 1. Chain priorities are signed and must keep
+# working.
+
+set -e
+
+$NFT add table ip t
+$NFT add set ip t s '{ type mark; }'
+
+if $NFT add element ip t s '{ "-1" }' 2>/dev/null; then
+ echo "E: accepted a negative set element" >&2
+ $NFT list set ip t s >&2
+ exit 1
+fi
+
+# a rejected add must not have committed anything
+out=$($NFT list set ip t s)
+case "$out" in
+*elements*)
+ echo "E: something was stored by the failed add" >&2
+ echo "$out" >&2
+ exit 1
+ ;;
+esac
+
+$NFT add chain ip t c
+
+if $NFT add rule ip t c meta mark '"-1"' 2>/dev/null; then
+ echo "E: accepted a negative value in a rule" >&2
+ exit 1
+fi
+
+# the signed exception: every spelling of a negative chain priority
+$NFT add chain ip t c1 '{ type filter hook prerouting priority -300; }'
+$NFT add chain ip t c2 '{ type filter hook prerouting priority filter - 10; }'
+
+$NFT -f - <<'NFT'
+define p = -300
+table ip t2 {
+ chain c { type filter hook prerouting priority $p; policy accept; }
+}
+NFT
+
+exit 0
base-commit: 49e418238ece947e92f87a35ef6cf50755485370
--
2.55.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH nft] evaluate: reject negative values for unsigned datatypes
2026-08-08 1:33 [PATCH nft] evaluate: reject negative values for unsigned datatypes Avinash Duduskar
@ 2026-08-13 11:21 ` Phil Sutter
2026-08-13 12:58 ` Pablo Neira Ayuso
0 siblings, 1 reply; 3+ messages in thread
From: Phil Sutter @ 2026-08-13 11:21 UTC (permalink / raw)
To: Avinash Duduskar; +Cc: netfilter-devel, Pablo Neira Ayuso
On Sat, Aug 08, 2026 at 07:03:18AM +0530, Avinash Duduskar wrote:
> expr_evaluate_integer() only tests the upper bound, so a negative value
> passes the range check and mpz_export() then drops the sign:
>
> # nft add element ip t m { "-1" }
> # nft list set ip t m
> table ip t {
> set m {
> type mark
> elements = { 0x00000001 }
> }
> }
>
> The error string has read "Value %s exceeds valid range 0-%s" since the
> check was added, so the contract was already unsigned; only the upper
> half of it was enforced. The result is not a wrap either: "-1" gives 1
> while "-4294967295" gives 0xffffffff.
>
> Reject a negative value, unless the evaluation context is a chain
> priority, which is signed.
>
> Fixes: cb7cb885d65e ("evaluate: add expr_evaluate_integer()")
> Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
> Signed-off-by: Avinash Duduskar <avinash.duduskar@gmail.com>
> ---
> src/evaluate.c | 12 +++++
> tests/py/any/meta.t | 2 +
> .../parsing/dumps/negative_values_0.nodump | 0
> .../shell/testcases/parsing/negative_values_0 | 46 +++++++++++++++++++
> 4 files changed, 60 insertions(+)
> create mode 100644 tests/shell/testcases/parsing/dumps/negative_values_0.nodump
> create mode 100755 tests/shell/testcases/parsing/negative_values_0
>
> diff --git a/src/evaluate.c b/src/evaluate.c
> index 8bb7b609..f5b88c0b 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -447,6 +447,18 @@ static int expr_evaluate_integer(struct eval_ctx *ctx, struct expr **exprp)
> return -1;
> }
>
> + /* chain priorities are signed, everything else is an unsigned key:
> + * mpz_export() drops the sign, so "-1" would silently become 1.
> + */
> + if (mpz_sgn(expr->value) < 0 && ctx->ectx.dtype != &priority_type) {
Although it might remain the only case, I don't like the special-casing
here. Looking at struct datatype, we have a 32bit field for flags with
only two bits used yet. Maybe introduce an 'is_signed' bit to check
here?
Thanks, Phil
> + valstr = mpz_get_str(NULL, 10, expr->value);
> + expr_error(ctx->msgs, expr,
> + "Value %s is negative, expecting an unsigned value",
> + valstr);
> + nft_gmp_free(valstr);
> + return -1;
> + }
> +
> if (ctx->stmt_len > ctx->ectx.len)
> masklen = ctx->stmt_len;
> else
> diff --git a/tests/py/any/meta.t b/tests/py/any/meta.t
> index c5ab2ad9..4f486307 100644
> --- a/tests/py/any/meta.t
> +++ b/tests/py/any/meta.t
> @@ -56,6 +56,8 @@ meta mark and 0x03 == 0x01;ok;meta mark & 0x00000003 == 0x00000001
> meta mark and 0x03 != 0x01;ok;meta mark & 0x00000003 != 0x00000001
> meta mark 0x10;ok;meta mark 0x00000010
> meta mark != 0x10;ok;meta mark != 0x00000010
> +meta mark "-1";fail
> +meta mark "-4294967295";fail
> meta mark 0xffffff00/24;ok;meta mark & 0xffffff00 == 0xffffff00
>
> meta mark or 0x03 == 0x01;ok;meta mark | 0x00000003 == 0x00000001
> diff --git a/tests/shell/testcases/parsing/dumps/negative_values_0.nodump b/tests/shell/testcases/parsing/dumps/negative_values_0.nodump
> new file mode 100644
> index 00000000..e69de29b
> diff --git a/tests/shell/testcases/parsing/negative_values_0 b/tests/shell/testcases/parsing/negative_values_0
> new file mode 100755
> index 00000000..663579a0
> --- /dev/null
> +++ b/tests/shell/testcases/parsing/negative_values_0
> @@ -0,0 +1,46 @@
> +#!/bin/bash
> +
> +# mpz_export() drops the sign, so a negative value used to land as its
> +# absolute value: "-1" became 1. Chain priorities are signed and must keep
> +# working.
> +
> +set -e
> +
> +$NFT add table ip t
> +$NFT add set ip t s '{ type mark; }'
> +
> +if $NFT add element ip t s '{ "-1" }' 2>/dev/null; then
> + echo "E: accepted a negative set element" >&2
> + $NFT list set ip t s >&2
> + exit 1
> +fi
> +
> +# a rejected add must not have committed anything
> +out=$($NFT list set ip t s)
> +case "$out" in
> +*elements*)
> + echo "E: something was stored by the failed add" >&2
> + echo "$out" >&2
> + exit 1
> + ;;
> +esac
> +
> +$NFT add chain ip t c
> +
> +if $NFT add rule ip t c meta mark '"-1"' 2>/dev/null; then
> + echo "E: accepted a negative value in a rule" >&2
> + exit 1
> +fi
> +
> +# the signed exception: every spelling of a negative chain priority
> +$NFT add chain ip t c1 '{ type filter hook prerouting priority -300; }'
> +$NFT add chain ip t c2 '{ type filter hook prerouting priority filter - 10; }'
> +
> +$NFT -f - <<'NFT'
> +define p = -300
> +table ip t2 {
> + chain c { type filter hook prerouting priority $p; policy accept; }
> +}
> +NFT
> +
> +exit 0
>
> base-commit: 49e418238ece947e92f87a35ef6cf50755485370
> --
> 2.55.0
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH nft] evaluate: reject negative values for unsigned datatypes
2026-08-13 11:21 ` Phil Sutter
@ 2026-08-13 12:58 ` Pablo Neira Ayuso
0 siblings, 0 replies; 3+ messages in thread
From: Pablo Neira Ayuso @ 2026-08-13 12:58 UTC (permalink / raw)
To: Phil Sutter; +Cc: Avinash Duduskar, netfilter-devel
On Thu, Aug 13, 2026 at 01:21:53PM +0200, Phil Sutter wrote:
> On Sat, Aug 08, 2026 at 07:03:18AM +0530, Avinash Duduskar wrote:
> > expr_evaluate_integer() only tests the upper bound, so a negative value
> > passes the range check and mpz_export() then drops the sign:
> >
> > # nft add element ip t m { "-1" }
> > # nft list set ip t m
> > table ip t {
> > set m {
> > type mark
> > elements = { 0x00000001 }
> > }
> > }
> >
> > The error string has read "Value %s exceeds valid range 0-%s" since the
> > check was added, so the contract was already unsigned; only the upper
> > half of it was enforced. The result is not a wrap either: "-1" gives 1
> > while "-4294967295" gives 0xffffffff.
> >
> > Reject a negative value, unless the evaluation context is a chain
> > priority, which is signed.
> >
> > Fixes: cb7cb885d65e ("evaluate: add expr_evaluate_integer()")
> > Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > Signed-off-by: Avinash Duduskar <avinash.duduskar@gmail.com>
> > ---
> > src/evaluate.c | 12 +++++
> > tests/py/any/meta.t | 2 +
> > .../parsing/dumps/negative_values_0.nodump | 0
> > .../shell/testcases/parsing/negative_values_0 | 46 +++++++++++++++++++
> > 4 files changed, 60 insertions(+)
> > create mode 100644 tests/shell/testcases/parsing/dumps/negative_values_0.nodump
> > create mode 100755 tests/shell/testcases/parsing/negative_values_0
> >
> > diff --git a/src/evaluate.c b/src/evaluate.c
> > index 8bb7b609..f5b88c0b 100644
> > --- a/src/evaluate.c
> > +++ b/src/evaluate.c
> > @@ -447,6 +447,18 @@ static int expr_evaluate_integer(struct eval_ctx *ctx, struct expr **exprp)
> > return -1;
> > }
> >
> > + /* chain priorities are signed, everything else is an unsigned key:
> > + * mpz_export() drops the sign, so "-1" would silently become 1.
> > + */
> > + if (mpz_sgn(expr->value) < 0 && ctx->ectx.dtype != &priority_type) {
>
> Although it might remain the only case, I don't like the special-casing
> here. Looking at struct datatype, we have a 32bit field for flags with
> only two bits used yet. Maybe introduce an 'is_signed' bit to check
> here?
Yes, I wonder if we can do this in a more generic way, like specifying
in the datatype itself the min and maximum value expected from the
integer. I don't expect "signed 128-bit only". Maybe add a new
interface to struct datatype to validate the value is within the
boundaries? Maybe something like:
if (ctx->ectx.dtype && ctx->ectx.dtype->validate) {
erec = ctx->ectx.dtype->validate(expr->value));
if (erec)
return -1;
}
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-08-13 12:58 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-08 1:33 [PATCH nft] evaluate: reject negative values for unsigned datatypes Avinash Duduskar
2026-08-13 11:21 ` Phil Sutter
2026-08-13 12:58 ` Pablo Neira Ayuso
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.