diff mbox series

[OpenWrt-Devel,RFC,libubox] blobmsg: another attrs iteration fix for blobmsg_check_array_len()

Message ID 20200525083106.30473-1-zajec5@gmail.com
State RFC
Headers show
Series [OpenWrt-Devel,RFC,libubox] blobmsg: another attrs iteration fix for blobmsg_check_array_len() | expand

Commit Message

Rafał Miłecki May 25, 2020, 8:31 a.m. UTC 
From: Rafał Miłecki <rafal@milecki.pl>

After more reviews is seems that blobmsg_for_each_attr() should not be
used when dealing with untrusted data as it reads length from blob data
itself. It means it can't be used in the blobmsg_check_array_len().

Switch back to using __blobmsg_for_each_attr() BUT pass correct length
to it. Calculate it by subtracting header length from blob length.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 blobmsg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Felix Fietkau May 25, 2020, 8:51 a.m. UTC | #1 
On 2020-05-25 10:31, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> After more reviews is seems that blobmsg_for_each_attr() should not be
> used when dealing with untrusted data as it reads length from blob data
> itself. It means it can't be used in the blobmsg_check_array_len().
> 
> Switch back to using __blobmsg_for_each_attr() BUT pass correct length
> to it. Calculate it by subtracting header length from blob length.
This should not be necessary, because the length is validated in the
blobmsg_check_attr_len call earlier in the same function.
I think your previous fix is completely fine as-is.

- Felix
Petr Štetiar May 25, 2020, 9:51 a.m. UTC | #2 
Felix Fietkau <nbd@nbd.name> [2020-05-25 10:51:36]:

Hi,

> I think your previous fix is completely fine as-is.

just FYI Rafał's fix triggered fuzzer CI failure[1], regression, I'm able to
reproduce it localy so it's not false positive. So perhaps there's some
additional check missing somewhere?

 $ echo AQQAAAABAAA1JQ== | base64 -d > ./tests/fuzz/corpus/crash-e0f8ecc694d96a09a1fced27b2a0838b670d34a0

 $ xxd ./tests/fuzz/corpus/crash-e0f8ecc694d96a09a1fced27b2a0838b670d34a0
 00000000: 0104 0000 0001 0000 3525                 ........5%

 $ gdb --args ./build/tests/fuzz/test-fuzz ./tests/fuzz/corpus/crash-e0f8ecc694d96a09a1fced27b2a0838b670d34a0
 Reading symbols from ./build/tests/fuzz/test-fuzz...done.

 (gdb) r

 Thread 1 "test-fuzz" received signal SIGSEGV, Segmentation fault.
 0x00007ffff70be2fc in blob_len (attr=0x6020000100d4) at libubox/blob.h:102
 102             return (be32_to_cpu(attr->id_len) & BLOB_ATTR_LEN_MASK) - sizeof(struct blob_attr);

 (gdb) bt
 #0  0x00007ffff70be2fc in blob_len (attr=0x6020000100d4) at /libubox/blob.h:102
 #1  0x00007ffff70bde65 in blob_raw_len (attr=0x6020000100d4) at /libubox/blob.h:111
 #2  0x00007ffff70be485 in blob_pad_len (attr=0x6020000100d4) at /libubox/blob.h:120
 #3  0x00007ffff70be22b in blobmsg_check_array_len (attr=0x6020000000d0, type=0, blob_len=10) at /libubox/blobmsg.c:145
 #4  0x000000000054f6b6 in fuzz_blobmsg_parse (data=0x6020000000d0 "\001\004", size=10) at /libubox/tests/fuzz/test-fuzz.c:57
 #5  0x000000000054f3c8 in LLVMFuzzerTestOneInput (input=<optimized out>, size=10) at /libubox/tests/fuzz/test-fuzz.c:104
 #6  0x0000000000459fc2 in ExecuteCallback () at /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:553
 #7  0x00000000004448e2 in RunOneTest () at /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:292
 #8  0x000000000044a4af in FuzzerDriver () at /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:775
 #9  0x00000000004739f3 in main () at /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:19
 
1. https://gitlab.com/openwrt/project/libubox/-/jobs/565328935

-- ynezz
Petr Štetiar May 25, 2020, 9:54 a.m. UTC | #3 
Rafał Miłecki <zajec5@gmail.com> [2020-05-25 10:31:06]:

Hi,

> From: Rafał Miłecki <rafal@milecki.pl>
> 
> After more reviews is seems that blobmsg_for_each_attr() should not be
> used when dealing with untrusted data as it reads length from blob data
> itself. It means it can't be used in the blobmsg_check_array_len().
> 
> Switch back to using __blobmsg_for_each_attr() BUT pass correct length
> to it. Calculate it by subtracting header length from blob length.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  blobmsg.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/blobmsg.c b/blobmsg.c
> index 59045e1..2295aaa 100644
> --- a/blobmsg.c
> +++ b/blobmsg.c
> @@ -142,7 +142,8 @@ int blobmsg_check_array_len(const struct blob_attr *attr, int type,
>  		return -1;
>  	}
>  
> -	blobmsg_for_each_attr(cur, attr, rem) {
> +	rem = blob_len - ((uint8_t *)blobmsg_data(attr) - (uint8_t *)blob_data(attr));

looks like blobmsg_data_len()?

> +	__blobmsg_for_each_attr(cur, attr, rem) {
>  		if (type != BLOBMSG_TYPE_UNSPEC && blobmsg_type(cur) != type)
>  			return -1;

-- ynezz
diff mbox series

Patch

diff --git a/blobmsg.c b/blobmsg.c
index 59045e1..2295aaa 100644
--- a/blobmsg.c
+++ b/blobmsg.c
@@ -142,7 +142,8 @@  int blobmsg_check_array_len(const struct blob_attr *attr, int type,
 		return -1;
 	}
 
-	blobmsg_for_each_attr(cur, attr, rem) {
+	rem = blob_len - ((uint8_t *)blobmsg_data(attr) - (uint8_t *)blob_data(attr));
+	__blobmsg_for_each_attr(cur, attr, rem) {
 		if (type != BLOBMSG_TYPE_UNSPEC && blobmsg_type(cur) != type)
 			return -1;