* [PATCH nft] expression: don't try to import empty string @ 2025-03-27 15:17 Florian Westphal 2025-03-31 11:22 ` Pablo Neira Ayuso 0 siblings, 1 reply; 4+ messages in thread From: Florian Westphal @ 2025-03-27 15:17 UTC (permalink / raw) To: netfilter-devel; +Cc: Florian Westphal The bogon will trigger the assertion in mpz_import_data: src/expression.c:418: constant_expr_alloc: Assertion `(((len) + (8) - 1) / (8)) > 0' failed. Signed-off-by: Florian Westphal <fw@strlen.de> --- src/expression.c | 2 +- .../bogons/nft-j-f/constant_expr_alloc_assert | 38 +++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 tests/shell/testcases/bogons/nft-j-f/constant_expr_alloc_assert diff --git a/src/expression.c b/src/expression.c index 156a66eb37f0..f230f5ad8935 100644 --- a/src/expression.c +++ b/src/expression.c @@ -494,7 +494,7 @@ struct expr *constant_expr_alloc(const struct location *loc, expr->flags = EXPR_F_CONSTANT | EXPR_F_SINGLETON; mpz_init2(expr->value, len); - if (data != NULL) + if (data != NULL && len) mpz_import_data(expr->value, data, byteorder, div_round_up(len, BITS_PER_BYTE)); diff --git a/tests/shell/testcases/bogons/nft-j-f/constant_expr_alloc_assert b/tests/shell/testcases/bogons/nft-j-f/constant_expr_alloc_assert new file mode 100644 index 000000000000..9c40030212ef --- /dev/null +++ b/tests/shell/testcases/bogons/nft-j-f/constant_expr_alloc_assert @@ -0,0 +1,38 @@ +{ + "nftables": [ + { + "table": { + "family": "ip", + "name": "t", + "handle": 0 + } + }, + { + "chain": { + "family": "ip", + "table": "t", + "name": "testchain", + "handle": 0 + } + }, + { + "map": { + "family": "ip", + "name": "testmap", + "table": "t", + "type": "ipv4_addr", + "handle": 0, + "map": "verdict", + "elem": [ + [ + { + "jump": { + "target": "" + } + } + ] + ] + } + } + ] +} -- 2.48.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH nft] expression: don't try to import empty string 2025-03-27 15:17 [PATCH nft] expression: don't try to import empty string Florian Westphal @ 2025-03-31 11:22 ` Pablo Neira Ayuso 2025-03-31 12:37 ` Florian Westphal 0 siblings, 1 reply; 4+ messages in thread From: Pablo Neira Ayuso @ 2025-03-31 11:22 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel [-- Attachment #1: Type: text/plain, Size: 2419 bytes --] Hi Florian, On Thu, Mar 27, 2025 at 04:17:11PM +0100, Florian Westphal wrote: > The bogon will trigger the assertion in mpz_import_data: > src/expression.c:418: constant_expr_alloc: Assertion `(((len) + (8) - 1) / (8)) > 0' failed. I took a quick look searching for {s:s} in src/parser_json.c The common idiom is json_parse_err() then a helper parser function to validate the string. It seems it is missing in this case. Maybe tigthen json parser instead? Caller invoking constant_expr_alloc() with data != NULL but no len looks broken to me. Maybe take both patches if you prefer? > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > src/expression.c | 2 +- > .../bogons/nft-j-f/constant_expr_alloc_assert | 38 +++++++++++++++++++ > 2 files changed, 39 insertions(+), 1 deletion(-) > create mode 100644 tests/shell/testcases/bogons/nft-j-f/constant_expr_alloc_assert > > diff --git a/src/expression.c b/src/expression.c > index 156a66eb37f0..f230f5ad8935 100644 > --- a/src/expression.c > +++ b/src/expression.c > @@ -494,7 +494,7 @@ struct expr *constant_expr_alloc(const struct location *loc, > expr->flags = EXPR_F_CONSTANT | EXPR_F_SINGLETON; > > mpz_init2(expr->value, len); > - if (data != NULL) > + if (data != NULL && len) > mpz_import_data(expr->value, data, byteorder, > div_round_up(len, BITS_PER_BYTE)); > > diff --git a/tests/shell/testcases/bogons/nft-j-f/constant_expr_alloc_assert b/tests/shell/testcases/bogons/nft-j-f/constant_expr_alloc_assert > new file mode 100644 > index 000000000000..9c40030212ef > --- /dev/null > +++ b/tests/shell/testcases/bogons/nft-j-f/constant_expr_alloc_assert > @@ -0,0 +1,38 @@ > +{ > + "nftables": [ > + { > + "table": { > + "family": "ip", > + "name": "t", > + "handle": 0 > + } > + }, > + { > + "chain": { > + "family": "ip", > + "table": "t", > + "name": "testchain", > + "handle": 0 > + } > + }, > + { > + "map": { > + "family": "ip", > + "name": "testmap", > + "table": "t", > + "type": "ipv4_addr", > + "handle": 0, > + "map": "verdict", > + "elem": [ > + [ > + { > + "jump": { > + "target": "" > + } > + } > + ] > + ] > + } > + } > + ] > +} > -- > 2.48.1 > > [-- Attachment #2: x.patch --] [-- Type: text/x-diff, Size: 467 bytes --] diff --git a/src/parser_json.c b/src/parser_json.c index 04d762741e4a..ef7740840710 100644 --- a/src/parser_json.c +++ b/src/parser_json.c @@ -1350,6 +1350,9 @@ static struct expr *json_parse_verdict_expr(struct json_ctx *ctx, json_unpack_err(ctx, root, "{s:s}", "target", &chain)) return NULL; + if (!chain || chain[0] == '\0') + return NULL; + return verdict_expr_alloc(int_loc, verdict_tbl[i].verdict, json_alloc_chain_expr(chain)); } ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH nft] expression: don't try to import empty string 2025-03-31 11:22 ` Pablo Neira Ayuso @ 2025-03-31 12:37 ` Florian Westphal 2025-03-31 16:10 ` Pablo Neira Ayuso 0 siblings, 1 reply; 4+ messages in thread From: Florian Westphal @ 2025-03-31 12:37 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel Pablo Neira Ayuso <pablo@netfilter.org> wrote: > Hi Florian, > > On Thu, Mar 27, 2025 at 04:17:11PM +0100, Florian Westphal wrote: > > The bogon will trigger the assertion in mpz_import_data: > > src/expression.c:418: constant_expr_alloc: Assertion `(((len) + (8) - 1) / (8)) > 0' failed. > > I took a quick look searching for {s:s} in src/parser_json.c > > The common idiom is json_parse_err() then a helper parser function to > validate the string. > > It seems it is missing in this case. Maybe tigthen json parser instead? > > Caller invoking constant_expr_alloc() with data != NULL but no len > looks broken to me. return constant_expr_alloc(int_loc, &string_type, BYTEORDER_HOST_ENDIAN, strlen(chain) * BITS_PER_BYTE, chain); chain name is '""'. There are other spots where we possibly call into constant_expr_alloc() with a 0 argument. I think it would be a lot more work and bloat to add all the checks on the json side while its a one-liner in constant_expr_alloc(). I could also add json_constant_expr_alloc() but it seems kinda silly to me. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH nft] expression: don't try to import empty string 2025-03-31 12:37 ` Florian Westphal @ 2025-03-31 16:10 ` Pablo Neira Ayuso 0 siblings, 0 replies; 4+ messages in thread From: Pablo Neira Ayuso @ 2025-03-31 16:10 UTC (permalink / raw) To: Florian Westphal; +Cc: netfilter-devel On Mon, Mar 31, 2025 at 02:37:15PM +0200, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > Hi Florian, > > > > On Thu, Mar 27, 2025 at 04:17:11PM +0100, Florian Westphal wrote: > > > The bogon will trigger the assertion in mpz_import_data: > > > src/expression.c:418: constant_expr_alloc: Assertion `(((len) + (8) - 1) / (8)) > 0' failed. > > > > I took a quick look searching for {s:s} in src/parser_json.c > > > > The common idiom is json_parse_err() then a helper parser function to > > validate the string. > > > > It seems it is missing in this case. Maybe tigthen json parser instead? > > > > Caller invoking constant_expr_alloc() with data != NULL but no len > > looks broken to me. > > return constant_expr_alloc(int_loc, &string_type, BYTEORDER_HOST_ENDIAN, > strlen(chain) * BITS_PER_BYTE, chain); > > chain name is '""'. > > There are other spots where we possibly call into constant_expr_alloc() > with a 0 argument. Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org> > I think it would be a lot more work and bloat to add all the checks on > the json side while its a one-liner in constant_expr_alloc(). I don't think this is needed, the idiom in src/parser_json.c is to: 1) fetch string 2) validate it For example: if (!json_unpack(root, "{s:s}", "ttl", &ttl)) { if (!strcmp(ttl, "loose")) { ttlval = 1; } else if (!strcmp(ttl, "skip")) { ttlval = 2; } else { json_error(ctx, "Invalid osf ttl option '%s'.", ttl); return NULL; } } > I could also add json_constant_expr_alloc() but it seems kinda silly to me. I agree, I don't think json_constant_expr_alloc is needed. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-03-31 16:10 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-03-27 15:17 [PATCH nft] expression: don't try to import empty string Florian Westphal 2025-03-31 11:22 ` Pablo Neira Ayuso 2025-03-31 12:37 ` Florian Westphal 2025-03-31 16:10 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).