Bug 14219 - [FUZZING] Heap-buffer-overflow in ndr_string_length()
Summary: [FUZZING] Heap-buffer-overflow in ndr_string_length()
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: DCE-RPCs and pipes (show other bugs)
Version: 4.11.0
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
Depends on:
Reported: 2019-12-16 15:58 UTC by Andreas Schneider
Modified: 2020-01-15 09:30 UTC (History)
4 users (show)

See Also:

patch for master (28.06 KB, patch)
2019-12-16 15:58 UTC, Andreas Schneider
no flags Details
patch for 4.11 (2.60 KB, patch)
2019-12-20 10:23 UTC, Andreas Schneider
slow: review+
patch for 4.10 (2.60 KB, patch)
2019-12-20 10:23 UTC, Andreas Schneider
slow: review+

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Schneider 2019-12-16 15:58:30 UTC
Created attachment 15690 [details]
patch for master

Original bug: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=19385

==1==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6070000000d4 at pc 0x55d680b37109 bp 0x7ffd4b404920 sp 0x7ffd4b4040c8
READ of size 2 at 0x6070000000d4 thread T0
SCARINESS: 14 (2-byte-read-heap-buffer-overflow)
    #0 0x55d680b37108 in MemcmpInterceptorCommon(void*, int (*)(void const*, void const*, unsigned long), void const*, void const*, unsigned long) /src/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc:839:7
    #1 0x55d680b3765a in __interceptor_bcmp /src/llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc:885:10
    #2 0x55d680d9d771 in ndr_string_length samba/librpc/ndr/ndr_string.c:558:14
    #3 0x55d680d9e2fd in ndr_pull_charset_to_null samba/librpc/ndr/ndr_string.c:632:12
    #4 0x55d680bd1c07 in ndr_pull_spoolss_DeviceMode samba/bin/default/librpc/gen_ndr/ndr_spoolss.c:1458:3
    #5 0x55d680d4b71c in LLVMFuzzerTestOneInput samba/bin/default/lib/fuzzing/fuzz_ndr_spoolss_TYPE_STRUCT.c:265:13
    #6 0x55d680ad0d81 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:556:15
    #7 0x55d680abb8a1 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:292:6
    #8 0x55d680ac155e in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:774:9
    #9 0x55d680aeb482 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:19:10
    #10 0x7f265acb682f in __libc_start_main
    #11 0x55d680a94ce8 in _start
Comment 1 Andreas Schneider 2019-12-16 16:00:06 UTC
I've worked on this bug with Günther. The ndr_string_length() function has no input length and if the string doesn't have a null terminator, we will continue reading the the memory till we hopefully find one.
Comment 2 Andreas Schneider 2019-12-16 16:00:32 UTC
This is bad, but I'm not sure if this has any security relevance.
Comment 3 Andrew Bartlett 2019-12-16 21:29:01 UTC
This adds a deprecated flag but does not remove the references from pidl/lib/Parse/Pidl/Samba4/NDR/Client.pm and pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm

Those are in the push/print case, the bug here is that ndr_string_length is being used on untrusted input in the pull case.

We are already breaking the ABI for ndr-1.0.0, so perhaps just rename ndr_string_length() -> ndr_trusted_string_length() and add ndr_string_n_length() with the extra buffer size argument?

Finally, in terms of paying a security bug, do we care if a user can crash the spoolss deamon if configured that way?  Otherwise it would 'just' be an smbd self-DoS, which we don't pay on.

We have may security bugs, I don't think we need the extra work for this one.  Some of our distributors (specifically Debian) are not universally shipping our security patches as is, so perhaps our bar is too low. 

For contrast, see here for a little guidance on a previous spoolss NDR issue (before we realised it couldn't be exploited):

Comment 4 Andreas Schneider 2019-12-20 10:23:28 UTC
Created attachment 15717 [details]
patch for 4.11
Comment 5 Andreas Schneider 2019-12-20 10:23:51 UTC
Created attachment 15718 [details]
patch for 4.10
Comment 6 Ralph Böhme 2019-12-20 10:43:38 UTC
Reassigning to Karolin for inclusion in 4.10 and 4.11.
Comment 7 Andrew Bartlett 2019-12-20 18:12:07 UTC
The patch is in master, removing embargo.  No CVE.
Comment 8 Karolin Seeger 2020-01-14 08:29:14 UTC
(In reply to Ralph Böhme from comment #6)
Pushed to autobuild-v4-{11,10}-test.
Comment 9 Karolin Seeger 2020-01-15 09:30:37 UTC
(In reply to Karolin Seeger from comment #8)
Pushed to both branches.
Closing out bug report.