Re: [PATCH nft] parser_json: only allow concatenations with 2 or more expressions

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

On Wed, Apr 02, 2025 at 07:18:18AM +0200, Florian Westphal wrote:
> The bison grammar enforces this implicitly by grammar rules, e.g.
> "mark . ip saddr" or similar, i.e., ALL concatenation expressions
> consist of at least two elements.
> 
> But this doesn't apply to the json frontend which just uses an
> array: it can be empty or only contain one element.
> 
> The included reproducer makes the eval stage set the "concatenation" flag
> on the interval set.  This prevents the needed conversion code to turn the
> element values into ranges from getting run.
> 
> The reproducer asserts with:
> nft: src/intervals.c:786: setelem_to_interval: Assertion `key->etype == EXPR_RANGE_VALUE' failed.
> 
> Convert the assertion to BUG() so we can see what element type got passed
> to the set interval code in case we have further issues in this area.
> 
> Reject 0-or-1-element concatenations from the json parser.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>

Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org>

Thanks

> ---
>  src/intervals.c                               |  4 ++-
>  src/parser_json.c                             | 20 ++++++++------
>  .../set_with_single_value_concat_assert       | 26 +++++++++++++++++++
>  3 files changed, 41 insertions(+), 9 deletions(-)
>  create mode 100644 tests/shell/testcases/bogons/nft-j-f/set_with_single_value_concat_assert
> 
> diff --git a/src/intervals.c b/src/intervals.c
> index 71ad210bf759..1ab443bcde87 100644
> --- a/src/intervals.c
> +++ b/src/intervals.c
> @@ -783,7 +783,9 @@ int setelem_to_interval(const struct set *set, struct expr *elem,
>  			next_key = NULL;
>  	}
>  
> -	assert(key->etype == EXPR_RANGE_VALUE);
> +	if (key->etype != EXPR_RANGE_VALUE)
> +		BUG("key must be RANGE_VALUE, not %s\n", expr_name(key));
> +
>  	assert(!next_key || next_key->etype == EXPR_RANGE_VALUE);
>  
>  	/* skip end element for adjacents intervals in anonymous sets. */
> diff --git a/src/parser_json.c b/src/parser_json.c
> index 94d09212314f..724cba881623 100644
> --- a/src/parser_json.c
> +++ b/src/parser_json.c
> @@ -1251,6 +1251,16 @@ static struct expr *json_parse_binop_expr(struct json_ctx *ctx,
>  	return binop_expr_alloc(int_loc, thisop, left, right);
>  }
>  
> +static struct expr *json_check_concat_expr(struct json_ctx *ctx, struct expr *e)
> +{
> +	if (e->size >= 2)
> +		return e;
> +
> +	json_error(ctx, "Concatenation with %d elements is illegal", e->size);
> +	expr_free(e);
> +	return NULL;
> +}
> +
>  static struct expr *json_parse_concat_expr(struct json_ctx *ctx,
>  					   const char *type, json_t *root)
>  {
> @@ -1284,7 +1294,7 @@ static struct expr *json_parse_concat_expr(struct json_ctx *ctx,
>  		}
>  		compound_expr_add(expr, tmp);
>  	}
> -	return expr;
> +	return expr ? json_check_concat_expr(ctx, expr) : NULL;
>  }
>  
>  static struct expr *json_parse_prefix_expr(struct json_ctx *ctx,
> @@ -1748,13 +1758,7 @@ static struct expr *json_parse_dtype_expr(struct json_ctx *ctx, json_t *root)
>  			compound_expr_add(expr, i);
>  		}
>  
> -		if (list_empty(&expr->expressions)) {
> -			json_error(ctx, "Empty concatenation");
> -			expr_free(expr);
> -			return NULL;
> -		}
> -
> -		return expr;
> +		return json_check_concat_expr(ctx, expr);
>  	} else if (json_is_object(root)) {
>  		const char *key;
>  		json_t *val;
> diff --git a/tests/shell/testcases/bogons/nft-j-f/set_with_single_value_concat_assert b/tests/shell/testcases/bogons/nft-j-f/set_with_single_value_concat_assert
> new file mode 100644
> index 000000000000..c99a26683470
> --- /dev/null
> +++ b/tests/shell/testcases/bogons/nft-j-f/set_with_single_value_concat_assert
> @@ -0,0 +1,26 @@
> +{
> +  "nftables": [
> +    {
> +  "metainfo": {
> +   "version": "nftables", "json_schema_version": 1
> +      }
> +    },
> +    {
> +      "table": {
> +	"family": "ip",
> +	"name": "t",
> +        "handle": 0
> +      }
> +    },
> +    {
> +      "set": {
> +        "family": "ip",
> +        "name": "s",
> +        "table": "t",
> +	"type": [ "ifname" ],
> +        "flags": [ "interval" ],
> +        "elem": [ [] ]
> +      }
> +    }
> +  ]
> +}
> -- 
> 2.49.0
> 
>