[PATCH nft] mnl: fix error rule reporting with missing table/chain and anonymous sets

[PATCH nft] mnl: fix error rule reporting with missing table/chain and anonymous sets

[Pablo Neira Ayuso]

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7f64f1e in erec_print (octx=0x55555555d2c0, erec=0x55555555fcf0, debug_mask=0) at erec.c:95
95              switch (indesc->type) {
(gdb) bt
    buf=0x55555555db20 "add rule inet traffic-filter input tcp dport { 22, 80, 443 } accept") at libnftables.c:459
(gdb) p indesc
$1 = (const struct input_descriptor *) 0x0

Closes: http://bugzilla.opensuse.org/show_bug.cgi?id=1171321
Fixes: 086ec6f30c96 ("mnl: extended error support for create command")
Reported-by: Jan Engelhardt <jengelh@inai.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/mnl.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/mnl.c b/src/mnl.c
index 94e80261afb7..9ce4072859b1 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -1048,7 +1048,10 @@ int mnl_nft_set_add(struct netlink_ctx *ctx, struct cmd *cmd,
 
 	cmd_add_loc(cmd, nlh->nlmsg_len, &h->table.location);
 	mnl_attr_put_strz(nlh, NFTA_SET_TABLE, h->table.name);
-	cmd_add_loc(cmd, nlh->nlmsg_len, &h->set.location);
+	if (set_is_anonymous(set->flags))
+		cmd_add_loc(cmd, nlh->nlmsg_len, &cmd->location);
+	else
+		cmd_add_loc(cmd, nlh->nlmsg_len, &h->set.location);
 	mnl_attr_put_strz(nlh, NFTA_SET_NAME, h->set.name);
 
 	nftnl_set_nlmsg_build_payload(nlh, nls);
-- 
2.20.1

Re: [PATCH nft] mnl: fix error rule reporting with missing table/chain and anonymous sets

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Program received signal SIGSEGV, Segmentation fault.
> 0x00007ffff7f64f1e in erec_print (octx=0x55555555d2c0, erec=0x55555555fcf0, debug_mask=0) at erec.c:95
> 95              switch (indesc->type) {
> (gdb) bt
>     buf=0x55555555db20 "add rule inet traffic-filter input tcp dport { 22, 80, 443 } accept") at libnftables.c:459
> (gdb) p indesc
> $1 = (const struct input_descriptor *) 0x0
> 
> Closes: http://bugzilla.opensuse.org/show_bug.cgi?id=1171321
> Fixes: 086ec6f30c96 ("mnl: extended error support for create command")
> Reported-by: Jan Engelhardt <jengelh@inai.de>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Yes, but there is something else going on.

The command above works without this patch if you use a shorter table name.
There is another bug that causes nft to pull the wrong error object
from the queue.

The kernel doesn't generate an error for NFTA_SET_NAME in the above
rule, so we should not crash even without this (correct) fix, because
nft should not find this particular error object.

Seems the generated error is for NFTA_SET_ELEM_LIST_TABLE when handling
nf_tables_newsetelem() in kernel (which makes sense, the table doesn't
exist).

With the above command (traffic-filter) NFTA_SET_NAMEs start offset
matches the offset of NFTA_SET_ELEM_LIST_TABLE error message in the
other netlink message (the one adding the element to the set), it will
erronously find the cmd_add_loc() of NFTA_SET_NAME and then barf because
of the bug fixed here.

Not sure how to fix nft_cmd_error(), it looks like the error queueing assumes
1:1 mapping of cmd struct and netlink message header...?

Re: [PATCH nft] mnl: fix error rule reporting with missing table/chain and anonymous sets

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1289 bytes --]

On Thu, May 07, 2020 at 04:11:48PM +0200, Florian Westphal wrote:
[...]
> Yes, but there is something else going on.
> 
> The command above works without this patch if you use a shorter table name.
> There is another bug that causes nft to pull the wrong error object
> from the queue.
> 
> The kernel doesn't generate an error for NFTA_SET_NAME in the above
> rule, so we should not crash even without this (correct) fix, because
> nft should not find this particular error object.
> 
> Seems the generated error is for NFTA_SET_ELEM_LIST_TABLE when handling
> nf_tables_newsetelem() in kernel (which makes sense, the table doesn't
> exist).
> 
> With the above command (traffic-filter) NFTA_SET_NAMEs start offset
> matches the offset of NFTA_SET_ELEM_LIST_TABLE error message in the
> other netlink message (the one adding the element to the set), it will
> erronously find the cmd_add_loc() of NFTA_SET_NAME and then barf because
> of the bug fixed here.
> 
> Not sure how to fix nft_cmd_error(), it looks like the error queueing assumes
> 1:1 mapping of cmd struct and netlink message header...?

Thanks for explaining.

I forgot anonymous sets are missing the expansion to achieve the 1:1
mapping between commands and netlink messages.

Please, have a look at attached sketch patch.

[-- Attachment #2: x.patch --]
[-- Type: text/x-diff, Size: 3040 bytes --]

diff --git a/include/rule.h b/include/rule.h
index 1a4ec3d8bc37..e11740d548ca 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -587,6 +587,7 @@ enum cmd_ops {
 enum cmd_obj {
 	CMD_OBJ_INVALID,
 	CMD_OBJ_SETELEM,
+	CMD_OBJ_SETELEM_ANONYMOUS,
 	CMD_OBJ_SET,
 	CMD_OBJ_SETS,
 	CMD_OBJ_RULE,
diff --git a/src/libnftables.c b/src/libnftables.c
index 32da0a29ee21..668e3fc43031 100644
--- a/src/libnftables.c
+++ b/src/libnftables.c
@@ -419,8 +419,12 @@ static int nft_evaluate(struct nft_ctx *nft, struct list_head *msgs,
 	if (nft->state->nerrs)
 		return -1;
 
-	list_for_each_entry(cmd, cmds, list)
+	list_for_each_entry(cmd, cmds, list) {
+		if (cmd->op != CMD_ADD)
+			continue;
+
 		nft_cmd_expand(cmd);
+	}
 
 	return 0;
 }
diff --git a/src/rule.c b/src/rule.c
index c58aa359259e..166c6c2befaf 100644
--- a/src/rule.c
+++ b/src/rule.c
@@ -1417,11 +1417,11 @@ void cmd_add_loc(struct cmd *cmd, uint16_t offset, struct location *loc)
 void nft_cmd_expand(struct cmd *cmd)
 {
 	struct list_head new_cmds;
+	struct set *set, *newset;
 	struct flowtable *ft;
 	struct table *table;
 	struct chain *chain;
 	struct rule *rule;
-	struct set *set;
 	struct obj *obj;
 	struct cmd *new;
 	struct handle h;
@@ -1477,6 +1477,22 @@ void nft_cmd_expand(struct cmd *cmd)
 		}
 		list_splice(&new_cmds, &cmd->list);
 		break;
+	case CMD_OBJ_SET:
+		set = cmd->set;
+		if (!set_is_anonymous(set->flags))
+			break;
+
+		memset(&h, 0, sizeof(h));
+		handle_merge(&h, &set->handle);
+		newset = set_clone(set);
+		newset->handle.set_id = set->handle.set_id;
+		newset->init = set->init;
+		set->init = NULL;
+		new = cmd_alloc(CMD_ADD, CMD_OBJ_SETELEM_ANONYMOUS, &h,
+				&set->location, newset);
+		list_add(&new->list, &cmd->list);
+		set->init = NULL;
+		break;
 	default:
 		break;
 	}
@@ -1525,6 +1541,7 @@ void cmd_free(struct cmd *cmd)
 			expr_free(cmd->expr);
 			break;
 		case CMD_OBJ_SET:
+		case CMD_OBJ_SETELEM_ANONYMOUS:
 			set_free(cmd->set);
 			break;
 		case CMD_OBJ_RULE:
@@ -1610,7 +1627,7 @@ static int do_add_setelems(struct netlink_ctx *ctx, struct cmd *cmd,
 }
 
 static int do_add_set(struct netlink_ctx *ctx, struct cmd *cmd,
-		      uint32_t flags)
+		      uint32_t flags, bool anonymous)
 {
 	struct set *set = cmd->set;
 
@@ -1621,7 +1638,7 @@ static int do_add_set(struct netlink_ctx *ctx, struct cmd *cmd,
 				     &ctx->nft->output) < 0)
 			return -1;
 	}
-	if (mnl_nft_set_add(ctx, cmd, flags) < 0)
+	if (!anonymous && mnl_nft_set_add(ctx, cmd, flags) < 0)
 		return -1;
 	if (set->init != NULL) {
 		return __do_add_setelems(ctx, set, set->init, flags);
@@ -1644,7 +1661,9 @@ static int do_command_add(struct netlink_ctx *ctx, struct cmd *cmd, bool excl)
 	case CMD_OBJ_RULE:
 		return mnl_nft_rule_add(ctx, cmd, flags | NLM_F_APPEND);
 	case CMD_OBJ_SET:
-		return do_add_set(ctx, cmd, flags);
+		return do_add_set(ctx, cmd, flags, false);
+	case CMD_OBJ_SETELEM_ANONYMOUS:
+		return do_add_set(ctx, cmd, flags, true);
 	case CMD_OBJ_SETELEM:
 		return do_add_setelems(ctx, cmd, flags);
 	case CMD_OBJ_COUNTER: