From 357f1e25e007771bb5faa783af7e73a584be9144 Mon Sep 17 00:00:00 2001 From: Florian Westphal Date: Fri, 14 Mar 2025 07:50:54 +0100 Subject: [PATCH] netlink: fix stack buffer overrun when emitting ranged expressions commit 37dfb1972cae061c09f278933af998a7c4fc2696 upstream. Included bogon input generates following Sanitizer splat: AddressSanitizer: dynamic-stack-buffer-overflow on address 0x7... WRITE of size 2 at 0x7fffffffcbe4 thread T0 #0 0x0000003a68b8 in __asan_memset (src/nft+0x3a68b8) (BuildId: 3678ff51a5405c77e3e0492b9a985910efee73b8) #1 0x0000004eb603 in __mpz_export_data src/gmputil.c:108:2 #2 0x0000004eb603 in netlink_export_pad src/netlink.c:256:2 #3 0x0000004eb603 in netlink_gen_range src/netlink.c:471:2 #4 0x0000004ea250 in __netlink_gen_data src/netlink.c:523:10 #5 0x0000004e8ee3 in alloc_nftnl_setelem src/netlink.c:205:3 #6 0x0000004d4541 in mnl_nft_setelem_batch src/mnl.c:1816:11 Problem is that the range end is emitted to the buffer at the *padded* location (rounded up to next register size), but buffer sizing is based of the expression length, not the padded length. Also extend the test script: Capture stderr and if we see AddressSanitizer warning, make it fail. Same bug as the one fixed in 600b84631410 ("netlink: fix stack buffer overflow with sub-reg sized prefixes"), just in a different function. Apply same fix: no dynamic array + add a range check. Joint work with Pablo Neira Ayuso. Signed-off-by: Florian Westphal --- src/netlink.c | 50 +++++++++++++------ tests/shell/testcases/bogons/assert_failures | 17 ++++++- ..._stack_buffer_overrun_in_netlink_gen_range | 6 +++ 3 files changed, 57 insertions(+), 16 deletions(-) create mode 100644 tests/shell/testcases/bogons/nft-f/asan_stack_buffer_overrun_in_netlink_gen_range diff --git a/src/netlink.c b/src/netlink.c index 10f3a901c..476301eae 100644 --- a/src/netlink.c +++ b/src/netlink.c @@ -334,11 +334,15 @@ static void nft_data_memcpy(struct nft_data_linearize *nld, static void netlink_gen_concat_key(const struct expr *expr, struct nft_data_linearize *nld) { - unsigned int len = expr->len / BITS_PER_BYTE, offset = 0; - unsigned char data[len]; + unsigned int len = netlink_padded_len(expr->len) / BITS_PER_BYTE; + unsigned char data[NFT_MAX_EXPR_LEN_BYTES]; + unsigned int offset = 0; const struct expr *i; - memset(data, 0, len); + if (len > sizeof(data)) + BUG("Value export of %u bytes would overflow", len); + + memset(data, 0, sizeof(data)); list_for_each_entry(i, &expr->expressions, list) offset += __netlink_gen_concat_key(expr->flags, i, data + offset); @@ -377,11 +381,15 @@ static int __netlink_gen_concat_data(int end, const struct expr *i, static void __netlink_gen_concat_expand(const struct expr *expr, struct nft_data_linearize *nld) { - unsigned int len = div_round_up(expr->len, BITS_PER_BYTE) * 2, offset = 0; - unsigned char data[len]; + unsigned int len = (netlink_padded_len(expr->len) / BITS_PER_BYTE) * 2; + unsigned char data[NFT_MAX_EXPR_LEN_BYTES]; + unsigned int offset = 0; const struct expr *i; - memset(data, 0, len); + if (len > sizeof(data)) + BUG("Value export of %u bytes would overflow", len); + + memset(data, 0, sizeof(data)); list_for_each_entry(i, &expr->expressions, list) offset += __netlink_gen_concat_data(false, i, data + offset); @@ -395,11 +403,15 @@ static void __netlink_gen_concat_expand(const struct expr *expr, static void __netlink_gen_concat(const struct expr *expr, struct nft_data_linearize *nld) { - unsigned int len = expr->len / BITS_PER_BYTE, offset = 0; - unsigned char data[len]; + unsigned int len = netlink_padded_len(expr->len) / BITS_PER_BYTE; + unsigned char data[NFT_MAX_EXPR_LEN_BYTES]; + unsigned int offset = 0; const struct expr *i; - memset(data, 0, len); + if (len > sizeof(data)) + BUG("Value export of %u bytes would overflow", len); + + memset(data, 0, sizeof(data)); list_for_each_entry(i, &expr->expressions, list) offset += __netlink_gen_concat_data(expr->flags, i, data + offset); @@ -466,11 +478,14 @@ static void netlink_gen_verdict(const struct expr *expr, static void netlink_gen_range(const struct expr *expr, struct nft_data_linearize *nld) { - unsigned int len = div_round_up(expr->left->len, BITS_PER_BYTE) * 2; - unsigned char data[len]; - unsigned int offset = 0; + unsigned int len = (netlink_padded_len(expr->left->len) / BITS_PER_BYTE) * 2; + unsigned char data[NFT_MAX_EXPR_LEN_BYTES]; + unsigned int offset; + + if (len > sizeof(data)) + BUG("Value export of %u bytes would overflow", len); - memset(data, 0, len); + memset(data, 0, sizeof(data)); offset = netlink_export_pad(data, expr->left->value, expr->left); netlink_export_pad(data + offset, expr->right->value, expr->right); nft_data_memcpy(nld, data, len); @@ -1218,10 +1233,15 @@ static struct expr *range_expr_reduce(struct expr *range) static struct expr *netlink_parse_interval_elem(const struct set *set, struct expr *expr) { - unsigned int len = div_round_up(expr->len, BITS_PER_BYTE); + unsigned int len = netlink_padded_len(expr->len) / BITS_PER_BYTE; const struct datatype *dtype = set->data->dtype; struct expr *range, *left, *right; - char data[len]; + char data[NFT_MAX_EXPR_LEN_BYTES]; + + if (len > sizeof(data)) + BUG("Value export of %u bytes would overflow", len); + + memset(data, 0, sizeof(data)); mpz_export_data(data, expr->value, dtype->byteorder, len); left = constant_expr_alloc(&internal_location, dtype, diff --git a/tests/shell/testcases/bogons/assert_failures b/tests/shell/testcases/bogons/assert_failures index 79099427c..3dee63b3f 100755 --- a/tests/shell/testcases/bogons/assert_failures +++ b/tests/shell/testcases/bogons/assert_failures @@ -1,12 +1,27 @@ #!/bin/bash dir=$(dirname $0)/nft-f/ +tmpfile=$(mktemp) + +cleanup() +{ + rm -f "$tmpfile" +} + +trap cleanup EXIT for f in $dir/*; do - $NFT --check -f "$f" + echo "Check $f" + $NFT --check -f "$f" 2> "$tmpfile" if [ $? -ne 1 ]; then echo "Bogus input file $f did not cause expected error code" 1>&2 exit 111 fi + + if grep AddressSanitizer "$tmpfile"; then + echo "Address sanitizer splat for $f" 1>&2 + cat "$tmpfile" + exit 111 + fi done diff --git a/tests/shell/testcases/bogons/nft-f/asan_stack_buffer_overrun_in_netlink_gen_range b/tests/shell/testcases/bogons/nft-f/asan_stack_buffer_overrun_in_netlink_gen_range new file mode 100644 index 000000000..2f7872e4a --- /dev/null +++ b/tests/shell/testcases/bogons/nft-f/asan_stack_buffer_overrun_in_netlink_gen_range @@ -0,0 +1,6 @@ +table ip test { + chain y { + redirect to :tcp dport map { 83 : 80/3, 84 :4 } + } +} + -- 2.47.2