Shorter test case:

echo -n 'AAAAAQEAAAAAAAAAAAAAAAgAAAAA/wAA/wAAAAAJAAAACAAzMzI3NjI2OTMyNzY4NAEAAIAyDf8AAP8AAAAACAAAABzxKQgAAA==' | \
  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

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++) {
+		}

Thankfully this only impacts client-side test code as no production client or server code uses this yet.

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.

(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.

fixed in 1aec742575252d1efcc47a8e9023889bfb0e5709