Original bug: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=20083 ================================================================= ==1==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6040000004fd at pc 0x56111f10bcbb bp 0x7ffec69b5b00 sp 0x7ffec69b5af8 READ of size 1 at 0x6040000004fd thread T0 SCARINESS: 12 (1-byte-read-heap-buffer-overflow) #0 0x56111f10bcba in lzxpress_decompress samba/lib/compression/lzxpress.c:257:16 #1 0x56111f258f78 in ndr_pull_compression_xpress_chunk samba/librpc/ndr/ndr_compression.c:575:8 #2 0x56111f257552 in ndr_pull_compression_start samba/librpc/ndr/ndr_compression.c:684:4 #3 0x56111f35b065 in ndr_pull_drsuapi_DsGetNCChangesXPRESSCtr6 samba/bin/default/librpc/gen_ndr/ndr_drsuapi.c:3644:6 #4 0x56111f35978f in ndr_pull_drsuapi_DsGetNCChangesCompressedCtr samba/bin/default/librpc/gen_ndr/ndr_drsuapi.c:3811:6 #5 0x56111f358677 in ndr_pull_drsuapi_DsGetNCChangesCtr7 samba/bin/default/librpc/gen_ndr/ndr_drsuapi.c:3929:3 #6 0x56111f3577c5 in ndr_pull_drsuapi_DsGetNCChangesCtr samba/bin/default/librpc/gen_ndr/ndr_drsuapi.c:4060:5 #7 0x56111f341320 in ndr_pull_drsuapi_DsGetNCChanges samba/bin/default/librpc/gen_ndr/ndr_drsuapi.c:15517:3 #8 0x56111ebd7313 in LLVMFuzzerTestOneInput samba/bin/default/lib/fuzzing/fuzz_ndr_drsuapi_TYPE_OUT.c:275:13 #9 0x56111e97a5f1 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:556:15
Created attachment 15732 [details] Proposed patch for master CI currently under way: https://gitlab.catalyst.net.nz/samba/samba.org-security-patches/pipelines/183274
Created attachment 15733 [details] Proposed patch for V4-11
Comment on attachment 15733 [details] Proposed patch for V4-11 And that obviously was not the correct patch.
Created attachment 15734 [details] Proposed patch for V4-11
Created attachment 15735 [details] Proposed patch for V4-10
Andrew could you take a look at this, I think it warrants a CVE.
In what situations does the attacker control the amount to advance and can give a number large enough to wrap? I've looked at the callers, and aside from ndr_compression (which we already decided was not a security concern, because the traffic comes from a trusted server, being a DC) I think we avoid this, by a hair-width (again...). Normally ndr_string would be my concern, but talloc limits input to 256MB, and there is a talloc call in many of those paths. There is also a call to NDR_PULL_NEED_BYTES in some of those paths already. A morning at a conference (I'm at linux.conf.au this week) is not a good spot to draw a conclusive view here but with careful analysis I think we might avoid a CVE. Just. However, I think it should be rewritten in terms of NDR_PULL_NEED_BYTES() and that routine, along with NDR_PULL_ALIGN() be hardened for extra caution. It is certainly possible that NDR_PULL_NEED_BYTES() has an issue, and those callers need checking, but at least to overflow that it would have to be a overflow of less than the data_size right up against the end of memory. Not impossible but not super-easy either.
Created attachment 15754 [details] Proposed patch for master V2 CI: https://gitlab.catalyst.net.nz/samba/samba.org-security-patches/pipelines/186041
Our current concern is that NDR_PULL_NEED_BYTES() could suffer a similar overflow and that the use of this in ndr_pull_subcontext_start() could be unsafe. More details and investigation tomorrow and later this week. Note that 6 Feb is a public holiday in NZ.
46 usages of NDR_PULL_NEED_BYTES librpc/ndr/ndr_drsblobs.c:98 Looks to be safe librpc/ndr/ndr.c: 810 ndr_pull_subcontext_start - checks r_content_size which gets pulled from a uint32_t, when header size is 4 - r_content_size checked against ssize_t size_is parameter librpc/ndr/ndr_nbt.c: 289 ndr_pull_NETLOGON_SAM_LOGON_REQUEST - passes sid_size which is pulled from a uint32_t So it looks like we need a CVE.
(In reply to Andrew Bartlett from comment #10) Looking at NDR_PULL_NEED_BYTES() again, can this overflow if data_size is limited (as it will be, due to the talloc limit) to < 256MB? That means that n is less than 256MB. Assuming that limit, it seems that n + offset can never wrap, as the most it can get to is 512MB.
Yeah with that constraint i.e. data_size < 256Mb it can't overflow. So not a CVE. I think we should still add the overflow checks, so we don't get caught out if talloc increases the max size.
Marking my preliminary CVSS score private just to avoid spooking folks. I agree, this isn't exploitable outside compression, which we already decided was not going to pay a CVE. I'll open this up now and we can move this to a public merge request.
DO we need to backport this, or should I close the bug.
Merged into master as 91d4e79c279283dd6fc953a274b02b1957db84d8 for Samba 4.13. I don't see any need to backport at this stage. The NDR tests only really go back to 4.12 anyway.