Re: [PATCH v2 nft] parser: tcpopt: fix tcp option parsing with NUM + length field

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

On Tue, Dec 05, 2023 at 12:56:08PM +0100, Florian Westphal wrote:
>   tcp option 254 length ge 4
> 
> ... will segfault.
> The crash bug is that tcpopt_expr_alloc() can return NULL if we cannot
> find a suitable template for the requested kind + field combination,
> so add the needed error handling in the bison parser.
> 
> However, we can handle this.  NOP and EOL have templates, all other
> options (known or unknown) must also have a length field.
> 
> So also add a fallback template to handle both kind and length, even
> if only a numeric option is given that nft doesn't recognize.
> 
> Don't bother with output, above will be printed via raw syntax, i.e.
> tcp option @254,8,8 >= 4.
> 
> Fixes: 24d8da308342 ("tcpopt: allow to check for presence of any tcp option")
> Reported-by: Maciej Żenczykowski <zenczykowski@gmail.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  v2: MUST bump exthdr.offset, else this continues to check for 'kind',
>  even if 'length' was asked for.
>  Also fix the dump file, it was not correct (254,0,8 instead of 254,8,8).
> 
>  src/parser_bison.y                            |  4 ++
>  src/tcpopt.c                                  | 28 +++++++++----
>  .../packetpath/dumps/tcp_options.nft          | 14 +++++++
>  tests/shell/testcases/packetpath/tcp_options  | 39 +++++++++++++++++++
>  4 files changed, 78 insertions(+), 7 deletions(-)
>  create mode 100644 tests/shell/testcases/packetpath/dumps/tcp_options.nft
>  create mode 100755 tests/shell/testcases/packetpath/tcp_options
> 
> diff --git a/src/parser_bison.y b/src/parser_bison.y
> index ee7e9e14c1f2..1a3d64f794cb 100644
> --- a/src/parser_bison.y
> +++ b/src/parser_bison.y
> @@ -5828,6 +5828,10 @@ tcp_hdr_expr		:	TCP	tcp_hdr_field
>  			|	TCP	OPTION	tcp_hdr_option_kind_and_field
>  			{
>  				$$ = tcpopt_expr_alloc(&@$, $3.kind, $3.field);
> +				if ($$ == NULL) {
> +					erec_queue(error(&@1, "Could not find a tcp option template"), state->msgs);
> +					YYERROR;
> +				}
>  			}
>  			|	TCP	OPTION	AT	close_scope_at	tcp_hdr_option_type	COMMA	NUM	COMMA	NUM
>  			{
> diff --git a/src/tcpopt.c b/src/tcpopt.c
> index 3fcb2731ae73..93b08c8cc0f2 100644
> --- a/src/tcpopt.c
> +++ b/src/tcpopt.c
> @@ -118,6 +118,13 @@ static const struct exthdr_desc tcpopt_mptcp = {
>  		[TCPOPT_MPTCP_SUBTYPE]  = PHT("subtype", 16, 4),
>  	},
>  };
> +
> +static const struct exthdr_desc tcpopt_fallback = {
> +	.templates	= {
> +		[TCPOPT_COMMON_KIND]	= PHT("kind",   0, 8),
> +		[TCPOPT_COMMON_LENGTH]	= PHT("length", 8, 8),
> +	},
> +};
>  #undef PHT
>  
>  const struct exthdr_desc *tcpopt_protocols[] = {
> @@ -182,19 +189,24 @@ struct expr *tcpopt_expr_alloc(const struct location *loc,
>  		desc = tcpopt_protocols[kind];
>  
>  	if (!desc) {
> -		if (field != TCPOPT_COMMON_KIND || kind > 255)
> +		if (kind > 255)
>  			return NULL;

Another suggestion: Remove this NULL, it leaves lhs as NULL in the
relational. kind > 255 cannot ever happen, parser rejects numbers over
255.

ruleset.nft:1:30-32: Error: value too large
add rule inet x y tcp option 256 length 4
                             ^^^

you could turn it into assert().

> +		switch (field) {
> +		case TCPOPT_COMMON_KIND:
> +		case TCPOPT_COMMON_LENGTH:
> +			break;
> +		default:
> +			return NULL;

Probably instead of this code above:

+		desc = &tcpopt_fallback;
+               if (field > TCPOPT_COMMON_LENGTH)
+                       tmpl = &tcpopt_unknown_template;
+               else
+                       tmpl = &desc->templates[field];

so no NULL is ever returned and fall back to the unknown template.

> +		}
> +
>  		expr = expr_alloc(loc, EXPR_EXTHDR, &integer_type,
>  				  BYTEORDER_BIG_ENDIAN, 8);
>  
> -		desc = tcpopt_protocols[TCPOPT_NOP];
> +		desc = &tcpopt_fallback;
>  		tmpl = &desc->templates[field];
> -		expr->exthdr.desc   = desc;
> -		expr->exthdr.tmpl   = tmpl;
> -		expr->exthdr.op = NFT_EXTHDR_OP_TCPOPT;
>  		expr->exthdr.raw_type = kind;
> -		return expr;
> +		goto out_finalize;

Maybe add a helper function to set up expr->exthdr to avoid the goto
trick. But not a deal breaker, it is OK if you prefer it this way.

Thanks!

>  	}
>  
>  	tmpl = &desc->templates[field];
> @@ -203,10 +215,12 @@ struct expr *tcpopt_expr_alloc(const struct location *loc,
>  
>  	expr = expr_alloc(loc, EXPR_EXTHDR, tmpl->dtype,
>  			  BYTEORDER_BIG_ENDIAN, tmpl->len);
> +
> +	expr->exthdr.raw_type = desc->type;
> +out_finalize:
>  	expr->exthdr.desc   = desc;
>  	expr->exthdr.tmpl   = tmpl;
>  	expr->exthdr.op     = NFT_EXTHDR_OP_TCPOPT;
> -	expr->exthdr.raw_type = desc->type;
>  	expr->exthdr.offset = tmpl->offset;
>  
>  	return expr;
> diff --git a/tests/shell/testcases/packetpath/dumps/tcp_options.nft b/tests/shell/testcases/packetpath/dumps/tcp_options.nft
> new file mode 100644
> index 000000000000..03e50a56e8c9
> --- /dev/null
> +++ b/tests/shell/testcases/packetpath/dumps/tcp_options.nft
> @@ -0,0 +1,14 @@
> +table inet t {
> +	chain c {
> +		type filter hook output priority filter; policy accept;
> +		tcp dport != 22345 accept
> +		tcp flags syn / fin,syn,rst,ack tcp option @254,8,8 >= 4 counter packets 0 bytes 0 drop
> +		tcp flags syn / fin,syn,rst,ack tcp option fastopen length >= 2 reset tcp option fastopen counter packets 0 bytes 0
> +		tcp flags syn / fin,syn,rst,ack tcp option sack-perm missing counter packets 0 bytes 0 drop
> +		tcp flags syn / fin,syn,rst,ack tcp option sack-perm exists counter packets 1 bytes 60
> +		tcp flags syn / fin,syn,rst,ack tcp option maxseg size > 1400 counter packets 1 bytes 60
> +		tcp flags syn / fin,syn,rst,ack tcp option nop missing counter packets 0 bytes 0
> +		tcp flags syn / fin,syn,rst,ack tcp option nop exists counter packets 1 bytes 60
> +		tcp flags syn / fin,syn,rst,ack drop
> +	}
> +}
> diff --git a/tests/shell/testcases/packetpath/tcp_options b/tests/shell/testcases/packetpath/tcp_options
> new file mode 100755
> index 000000000000..0f1ca2644655
> --- /dev/null
> +++ b/tests/shell/testcases/packetpath/tcp_options
> @@ -0,0 +1,39 @@
> +#!/bin/bash
> +
> +have_socat="no"
> +socat -h > /dev/null && have_socat="yes"
> +
> +ip link set lo up
> +
> +$NFT -f /dev/stdin <<EOF
> +table inet t {
> +	chain c {
> +		type filter hook output priority 0;
> +		tcp dport != 22345 accept
> +		tcp flags syn / fin,syn,rst,ack tcp option 254  length ge 4 counter drop
> +		tcp flags syn / fin,syn,rst,ack tcp option fastopen length ge 2 reset tcp option fastopen counter
> +		tcp flags syn / fin,syn,rst,ack tcp option sack-perm missing counter drop
> +		tcp flags syn / fin,syn,rst,ack tcp option sack-perm exists counter
> +		tcp flags syn / fin,syn,rst,ack tcp option maxseg size gt 1400 counter
> +		tcp flags syn / fin,syn,rst,ack tcp option nop missing counter
> +		tcp flags syn / fin,syn,rst,ack tcp option nop exists counter
> +		tcp flags syn / fin,syn,rst,ack drop
> +	}
> +}
> +EOF
> +
> +if [ $? -ne 0 ]; then
> +	exit 1
> +fi
> +
> +if [ $have_socat != "yes" ]; then
> +	echo "Ran partial test, socat not available (skipped)"
> +	exit 77
> +fi
> +
> +# This will fail (drop in output -> connect fails with eperm)
> +socat -t 3 -u STDIN TCP:127.0.0.1:22345,connect-timeout=1 < /dev/null > /dev/null
> +
> +# Indicate success, dump file has incremented packet counter where its
> +# expected to match.
> +exit 0
> -- 
> 2.41.0
> 
>