Created attachment 16154 [details] the likely fix My reading of [MS-SWN] 2.2.2.1 is that the IPADDR_INFO struct has the ipv4 and ipv6 addresses inline, as 4 and 16 bytes respectively. We have been treating them as char* pointers, and it has been leading to oss-fuzz crashes (due to pidl limitations). https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-swn/eca3e933-07fe-42e6-8ddf-5fc5523210db The attached patch makes the necessary change. I don't know how it affects any tests. As far as I understand, we don't actually use this code and there are no security implications from the crash, but I have marked the bug as private for now just in case.
Comment on attachment 16154 [details] the likely fix This doesn't look correct. The fact that we use strings at the API level should not matter. Can you also attach the backtrace or what ever report to this bug? Thanks!
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=22175 https://oss-fuzz.com/testcase-detail/5686294157197312 ================================================================ ==1==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x608000000100 at pc 0x55d68a2931d4 bp 0x7ffdc0531a00 sp 0x7ffdc05319f8 READ of size 4 at 0x608000000100 thread T0 SCARINESS: 17 (4-byte-read-heap-buffer-overflow) #0 0x55d68a2931d3 in ndr_push_witness_IPaddrInfo samba/bin/default/librpc/gen_ndr/ndr_witness.c:416:4 #1 0x55d68a5f9971 in ndr_size_struct samba/librpc/ndr/ndr.c:1474:11 #2 0x55d68a28ab1d in ndr_size_witness_IPaddrInfo samba/bin/default/librpc/gen_ndr/ndr_witness.c:497:9 #3 0x55d68a28b217 in ndr_push_witness_IPaddrInfoList samba/bin/default/librpc/gen_ndr/ndr_witness.c:509:4 #4 0x55d68a28aea1 in ndr_push_witness_notifyResponse_message samba/bin/default/librpc/gen_ndr/ndr_witness.c:603:6 #5 0x55d68a2977f1 in ndr_push_witness_notifyResponse samba/librpc/ndr/ndr_witness.c:46:8 #6 0x55d68a291609 in ndr_push_witness_AsyncNotify samba/bin/default/librpc/gen_ndr/ndr_witness.c:1101:4 #7 0x55d68a8d6b5d in LLVMFuzzerTestOneInput samba/bin/default/lib/fuzzing/fuzz_ndr_witness_TYPE_OUT.c:305:13 #8 0x55d68a18eb11 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:558:15 #9 0x55d68a179e42 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:296:6 #10 0x55d68a17ff9e in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:796:9 #11 0x55d68a1a7e62 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:19:10 #12 0x7fc096fa282f in __libc_start_main #13 0x55d68a154508 in _start 0x608000000100 is located 0 bytes to the right of 96-byte region [0x6080000000a0,0x608000000100) allocated by thread T0 here: #0 0x55d68a253a5d in __interceptor_malloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:145:3 #1 0x55d68a8d578d in __talloc_with_prefix samba/lib/talloc/talloc.c:782:9 #2 0x55d68a8d02d5 in __talloc samba/lib/talloc/talloc.c:824:9 #3 0x55d68a8ce6ca in _talloc_named_const samba/lib/talloc/talloc.c:981:8 #4 0x55d68a8d4cfa in _talloc_array samba/lib/talloc/talloc.c:2764:9 #5 0x55d68a28bd47 in ndr_pull_witness_IPaddrInfoList samba/bin/default/librpc/gen_ndr/ndr_witness.c:539:4 #6 0x55d68a28b911 in ndr_pull_witness_notifyResponse_message samba/bin/default/librpc/gen_ndr/ndr_witness.c:647:6 #7 0x55d68a298327 in ndr_pull_witness_notifyResponse samba/librpc/ndr/ndr_witness.c:95:7 #8 0x55d68a291ef4 in ndr_pull_witness_AsyncNotify samba/bin/default/librpc/gen_ndr/ndr_witness.c:1138:4 #9 0x55d68a8d6aba in LLVMFuzzerTestOneInput samba/bin/default/lib/fuzzing/fuzz_ndr_witness_TYPE_OUT.c:276:13 #10 0x55d68a18eb11 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:558:15 #11 0x55d68a179e42 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:296:6 #12 0x55d68a17ff9e in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:796:9 #13 0x55d68a1a7e62 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:19:10 #14 0x7fc096fa282f in __libc_start_main
Created attachment 16156 [details] testcase for fuzz_ndr_witness_TYPE_OUT
(In reply to Douglas Bagnall from comment #3) With ndrdump it bails out in ndr_pull already. ndrdump witness witness_AsyncNotify out /dev/shm/clusterfuzz-testcase-minimized-fuzz_ndr_witness_TYPE_OUT-5686294157197312 -d1 ndr_pull_subcontext_start: ndr_pull_error(Subcontext Error): Bad subcontext (PULL) size_is(4) (0x00000004) mismatch content_size 538976288 (0x20202020) at ../../librpc/ndr/ndr.c:711 pull returned Subcontext Error I'm wondering how LLVMFuzzerTestOneInput() reached f->ndr_push() at all.
As far as I can see line 416 is: NDR_CHECK(ndr_push_witness_IPaddrInfo_flags(ndr, NDR_SCALARS, r->flags)); And the memory is behind the block allocated in line 538-539: size_addr_0 = r->num; NDR_PULL_ALLOC_N(ndr, r->addr, size_addr_0); Which means we're already within the subcontext of [subcontext(4), subcontext_size(length), flag(NDR_REMAINING), switch_is(type), size_is(num)] witness_notifyResponse_message *messages; So I guess the attached blob is not what was used to generate that backtrace.
The problem is in line 509: NDR_CHECK(ndr_push_uint32(ndr, NDR_SCALARS, 12 + (r->num * ndr_size_witness_IPaddrInfo(r->addr, ndr->flags)))); And r->num is 0, which means r->addr is talloc pointer of size 0. In IDL we have: typedef [flag(NDR_NOALIGN|NDR_LITTLE_ENDIAN)] struct { [value(12+(r->num*ndr_size_witness_IPaddrInfo(r->addr, ndr->flags)))] uint32 length; [value(0)] uint32 reserved; uint32 num; witness_IPaddrInfo addr[num]; } witness_IPaddrInfoList; I guess the magic 12 comes from the 3 uint32 fields length, reserved, num. Which means length should just be the size of the whole witness_IPaddrInfoList blob. I think the fix would be this: diff --git a/librpc/idl/witness.idl b/librpc/idl/witness.idl index e230a5ea709b..652c0e9cb651 100644 --- a/librpc/idl/witness.idl +++ b/librpc/idl/witness.idl @@ -98,14 +98,14 @@ interface witness WITNESS_IPADDR_OFFLINE = 0x10 } witness_IPaddrInfo_flags; - typedef [flag(NDR_NOALIGN|NDR_LITTLE_ENDIAN),gensize] struct { + typedef [flag(NDR_NOALIGN|NDR_LITTLE_ENDIAN)] struct { witness_IPaddrInfo_flags flags; [flag(NDR_BIG_ENDIAN)] ipv4address ipv4; [flag(NDR_BIG_ENDIAN)] ipv6address ipv6; } witness_IPaddrInfo; - typedef [flag(NDR_NOALIGN|NDR_LITTLE_ENDIAN)] struct { - [value(12+(r->num*ndr_size_witness_IPaddrInfo(r->addr, ndr->flags)))] uint32 length; + typedef [public,flag(NDR_NOALIGN|NDR_LITTLE_ENDIAN),gensize] struct { + [value(ndr_size_witness_IPaddrInfoList(r, ndr->flags))] uint32 length; [value(0)] uint32 reserved; uint32 num; witness_IPaddrInfo addr[num];
Created attachment 16157 [details] Possible patch for master Is the test case reference really correct, as it doesn't reproduce the problem. It would be good to get the correct blob, that triggered it. This is not a security problem because the ndr_push_witness_AsyncNotify function is only called on the server side. I don't think we need a security release for a crash in the use of the rpc.witness test with [validate].
(In reply to Stefan Metzmacher from comment #7) I agree it is not a security bug. I reproduce it like this: rm -r bin/ && buildtools/bin/waf -C --without-gettext --enable-debug --enable-developer --enable-libfuzzer CC=~/src/honggfuzz/hfuzz_cc/hfuzz-clang configure LINK_CC=~/src/honggfuzz/hfuzz_cc/hfuzz-clang --disable-warnings-as-errors --abi-check-disable && make -j && bin/fuzz_ndr_witness_TYPE_OUT ~/Downloads/clusterfuzz-testcase-minimized-fuzz_ndr_witness_TYPE_OUT-5686294157197312 The ndrdump equivalent is bin/ndrdump witness 3 out --base64-input --input ICAgIAQAAAC2AAAAAQAAACAgICC2AAAAICAgICAgICAAAAAAICAg//////////8gICAgICAgICD/ICD/ICAg/yAgICAgICAgICAgIP//////////ICAgICAgICAgICAgICD//////////yAgICAgICAgICAgIP//ICAgIP//ICD//////yAgICAgICAgICAgIP//ICAgIP//ICAgICAgICD//////////yAgICAgICAgICAgICAg/////////yAgICAgICAgICAgIP//ICAgIP//ICA= --validate which is generated using lib/fuzzing/decode_ndr_X_crash -p witness clusterfuzz-testcase-minimized-fuzz_ndr_witness_TYPE_OUT-5686294157197312 then adding the --validate.
(In reply to Stefan Metzmacher from comment #6) > And r->num is 0, which means r->addr is talloc pointer of size 0. > I guess the magic 12 comes from the 3 uint32 fields length, reserved, num. > Which means length should just be the size of the whole > witness_IPaddrInfoList blob. I agree with all of this, and also blame line 509.
Removing embargo per comment #7 and comment #8
This bug was referenced in samba master: f0a1f1789c3c96a654ccaa0f803e247070be117b cb60901604cd2f2e45acb609281b5a52b0b39aea cf1baa8be90a3f141d07da430146f06cbd2d1e09 8cce23acb9f9bdde8bff3c3a7ffa83361e3a64a6
Will be fixed in 4.15.0rc1