Bug 14452 - witness.idl has wrong types for witness_IPaddrInfo, causing fuzz crash
Summary: witness.idl has wrong types for witness_IPaddrInfo, causing fuzz crash
Status: NEW
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: DCE-RPCs and pipes (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Samba QA Contact
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-07-30 05:30 UTC by Douglas Bagnall
Modified: 2021-05-06 01:30 UTC (History)
3 users (show)

See Also:


Attachments
the likely fix (1.24 KB, patch)
2020-07-30 05:30 UTC, Douglas Bagnall
metze: review-
Details
testcase for fuzz_ndr_witness_TYPE_OUT (216 bytes, application/octet-stream)
2020-07-31 03:45 UTC, Douglas Bagnall
no flags Details
Possible patch for master (23.93 KB, patch)
2020-07-31 11:40 UTC, Stefan Metzmacher
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Douglas Bagnall 2020-07-30 05:30:08 UTC
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 1 Stefan Metzmacher 2020-07-30 18:42:16 UTC
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!
Comment 2 Douglas Bagnall 2020-07-31 03:27:15 UTC
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
Comment 3 Douglas Bagnall 2020-07-31 03:45:23 UTC
Created attachment 16156 [details]
testcase for  fuzz_ndr_witness_TYPE_OUT
Comment 4 Stefan Metzmacher 2020-07-31 08:39:02 UTC
(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.
Comment 5 Stefan Metzmacher 2020-07-31 08:46:20 UTC
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.
Comment 6 Stefan Metzmacher 2020-07-31 09:11:33 UTC
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];
Comment 7 Stefan Metzmacher 2020-07-31 11:40:36 UTC
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].
Comment 8 Douglas Bagnall 2020-08-02 09:51:55 UTC
(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.
Comment 9 Douglas Bagnall 2020-08-02 10:04:11 UTC
(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.
Comment 10 Andrew Bartlett 2021-05-06 01:30:41 UTC
Removing embargo per comment #7 and comment #8