Re: [OpenWrt-Devel] [PATCH RFC libubox] blobmsg: another attrs iteration fix for blobmsg_check_array_len()
Rafał Miłecki [2020-05-25 10:31:06]:
Hi,
> From: Rafał Miłecki
>
> 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
> ---
> 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
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [OpenWrt-Devel] [PATCH RFC libubox] blobmsg: another attrs iteration fix for blobmsg_check_array_len()
Felix Fietkau [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 AQQBAAA1JQ== | base64 -d > ./tests/fuzz/corpus/crash-e0f8ecc694d96a09a1fced27b2a0838b670d34a0 $ xxd ./tests/fuzz/corpus/crash-e0f8ecc694d96a09a1fced27b2a0838b670d34a0 : 0104 0001 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. 0x770be2fc in blob_len (attr=0x602100d4) at libubox/blob.h:102 102 return (be32_to_cpu(attr->id_len) & BLOB_ATTR_LEN_MASK) - sizeof(struct blob_attr); (gdb) bt #0 0x770be2fc in blob_len (attr=0x602100d4) at /libubox/blob.h:102 #1 0x770bde65 in blob_raw_len (attr=0x602100d4) at /libubox/blob.h:111 #2 0x770be485 in blob_pad_len (attr=0x602100d4) at /libubox/blob.h:120 #3 0x770be22b in blobmsg_check_array_len (attr=0x602000d0, type=0, blob_len=10) at /libubox/blobmsg.c:145 #4 0x0054f6b6 in fuzz_blobmsg_parse (data=0x602000d0 "\001\004", size=10) at /libubox/tests/fuzz/test-fuzz.c:57 #5 0x0054f3c8 in LLVMFuzzerTestOneInput (input=, size=10) at /libubox/tests/fuzz/test-fuzz.c:104 #6 0x00459fc2 in ExecuteCallback () at /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:553 #7 0x004448e2 in RunOneTest () at /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:292 #8 0x0044a4af in FuzzerDriver () at /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:775 #9 0x004739f3 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 ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [OpenWrt-Devel] [PATCH RFC libubox] blobmsg: another attrs iteration fix for blobmsg_check_array_len()
On 2020-05-25 10:31, Rafał Miłecki wrote: > From: Rafał Miłecki > > 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 ___ openwrt-devel mailing list openwrt-devel@lists.openwrt.org https://lists.openwrt.org/mailman/listinfo/openwrt-devel
[OpenWrt-Devel] [PATCH RFC libubox] blobmsg: another attrs iteration fix for blobmsg_check_array_len()
From: Rafał Miłecki
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
---
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));
+ __blobmsg_for_each_attr(cur, attr, rem) {
if (type != BLOBMSG_TYPE_UNSPEC && blobmsg_type(cur) != type)
return -1;
--
2.26.1
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
