[PATCH 1/2 nft] jump: Introduce chain_expr in jump and goto statements

[PATCH 1/2 nft] jump: Introduce chain_expr in jump and goto statements

[Fernando Fernandez Mancera]

Now we can introduce expressions as a chain in jump and goto statements. This
is going to be used to support variables as a chain in the following patches.

Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
---
 include/expression.h |  4 ++--
 src/datatype.c       | 10 ++++++++--
 src/evaluate.c       |  4 ++++
 src/expression.c     | 11 ++++++-----
 src/netlink.c        | 26 +++++++++++++++++++++-----
 src/parser_bison.y   | 16 ++++++++++++----
 6 files changed, 53 insertions(+), 18 deletions(-)

diff --git a/include/expression.h b/include/expression.h
index 6416ac0..ef41255 100644
--- a/include/expression.h
+++ b/include/expression.h
@@ -240,7 +240,7 @@ struct expr {
 		struct {
 			/* EXPR_VERDICT */
 			int			verdict;
-			const char		*chain;
+			struct expr		*chain;
 		};
 		struct {
 			/* EXPR_VALUE */
@@ -403,7 +403,7 @@ extern void relational_expr_pctx_update(struct proto_ctx *ctx,
 					const struct expr *expr);
 
 extern struct expr *verdict_expr_alloc(const struct location *loc,
-				       int verdict, const char *chain);
+				       int verdict, struct expr *chain);
 
 extern struct expr *symbol_expr_alloc(const struct location *loc,
 				      enum symbol_types type, struct scope *scope,
diff --git a/src/datatype.c b/src/datatype.c
index ac9f2af..6aaf9ea 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -254,6 +254,8 @@ const struct datatype invalid_type = {
 
 static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
 {
+	char chain[NFT_CHAIN_MAXNAMELEN];
+
 	switch (expr->verdict) {
 	case NFT_CONTINUE:
 		nft_print(octx, "continue");
@@ -262,10 +264,14 @@ static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
 		nft_print(octx, "break");
 		break;
 	case NFT_JUMP:
-		nft_print(octx, "jump %s", expr->chain);
+		mpz_export_data(chain, expr->chain->value,
+				BYTEORDER_HOST_ENDIAN, NFT_CHAIN_MAXNAMELEN);
+		nft_print(octx, "jump %s", chain);
 		break;
 	case NFT_GOTO:
-		nft_print(octx, "goto %s", expr->chain);
+		mpz_export_data(chain, expr->chain->value,
+				BYTEORDER_HOST_ENDIAN, NFT_CHAIN_MAXNAMELEN);
+		nft_print(octx, "goto %s", chain);
 		break;
 	case NFT_RETURN:
 		nft_print(octx, "return");
diff --git a/src/evaluate.c b/src/evaluate.c
index 21d9e14..8394037 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1947,6 +1947,10 @@ static int stmt_evaluate_verdict(struct eval_ctx *ctx, struct stmt *stmt)
 	case EXPR_VERDICT:
 		if (stmt->expr->verdict != NFT_CONTINUE)
 			stmt->flags |= STMT_F_TERMINAL;
+		if (stmt->expr->chain != NULL) {
+			if (expr_evaluate(ctx, &stmt->expr->chain) < 0)
+				return -1;
+		}
 		break;
 	case EXPR_MAP:
 		break;
diff --git a/src/expression.c b/src/expression.c
index eece12e..55a4ad7 100644
--- a/src/expression.c
+++ b/src/expression.c
@@ -207,17 +207,18 @@ static bool verdict_expr_cmp(const struct expr *e1, const struct expr *e2)
 
 	if ((e1->verdict == NFT_JUMP ||
 	     e1->verdict == NFT_GOTO) &&
-	    strcmp(e1->chain, e2->chain))
-		return false;
+	     (expr_basetype(e1) == expr_basetype(e2) &&
+	     !mpz_cmp(e1->value, e2->value)))
+		return true;
 
-	return true;
+	return false;
 }
 
 static void verdict_expr_clone(struct expr *new, const struct expr *expr)
 {
 	new->verdict = expr->verdict;
 	if (expr->chain != NULL)
-		new->chain = xstrdup(expr->chain);
+		mpz_init_set(new->chain->value, expr->chain->value);
 }
 
 static void verdict_expr_destroy(struct expr *expr)
@@ -236,7 +237,7 @@ static const struct expr_ops verdict_expr_ops = {
 };
 
 struct expr *verdict_expr_alloc(const struct location *loc,
-				int verdict, const char *chain)
+				int verdict, struct expr *chain)
 {
 	struct expr *expr;
 
diff --git a/src/netlink.c b/src/netlink.c
index c051ae6..ef12cb0 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -218,12 +218,17 @@ static void netlink_gen_constant_data(const struct expr *expr,
 static void netlink_gen_verdict(const struct expr *expr,
 				struct nft_data_linearize *data)
 {
+	char chain[NFT_CHAIN_MAXNAMELEN];
+
 	data->verdict = expr->verdict;
 
 	switch (expr->verdict) {
 	case NFT_JUMP:
 	case NFT_GOTO:
-		snprintf(data->chain, NFT_CHAIN_MAXNAMELEN, "%s", expr->chain);
+		mpz_export_data(chain, expr->chain->value,
+				BYTEORDER_HOST_ENDIAN,
+				NFT_CHAIN_MAXNAMELEN);
+		snprintf(data->chain, NFT_CHAIN_MAXNAMELEN, "%s", chain);
 		data->chain[NFT_CHAIN_MAXNAMELEN-1] = '\0';
 		break;
 	}
@@ -253,12 +258,15 @@ struct expr *netlink_alloc_value(const struct location *loc,
 static struct expr *netlink_alloc_verdict(const struct location *loc,
 					  const struct nft_data_delinearize *nld)
 {
-	char *chain;
+	struct expr *chain;
 
 	switch (nld->verdict) {
 	case NFT_JUMP:
 	case NFT_GOTO:
-		chain = xstrdup(nld->chain);
+		chain = constant_expr_alloc(loc, &string_type,
+					    BYTEORDER_HOST_ENDIAN,
+					    NFT_CHAIN_MAXNAMELEN *
+					    BITS_PER_BYTE, nld->chain);
 		break;
 	default:
 		chain = NULL;
@@ -1153,14 +1161,22 @@ static void trace_print_expr(const struct nftnl_trace *nlt, unsigned int attr,
 static void trace_print_verdict(const struct nftnl_trace *nlt,
 				 struct output_ctx *octx)
 {
+	struct expr *chain_expr = NULL;
 	const char *chain = NULL;
 	unsigned int verdict;
 	struct expr *expr;
 
 	verdict = nftnl_trace_get_u32(nlt, NFTNL_TRACE_VERDICT);
-	if (nftnl_trace_is_set(nlt, NFTNL_TRACE_JUMP_TARGET))
+	if (nftnl_trace_is_set(nlt, NFTNL_TRACE_JUMP_TARGET)) {
 		chain = xstrdup(nftnl_trace_get_str(nlt, NFTNL_TRACE_JUMP_TARGET));
-	expr = verdict_expr_alloc(&netlink_location, verdict, chain);
+		chain_expr = constant_expr_alloc(&netlink_location,
+						 &string_type,
+						 BYTEORDER_HOST_ENDIAN,
+						 NFT_CHAIN_MAXNAMELEN
+						 * BITS_PER_BYTE,
+						 chain);
+	}
+	expr = verdict_expr_alloc(&netlink_location, verdict, chain_expr);
 
 	nft_print(octx, "verdict ");
 	expr_print(expr, octx);
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 9e632c0..69b5773 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -618,8 +618,8 @@ int nft_lex(void *, void *, void *);
 %type <stmt>			meter_stmt meter_stmt_alloc flow_stmt_legacy_alloc
 %destructor { stmt_free($$); }	meter_stmt meter_stmt_alloc flow_stmt_legacy_alloc
 
-%type <expr>			symbol_expr verdict_expr integer_expr variable_expr
-%destructor { expr_free($$); }	symbol_expr verdict_expr integer_expr variable_expr
+%type <expr>			symbol_expr verdict_expr integer_expr variable_expr chain_expr
+%destructor { expr_free($$); }	symbol_expr verdict_expr integer_expr variable_expr chain_expr
 %type <expr>			primary_expr shift_expr and_expr
 %destructor { expr_free($$); }	primary_expr shift_expr and_expr
 %type <expr>			exclusive_or_expr inclusive_or_expr
@@ -3827,11 +3827,11 @@ verdict_expr		:	ACCEPT
 			{
 				$$ = verdict_expr_alloc(&@$, NFT_CONTINUE, NULL);
 			}
-			|	JUMP			identifier
+			|	JUMP			chain_expr
 			{
 				$$ = verdict_expr_alloc(&@$, NFT_JUMP, $2);
 			}
-			|	GOTO			identifier
+			|	GOTO			chain_expr
 			{
 				$$ = verdict_expr_alloc(&@$, NFT_GOTO, $2);
 			}
@@ -3841,6 +3841,14 @@ verdict_expr		:	ACCEPT
 			}
 			;
 
+chain_expr		:	identifier
+			{
+				$$ = constant_expr_alloc(&@$, &string_type,
+							 BYTEORDER_HOST_ENDIAN,
+							 NFT_NAME_MAXLEN * BITS_PER_BYTE, $1);
+			}
+			;
+
 meta_expr		:	META	meta_key
 			{
 				$$ = meta_expr_alloc(&@$, $2);
-- 
2.20.1

[PATCH 2/2 nft] jump: Allow goto and jump to a variable using nft input files

[Fernando Fernandez Mancera]

This patch introduces the use of nft input files variables in 'jump' and 'goto'
statements, e.g.

define dest = ber

add table ip foo
add chain ip foo bar {type filter hook input priority 0;}
add chain ip foo ber
add rule ip foo ber counter
add rule ip foo bar jump $dest

table ip foo {
	chain bar {
		type filter hook input priority filter; policy accept;
		jump ber
	}

	chain ber {
		counter packets 71 bytes 6664
	}
}

Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
---
 src/datatype.c     | 11 +++++++++++
 src/parser_bison.y |  6 +++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/src/datatype.c b/src/datatype.c
index 6aaf9ea..7e9ec5e 100644
--- a/src/datatype.c
+++ b/src/datatype.c
@@ -297,11 +297,22 @@ static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
 	}
 }
 
+static struct error_record *verdict_type_parse(const struct expr *sym,
+					       struct expr **res)
+{
+	*res = constant_expr_alloc(&sym->location, &string_type,
+				   BYTEORDER_HOST_ENDIAN,
+				   (strlen(sym->identifier) + 1) * BITS_PER_BYTE,
+				   sym->identifier);
+	return NULL;
+}
+
 const struct datatype verdict_type = {
 	.type		= TYPE_VERDICT,
 	.name		= "verdict",
 	.desc		= "netfilter verdict",
 	.print		= verdict_type_print,
+	.parse		= verdict_type_parse,
 };
 
 static const struct symbol_table nfproto_tbl = {
diff --git a/src/parser_bison.y b/src/parser_bison.y
index 69b5773..a955cb5 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -3841,7 +3841,11 @@ verdict_expr		:	ACCEPT
 			}
 			;
 
-chain_expr		:	identifier
+chain_expr		:	variable_expr
+			{
+				$$ = $1;
+			}
+			|	identifier
 			{
 				$$ = constant_expr_alloc(&@$, &string_type,
 							 BYTEORDER_HOST_ENDIAN,
-- 
2.20.1

Re: [PATCH 1/2 nft] jump: Introduce chain_expr in jump and goto statements

[Pablo Neira Ayuso]

On Tue, May 14, 2019 at 11:13:39PM +0200, Fernando Fernandez Mancera wrote:
[...]
> diff --git a/src/expression.c b/src/expression.c
> index eece12e..55a4ad7 100644
> --- a/src/expression.c
> +++ b/src/expression.c
> @@ -207,17 +207,18 @@ static bool verdict_expr_cmp(const struct expr *e1, const struct expr *e2)
>  
>  	if ((e1->verdict == NFT_JUMP ||
>  	     e1->verdict == NFT_GOTO) &&
> -	    strcmp(e1->chain, e2->chain))
> -		return false;
> +	     (expr_basetype(e1) == expr_basetype(e2) &&
> +	     !mpz_cmp(e1->value, e2->value)))

Maybe replace these two new lines above by:

              expr_cmp(e1->chain, e2->chain)

> +		return true;
>  
> -	return true;
> +	return false;
>  }
>  
>  static void verdict_expr_clone(struct expr *new, const struct expr *expr)
>  {
>  	new->verdict = expr->verdict;
>  	if (expr->chain != NULL)
> -		new->chain = xstrdup(expr->chain);
> +		mpz_init_set(new->chain->value, expr->chain->value);
>  }
>  
>  static void verdict_expr_destroy(struct expr *expr)

Memleak here in verdict_expr_destroy(), you need to call:

        expr_free(expr->chain);

instead of:

        xfree(expr->chain)

Please, run valgrind with --leak-check=full to make sure there are no
leaks either from adding a new rule nor when listing the ruleset.

Thanks!

Re: [PATCH 2/2 nft] jump: Allow goto and jump to a variable using nft input files

[Pablo Neira Ayuso]

On Tue, May 14, 2019 at 11:13:40PM +0200, Fernando Fernandez Mancera wrote:
> This patch introduces the use of nft input files variables in 'jump' and 'goto'
> statements, e.g.
> 
> define dest = ber
> 
> add table ip foo
> add chain ip foo bar {type filter hook input priority 0;}
> add chain ip foo ber
> add rule ip foo ber counter
> add rule ip foo bar jump $dest
> 
> table ip foo {
> 	chain bar {
> 		type filter hook input priority filter; policy accept;
> 		jump ber
> 	}
> 
> 	chain ber {
> 		counter packets 71 bytes 6664
> 	}
> }

Please, add a tests/shell/ script for this in your next patch version.

Thanks.

Re: [PATCH 2/2 nft] jump: Allow goto and jump to a variable using nft input files

[Phil Sutter]

Hi,

On Tue, May 14, 2019 at 11:13:40PM +0200, Fernando Fernandez Mancera wrote:
> This patch introduces the use of nft input files variables in 'jump' and 'goto'
> statements, e.g.
> 
> define dest = ber
> 
> add table ip foo
> add chain ip foo bar {type filter hook input priority 0;}
> add chain ip foo ber
> add rule ip foo ber counter
> add rule ip foo bar jump $dest
> 
> table ip foo {
> 	chain bar {
> 		type filter hook input priority filter; policy accept;
> 		jump ber
> 	}
> 
> 	chain ber {
> 		counter packets 71 bytes 6664
> 	}
> }
> 
> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
> ---
>  src/datatype.c     | 11 +++++++++++
>  src/parser_bison.y |  6 +++++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/src/datatype.c b/src/datatype.c
> index 6aaf9ea..7e9ec5e 100644
> --- a/src/datatype.c
> +++ b/src/datatype.c
> @@ -297,11 +297,22 @@ static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
>  	}
>  }
>  
> +static struct error_record *verdict_type_parse(const struct expr *sym,
> +					       struct expr **res)
> +{
> +	*res = constant_expr_alloc(&sym->location, &string_type,
> +				   BYTEORDER_HOST_ENDIAN,
> +				   (strlen(sym->identifier) + 1) * BITS_PER_BYTE,
> +				   sym->identifier);
> +	return NULL;
> +}
> +
>  const struct datatype verdict_type = {
>  	.type		= TYPE_VERDICT,
>  	.name		= "verdict",
>  	.desc		= "netfilter verdict",
>  	.print		= verdict_type_print,
> +	.parse		= verdict_type_parse,
>  };
>  
>  static const struct symbol_table nfproto_tbl = {
> diff --git a/src/parser_bison.y b/src/parser_bison.y
> index 69b5773..a955cb5 100644
> --- a/src/parser_bison.y
> +++ b/src/parser_bison.y
> @@ -3841,7 +3841,11 @@ verdict_expr		:	ACCEPT
>  			}
>  			;
>  
> -chain_expr		:	identifier
> +chain_expr		:	variable_expr
> +			{
> +				$$ = $1;
> +			}

Are you sure this is needed? The provided code should be what bison does
by default if no body was given.

Cheers, Phil

Re: [PATCH 2/2 nft] jump: Allow goto and jump to a variable using nft input files

[Fernando Fernandez Mancera]

Hi,

On 5/15/19 12:46 PM, Phil Sutter wrote:
> Hi,
> 
> On Tue, May 14, 2019 at 11:13:40PM +0200, Fernando Fernandez Mancera wrote:
>> This patch introduces the use of nft input files variables in 'jump' and 'goto'
>> statements, e.g.
>>
>> define dest = ber
>>
>> add table ip foo
>> add chain ip foo bar {type filter hook input priority 0;}
>> add chain ip foo ber
>> add rule ip foo ber counter
>> add rule ip foo bar jump $dest
>>
>> table ip foo {
>> 	chain bar {
>> 		type filter hook input priority filter; policy accept;
>> 		jump ber
>> 	}
>>
>> 	chain ber {
>> 		counter packets 71 bytes 6664
>> 	}
>> }
>>
>> Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
>> ---
>>  src/datatype.c     | 11 +++++++++++
>>  src/parser_bison.y |  6 +++++-
>>  2 files changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/datatype.c b/src/datatype.c
>> index 6aaf9ea..7e9ec5e 100644
>> --- a/src/datatype.c
>> +++ b/src/datatype.c
>> @@ -297,11 +297,22 @@ static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
>>  	}
>>  }
>>  
>> +static struct error_record *verdict_type_parse(const struct expr *sym,
>> +					       struct expr **res)
>> +{
>> +	*res = constant_expr_alloc(&sym->location, &string_type,
>> +				   BYTEORDER_HOST_ENDIAN,
>> +				   (strlen(sym->identifier) + 1) * BITS_PER_BYTE,
>> +				   sym->identifier);
>> +	return NULL;
>> +}
>> +
>>  const struct datatype verdict_type = {
>>  	.type		= TYPE_VERDICT,
>>  	.name		= "verdict",
>>  	.desc		= "netfilter verdict",
>>  	.print		= verdict_type_print,
>> +	.parse		= verdict_type_parse,
>>  };
>>  
>>  static const struct symbol_table nfproto_tbl = {
>> diff --git a/src/parser_bison.y b/src/parser_bison.y
>> index 69b5773..a955cb5 100644
>> --- a/src/parser_bison.y
>> +++ b/src/parser_bison.y
>> @@ -3841,7 +3841,11 @@ verdict_expr		:	ACCEPT
>>  			}
>>  			;
>>  
>> -chain_expr		:	identifier
>> +chain_expr		:	variable_expr
>> +			{
>> +				$$ = $1;
>> +			}
> 
> Are you sure this is needed? The provided code should be what bison does
> by default if no body was given.
> 

Yes, you are right! Thanks to point that. I am going to remove it in the
next patch series.

> Cheers, Phil
> 

Re: [PATCH 2/2 nft] jump: Allow goto and jump to a variable using nft input files

[Phil Sutter]

Hey,

On Tue, May 14, 2019 at 11:13:40PM +0200, Fernando Fernandez Mancera wrote:
[...]
> diff --git a/src/datatype.c b/src/datatype.c
> index 6aaf9ea..7e9ec5e 100644
> --- a/src/datatype.c
> +++ b/src/datatype.c
> @@ -297,11 +297,22 @@ static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
>  	}
>  }
>  
> +static struct error_record *verdict_type_parse(const struct expr *sym,
> +					       struct expr **res)
> +{
> +	*res = constant_expr_alloc(&sym->location, &string_type,
> +				   BYTEORDER_HOST_ENDIAN,
> +				   (strlen(sym->identifier) + 1) * BITS_PER_BYTE,
> +				   sym->identifier);
> +	return NULL;
> +}

One more thing: The above lacks error checking of any kind. I *think*
this is the place where one should make sure the symbol expression is
actually a string (but I'm not quite sure how you do that).

In any case, please try to exploit that variable support in the testcase
(or maybe a separate one), just to make sure we don't allow weird
things.

Thanks, Phil

Re: [PATCH 2/2 nft] jump: Allow goto and jump to a variable using nft input files

[Fernando Fernandez Mancera]

On 5/15/19 12:58 PM, Phil Sutter wrote:
> Hey,
> 
> On Tue, May 14, 2019 at 11:13:40PM +0200, Fernando Fernandez Mancera wrote:
> [...]
>> diff --git a/src/datatype.c b/src/datatype.c
>> index 6aaf9ea..7e9ec5e 100644
>> --- a/src/datatype.c
>> +++ b/src/datatype.c
>> @@ -297,11 +297,22 @@ static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
>>  	}
>>  }
>>  
>> +static struct error_record *verdict_type_parse(const struct expr *sym,
>> +					       struct expr **res)
>> +{
>> +	*res = constant_expr_alloc(&sym->location, &string_type,
>> +				   BYTEORDER_HOST_ENDIAN,
>> +				   (strlen(sym->identifier) + 1) * BITS_PER_BYTE,
>> +				   sym->identifier);
>> +	return NULL;
>> +}
> 
> One more thing: The above lacks error checking of any kind. I *think*
> this is the place where one should make sure the symbol expression is
> actually a string (but I'm not quite sure how you do that).
> 
> In any case, please try to exploit that variable support in the testcase
> (or maybe a separate one), just to make sure we don't allow weird
> things.
> 

I think I can get the symbol type and check if it is a string. I will
check this on the testcase as you said. Thanks!

> Thanks, Phil
> 

Re: [PATCH 2/2 nft] jump: Allow goto and jump to a variable using nft input files

[Pablo Neira Ayuso]

On Wed, May 15, 2019 at 01:02:05PM +0200, Fernando Fernandez Mancera wrote:
> 
> 
> On 5/15/19 12:58 PM, Phil Sutter wrote:
> > Hey,
> > 
> > On Tue, May 14, 2019 at 11:13:40PM +0200, Fernando Fernandez Mancera wrote:
> > [...]
> >> diff --git a/src/datatype.c b/src/datatype.c
> >> index 6aaf9ea..7e9ec5e 100644
> >> --- a/src/datatype.c
> >> +++ b/src/datatype.c
> >> @@ -297,11 +297,22 @@ static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
> >>  	}
> >>  }
> >>  
> >> +static struct error_record *verdict_type_parse(const struct expr *sym,
> >> +					       struct expr **res)
> >> +{
> >> +	*res = constant_expr_alloc(&sym->location, &string_type,
> >> +				   BYTEORDER_HOST_ENDIAN,
> >> +				   (strlen(sym->identifier) + 1) * BITS_PER_BYTE,
> >> +				   sym->identifier);
> >> +	return NULL;
> >> +}
> > 
> > One more thing: The above lacks error checking of any kind. I *think*
> > this is the place where one should make sure the symbol expression is
> > actually a string (but I'm not quite sure how you do that).
> > 
> > In any case, please try to exploit that variable support in the testcase
> > (or maybe a separate one), just to make sure we don't allow weird
> > things.
> > 
> 
> I think I can get the symbol type and check if it is a string. I will
> check this on the testcase as you said. Thanks!

There's not much we can do in this case I think, have a look at
string_type_parse().

Re: [PATCH 2/2 nft] jump: Allow goto and jump to a variable using nft input files

[Phil Sutter]

On Wed, May 15, 2019 at 01:12:32PM +0200, Pablo Neira Ayuso wrote:
> On Wed, May 15, 2019 at 01:02:05PM +0200, Fernando Fernandez Mancera wrote:
> > 
> > 
> > On 5/15/19 12:58 PM, Phil Sutter wrote:
> > > Hey,
> > > 
> > > On Tue, May 14, 2019 at 11:13:40PM +0200, Fernando Fernandez Mancera wrote:
> > > [...]
> > >> diff --git a/src/datatype.c b/src/datatype.c
> > >> index 6aaf9ea..7e9ec5e 100644
> > >> --- a/src/datatype.c
> > >> +++ b/src/datatype.c
> > >> @@ -297,11 +297,22 @@ static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
> > >>  	}
> > >>  }
> > >>  
> > >> +static struct error_record *verdict_type_parse(const struct expr *sym,
> > >> +					       struct expr **res)
> > >> +{
> > >> +	*res = constant_expr_alloc(&sym->location, &string_type,
> > >> +				   BYTEORDER_HOST_ENDIAN,
> > >> +				   (strlen(sym->identifier) + 1) * BITS_PER_BYTE,
> > >> +				   sym->identifier);
> > >> +	return NULL;
> > >> +}
> > > 
> > > One more thing: The above lacks error checking of any kind. I *think*
> > > this is the place where one should make sure the symbol expression is
> > > actually a string (but I'm not quite sure how you do that).
> > > 
> > > In any case, please try to exploit that variable support in the testcase
> > > (or maybe a separate one), just to make sure we don't allow weird
> > > things.
> > > 
> > 
> > I think I can get the symbol type and check if it is a string. I will
> > check this on the testcase as you said. Thanks!
> 
> There's not much we can do in this case I think, have a look at
> string_type_parse().

OK, maybe it's not as bad as I feared, symbol_parse() is called only if
we do have a symbol expr. Still I guess we should make sure nft
complains if the variable points to any other primary_expr or a set
reference ('@<something>').

Cheers, Phil

Re: [PATCH 2/2 nft] jump: Allow goto and jump to a variable using nft input files

[Pablo Neira Ayuso]

On Wed, May 15, 2019 at 01:46:17PM +0200, Phil Sutter wrote:
> On Wed, May 15, 2019 at 01:12:32PM +0200, Pablo Neira Ayuso wrote:
> > On Wed, May 15, 2019 at 01:02:05PM +0200, Fernando Fernandez Mancera wrote:
> > > 
> > > 
> > > On 5/15/19 12:58 PM, Phil Sutter wrote:
> > > > Hey,
> > > > 
> > > > On Tue, May 14, 2019 at 11:13:40PM +0200, Fernando Fernandez Mancera wrote:
> > > > [...]
> > > >> diff --git a/src/datatype.c b/src/datatype.c
> > > >> index 6aaf9ea..7e9ec5e 100644
> > > >> --- a/src/datatype.c
> > > >> +++ b/src/datatype.c
> > > >> @@ -297,11 +297,22 @@ static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
> > > >>  	}
> > > >>  }
> > > >>  
> > > >> +static struct error_record *verdict_type_parse(const struct expr *sym,
> > > >> +					       struct expr **res)
> > > >> +{
> > > >> +	*res = constant_expr_alloc(&sym->location, &string_type,
> > > >> +				   BYTEORDER_HOST_ENDIAN,
> > > >> +				   (strlen(sym->identifier) + 1) * BITS_PER_BYTE,
> > > >> +				   sym->identifier);
> > > >> +	return NULL;
> > > >> +}
> > > > 
> > > > One more thing: The above lacks error checking of any kind. I *think*
> > > > this is the place where one should make sure the symbol expression is
> > > > actually a string (but I'm not quite sure how you do that).
> > > > 
> > > > In any case, please try to exploit that variable support in the testcase
> > > > (or maybe a separate one), just to make sure we don't allow weird
> > > > things.
> > > > 
> > > 
> > > I think I can get the symbol type and check if it is a string. I will
> > > check this on the testcase as you said. Thanks!
> > 
> > There's not much we can do in this case I think, have a look at
> > string_type_parse().
> 
> OK, maybe it's not as bad as I feared, symbol_parse() is called only if
> we do have a symbol expr. Still I guess we should make sure nft
> complains if the variable points to any other primary_expr or a set
> reference ('@<something>').

'@<something>' is currently allowed, as any arbitrary string can be
placed in between strings - although in some way this is taking us
back to the quote debate that needs to be addressed. If we want to
disallow something enclosed in quotes then we'll have to apply this
function everywhere we allow variables.

Re: [PATCH 2/2 nft] jump: Allow goto and jump to a variable using nft input files

[Phil Sutter]

Hi Pablo,

On Wed, May 15, 2019 at 05:21:32PM +0200, Pablo Neira Ayuso wrote:
> On Wed, May 15, 2019 at 01:46:17PM +0200, Phil Sutter wrote:
> > On Wed, May 15, 2019 at 01:12:32PM +0200, Pablo Neira Ayuso wrote:
> > > On Wed, May 15, 2019 at 01:02:05PM +0200, Fernando Fernandez Mancera wrote:
> > > > 
> > > > 
> > > > On 5/15/19 12:58 PM, Phil Sutter wrote:
> > > > > Hey,
> > > > > 
> > > > > On Tue, May 14, 2019 at 11:13:40PM +0200, Fernando Fernandez Mancera wrote:
> > > > > [...]
> > > > >> diff --git a/src/datatype.c b/src/datatype.c
> > > > >> index 6aaf9ea..7e9ec5e 100644
> > > > >> --- a/src/datatype.c
> > > > >> +++ b/src/datatype.c
> > > > >> @@ -297,11 +297,22 @@ static void verdict_type_print(const struct expr *expr, struct output_ctx *octx)
> > > > >>  	}
> > > > >>  }
> > > > >>  
> > > > >> +static struct error_record *verdict_type_parse(const struct expr *sym,
> > > > >> +					       struct expr **res)
> > > > >> +{
> > > > >> +	*res = constant_expr_alloc(&sym->location, &string_type,
> > > > >> +				   BYTEORDER_HOST_ENDIAN,
> > > > >> +				   (strlen(sym->identifier) + 1) * BITS_PER_BYTE,
> > > > >> +				   sym->identifier);
> > > > >> +	return NULL;
> > > > >> +}
> > > > > 
> > > > > One more thing: The above lacks error checking of any kind. I *think*
> > > > > this is the place where one should make sure the symbol expression is
> > > > > actually a string (but I'm not quite sure how you do that).
> > > > > 
> > > > > In any case, please try to exploit that variable support in the testcase
> > > > > (or maybe a separate one), just to make sure we don't allow weird
> > > > > things.
> > > > > 
> > > > 
> > > > I think I can get the symbol type and check if it is a string. I will
> > > > check this on the testcase as you said. Thanks!
> > > 
> > > There's not much we can do in this case I think, have a look at
> > > string_type_parse().
> > 
> > OK, maybe it's not as bad as I feared, symbol_parse() is called only if
> > we do have a symbol expr. Still I guess we should make sure nft
> > complains if the variable points to any other primary_expr or a set
> > reference ('@<something>').
> 
> '@<something>' is currently allowed, as any arbitrary string can be
> placed in between strings - although in some way this is taking us
> back to the quote debate that needs to be addressed. If we want to
> disallow something enclosed in quotes then we'll have to apply this
> function everywhere we allow variables.

Oh, sorry. I put those ticks in there just to quote the value, not as
part of the value. The intention was to point out that something like:

| define foo = @set1
| add rule ip t c jump $foo

Might pass evaluation stage and since there is a special case for things
starting with '@' in symbol_expr, the added rule would turn into

| add rule ip t c jump set1

We could detect this situation by checking expr->symtype.

On the other hand, can we maybe check if given string points to an
*existing* chain in verdict_type_parse()? Or will that happen later
anyway?

Cheers, Phil

Re: [PATCH 2/2 nft] jump: Allow goto and jump to a variable using nft input files

[Fernando Fernandez Mancera]

Hi Phil,

On 5/15/19 9:26 PM, Phil Sutter wrote:
> Hi Pablo,
> 
> On Wed, May 15, 2019 at 05:21:32PM +0200, Pablo Neira Ayuso wrote:
>> On Wed, May 15, 2019 at 01:46:17PM +0200, Phil Sutter wrote>> [...]
>> '@<something>' is currently allowed, as any arbitrary string can be
>> placed in between strings - although in some way this is taking us
>> back to the quote debate that needs to be addressed. If we want to
>> disallow something enclosed in quotes then we'll have to apply this
>> function everywhere we allow variables.
> 
> Oh, sorry. I put those ticks in there just to quote the value, not as
> part of the value. The intention was to point out that something like:
> 
> | define foo = @set1
> | add rule ip t c jump $foo
> 
> Might pass evaluation stage and since there is a special case for things
> starting with '@' in symbol_expr, the added rule would turn into
> 
> | add rule ip t c jump set1
> 
> We could detect this situation by checking expr->symtype.
> 

I agree about that. We could check if the symbol type is SYMBOL_VALUE.
But I am not sure about where should we do it, maybe in the parser?

> On the other hand, can we maybe check if given string points to an
> *existing* chain in verdict_type_parse()? Or will that happen later
> anyway?
> 

It happens later, right now if the given string does not point to an
existing chain it returns the usual error for this situation. e.g

define dest = randomchain

table ip foo {
	chain bar {
		jump $dest
	}

	chain ber {
	}
}

test_file.nft:7:3-12: Error: Could not process rule: No such file or
directory
		jump $dest
		^^^^^^^^^^

> Cheers, Phil
> 

Thanks!

Re: [PATCH 2/2 nft] jump: Allow goto and jump to a variable using nft input files

[Phil Sutter]

Hi,

On Wed, May 15, 2019 at 09:56:11PM +0200, Fernando Fernandez Mancera wrote:
> Hi Phil,
> 
> On 5/15/19 9:26 PM, Phil Sutter wrote:
> > Hi Pablo,
> > 
> > On Wed, May 15, 2019 at 05:21:32PM +0200, Pablo Neira Ayuso wrote:
> >> On Wed, May 15, 2019 at 01:46:17PM +0200, Phil Sutter wrote>> [...]
> >> '@<something>' is currently allowed, as any arbitrary string can be
> >> placed in between strings - although in some way this is taking us
> >> back to the quote debate that needs to be addressed. If we want to
> >> disallow something enclosed in quotes then we'll have to apply this
> >> function everywhere we allow variables.
> > 
> > Oh, sorry. I put those ticks in there just to quote the value, not as
> > part of the value. The intention was to point out that something like:
> > 
> > | define foo = @set1
> > | add rule ip t c jump $foo
> > 
> > Might pass evaluation stage and since there is a special case for things
> > starting with '@' in symbol_expr, the added rule would turn into
> > 
> > | add rule ip t c jump set1
> > 
> > We could detect this situation by checking expr->symtype.
> > 
> 
> I agree about that. We could check if the symbol type is SYMBOL_VALUE.
> But I am not sure about where should we do it, maybe in the parser?
> 
> > On the other hand, can we maybe check if given string points to an
> > *existing* chain in verdict_type_parse()? Or will that happen later
> > anyway?
> > 
> 
> It happens later, right now if the given string does not point to an
> existing chain it returns the usual error for this situation. e.g

I just played around a bit and could provoke some segfaults:

* define foo = @set1 (a set named 'set1' must exist)
* define foo = { 1024 }
* define foo = *

I didn't check how we could avoid those. Maybe this is even follow-up
work, but we should definitely try to address those eventually.

Cheers, Phil

Re: [PATCH 2/2 nft] jump: Allow goto and jump to a variable using nft input files

[Fernando Fernandez Mancera]

Hi!

On 5/15/19 10:31 PM, Phil Sutter wrote:
> Hi,
> 
> On Wed, May 15, 2019 at 09:56:11PM +0200, Fernando Fernandez Mancera wrote:
>> Hi Phil,
>>
>> On 5/15/19 9:26 PM, Phil Sutter wrote:
>>> Hi Pablo,
>>>
>>> On Wed, May 15, 2019 at 05:21:32PM +0200, Pablo Neira Ayuso wrote:
>>>> On Wed, May 15, 2019 at 01:46:17PM +0200, Phil Sutter wrote>> [...]
>>>> '@<something>' is currently allowed, as any arbitrary string can be
>>>> placed in between strings - although in some way this is taking us
>>>> back to the quote debate that needs to be addressed. If we want to
>>>> disallow something enclosed in quotes then we'll have to apply this
>>>> function everywhere we allow variables.
>>>
>>> Oh, sorry. I put those ticks in there just to quote the value, not as
>>> part of the value. The intention was to point out that something like:
>>>
>>> | define foo = @set1
>>> | add rule ip t c jump $foo
>>>
>>> Might pass evaluation stage and since there is a special case for things
>>> starting with '@' in symbol_expr, the added rule would turn into
>>>
>>> | add rule ip t c jump set1
>>>
>>> We could detect this situation by checking expr->symtype.
>>>
>>
>> I agree about that. We could check if the symbol type is SYMBOL_VALUE.
>> But I am not sure about where should we do it, maybe in the parser?
>>
>>> On the other hand, can we maybe check if given string points to an
>>> *existing* chain in verdict_type_parse()? Or will that happen later
>>> anyway?
>>>
>>
>> It happens later, right now if the given string does not point to an
>> existing chain it returns the usual error for this situation. e.g
> 
> I just played around a bit and could provoke some segfaults:
> 
> * define foo = @set1 (a set named 'set1' must exist)
> * define foo = { 1024 }
> * define foo = *
> 
> I didn't check how we could avoid those. Maybe this is even follow-up
> work, but we should definitely try to address those eventually.
> 

I have been working on fixing this. I propose the following fix.

diff --git a/src/evaluate.c b/src/evaluate.c
index 8394037..edab370 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1950,6 +1950,12 @@ static int stmt_evaluate_verdict(struct eval_ctx
*ctx, struct stmt *stmt)
                if (stmt->expr->chain != NULL) {
                        if (expr_evaluate(ctx, &stmt->expr->chain) < 0)
                                return -1;
+                       if (stmt->expr->chain->etype != EXPR_SYMBOL ||
+                           stmt->expr->chain->symtype != SYMBOL_VALUE) {
+                               BUG("invalid verdict chain expression %s\n",
+                                   expr_name(stmt->expr->chain));
+                               return -1;
+                       }
                }
                break;
        case EXPR_MAP:

That works for all the cases that you mentioned above. What do you think?

Thanks :-)

> Cheers, Phil
> 

Re: [PATCH 2/2 nft] jump: Allow goto and jump to a variable using nft input files

[Pablo Neira Ayuso]

On Thu, May 16, 2019 at 01:58:17PM +0200, Fernando Fernandez Mancera wrote:
> Hi!
> 
> On 5/15/19 10:31 PM, Phil Sutter wrote:
> > Hi,
> > 
> > On Wed, May 15, 2019 at 09:56:11PM +0200, Fernando Fernandez Mancera wrote:
> >> Hi Phil,
> >>
> >> On 5/15/19 9:26 PM, Phil Sutter wrote:
> >>> Hi Pablo,
> >>>
> >>> On Wed, May 15, 2019 at 05:21:32PM +0200, Pablo Neira Ayuso wrote:
> >>>> On Wed, May 15, 2019 at 01:46:17PM +0200, Phil Sutter wrote>> [...]
> >>>> '@<something>' is currently allowed, as any arbitrary string can be
> >>>> placed in between strings - although in some way this is taking us
> >>>> back to the quote debate that needs to be addressed. If we want to
> >>>> disallow something enclosed in quotes then we'll have to apply this
> >>>> function everywhere we allow variables.
> >>>
> >>> Oh, sorry. I put those ticks in there just to quote the value, not as
> >>> part of the value. The intention was to point out that something like:
> >>>
> >>> | define foo = @set1
> >>> | add rule ip t c jump $foo
> >>>
> >>> Might pass evaluation stage and since there is a special case for things
> >>> starting with '@' in symbol_expr, the added rule would turn into
> >>>
> >>> | add rule ip t c jump set1
> >>>
> >>> We could detect this situation by checking expr->symtype.
> >>>
> >>
> >> I agree about that. We could check if the symbol type is SYMBOL_VALUE.
> >> But I am not sure about where should we do it, maybe in the parser?
> >>
> >>> On the other hand, can we maybe check if given string points to an
> >>> *existing* chain in verdict_type_parse()? Or will that happen later
> >>> anyway?
> >>>
> >>
> >> It happens later, right now if the given string does not point to an
> >> existing chain it returns the usual error for this situation. e.g
> > 
> > I just played around a bit and could provoke some segfaults:
> > 
> > * define foo = @set1 (a set named 'set1' must exist)
> > * define foo = { 1024 }
> > * define foo = *
> > 
> > I didn't check how we could avoid those. Maybe this is even follow-up
> > work, but we should definitely try to address those eventually.
> > 
> 
> I have been working on fixing this. I propose the following fix.
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index 8394037..edab370 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -1950,6 +1950,12 @@ static int stmt_evaluate_verdict(struct eval_ctx
> *ctx, struct stmt *stmt)
>                 if (stmt->expr->chain != NULL) {
>                         if (expr_evaluate(ctx, &stmt->expr->chain) < 0)
>                                 return -1;
> +                       if (stmt->expr->chain->etype != EXPR_SYMBOL ||
> +                           stmt->expr->chain->symtype != SYMBOL_VALUE) {
> +                               BUG("invalid verdict chain expression %s\n",
> +                                   expr_name(stmt->expr->chain));

Instead of BUG(), I'd suggest you do proper error reporting.

> +                               return -1;
> +                       }
>                 }
>                 break;
>         case EXPR_MAP:
> 
> That works for all the cases that you mentioned above. What do you think?
> 
> Thanks :-)
> 
> > Cheers, Phil
> > 

Re: [PATCH 2/2 nft] jump: Allow goto and jump to a variable using nft input files

[Fernando Fernandez Mancera]

El 16 de mayo de 2019 16:39:42 CEST, Pablo Neira Ayuso <pablo@netfilter.org> escribió:
>On Thu, May 16, 2019 at 01:58:17PM +0200, Fernando Fernandez Mancera
>wrote:
>> Hi!
>> 
>> On 5/15/19 10:31 PM, Phil Sutter wrote:
>> > Hi,
>> > 
>> > On Wed, May 15, 2019 at 09:56:11PM +0200, Fernando Fernandez
>Mancera wrote:
>> >> Hi Phil,
>> >>
>> >> On 5/15/19 9:26 PM, Phil Sutter wrote:
>> >>> Hi Pablo,
>> >>>
>> >>> On Wed, May 15, 2019 at 05:21:32PM +0200, Pablo Neira Ayuso
>wrote:
>> >>>> On Wed, May 15, 2019 at 01:46:17PM +0200, Phil Sutter wrote>>
>[...]
>> >>>> '@<something>' is currently allowed, as any arbitrary string can
>be
>> >>>> placed in between strings - although in some way this is taking
>us
>> >>>> back to the quote debate that needs to be addressed. If we want
>to
>> >>>> disallow something enclosed in quotes then we'll have to apply
>this
>> >>>> function everywhere we allow variables.
>> >>>
>> >>> Oh, sorry. I put those ticks in there just to quote the value,
>not as
>> >>> part of the value. The intention was to point out that something
>like:
>> >>>
>> >>> | define foo = @set1
>> >>> | add rule ip t c jump $foo
>> >>>
>> >>> Might pass evaluation stage and since there is a special case for
>things
>> >>> starting with '@' in symbol_expr, the added rule would turn into
>> >>>
>> >>> | add rule ip t c jump set1
>> >>>
>> >>> We could detect this situation by checking expr->symtype.
>> >>>
>> >>
>> >> I agree about that. We could check if the symbol type is
>SYMBOL_VALUE.
>> >> But I am not sure about where should we do it, maybe in the
>parser?
>> >>
>> >>> On the other hand, can we maybe check if given string points to
>an
>> >>> *existing* chain in verdict_type_parse()? Or will that happen
>later
>> >>> anyway?
>> >>>
>> >>
>> >> It happens later, right now if the given string does not point to
>an
>> >> existing chain it returns the usual error for this situation. e.g
>> > 
>> > I just played around a bit and could provoke some segfaults:
>> > 
>> > * define foo = @set1 (a set named 'set1' must exist)
>> > * define foo = { 1024 }
>> > * define foo = *
>> > 
>> > I didn't check how we could avoid those. Maybe this is even
>follow-up
>> > work, but we should definitely try to address those eventually.
>> > 
>> 
>> I have been working on fixing this. I propose the following fix.
>> 
>> diff --git a/src/evaluate.c b/src/evaluate.c
>> index 8394037..edab370 100644
>> --- a/src/evaluate.c
>> +++ b/src/evaluate.c
>> @@ -1950,6 +1950,12 @@ static int stmt_evaluate_verdict(struct
>eval_ctx
>> *ctx, struct stmt *stmt)
>>                 if (stmt->expr->chain != NULL) {
>>                         if (expr_evaluate(ctx, &stmt->expr->chain) <
>0)
>>                                 return -1;
>> +                       if (stmt->expr->chain->etype != EXPR_SYMBOL
>||
>> +                           stmt->expr->chain->symtype !=
>SYMBOL_VALUE) {
>> +                               BUG("invalid verdict chain expression
>%s\n",
>> +                                   expr_name(stmt->expr->chain));
>
>Instead of BUG(), I'd suggest you do proper error reporting.
>

Ok. I will send a new patch series. Thanks.

>> +                               return -1;
>> +                       }
>>                 }
>>                 break;
>>         case EXPR_MAP:
>> 
>> That works for all the cases that you mentioned above. What do you
>think?
>> 
>> Thanks :-)
>> 
>> > Cheers, Phil
>> >