Bug 13877 - client-side crash in clusapi clusapi_QueryAllValues out
Summary: client-side crash in clusapi clusapi_QueryAllValues out
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: PIDL and libndr (show other bugs)
Version: 4.10.0
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Andrew Bartlett
QA Contact: Samba QA Contact
Depends on:
Reported: 2019-04-01 21:49 UTC by Andrew Bartlett
Modified: 2020-04-17 02:19 UTC (History)
3 users (show)

See Also:

patch to check array sizes in a deferred loop (1.33 KB, patch)
2019-10-30 21:33 UTC, Douglas Bagnall
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Bartlett 2019-04-01 21:49:11 UTC
Reported by Michael Hanselmann 

echo -n 'CUQBAQEAAAAAAAAAAQFD+fn5+fn5+fn5+fn5+fn5+fn4+fn5+fn5+fn5+fn5+fn5+fn5+fn5+fn5+eIaL0uShQEBzT+FFFredsKXrQyOgrtWLr0oyrQlyvjdzlRnQnrOIPxU/y6g2D/vgDxV' | \
  base64 -d | bin/ndrdump clusapi clusapi_QueryAllValues out
==25898==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60b000000208 at pc 0x7f6d70dea798 bp 0x7ffef36ff2f0 sp 0x7ffef36ff2e8
READ of size 8 at 0x60b000000208 thread T0
    #0 0x7f6d70dea797 in ndr_pull_clusapi_QueryAllValues /src/samba/bin/default/librpc/gen_ndr/ndr_clusapi.c:18203:7
    #1 0x55646a95801b in LLVMFuzzerTestOneInput /src/samba/bin/default/../../librpc/tools/fuzz_ndrdump.c:129:12
    #2 0x55646a968e1b in HonggfuzzMain (/src/samba/bin/default/librpc/tools/fuzz_ndrdump+0x1d8e1b)
    #3 0x7f6d6808809a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #4 0x55646a7d9dc9 in _start (/src/samba/bin/default/librpc/tools/fuzz_ndrdump+0x49dc9)

This is in pure PIDL-generated code so the same issue may appear elsewhere
Comment 1 hansmi 2019-04-03 20:57:57 UTC
Shorter test case:

  base64 -d | bin/ndrdump clusapi clusapi_QueryAllValues out

When built from master branch at 377d27359ccdb8f2680fda36ca388f44456590e5:

==17601==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60b000005278 at pc 0x7f1ff4edcdc5 bp 0x7ffd54477c90 sp 0x7ffd54477c88
READ of size 8 at 0x60b000005278 thread T0
    #0 0x7f1ff4edcdc4 in ndr_pull_clusapi_QueryAllValues /src/samba-upstream/bin/default/librpc/gen_ndr/ndr_clusapi.c:18203:7
    #1 0x563b5f2527c0 in main /src/samba-upstream/bin/default/../../librpc/tools/ndrdump.c:456:12
    #2 0x7f1fef3ad09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #3 0x563b5f179e79 in _start (/src/samba-upstream/bin/default/librpc/tools/ndrdump+0x4de79)

0x60b000005278 is located 0 bytes to the right of 104-byte region [0x60b000005210,0x60b000005278)
allocated by thread T0 here:
    #0 0x563b5f221cc3 in __interceptor_malloc (/src/samba-upstream/bin/default/librpc/tools/ndrdump+0xf5cc3)
    #1 0x7f1ff5960175 in __talloc_with_prefix /src/samba-upstream/bin/default/../../lib/talloc/talloc.c:782:9
    #2 0x7f1ff5946bb5 in _talloc_named_const /src/samba-upstream/bin/default/../../lib/talloc/talloc.c:981:8
    #3 0x7f1ff595e1e5 in _talloc_array /src/samba-upstream/bin/default/../../lib/talloc/talloc.c:2764:9
    #4 0x7f1ff4ed8144 in ndr_pull_clusapi_QueryAllValues /src/samba-upstream/bin/default/librpc/gen_ndr/ndr_clusapi.c:18168:4
    #5 0x563b5f2527c0 in main /src/samba-upstream/bin/default/../../librpc/tools/ndrdump.c:456:12
    #6 0x7f1fef3ad09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)

SUMMARY: AddressSanitizer: heap-buffer-overflow /src/samba-upstream/bin/default/librpc/gen_ndr/ndr_clusapi.c:18203:7 in ndr_pull_clusapi_QueryAllValues
Comment 2 Douglas Bagnall 2019-10-30 21:33:34 UTC
Created attachment 15588 [details]
patch to check array sizes in a deferred loop

The clusapi_QueryAllValues reads after the end of the array:

$ valgrind bin/ndrdump clusapi clusapi_QueryAllValues out /tmp/clusapi-1.dat

==1970== Invalid read of size 8
==1970==    at 0x4923995: ndr_pull_clusapi_QueryAllValues (ndr_clusapi.c:18203)
==1970==    by 0x11FEF6: main (ndrdump.c:507)
==1970==  Address 0x80ee378 is 0 bytes after a block of size 104 alloc'd
==1970==    at 0x483874F: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1970==    by 0x4AAB32F: __talloc_with_prefix (talloc.c:782)
==1970==    by 0x4AAB4C9: __talloc (talloc.c:824)
==1970==    by 0x4AAB952: _talloc_named_const (talloc.c:981)
==1970==    by 0x4AAF9B1: _talloc_array (talloc.c:2764)
==1970==    by 0x4923210: ndr_pull_clusapi_QueryAllValues (ndr_clusapi.c:18168)
==1970==    by 0x11FEF6: main (ndrdump.c:507)

It wants to check the sizes in a deferred loop, but forgot to do the looping, meaning it used the counter value that is always just past the end of the list from the previous loop.

The invalid read is one problem, and the unchecked sizes are another.

The patch as it stands adds a lot of empty loops like this:

+		for (cntr_info_1 = 0; cntr_info_1 < (size_info_1); cntr_info_1++) {
+		}
Comment 3 Douglas Bagnall 2019-11-08 04:48:47 UTC
Thankfully this only impacts client-side test code as no production client or server code uses this yet.
Comment 4 Andrew Bartlett 2019-11-18 20:36:57 UTC
G'Day Michael Hanselmann,

Can you also confirm here that you are happy to be credited by name in the commit message for this but as well, and let me know any details you want included (eg the name of the fuzzing tool you used)?

Then we can merge this fix into master pretty soon.
Comment 5 hansmi 2019-11-18 22:22:20 UTC
(In reply to Andrew Bartlett from comment #4)
Yes, I'm happy to be credited. The primary tool was Honggfuzz in combination with fuzzing code which is not yet, as of this writing, available in a public repository. I intend to take care of that soon.
Comment 6 Douglas Bagnall 2020-04-17 02:19:18 UTC
fixed in 1aec742575252d1efcc47a8e9023889bfb0e5709