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

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

[Avinash Duduskar]

expr_evaluate_integer() only checks the upper bound, so a negative value
passes 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 }
          }
  }

Same through json, both as "elem": ["-1"] and as "elem": [-1].

Reject a negative value. Priorities are unaffected: integer_type_parse()
is the only caller of mpz_set_str() outside mini-gmp, and
priority_type_parse() discards its result for an atoi() int before
evaluation.

Values from a define are not covered. initializer_expr's DASH NUM rule
loses the sign before evaluation, so "define x = -1" still lands as
0xffffffff on an unsigned type. Separate bug, left alone.

Fixes: cb7cb885d65e ("evaluate: add expr_evaluate_integer()")
Link: https://lore.kernel.org/netfilter-devel/amnndnoOMoR05sJK@chamomile/
Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Avinash Duduskar <avinash.duduskar@gmail.com>
---
v3: sign check moved ahead of the maxval test. No other code change.
    Commit message now records that a define loses the sign before
    evaluation, so those are out of scope.

    v2 dropped the priority special case Phil objected to and had no
    reply. Pablo, this still does not add the ->validate interface you
    asked for. I did not want a new datatype hook and its per-type
    min/max in the same patch as the fix, but I will do that first as a
    prerequisite if it is the shape you want.

    v2: https://lore.kernel.org/netfilter-devel/20260814211022.2813482-1-avinash.duduskar@gmail.com/
    Tested on fcef13a3: shell 507 ok / 6 skipped / 0 failed, py 2083
    tests 0 error.

 src/evaluate.c                                | 10 +++
 tests/py/any/meta.t                           |  2 +
 .../parsing/dumps/negative_values_0.nodump    |  0
 .../shell/testcases/parsing/negative_values_0 | 71 +++++++++++++++++++
 4 files changed, 83 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 8115867f..7af800d3 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -437,6 +437,16 @@ static int expr_evaluate_integer(struct eval_ctx *ctx, struct expr **exprp)
 	uint32_t masklen;
 	mpz_t mask;
 
+	/* mpz_export() ignores the sign, so "-1" would silently become 1. */
+	if (mpz_sgn(expr->value) < 0) {
+		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->ectx.maxval > 0 &&
 	    mpz_cmp_ui(expr->value, ctx->ectx.maxval) > 0) {
 		valstr = mpz_get_str(NULL, 10, expr->value);
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..3c7e3a64
--- /dev/null
+++ b/tests/shell/testcases/parsing/negative_values_0
@@ -0,0 +1,71 @@
+#!/bin/bash
+
+# mpz_export() drops the sign, so "-1" used to land as element 1, in rules
+# and in set elements, from both frontends. Priorities are signed and must
+# keep working. A negative from a define is not rejected, the sign is lost
+# before evaluation, which is why the priority arms below can still use one.
+
+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 json frontend must reject a negative element as a string and as a
+# bare number, both used to land as element 1
+if [ "$NFT_TEST_HAVE_json" != n ]; then
+	if echo '{"nftables":[{"add":{"element":{"family":"ip","table":"t","name":"s","elem":["-1"]}}}]}' | $NFT -j -f - 2>/dev/null; then
+		echo "E: json accepted a negative element as a string" >&2
+		exit 1
+	fi
+	if echo '{"nftables":[{"add":{"element":{"family":"ip","table":"t","name":"s","elem":[-1]}}}]}' | $NFT -j -f - 2>/dev/null; then
+		echo "E: json accepted a negative element as a number" >&2
+		exit 1
+	fi
+	out=$($NFT list set ip t s)
+	case "$out" in
+	*elements*)
+		echo "E: something was stored by the failed json adds" >&2
+		echo "$out" >&2
+		exit 1
+		;;
+	esac
+fi
+
+# priorities are signed: every spelling of a negative one must keep working
+$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
+
+# flowtable priority, --check so this needs no kernel support
+$NFT -c add flowtable ip t f '{ hook ingress priority -300; }'
+
+exit 0

base-commit: fcef13a358a0755a0bd980910d47989b064a756d
-- 
2.55.0