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

[PATCH nft v2] 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 json frontend has it twice over: "elem": ["-1"] and the bare number
"elem": [-1] are both accepted and land as element 1 the same way.

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 any negative value. Chain and flowtable priorities are signed but
never reach this check as a negative mpz: bare and json numeric
priorities are built as raw C ints by both frontends, symbols are
rebuilt from atoi() in priority_type_parse(), and the name-plus-offset
forms are computed as C ints in evaluate_priority().

Fixes: cb7cb885d65e ("evaluate: add expr_evaluate_integer()")
Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Avinash Duduskar <avinash.duduskar@gmail.com>
---
v2: drop the priority_type special case, it can never see a negative
    mpz (Phil, Pablo); name the json string and number forms in the
    message and cover them plus flowtable priority in the test

 src/evaluate.c                                | 10 +++
 tests/py/any/meta.t                           |  2 +
 .../parsing/dumps/negative_values_0.nodump    |  0
 .../shell/testcases/parsing/negative_values_0 | 70 +++++++++++++++++++
 4 files changed, 82 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..250ed609 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -447,6 +447,16 @@ static int expr_evaluate_integer(struct eval_ctx *ctx, struct expr **exprp)
 		return -1;
 	}
 
+	/* 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->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..b2909a1a
--- /dev/null
+++ b/tests/shell/testcases/parsing/negative_values_0
@@ -0,0 +1,70 @@
+#!/bin/bash
+
+# mpz_export() drops the sign, so a negative value used to land as its
+# absolute value: "-1" became 1, from every frontend spelling. Chain and
+# flowtable 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 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
+
+# flowtables share evaluate_priority(); --check keeps the kernel out of it
+$NFT -c add flowtable ip t f '{ hook ingress priority -300; }'
+
+exit 0

base-commit: 4f54425ea59250dcd02518ef4a50e6c0b7c162bf
-- 
2.55.0