[PATCH nft] evaluate: do not crash on runaway number of concatenation components

[PATCH nft] evaluate: do not crash on runaway number of concatenation components

[Pablo Neira Ayuso]

Display error message in case user specifies more data components than
those defined by the concatenation of selectors.

 # cat example.nft
 table ip x {
        chain y {
                type filter hook prerouting priority 0; policy drop;
                ip saddr . meta mark { 1.2.3.4 . 0x00000100 . 1.2.3.6-1.2.3.8 } accept
        }
 }
 # nft -f example.nft
 example.nft:4:3-22: Error: too many concatenation components
                ip saddr . meta mark { 1.2.3.4 . 0x00000100 . 1.2.3.6-1.2.3.8 } accept
                ^^^^^^^^^^^^^^^^^^^^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Without this patch, nft crashes:

==464771==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60d000000418 at pc 0x7fbc17513aa5 bp 0x7ffc73d33c90 sp 0x7ffc73d33c88
READ of size 8 at 0x60d000000418 thread T0
    #0 0x7fbc17513aa4 in expr_evaluate_concat /home/pablo/devel/scm/git-netfilter/nftables/src/evaluate.c:1348
    #1 0x7fbc1752a9da in expr_evaluate /home/pablo/devel/scm/git-netfilter/nftables/src/evaluate.c:2476
    #2 0x7fbc175175e2 in expr_evaluate_set_elem /home/pablo/devel/scm/git-netfilter/nftables/src/evaluate.c:1504
    #3 0x7fbc1752aa22 in expr_evaluate /home/pablo/devel/scm/git-netfilter/nftables/src/evaluate.c:2482
    #4 0x7fbc17512cb5 in list_member_evaluate /home/pablo/devel/scm/git-netfilter/nftables/src/evaluate.c:1310
    #5 0x7fbc17518ca0 in expr_evaluate_set /home/pablo/devel/scm/git-netfilter/nftables/src/evaluate.c:1590
    [...]

Fixes: 64bb3f43bb96 ("src: allow to use typeof of raw expressions in set declaration")
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/evaluate.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/evaluate.c b/src/evaluate.c
index 991de1d7e977..0a77031341fd 100644
--- a/src/evaluate.c
+++ b/src/evaluate.c
@@ -1318,20 +1318,28 @@ static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
 	uint32_t type = dtype ? dtype->type : 0, ntype = 0;
 	int off = dtype ? dtype->subtypes : 0;
 	unsigned int flags = EXPR_F_CONSTANT | EXPR_F_SINGLETON;
+	const struct list_head *expressions;
 	struct expr *i, *next, *key = NULL;
 	const struct expr *key_ctx = NULL;
+	bool runaway = false;
 	uint32_t size = 0;
 
 	if (ctx->ectx.key && ctx->ectx.key->etype == EXPR_CONCAT) {
 		key_ctx = ctx->ectx.key;
 		assert(!list_empty(&ctx->ectx.key->expressions));
 		key = list_first_entry(&ctx->ectx.key->expressions, struct expr, list);
+		expressions = &ctx->ectx.key->expressions;
 	}
 
 	list_for_each_entry_safe(i, next, &(*expr)->expressions, list) {
 		enum byteorder bo = BYTEORDER_INVALID;
 		unsigned dsize_bytes, dsize = 0;
 
+		if (runaway) {
+			return expr_binary_error(ctx->msgs, key_ctx, *expr,
+						 "too many concatenation components");
+		}
+
 		if (i->etype == EXPR_CT &&
 		    (i->ct.key == NFT_CT_SRC ||
 		     i->ct.key == NFT_CT_DST))
@@ -1387,8 +1395,12 @@ static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
 		dsize_bytes = div_round_up(dsize, BITS_PER_BYTE);
 		(*expr)->field_len[(*expr)->field_count++] = dsize_bytes;
 		size += netlink_padded_len(dsize);
-		if (key)
+		if (key) {
+			if (list_is_last(&key->list, expressions))
+				runaway = true;
+
 			key = list_next_entry(key, list);
+		}
 	}
 
 	(*expr)->flags |= flags;
-- 
2.30.2

[PATCH nft] netlink: unfold function to generate concatenations for keys and data

[Pablo Neira Ayuso]

Add a specific function to generate concatenation with and without
intervals in maps. This restores the original function added by
8ac2f3b2fca3 ("src: Add support for concatenated set ranges") which is
used by 66746e7dedeb ("src: support for nat with interval
concatenation") to generate the data concatenations in maps.

Only the set element key requires the byteswap introduced by 1017d323cafa
("src: support for selectors with different byteorder with interval
concatenations"). Therefore, better not to reuse the same function for
key and data as the future might bring support for more kind of
concatenations in data maps.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/netlink.c | 45 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/src/netlink.c b/src/netlink.c
index 2ede25b9ce9d..026b599826ce 100644
--- a/src/netlink.c
+++ b/src/netlink.c
@@ -253,8 +253,8 @@ static int netlink_export_pad(unsigned char *data, const mpz_t v,
 	return netlink_padded_len(i->len) / BITS_PER_BYTE;
 }
 
-static int netlink_gen_concat_data_expr(uint32_t flags, const struct expr *i,
-					unsigned char *data)
+static int netlink_gen_concat_key(uint32_t flags, const struct expr *i,
+				  unsigned char *data)
 {
 	struct expr *expr;
 
@@ -316,12 +316,40 @@ static void __netlink_gen_concat(const struct expr *expr,
 	memset(data, 0, len);
 
 	list_for_each_entry(i, &expr->expressions, list)
-		offset += netlink_gen_concat_data_expr(expr->flags, i, data + offset);
+		offset += netlink_gen_concat_key(expr->flags, i, data + offset);
 
 	memcpy(nld->value, data, len);
 	nld->len = len;
 }
 
+static int netlink_gen_concat_data(int end, const struct expr *i,
+				   unsigned char *data)
+{
+	switch (i->etype) {
+	case EXPR_RANGE:
+		i = end ? i->right : i->left;
+		break;
+	case EXPR_PREFIX:
+		if (end) {
+			int count;
+			mpz_t v;
+
+			mpz_init_bitmask(v, i->len - i->prefix_len);
+			mpz_add(v, i->prefix->value, v);
+			count = netlink_export_pad(data, v, i);
+			mpz_clear(v);
+			return count;
+		}
+		return netlink_export_pad(data, i->prefix->value, i);
+	case EXPR_VALUE:
+		break;
+	default:
+		BUG("invalid expression type '%s' in set", expr_ops(i)->name);
+	}
+
+	return netlink_export_pad(data, i->value, i);
+}
+
 static void __netlink_gen_concat_expand(const struct expr *expr,
 				        struct nft_data_linearize *nld)
 {
@@ -332,18 +360,17 @@ static void __netlink_gen_concat_expand(const struct expr *expr,
 	memset(data, 0, len);
 
 	list_for_each_entry(i, &expr->expressions, list)
-		offset += netlink_gen_concat_data_expr(0, i, data + offset);
+		offset += netlink_gen_concat_data(false, i, data + offset);
 
 	list_for_each_entry(i, &expr->expressions, list)
-		offset += netlink_gen_concat_data_expr(EXPR_F_INTERVAL_END, i, data + offset);
+		offset += netlink_gen_concat_data(true, i, data + offset);
 
 	memcpy(nld->value, data, len);
 	nld->len = len;
 }
 
-static void netlink_gen_concat_data(const struct expr *expr,
-				    struct nft_data_linearize *nld,
-				    bool expand)
+static void netlink_gen_concat(const struct expr *expr,
+			       struct nft_data_linearize *nld, bool expand)
 {
 	if (expand)
 		__netlink_gen_concat_expand(expr, nld);
@@ -437,7 +464,7 @@ void __netlink_gen_data(const struct expr *expr,
 	case EXPR_VALUE:
 		return netlink_gen_constant_data(expr, data);
 	case EXPR_CONCAT:
-		return netlink_gen_concat_data(expr, data, expand);
+		return netlink_gen_concat(expr, data, expand);
 	case EXPR_VERDICT:
 		return netlink_gen_verdict(expr, data);
 	case EXPR_RANGE:
-- 
2.30.2

Re: [PATCH nft] netlink: unfold function to generate concatenations for keys and data

[Pablo Neira Ayuso]

On Thu, Dec 08, 2022 at 02:32:17AM +0100, Pablo Neira Ayuso wrote:
> Add a specific function to generate concatenation with and without
> intervals in maps. This restores the original function added by
> 8ac2f3b2fca3 ("src: Add support for concatenated set ranges") which is
> used by 66746e7dedeb ("src: support for nat with interval
> concatenation") to generate the data concatenations in maps.
> 
> Only the set element key requires the byteswap introduced by 1017d323cafa
> ("src: support for selectors with different byteorder with interval
> concatenations"). Therefore, better not to reuse the same function for
> key and data as the future might bring support for more kind of
> concatenations in data maps.

This patch needs more work, netlink_gen_concat_key() is still called
for rhs element data in a mapping in this patch.

I'll post a v2.