Bug 14206 - dnsProperty round-trip memory handling error in fuzzer (only)
Summary: dnsProperty round-trip memory handling error in fuzzer (only)
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: DNS server (internal) (show other bugs)
Version: 4.11.2
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL: https://gitlab.com/samba-team/samba/m...
Keywords:
Depends on:
Blocks:
 
Reported: 2019-11-29 04:36 UTC by Andrew Bartlett
Modified: 2020-05-18 21:52 UTC (History)
2 users (show)

See Also:

Attachments
an initial patch, mitigate this at the IDL level (588 bytes, patch)
2019-11-29 04:36 UTC, Andrew Bartlett 		no flags Details
patch for master (2.29 KB, patch)
2019-12-01 23:09 UTC, Andrew Bartlett 		no flags Details
patch for master: mitigate this at the IDL level (2.37 KB, patch)
2019-12-01 23:13 UTC, Andrew Bartlett 		no flags Details
Proposed test case (6.15 KB, patch)
2019-12-08 20:31 UTC, Gary Lockyer 		gary: review? (abartlet)
 Details
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Bartlett 2019-11-29 04:36:56 UTC 
Created attachment 15641 [details]
an initial patch, mitigate this at the IDL level

A user with write access to a dnsZone object (eg a DNS administrator) can crash the DNS Management server because dnsProperty records can be parsed that when re-pushed point to invalid memory.

The id element in dnsp_DnsProperty was not the switch_is() value for data, instead it was entangled with a check on r->wDataLength, presumably to allow a 0 wDataLenght to take priority.

We do not know if dnsPropertyRecords with a 0 wDataLenght exist in the wild, but the IDL was originally created to provide for this.

However the application layer code does not look at wDataLength and the value() statement (calling an ndr_size_() function) uses id alone as the switch.

This means that when re-pushing a dnsProperty record that either a null pointer will be followed or uninitialised memory will be pushed back into the dnsPropertyRecord. 

CVSS:3.1/AV:N/AC:L/PR:H/UI:N/S:U/C:L/I:N/A:H
Comment 1 Andrew Bartlett 2019-11-29 04:37:33 UTC 
Found by Douglas Bagnall using Hongfuzz and the new fuzz_ndr_X fuzzer.
Comment 2 Andrew Bartlett 2019-12-01 23:09:47 UTC 
Created attachment 15657 [details]
patch for master

I've done a more complete triage of this today to try and assess how this could or could not be exploited.  

My assessment is that this cannot be exploited per the rationale in the commit message.  In short, we are saved by zeroed buffers. 

However we should apply this to be stricter in the future and avoid missteps handling this, as we do should be able to expect fully initialised output from the NDR parser. 

I'll remove the embargo once I get a peer review of this assessment. 

The patch needs a test added before it goes to master.
Comment 3 Andrew Bartlett 2019-12-01 23:13:03 UTC 
Created attachment 15658 [details]
patch for master: mitigate this at the IDL level
Comment 4 Andrew Bartlett 2019-12-04 01:06:11 UTC 
This was CVE-2019-14908 but has been downgraded and un-embargoed after further triage.
Comment 5 Gary Lockyer 2019-12-08 20:29:59 UTC 
I think the range check is unnecessary as it will fail with in normal parsing, i
Comment 6 Gary Lockyer 2019-12-08 20:31:32 UTC 
Created attachment 15671 [details]
Proposed test case
Comment 7 Gary Lockyer 2019-12-11 22:08:59 UTC 
Fixes in merge request 995
Comment 8 Douglas Bagnall 2020-04-17 03:57:13 UTC 
fixed in fee5c6a4247aeac71318186bbff7708d25de5912
Comment 9 Andrew Bartlett 2020-05-18 21:52:24 UTC 
Sorry, re-opened wrong bug.  Bug 14310 is the regression from this bug.