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 Signed-off-by: Avinash Duduskar --- 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