[PATCH nft 1/3] evaluate: Add set to cache only when well-formed

[PATCH nft 1/3] evaluate: Add set to cache only when well-formed

[Anatole Denis]

When creating a set (in set_evaluate), it is added to the table cache before
being checked for correctness. When the set is ill-formed, the function returns
without removing the (non-existent, since the function returned) set. Further
references to this set will not result in an error (since the set is in the
lookup table), but the malformed set will probably cause a segfault.

The symptom (the segfault) was fixed by checking for NULL when evaluating a
reference to the set (commit 5afa5a164ff1c066af1ec56d875b91562882bd50), this
should fix the root cause.

Signed-off-by: Anatole Denis <anatole@rezel.net>
---
 src/evaluate.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 8b113c8..b12af14 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -2550,9 +2550,6 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
 		return cmd_error(ctx, "Could not process rule: Table '%s' does not exist",
 				 ctx->cmd->handle.table);
 
-	if (set_lookup(table, set->handle.set) == NULL)
-		set_add_hash(set_get(set), table);
-
 	type = set->flags & SET_F_MAP ? "map" : "set";
 
 	if (set->keytype == NULL)
@@ -2583,6 +2580,9 @@ static int set_evaluate(struct eval_ctx *ctx, struct set *set)
 	}
 	ctx->set = NULL;
 
+	if (set_lookup(table, set->handle.set) == NULL)
+		set_add_hash(set_get(set), table);
+
 	/* Default timeout value implies timeout support */
 	if (set->timeout)
 		set->flags |= SET_F_TIMEOUT;
-- 
2.11.0.rc2

[PATCH nft 2/3] tests: Add regression test for malformed sets

[Anatole Denis]

see: 5afa5a164ff1c066af1ec56d875b91562882bd50
When a malformed set is added, it was added before erroring out, causing a
segfault further down when used. This tests for this case, ensuring that
nftables doesn't segfault but errors correctly

Signed-off-by: Anatole Denis <anatole@rezel.net>
---
 This test is maybe not that useful, but I used it to test that I didn't
 reintroduce the bug. Maybe a different naming rule for regression tests could
 be used ?

 .../sets/0014malformed_set_is_not_defined_0        | 33 ++++++++++++++++++++++
 1 file changed, 33 insertions(+)
 create mode 100755 tests/shell/testcases/sets/0014malformed_set_is_not_defined_0

diff --git a/tests/shell/testcases/sets/0014malformed_set_is_not_defined_0 b/tests/shell/testcases/sets/0014malformed_set_is_not_defined_0
new file mode 100755
index 0000000..720ec49
--- /dev/null
+++ b/tests/shell/testcases/sets/0014malformed_set_is_not_defined_0
@@ -0,0 +1,33 @@
+#!/bin/bash
+
+# This tests for the bug corrected in commit 5afa5a164ff1c066af1ec56d875b91562882bd50.
+# Sets were added to the table before checking for errors, and not removed from
+# the table on error, leading to an uninitialized set in the table, causing a
+# segfault for rules that tried to use it.
+# In this case, nft should error out because the set doesn't exist instead of
+# segfaulting
+
+tmpfile=$(mktemp)
+if [ ! -w $tmpfile ] ; then
+	echo "Failed to create tmp file" >&2
+	exit 0
+fi
+
+trap "rm -rf $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
+ret=$?
+
+trap - EXIT
+if [[ $ret -eq 1 ]]; then
+	exit 0
+else
+	exit 1
+fi
-- 
2.11.0.rc2

Re: [PATCH nft 2/3] tests: Add regression test for malformed sets

[Pablo Neira Ayuso]

On Mon, Nov 28, 2016 at 05:43:09PM +0100, Anatole Denis wrote:
> see: 5afa5a164ff1c066af1ec56d875b91562882bd50
> When a malformed set is added, it was added before erroring out, causing a
> segfault further down when used. This tests for this case, ensuring that
> nftables doesn't segfault but errors correctly
> 
> Signed-off-by: Anatole Denis <anatole@rezel.net>
> ---
>  This test is maybe not that useful, but I used it to test that I didn't
>  reintroduce the bug. Maybe a different naming rule for regression tests could
>  be used ?

Another test to increase coverage doesn't harm, so I'm applying this.

Thanks!

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

[Anatole Denis]

This reverts commit 5afa5a164ff1c066af1ec56d875b91562882bd50.
This commit is obsoleted by removing the possibility for a NULL right->dtype in
the first place, at set declaration.

Signed-off-by: Anatole Denis <anatole@rezel.net>
---
 src/evaluate.c | 31 ++++++++-----------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index b12af14..51d644f 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1467,33 +1467,18 @@ static int expr_evaluate_relational(struct eval_ctx *ctx, struct expr **expr)
 
 	switch (rel->op) {
 	case OP_LOOKUP:
-		switch (right->ops->type) {
-		case EXPR_SET:
-			/* A literal set expression implicitly declares
-			 * the set
-			 */
+		/* A literal set expression implicitly declares the set */
+		if (right->ops->type == EXPR_SET)
 			right = rel->right =
 				implicit_set_declaration(ctx, "__set%d",
 							 left->dtype,
 							 left->len, right);
-			break;
-		case EXPR_SET_REF:
-			if (right->dtype == NULL)
-				return expr_binary_error(ctx->msgs, right,
-							 left, "the referenced"
-							 " set does not "
-							 "exist");
-			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("Unknown expression %s\n", right->ops->name);
-		}
+		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);
 
 		/* Data for range lookups needs to be in big endian order */
 		if (right->set->flags & SET_F_INTERVAL &&
-- 
2.11.0.rc2

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

[Pablo Neira Ayuso]

On Mon, Nov 28, 2016 at 05:43:10PM +0100, Anatole Denis wrote:
> This reverts commit 5afa5a164ff1c066af1ec56d875b91562882bd50.
> This commit is obsoleted by removing the possibility for a NULL right->dtype in
> the first place, at set declaration.

Also applied, thanks.

Re: [PATCH nft 1/3] evaluate: Add set to cache only when well-formed

[Pablo Neira Ayuso]

On Mon, Nov 28, 2016 at 05:43:08PM +0100, Anatole Denis wrote:
> When creating a set (in set_evaluate), it is added to the table cache before
> being checked for correctness. When the set is ill-formed, the function returns
> without removing the (non-existent, since the function returned) set. Further
> references to this set will not result in an error (since the set is in the
> lookup table), but the malformed set will probably cause a segfault.
> 
> The symptom (the segfault) was fixed by checking for NULL when evaluating a
> reference to the set (commit 5afa5a164ff1c066af1ec56d875b91562882bd50), this
> should fix the root cause.

Applied, thanks.