[PATCH nft] evaluate: reject negative values for unsigned datatypes

[PATCH nft] evaluate: reject negative values for unsigned datatypes

[Avinash Duduskar]

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

Re: [PATCH nft] evaluate: reject negative values for unsigned datatypes

[Phil Sutter]

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

Re: [PATCH nft] evaluate: reject negative values for unsigned datatypes

[Pablo Neira Ayuso]

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;
 }

Re: [PATCH nft] evaluate: reject negative values for unsigned datatypes

[Avinash Duduskar]

On Thu, Aug 13, 2026 at 02:58:41PM +0200, Pablo Neira Ayuso wrote:
> 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.

Turns out the special case should not exist at all: it is dead code, so
neither the flag bit nor a validate callback would have a user here.

A negative priority never arrives at this check as a negative mpz. Bare
and json numeric priorities are built as raw C ints by both frontends,
priority_type_parse() throws away integer_type_parse()'s result and
rebuilds symbols from atoi(), and the name-plus-offset forms are
computed as C ints in evaluate_priority(). Instrumenting the top of
expr_evaluate_integer() agrees:

  priority -300 (text)  dtype=priority  sgn=1   val=4294966996
  prio: -300    (json)  dtype=priority  sgn=1   val=4294966996
  element "-1"          dtype=mark      sgn=-1  val=-1

On min/max: both bounds this function enforces today come from the eval
context (ectx.maxval from numgen/hash, the mask from ectx.len), not from
the datatype, so a validate hook would sit beside them with no in-tree
user.

Dropping the special case also covers a third spelling of the bug found
while testing: "elem": [-1] as a json number is accepted today and
stored as element 1, like the string forms. The plain check rejects it
too.

v2 follows with the unconditional check and test arms for all three
forms plus flowtable priority. If the validate interface is wanted for
other reasons, I would do it as a separate patch on top.

Thanks,
Avinash