Bug 14236 - [FUZZING] Unsigned integer overflow in ndr_pull_advance
Summary: [FUZZING] Unsigned integer overflow in ndr_pull_advance
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: AD: LDB/DSDB/SAMDB (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Andrew Bartlett
QA Contact: Samba QA Contact
URL: https://bugs.chromium.org/p/oss-fuzz/...
Depends on:
Reported: 2020-01-14 23:27 UTC by Gary Lockyer
Modified: 2020-05-07 14:58 UTC (History)
3 users (show)

See Also:

Proposed patch for master (1.02 KB, patch)
2020-01-15 00:25 UTC, Gary Lockyer
gary: ci-passed+
Proposed patch for V4-11 (1.23 MB, patch)
2020-01-15 20:15 UTC, Gary Lockyer
gary: ci-passed+
Proposed patch for V4-11 (1.02 KB, patch)
2020-01-15 20:25 UTC, Gary Lockyer
gary: ci-passed+
Proposed patch for V4-10 (1.02 KB, patch)
2020-01-15 20:26 UTC, Gary Lockyer
gary: ci-passed+
Proposed patch for master V2 (17.42 KB, patch)
2020-01-27 01:29 UTC, Gary Lockyer
gary: review? (abartlet)
gary: ci-passed+

Note You need to log in before you can comment on or make changes to this bug.
Description Gary Lockyer 2020-01-14 23:27:01 UTC

Comment 1 Gary Lockyer 2020-01-14 23:29:59 UTC
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
Comment 2 Gary Lockyer 2020-01-15 00:25:03 UTC
Created attachment 15732 [details]
Proposed patch for master

CI currently under way:

Comment 3 Gary Lockyer 2020-01-15 20:15:47 UTC
Created attachment 15733 [details]
Proposed patch for V4-11
Comment 4 Gary Lockyer 2020-01-15 20:19:21 UTC
Comment on attachment 15733 [details]
Proposed patch for V4-11

And that obviously was not the correct patch.
Comment 5 Gary Lockyer 2020-01-15 20:25:36 UTC
Created attachment 15734 [details]
Proposed patch for V4-11
Comment 6 Gary Lockyer 2020-01-15 20:26:19 UTC
Created attachment 15735 [details]
Proposed patch for V4-10
Comment 7 Gary Lockyer 2020-01-15 20:27:06 UTC
Andrew could you take a look at this, I think it warrants a CVE.
Comment 8 Andrew Bartlett 2020-01-15 21:23:41 UTC
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.
Comment 9 Gary Lockyer 2020-01-27 01:29:57 UTC
Created attachment 15754 [details]
Proposed patch for master V2

CI: https://gitlab.catalyst.net.nz/samba/samba.org-security-patches/pipelines/186041
Comment 10 Andrew Bartlett 2020-02-04 08:20:35 UTC
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.
Comment 11 Gary Lockyer 2020-02-04 19:54:50 UTC
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.
Comment 13 Andrew Bartlett 2020-02-04 22:25:10 UTC
(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.
Comment 14 Gary Lockyer 2020-02-04 22:48:52 UTC
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.
Comment 15 Andrew Bartlett 2020-02-04 23:50:14 UTC
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.
Comment 16 Gary Lockyer 2020-02-09 20:01:41 UTC
DO we need to backport this, or should I close the bug.
Comment 17 Andrew Bartlett 2020-02-09 22:14:31 UTC
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.