Re: [PATCH nft 2/2] evaluate: only allow stateful statements in set and map definitions

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Mon, Mar 31, 2025 at 06:15:30PM +0200, Florian Westphal wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > The bison parser doesn't allow this to happen due to grammar
> > restrictions, but the json input has no such issues.
> > 
> > The bogon input assigns 'notrack' which triggers:
> > BUG: unknown stateful statement type 19
> > nft: src/netlink_linearize.c:1061: netlink_gen_stmt_stateful: Assertion `0' failed.
> > 
> > After patch, we get:
> > Error: map statement must be stateful
> 
> On the same subject of 'do I fix this in evaluate.c or parser_json.c':
> 
> cat bla
> table t {
>         set sc {
>                 type inet_service . ifname
>         }
> 
>         chain c {
>                 tcp dport . bla* @sc accept
>         }
> }
> nft -f bla
> BUG: unknown expression type prefix
> nft: src/netlink_linearize.c:914: netlink_gen_expr: Assertion `0' failed.
> Aborted (core dumped)
> 
> I can either fix this in evaluate.c, or I try to rework both
> parser_bison.y and parser_json.c to no longer allow prefix expressions
> when specifying the lookup key.

It is probably too complex from the parser in this case.

> I suspect that fixing it in evaluate.c is going to be a lot simpler.

I agree.

But for things that are obviously incorrect at parsing stage and that
are simple to reject, like the empty string case for goto/jump in
verdict map, then it is easy to reject early. The json parser already
rejects unexisting/unsupported keys, the parser is already making a
first pass in validating the input.

I don't think it is a matter of picking between validating _only_ from
parser or from evaluation, I think tightening from parser (when
trivial) and evaluation for more complex invalid input is fine.

> We can't disable prefixes in concatenations, its valid for set element
> keys, but I suspect that we can use recursion counter to figure out if
> the concatenation is on the RHS of something else (such as part of
> EXPR_SET_ELEM).
> 
> I'll work on it tomorrow.

Thanks!