Re: [PATCH nft 2/2] evaluate: fix crash when generating reject statement error

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

On Mon, Mar 31, 2025 at 02:43:34PM +0200, Florian Westphal wrote:
> After patch, this gets rejected with:
> internal:0:0-0: Error: conflicting protocols specified: ip vs ip6
> 
> Without patch, we crash with a NULL dereference: we cannot use
> reject.expr->location unconditionally.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>

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

Thanks!

> ---
>  src/evaluate.c                                | 16 ++++++++--
>  .../reject_stmt_with_no_expression_crash      | 32 +++++++++++++++++++
>  2 files changed, 46 insertions(+), 2 deletions(-)
>  create mode 100644 tests/shell/testcases/bogons/nft-j-f/reject_stmt_with_no_expression_crash
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index 507b1c86cafc..e4a7b5ceaafa 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -3791,6 +3791,18 @@ static int stmt_evaluate_reject_bridge(struct eval_ctx *ctx, struct stmt *stmt)
>  	return 0;
>  }
>  
> +static int stmt_reject_error(struct eval_ctx *ctx,
> +			     const struct stmt *stmt,
> +			     const char *msg)
> +{
> +	struct expr *e = stmt->reject.expr;
> +
> +	if (e)
> +		return stmt_binary_error(ctx, e, stmt, "%s", msg);
> +
> +	return stmt_error(ctx, stmt, "%s", msg);
> +}
> +
>  static int stmt_evaluate_reject_family(struct eval_ctx *ctx, struct stmt *stmt)
>  {
>  	struct proto_ctx *pctx = eval_proto_ctx(ctx);
> @@ -3806,12 +3818,12 @@ static int stmt_evaluate_reject_family(struct eval_ctx *ctx, struct stmt *stmt)
>  				return -1;
>  			break;
>  		case NFT_REJECT_ICMPX_UNREACH:
> -			return stmt_binary_error(ctx, stmt->reject.expr, stmt,
> +			return stmt_reject_error(ctx, stmt,
>  				   "abstracted ICMP unreachable not supported");
>  		case NFT_REJECT_ICMP_UNREACH:
>  			if (stmt->reject.family == pctx->family)
>  				break;
> -			return stmt_binary_error(ctx, stmt->reject.expr, stmt,
> +			return stmt_reject_error(ctx, stmt,
>  				  "conflicting protocols specified: ip vs ip6");
>  		}
>  		break;
> diff --git a/tests/shell/testcases/bogons/nft-j-f/reject_stmt_with_no_expression_crash b/tests/shell/testcases/bogons/nft-j-f/reject_stmt_with_no_expression_crash
> new file mode 100644
> index 000000000000..04c01aa77a29
> --- /dev/null
> +++ b/tests/shell/testcases/bogons/nft-j-f/reject_stmt_with_no_expression_crash
> @@ -0,0 +1,32 @@
> +{
> +  "nftables": [
> +    {
> +      "table": { "family": "ip", "name": "x",
> +        "handle": 0
> +      }
> +    },
> +    {
> +      "chain": {
> +        "family": "ip",
> +        "table": "x",
> +        "name": "c",
> +        "handle": 0
> +      }
> +    },
> +    {
> +      "rule": {
> +        "family": "ip",
> +        "table": "x",
> +        "chain": "c",
> +             "expr": [
> +          {
> +            "reject": {
> +              "type": "icmpv6",
> +              "exprlimit": "port-unreachable"
> +            }
> +          }
> +        ]
> +      }
> +    }
> +  ]
> +}
> -- 
> 2.49.0
> 
>