[nft PATCH 1/3] evaluate: check for NULL datatype in rhs in lookup expr

[nft PATCH 1/3] evaluate: check for NULL datatype in rhs in lookup expr

[Arturo Borrero Gonzalez]

If we are evaluating an EXPR_SET_REF, check if right->dtype is not NULL.
We can hit SEGFAULT if for whatever reason the referenced object does not
exists.

Using this testfile (note the invalid set syntax):

% cat test.nft
flush ruleset
add table t
add chain t c
add set t s {type ipv4_addr\;}
add rule t c ip saddr @s

Without this patch:

% nft -f test.nft
Segmentation fault

With this patch:

% nft -f test.nft
t.nft:4:28-28: Error: syntax error, unexpected junk, expecting newline or semicolon
add set t s {type ipv4_addr\;}
                           ^
t.nft:4:13-29: Error: set definition does not specify key data type
add set t s {type ipv4_addr\;}
            ^^^^^^^^^^^^^^^^^
t.nft:5:23-24: Error: the referenced object does not exists
add rule t c ip saddr @s
             ~~~~~~~~ ^^

Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
 src/evaluate.c |   35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 7444d09..6840790 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1210,16 +1210,33 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 
 	switch (rel->op) {
 	case OP_LOOKUP:
-		/* A literal set expression implicitly declares the set */
-		if (right->ops->type == EXPR_SET)
+		switch (right->ops->type) {
+		case EXPR_SET:
+			/* A literal set expression implicitly declares
+			 * the set
+			 */
 			right = rel->right =
-				implicit_set_declaration(ctx, left->dtype, left->len, right);
-		else if (!datatype_equal(left->dtype, right->dtype))
-			return expr_binary_error(ctx->msgs, right, left,
-						 "datatype mismatch, expected %s, "
-						 "set has type %s",
-						 left->dtype->desc,
-						 right->dtype->desc);
+				implicit_set_declaration(ctx, left->dtype,
+							 left->len, right);
+			break;
+		case EXPR_SET_REF:
+			if (right->dtype == NULL)
+				return expr_binary_error(ctx->msgs, right,
+							 left, "the referenced"
+							 " object does not "
+							 "exists");
+			if (!datatype_equal(left->dtype, right->dtype))
+				return expr_binary_error(ctx->msgs, right,
+							 left, "datatype "
+							 "mismatch, expected "
+							 "%s, set has type %s",
+							 left->dtype->desc,
+							 right->dtype->desc);
+			break;
+		default:
+			BUG("unhandled right expression type %u\n",
+			    right->ops->type);
+		}
 
 		/* Data for range lookups needs to be in big endian order */
 		if (right->set->flags & SET_F_INTERVAL &&

[nft PATCH 2/3] tests/shell: add testcase for 'nft -f' load with actions

[Arturo Borrero Gonzalez]

Let's tests loading a ruleset with actions.

Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
 tests/shell/testcases/nft-f/0006action_object_0 |   68 +++++++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100755 tests/shell/testcases/nft-f/0006action_object_0

diff --git a/tests/shell/testcases/nft-f/0006action_object_0 b/tests/shell/testcases/nft-f/0006action_object_0
new file mode 100755
index 0000000..f4ec41d
--- /dev/null
+++ b/tests/shell/testcases/nft-f/0006action_object_0
@@ -0,0 +1,68 @@
+#!/bin/bash
+
+# test loading a ruleset with the 'action object' pattern
+
+tmpfile=$(mktemp)
+if [ ! -w $tmpfile ] ; then
+	echo "Failed to create tmp file" >&2
+	exit 0
+fi
+
+trap "rm -f $tmpfile" EXIT # cleanup if aborted
+
+set -e
+
+FAMILIES="ip ip6 inet arp bridge"
+
+generate1()
+{
+	local family=$1
+	echo "
+	add table $family t
+	add chain $family t c
+	add rule $family t c accept
+	add set $family t s {type inet_service;}
+	add element $family t s {8080}
+	insert rule $family t c meta l4proto tcp tcp dport @s accept
+	replace rule $family t c handle 2 meta l4proto tcp tcp dport {9090}
+	add map $family t m {type inet_service:verdict;}
+	add element $family t m {10080:drop}
+	insert rule $family t c meta l4proto tcp tcp dport vmap @m
+	add rule $family t c meta l4proto udp udp sport vmap {1111:accept}
+	" >> $tmpfile
+}
+
+generate2()
+{
+	local family=$1
+	echo "
+	flush chain $family t c
+	delete element $family t m {10080:drop}
+	delete element $family t s {8080}
+	delete chain $family t c
+	delete table $family t
+	" >> $tmpfile
+}
+
+for family in $FAMILIES ; do
+	generate1 $family
+done
+
+$NFT -f $tmpfile
+if [ $? -ne 0 ] ; then
+	echo "E: unable to load ruleset 1" >&2
+	exit 1
+fi
+
+echo "" > $tmpfile
+for family in $FAMILIES ; do
+	generate2 $family
+done
+
+$NFT -f $tmpfile
+if [ $? -ne 0 ] ; then
+	echo "E: unable to load ruleset 2" >&2
+	exit 1
+fi
+
+exit 0

[nft PATCH 3/3] tests/shell: add testcase to catch segfault if invalid syntax was used

[Arturo Borrero Gonzalez]

Let's add

Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
---
 .../nft-f/0007action_object_set_segfault_1         |   21 ++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100755 tests/shell/testcases/nft-f/0007action_object_set_segfault_1

diff --git a/tests/shell/testcases/nft-f/0007action_object_set_segfault_1 b/tests/shell/testcases/nft-f/0007action_object_set_segfault_1
new file mode 100755
index 0000000..3a4183b
--- /dev/null
+++ b/tests/shell/testcases/nft-f/0007action_object_set_segfault_1
@@ -0,0 +1,21 @@
+#!/bin/bash
+
+# test for a segfault if bad syntax was used in set declaration
+# and the set is referenced in the same batch
+
+tmpfile=$(mktemp)
+if [ ! -w $tmpfile ] ; then
+	echo "Failed to create tmp file" >&2
+	exit 0
+fi
+
+trap "rm -f $tmpfile" EXIT # cleanup if aborted
+
+echo "
+add table t
+add chain t c
+add set t s {type ipv4_addr\;}
+add rule t c ip saddr @s
+" > $tmpfile
+
+$NFT -f $tmpfile 2>/dev/null

Re: [nft PATCH 1/3] evaluate: check for NULL datatype in rhs in lookup expr

[Pablo Neira Ayuso]

On Wed, May 11, 2016 at 01:30:02PM +0200, Arturo Borrero Gonzalez wrote:
> If we are evaluating an EXPR_SET_REF, check if right->dtype is not NULL.
> We can hit SEGFAULT if for whatever reason the referenced object does not
> exists.
> 
> Using this testfile (note the invalid set syntax):
> 
> % cat test.nft
> flush ruleset
> add table t
> add chain t c
> add set t s {type ipv4_addr\;}
> add rule t c ip saddr @s
> 
> Without this patch:
> 
> % nft -f test.nft
> Segmentation fault
> 
> With this patch:
> 
> % nft -f test.nft
> t.nft:4:28-28: Error: syntax error, unexpected junk, expecting newline or semicolon
> add set t s {type ipv4_addr\;}
>                            ^
> t.nft:4:13-29: Error: set definition does not specify key data type
> add set t s {type ipv4_addr\;}
>             ^^^^^^^^^^^^^^^^^
> t.nft:5:23-24: Error: the referenced object does not exists

I have reworded this to: "the referenced set does not exist"

> add rule t c ip saddr @s
>              ~~~~~~~~ ^^

Applied, thanks Arturo.

> Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com>
> ---
>  src/evaluate.c |   35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/src/evaluate.c b/src/evaluate.c
> index 7444d09..6840790 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -1210,16 +1210,33 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
>  
>  	switch (rel->op) {
>  	case OP_LOOKUP:
> -		/* A literal set expression implicitly declares the set */
> -		if (right->ops->type == EXPR_SET)
> +		switch (right->ops->type) {
> +		case EXPR_SET:
> +			/* A literal set expression implicitly declares
> +			 * the set
> +			 */
>  			right = rel->right =
> -				implicit_set_declaration(ctx, left->dtype, left->len, right);
> -		else if (!datatype_equal(left->dtype, right->dtype))
> -			return expr_binary_error(ctx->msgs, right, left,
> -						 "datatype mismatch, expected %s, "
> -						 "set has type %s",
> -						 left->dtype->desc,
> -						 right->dtype->desc);
> +				implicit_set_declaration(ctx, left->dtype,
> +							 left->len, right);
> +			break;
> +		case EXPR_SET_REF:
> +			if (right->dtype == NULL)
> +				return expr_binary_error(ctx->msgs, right,
> +							 left, "the referenced"
> +							 " object does not "
> +							 "exists");
> +			if (!datatype_equal(left->dtype, right->dtype))
> +				return expr_binary_error(ctx->msgs, right,
> +							 left, "datatype "
> +							 "mismatch, expected "
> +							 "%s, set has type %s",
> +							 left->dtype->desc,
> +							 right->dtype->desc);
> +			break;
> +		default:
> +			BUG("unhandled right expression type %u\n",
> +			    right->ops->type);

I have also replaced this by the typical:

                       BUG("Unknown expression %s\n", right->ops->name);

Re: [nft PATCH 2/3] tests/shell: add testcase for 'nft -f' load with actions

[Pablo Neira Ayuso]

On Wed, May 11, 2016 at 01:30:08PM +0200, Arturo Borrero Gonzalez wrote:
> Let's tests loading a ruleset with actions.

Also applied.

Re: [nft PATCH 3/3] tests/shell: add testcase to catch segfault if invalid syntax was used

[Pablo Neira Ayuso]

On Wed, May 11, 2016 at 01:30:13PM +0200, Arturo Borrero Gonzalez wrote:
> Let's add

Applied.

I have included in the commit log message that this refers to
"evaluate: check for NULL datatype in rhs in lookup expr".

Re: [nft PATCH 1/3] evaluate: check for NULL datatype in rhs in lookup expr

[Arturo Borrero Gonzalez]

On 13 May 2016 at 11:38, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, May 11, 2016 at 01:30:02PM +0200, Arturo Borrero Gonzalez wrote:
>> If we are evaluating an EXPR_SET_REF, check if right->dtype is not NULL.
>> We can hit SEGFAULT if for whatever reason the referenced object does not
>> exists.
>>
>> Using this testfile (note the invalid set syntax):
>>
>> % cat test.nft
>> flush ruleset
>> add table t
>> add chain t c
>> add set t s {type ipv4_addr\;}
>> add rule t c ip saddr @s
>>
>> Without this patch:
>>
>> % nft -f test.nft
>> Segmentation fault
>>
>> With this patch:
>>
>> % nft -f test.nft
>> t.nft:4:28-28: Error: syntax error, unexpected junk, expecting newline or semicolon
>> add set t s {type ipv4_addr\;}
>>                            ^
>> t.nft:4:13-29: Error: set definition does not specify key data type
>> add set t s {type ipv4_addr\;}
>>             ^^^^^^^^^^^^^^^^^
>> t.nft:5:23-24: Error: the referenced object does not exists
>
> I have reworded this to: "the referenced set does not exist"
>

Ok, I used the generic word 'object' because this could apply to maps as well.

>> add rule t c ip saddr @s
>>              ~~~~~~~~ ^^
>
> Applied, thanks Arturo.

thanks

-- 
Arturo Borrero González
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [nft PATCH 3/3] tests/shell: add testcase to catch segfault if invalid syntax was used

[Arturo Borrero Gonzalez]

On 13 May 2016 at 11:40, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, May 11, 2016 at 01:30:13PM +0200, Arturo Borrero Gonzalez wrote:
>> Let's add
>
> Applied.
>
> I have included in the commit log message that this refers to
> "evaluate: check for NULL datatype in rhs in lookup expr".

Oops, thanks, my fault.

-- 
Arturo Borrero González
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html