[libnftnl PATCH 1/3] set: Fix for array overrun when setting NFTNL_SET_DESC_CONCAT

[libnftnl PATCH 1/3] set: Fix for array overrun when setting NFTNL_SET_DESC_CONCAT

[Phil Sutter]

Assuming max data_len of 16 * 4B and no zero bytes in 'data':
The while loop will increment field_count, use it as index for the
field_len array and afterwards make sure it hasn't increased to
NFT_REG32_COUNT. Thus a value of NFT_REG32_COUNT - 1 (= 15) will pass
the check, get incremented to 16 and used as index to the 16 fields long
array.
Use a less fancy for-loop to avoid the increment vs. check problem.

Fixes: 407f616ea5318 ("set: buffer overflow in NFTNL_SET_DESC_CONCAT setter")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/set.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/set.c b/src/set.c
index f127c19b7b8b8..5746397277c48 100644
--- a/src/set.c
+++ b/src/set.c
@@ -185,8 +185,10 @@ int nftnl_set_set_data(struct nftnl_set *s, uint16_t attr, const void *data,
 			return -1;
 
 		memcpy(&s->desc.field_len, data, data_len);
-		while (s->desc.field_len[++s->desc.field_count]) {
-			if (s->desc.field_count >= NFT_REG32_COUNT)
+		for (s->desc.field_count = 0;
+		     s->desc.field_count < NFT_REG32_COUNT;
+		     s->desc.field_count++) {
+			if (!s->desc.field_len[s->desc.field_count])
 				break;
 		}
 		break;
-- 
2.47.0

[libnftnl PATCH 2/3] tests: Extend set test by NFTNL_SET_DESC_CONCAT

[Phil Sutter]

Just to cover setter and getter code for that attribute.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tests/nft-set-test.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tests/nft-set-test.c b/tests/nft-set-test.c
index e264c735a2de6..1cb66e4a3ea64 100644
--- a/tests/nft-set-test.c
+++ b/tests/nft-set-test.c
@@ -21,6 +21,9 @@ static void print_err(const char *msg)
 
 static void cmp_nftnl_set(struct nftnl_set *a, struct nftnl_set *b)
 {
+	const uint8_t *data_a, *data_b;
+	uint32_t datalen_a, datalen_b;
+
 	if (strcmp(nftnl_set_get_str(a, NFTNL_SET_TABLE),
 		   nftnl_set_get_str(b, NFTNL_SET_TABLE)) != 0)
 		print_err("Set table mismatches");
@@ -45,11 +48,18 @@ static void cmp_nftnl_set(struct nftnl_set *a, struct nftnl_set *b)
 	if (strcmp(nftnl_set_get_str(a, NFTNL_SET_USERDATA),
 		   nftnl_set_get_str(b, NFTNL_SET_USERDATA)) != 0)
 		print_err("Set userdata mismatches");
+
+	data_a = nftnl_set_get_data(a, NFTNL_SET_DESC_CONCAT, &datalen_a);
+	data_b = nftnl_set_get_data(b, NFTNL_SET_DESC_CONCAT, &datalen_b);
+	if (datalen_a != datalen_b ||
+	    memcmp(data_a, data_b, datalen_a))
+		print_err("Set desc concat mismatches");
 }
 
 int main(int argc, char *argv[])
 {
 	struct nftnl_set *a, *b = NULL;
+	uint8_t field_lengths[16];
 	char buf[4096];
 	struct nlmsghdr *nlh;
 
@@ -68,6 +78,13 @@ int main(int argc, char *argv[])
 	nftnl_set_set_u32(a, NFTNL_SET_FAMILY, 0x12345678);
 	nftnl_set_set_str(a, NFTNL_SET_USERDATA, "testing user data");
 
+	memset(field_lengths, 0xff, sizeof(field_lengths));
+	if (!nftnl_set_set_data(a, NFTNL_SET_DESC_CONCAT, field_lengths, 17))
+		print_err("oversized NFTNL_SET_DESC_CONCAT data accepted");
+	if (nftnl_set_set_data(a, NFTNL_SET_DESC_CONCAT, field_lengths, 16))
+		print_err("setting NFTNL_SET_DESC_CONCAT failed");
+
+
 	/* cmd extracted from include/linux/netfilter/nf_tables.h */
 	nlh = nftnl_nlmsg_build_hdr(buf, NFT_MSG_NEWSET, AF_INET, 0, 1234);
 	nftnl_set_nlmsg_build_payload(nlh, a);
-- 
2.47.0

[libnftnl PATCH 3/3] tests: Fix for ASAN

[Phil Sutter]

The 'data' arrays in match and target expression tests were undersized
as they did not cover for the terminating NUL-char of the string used to
initialize them. When passing such array to strdup(), the latter reads
until after the defined array boundary.

Fixes: 93483364369d8 ("src: get rid of cached copies of x_tables.h and xt_LOG.h")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tests/nft-expr_match-test.c  | 2 +-
 tests/nft-expr_target-test.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/nft-expr_match-test.c b/tests/nft-expr_match-test.c
index 53a8b849c4847..bc9f6ac1b9ac8 100644
--- a/tests/nft-expr_match-test.c
+++ b/tests/nft-expr_match-test.c
@@ -54,7 +54,7 @@ int main(int argc, char *argv[])
 	char buf[4096];
 	struct nftnl_expr_iter *iter_a, *iter_b;
 	struct nftnl_expr *rule_a, *rule_b;
-	char data[16] = "0123456789abcdef";
+	char data[] = "0123456789abcdef";
 
 	a = nftnl_rule_alloc();
 	b = nftnl_rule_alloc();
diff --git a/tests/nft-expr_target-test.c b/tests/nft-expr_target-test.c
index 89de945e58348..a483e7ac24dd8 100644
--- a/tests/nft-expr_target-test.c
+++ b/tests/nft-expr_target-test.c
@@ -53,7 +53,7 @@ int main(int argc, char *argv[])
 	char buf[4096];
 	struct nftnl_expr_iter *iter_a, *iter_b;
 	struct nftnl_expr *rule_a, *rule_b;
-	char data[16] = "0123456789abcdef";
+	char data[] = "0123456789abcdef";
 
 	a = nftnl_rule_alloc();
 	b = nftnl_rule_alloc();
-- 
2.47.0

Re: [libnftnl PATCH 1/3] set: Fix for array overrun when setting NFTNL_SET_DESC_CONCAT

[Pablo Neira Ayuso]

Hi Phil,

On Wed, Nov 27, 2024 at 07:01:01PM +0100, Phil Sutter wrote:
> Assuming max data_len of 16 * 4B and no zero bytes in 'data':
> The while loop will increment field_count, use it as index for the
> field_len array and afterwards make sure it hasn't increased to
> NFT_REG32_COUNT. Thus a value of NFT_REG32_COUNT - 1 (= 15) will pass
> the check, get incremented to 16 and used as index to the 16 fields long
> array.
> Use a less fancy for-loop to avoid the increment vs. check problem.

for-loop is indeed better.

Patch LGTM, thanks.

> Fixes: 407f616ea5318 ("set: buffer overflow in NFTNL_SET_DESC_CONCAT setter")
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org>

> ---
>  src/set.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/set.c b/src/set.c
> index f127c19b7b8b8..5746397277c48 100644
> --- a/src/set.c
> +++ b/src/set.c
> @@ -185,8 +185,10 @@ int nftnl_set_set_data(struct nftnl_set *s, uint16_t attr, const void *data,
>  			return -1;
>  
>  		memcpy(&s->desc.field_len, data, data_len);
> -		while (s->desc.field_len[++s->desc.field_count]) {
> -			if (s->desc.field_count >= NFT_REG32_COUNT)
> +		for (s->desc.field_count = 0;
> +		     s->desc.field_count < NFT_REG32_COUNT;
> +		     s->desc.field_count++) {
> +			if (!s->desc.field_len[s->desc.field_count])
>  				break;
>  		}
>  		break;
> -- 
> 2.47.0
> 

Re: [libnftnl PATCH 1/3] set: Fix for array overrun when setting NFTNL_SET_DESC_CONCAT

[Phil Sutter]

On Wed, Dec 04, 2024 at 03:30:56PM +0100, Pablo Neira Ayuso wrote:
> Hi Phil,
> 
> On Wed, Nov 27, 2024 at 07:01:01PM +0100, Phil Sutter wrote:
> > Assuming max data_len of 16 * 4B and no zero bytes in 'data':
> > The while loop will increment field_count, use it as index for the
> > field_len array and afterwards make sure it hasn't increased to
> > NFT_REG32_COUNT. Thus a value of NFT_REG32_COUNT - 1 (= 15) will pass
> > the check, get incremented to 16 and used as index to the 16 fields long
> > array.
> > Use a less fancy for-loop to avoid the increment vs. check problem.
> 
> for-loop is indeed better.
> 
> Patch LGTM, thanks.
> 
> > Fixes: 407f616ea5318 ("set: buffer overflow in NFTNL_SET_DESC_CONCAT setter")
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> 
> Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org>

Series applied, thanks!