Bug 14378 (CVE-2020-10745) - CVE-2020-10745 [SECURITY] invalid DNS or NBT queries containing dots use several seconds of CPU each
Summary: CVE-2020-10745 [SECURITY] invalid DNS or NBT queries containing dots use seve...
Status: REOPENED
Alias: CVE-2020-10745
Product: Samba 4.1 and newer
Classification: Unclassified
Component: DNS server (internal) (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Douglas Bagnall
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 14412
  Show dependency treegraph
 
Reported: 2020-05-14 05:15 UTC by Douglas Bagnall
Modified: 2020-07-21 19:50 UTC (History)
5 users (show)

See Also:


Attachments
proof of concept/ test case (2.56 KB, text/x-python)
2020-05-14 09:04 UTC, Douglas Bagnall
no flags Details
draft sufficient patch (16.95 KB, patch)
2020-05-15 01:56 UTC, Douglas Bagnall
no flags Details
patch for master v2 (17.52 KB, patch)
2020-05-21 03:54 UTC, Douglas Bagnall
no flags Details
additional patches for master (29.66 KB, patch)
2020-05-21 03:55 UTC, Douglas Bagnall
no flags Details
script used for timings mentioned in comments (3.10 KB, text/x-python)
2020-05-21 03:57 UTC, Douglas Bagnall
no flags Details
patch for master v3 -- with tests (26.84 KB, patch)
2020-06-03 00:58 UTC, Douglas Bagnall
no flags Details
additional patches for master v2 (55.33 KB, patch)
2020-06-03 05:06 UTC, Douglas Bagnall
no flags Details
patch for master v4 (37.79 KB, patch)
2020-06-12 05:11 UTC, Douglas Bagnall
no flags Details
additional patches for master (39.45 KB, patch)
2020-06-12 05:13 UTC, Douglas Bagnall
no flags Details
patch for master v5 (38.05 KB, patch)
2020-06-13 00:30 UTC, Douglas Bagnall
abartlet: review+
dbagnall: ci-passed+
Details
extra patches for master v4 (39.45 KB, patch)
2020-06-13 00:32 UTC, Douglas Bagnall
abartlet: review-
dbagnall: ci-passed+
Details
Advisory v1 with CVE number. Still needs release versions (2.27 KB, text/plain)
2020-06-14 22:20 UTC, Andrew Bartlett
no flags Details
patch for 4.10 (v05) (38.30 KB, patch)
2020-06-15 10:32 UTC, Andrew Bartlett
dbagnall: review+
Details
patch for 4.11 (v05) (38.27 KB, patch)
2020-06-15 10:32 UTC, Andrew Bartlett
dbagnall: review+
Details
patch for 4.12 (v05) (38.24 KB, patch)
2020-06-15 10:33 UTC, Andrew Bartlett
dbagnall: review+
abartlet: ci-passed+
Details
patch for 4.10 (v05.1) - fixes to build rules in backport (38.29 KB, patch)
2020-06-16 01:21 UTC, Andrew Bartlett
abartlet: ci-passed-
Details
patch for 4.11 (v05.1) - fixes to build rules in backport (38.27 KB, patch)
2020-06-16 01:36 UTC, Andrew Bartlett
no flags Details
patch for 4.10 (v05.2) - fixes to build rules in backport, py3 -> py2 (38.38 KB, patch)
2020-06-16 02:08 UTC, Andrew Bartlett
dbagnall: review+
Details
patch for master (v6) (38.05 KB, patch)
2020-06-16 02:32 UTC, Andrew Bartlett
dbagnall: review+
abartlet: ci-passed+
Details
patch for 4.12 (v06) (38.24 KB, patch)
2020-06-16 02:32 UTC, Andrew Bartlett
dbagnall: review+
abartlet: ci-passed+
Details
patch for 4.11 (v06) (38.27 KB, patch)
2020-06-16 02:33 UTC, Andrew Bartlett
dbagnall: review+
abartlet: ci-passed+
Details
patch for 4.10 (v06) (38.38 KB, patch)
2020-06-16 02:34 UTC, Andrew Bartlett
no flags Details
patch for 4.10 (v06.1) - fix python 3.4 issue (38.48 KB, patch)
2020-06-16 20:33 UTC, Andrew Bartlett
dbagnall: review+
abartlet: ci-passed+
Details
Advisory v2 with CVE number with speculative release numbers (2.29 KB, text/plain)
2020-06-16 22:49 UTC, Douglas Bagnall
no flags Details
Advisory v3 with speculative release versions (2.80 KB, text/plain)
2020-06-16 23:25 UTC, Andrew Bartlett
gary: review+
Details
Advisory V4 (2.80 KB, text/plain)
2020-06-18 01:52 UTC, Gary Lockyer
no flags Details
Advisory v5 (2.80 KB, text/plain)
2020-06-23 22:33 UTC, Douglas Bagnall
abartlet: review+
Details
Patch for V4.5 (29.60 KB, patch)
2020-06-25 22:02 UTC, Gary Lockyer
dbagnall: review+
Details
Patch for V4.5 (v2) (29.77 KB, patch)
2020-06-26 03:18 UTC, Gary Lockyer
dbagnall: review+
Details
updated advisory to indicate AD DC only (2.88 KB, text/plain)
2020-06-29 22:36 UTC, Andrew Bartlett
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Douglas Bagnall 2020-05-14 05:15:20 UTC
Although dots are conventionally used as delimiters for DNS names, they are not part of the wire format. Instead each DNS label ("component" in Samba usage) has a 6 bit length field, followed by the unterminated label string. 

However dots *are* also used as DNS delimiters inside Samba, but Samba does not reject labels that contain dots.

The longest label is 63 characters, and Samba enforces a limit of 128 components. That means you can make a query for the address with 127 components, each of which is "...............................................................".

In processing that query, Samba rewrites the name in dot-separated form, then converts it back to the wire format in order to reply. Unfortunately for Samba, it now finds the name is just 8127 dots, which it duly converts into over 8127 zero length labels.

Generating this (erroneous) reply involves much talloc_asprintf()ing and a lot of fiddling with the DNS compression cache,
which takes around 6.5 seconds on my laptop.

The same applies for NBT.

Windows 2012r2 tends to just not reply to badly malformed DNS requests, though I have not yet tested it with exactly this issue.

I have proofs of concept, and mitigating patches, which I will attach soon.
Comment 1 Andrew Bartlett 2020-05-14 06:38:26 UTC
CVE requested.

CVSS v3.1 Vector: AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H (7.5)

This is on the basis that if one UDP packet can get to 6s of CPU time, it wouldn't even need a flood to have the CPU pegged for the single threads that handles DNS and NBT.
Comment 2 Douglas Bagnall 2020-05-14 09:04:43 UTC
Created attachment 15978 [details]
proof of concept/ test case

The attached script starts a few simultaneous requests.

Requires Python 3.6+ probably.

python3  dns_dos.py
nbt 0 connected after 0.0007407665252685547 seconds
dns 0 connected after 0.0002942085266113281 seconds
nbt 1 connected after 0.00030112266540527344 seconds
dns 1 connected after 0.0003330707550048828 seconds
nbt 2 connected after 0.0004012584686279297 seconds
dns 2 connected after 0.0003077983856201172 seconds
dns 0 received 8144 bytes after 7.483375787734985 seconds
dns 1 received 8144 bytes after 15.361396312713623 seconds
nbt 0 received 8120 bytes after 23.120903491973877 seconds
nbt 1 received 8120 bytes after 23.11763596534729 seconds
nbt 2 received 8120 bytes after 23.114854335784912 seconds
dns 2 received 8144 bytes after 23.517632007598877 seconds


It takes an number-of-requests argument if you prefer a different number of requests:

python3  dns_dos.py 5

if you prefer to wait longer. elapsed and CPU time are linearly proportional to the number of requests, while memory is untroubled.
Comment 3 Douglas Bagnall 2020-05-15 01:56:17 UTC
Created attachment 15982 [details]
draft sufficient patch

I have a few more things to go on top of this patch, but this much seems to do the trick:

python3 dns_dos.py 
nbt 0 connected after 0.0004799365997314453 seconds
dns 0 connected after 5.1021575927734375e-05 seconds
nbt 1 connected after 6.079673767089844e-05 seconds
dns 1 connected after 6.008148193359375e-05 seconds
nbt 2 connected after 7.772445678710938e-05 seconds
dns 0 received 8018 bytes after 0.0017342567443847656 seconds
dns 1 received 8018 bytes after 0.0011625289916992188 seconds
dns 2 connected after 0.00010371208190917969 seconds
dns 2 received 8018 bytes after 0.0006394386291503906 seconds
Comment 4 Andrew Bartlett 2020-05-15 05:13:51 UTC
Comment on attachment 15982 [details]
draft sufficient patch

Please add CVE and BUG: to the patches for the next draft.

But looking good so far.  

Thanks!
Comment 5 Douglas Bagnall 2020-05-21 03:54:52 UTC
Created attachment 15999 [details]
patch for master v2
Comment 6 Douglas Bagnall 2020-05-21 03:55:33 UTC
Created attachment 16000 [details]
additional patches for master
Comment 7 Douglas Bagnall 2020-05-21 03:57:17 UTC
Created attachment 16001 [details]
script used for timings mentioned in comments
Comment 8 Douglas Bagnall 2020-05-21 04:39:48 UTC
"patch for master v2" fixes the problem but doesn't include tests. (tests are on the way).

"additional patches for master" fixes the problem even more, by making it harder to pull a bad dns name.

Timing results, dealing with theese three DNS names:

"dns 0": 127 labels of '...............................................................' 
"dns 1": 127 labels of 'x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x.x'
"dns 2": 127 labels of 'x'

That is, "dns 0" and "dns 1" are 8129 bytes long (127 * 63 + 128 length feilds) on the wire, while "dns 2" is 253 bytes.

"dns 2" is a worst possible valid control case.


master
======

dns 0 received 8144 bytes 6.9 seconds later
dns 1 received 8145 bytes 1.7 seconds later
dns 2 received 271 bytes 0.00046 seconds later


after [3/6] do not allow consecutive dots
=========================================

dns 0 received 8145 bytes 0.0014 seconds later
dns 1 received 8145 bytes 1.9 seconds later       <--- still not good
dns 2 received 271 bytes 0.00063 seconds later

after [4/6] forbid names > 255 bytes
====================================

dns 0 received 8145 bytes 0.0015 seconds later
dns 1 received 8145 bytes 0.0054 seconds later
dns 2 received 271 bytes 0.00043 seconds later

The denial of service is gone; patches 5/6 and 6/6 introduce the fix to NBT.


after the second set of patches
===============================

dns 0 received 8145 bytes 0.00039 seconds later  <--- 18000x faster than master
dns 1 received 8145 bytes 0.00022 seconds later  <---  8000x faster
dns 2 received 271 bytes 0.00035 seconds later   <---   1.3x faster
Comment 9 Douglas Bagnall 2020-06-03 00:58:22 UTC
Created attachment 16015 [details]
patch for master v3 -- with tests
Comment 10 Douglas Bagnall 2020-06-03 05:06:48 UTC
Created attachment 16016 [details]
additional patches for master v2
Comment 11 Douglas Bagnall 2020-06-12 05:11:02 UTC
Created attachment 16034 [details]
patch for master v4
Comment 12 Douglas Bagnall 2020-06-12 05:13:02 UTC
Created attachment 16035 [details]
additional patches for master
Comment 13 Douglas Bagnall 2020-06-12 05:34:59 UTC
The timing tests addressing real servers over sockets proved unreliable in the cloud, so I added cmocka tests that directly target the troublesome routines.

On the same (rather useless) hardware as before, the time dropped from 7 seconds to 95 microseconds.
Comment 14 Douglas Bagnall 2020-06-13 00:30:26 UTC
Created attachment 16036 [details]
patch for master v5
Comment 15 Douglas Bagnall 2020-06-13 00:32:21 UTC
Created attachment 16037 [details]
extra patches for master v4
Comment 16 Douglas Bagnall 2020-06-13 00:53:39 UTC
metze:

Patch 4 of "patch for master v5" follows the logic introduced in e6e2ec0001fe3c010445e26cc0efddbc1f73416b that allows a training dot on an
NBT scope.

I assume this means we need to retain the trailing dot on the pull side
(as is done in "extra patches for master v4"), but I don't know enough about netlogon to be sure. Is there anything that tests this case?

This question doesn't affect the security release, just the further hardening and optimisation patches for master.
Comment 17 Andrew Bartlett 2020-06-14 22:20:46 UTC
Created attachment 16038 [details]
Advisory v1 with CVE number.  Still needs release versions

Attached is the first draft of an advisory.
Comment 18 Andrew Bartlett 2020-06-15 04:06:39 UTC
Comment on attachment 16037 [details]
extra patches for master v4

Remembering that this patch doesn't block the release, I would like:
 - fix the #print() in the first patch

 - in the last patch, make it easier for the parionid reviewer by more strongly linking the size of compname with 0x3F by moving that to a named #define and using sizeof() in the test for overflow, ensuring we can trust it clearly by the time we get to the memcpy.
Comment 19 Andrew Bartlett 2020-06-15 10:32:13 UTC
Created attachment 16039 [details]
patch for 4.10 (v05)
Comment 20 Andrew Bartlett 2020-06-15 10:32:51 UTC
Created attachment 16040 [details]
patch for 4.11 (v05)
Comment 21 Andrew Bartlett 2020-06-15 10:33:38 UTC
Created attachment 16041 [details]
patch for 4.12 (v05)

Patches are under CI at the moment.
Comment 22 Douglas Bagnall 2020-06-16 00:51:47 UTC
Comment on attachment 16041 [details]
patch for 4.12 (v05)

I notice I made a mistake in a comment:

> +		 * We make the size limit slightly larger than 255 + 32,

that should read "255 + 16" (or should be better explained).
Comment 23 Douglas Bagnall 2020-06-16 00:54:35 UTC
For those interested in deep backports, the fix should apply cleanly back to 4.4, and even further with minor fiddling. 

Hooking in the tests might be more complicated.
Comment 24 Douglas Bagnall 2020-06-16 00:57:34 UTC
Comment on attachment 16038 [details]
Advisory v1 with CVE number.  Still needs release versions

Advisory v1 looks good (pending $VERSIONS expansion), except I think credit goes to Honggfuzz, not oss-fuzz.
Comment 25 Andrew Bartlett 2020-06-16 01:21:50 UTC
Created attachment 16042 [details]
patch for 4.10 (v05.1) - fixes to build rules in backport
Comment 26 Andrew Bartlett 2020-06-16 01:36:55 UTC
Created attachment 16045 [details]
patch for 4.11 (v05.1) - fixes to build rules in backport

CI of the fixed patch still running, but looking good so far.
Comment 27 Andrew Bartlett 2020-06-16 01:40:06 UTC
Comment on attachment 16042 [details]
patch for 4.10 (v05.1) - fixes to build rules in backport

CI on 4.10 fails on python2 run of tests due to use of f"" format strings.
Comment 28 Andrew Bartlett 2020-06-16 01:58:25 UTC
(In reply to Douglas Bagnall from comment #22)
If you give me the fix-up patch and where to put it I'll flow it though if I have to re-do the series, otherwise lets just fix in master.
Comment 29 Douglas Bagnall 2020-06-16 01:59:26 UTC
(In reply to Andrew Bartlett from comment #27)

The cmocka test is probably sufficient for the backports -- it proves the problem and the fix.

f-strings are technically a problem for 4.11 too as they are new in Python 3.6.
Comment 30 Andrew Bartlett 2020-06-16 02:08:19 UTC
Created attachment 16046 [details]
patch for 4.10 (v05.2) - fixes to build rules in backport, py3 -> py2
Comment 31 Andrew Bartlett 2020-06-16 02:32:21 UTC
Created attachment 16047 [details]
patch for master (v6)
Comment 32 Andrew Bartlett 2020-06-16 02:32:55 UTC
Created attachment 16048 [details]
patch for 4.12 (v06)
Comment 33 Andrew Bartlett 2020-06-16 02:33:25 UTC
Created attachment 16049 [details]
patch for 4.11 (v06)
Comment 34 Andrew Bartlett 2020-06-16 02:34:01 UTC
Created attachment 16050 [details]
patch for 4.10 (v06)
Comment 35 Andrew Bartlett 2020-06-16 04:49:09 UTC
On Python 2.7.6 in Ubuntu 14.04 the Samba 4.10 patch CI fails with:

UNEXPECTED(error): samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_127_very_dotty_components(ad_dc)
REASON: Exception: Exception: Traceback (most recent call last):
  File "/home/gitlab-runner/src/bin/python/samba/tests/dns_packet.py", line 61, in tearDown
    ok = self._known_good_query()
  File "/home/gitlab-runner/src/bin/python/samba/tests/dns_packet.py", line 146, in _known_good_query
    packet = self.construct_query([name])
  File "/home/gitlab-runner/src/bin/python/samba/tests/dns_packet.py", line 116, in construct_query
    encoded_bits.append(b'%c%s' % (len(b), b))
TypeError: unsupported operand type(s) for %: 'bytes' and 'tuple'

(This passes on Python 2.7.17 in Ubuntu 18.04)
Comment 36 Douglas Bagnall 2020-06-16 07:01:48 UTC
(In reply to Andrew Bartlett from comment #35)
>     encoded_bits.append(b'%c%s' % (len(b), b))
> TypeError: unsupported operand type(s) for %: 'bytes' and 'tuple'

I think that must actually be in Python 3.4?

In 2.7.x bytes is just an alias for str, which has always handled %. However it is not an alias on 2.6 so that will fail differently.

I don't think anything can work there without an `if PY3`. Something like:


from samba.compat import PY3

if PY3:
    encoded_bits.append(bytes([len(b)]) +  b)
else:
    encoded_bits.append('%c%s' % (len(b), b))



I will try that tomorrow.
Comment 37 Andrew Bartlett 2020-06-16 07:50:06 UTC
(In reply to Douglas Bagnall from comment #36)
Thanks for the hint.  Looking at what we do and don't run, I think we just need to make this work for 3.4, so I can avoid the 'if PY3:'.

I'll be so glad when we are rid of this.
Comment 38 Andrew Bartlett 2020-06-16 20:33:16 UTC
Created attachment 16051 [details]
patch for 4.10 (v06.1) - fix python 3.4 issue

Finally, the patches are ready for final review etc.
Comment 39 Douglas Bagnall 2020-06-16 22:49:19 UTC
Created attachment 16052 [details]
Advisory v2 with CVE number with speculative release numbers

Advisory changes from v1: 

1. Release numbers assigned, assuming this release comes after 4.11.10 and 4.12.4 which are planned for June 30 and July 2 respectively.

2. credit given to Honggfuzz, because it was the local runs of the fuzzer that I looked at.
Comment 40 Andrew Bartlett 2020-06-16 23:25:53 UTC
Created attachment 16053 [details]
Advisory v3 with speculative release versions

I've taken some text form this bug and included it in the notes, while fixing the described limit on number of components.
Comment 41 Gary Lockyer 2020-06-18 01:52:09 UTC
Created attachment 16057 [details]
Advisory V4
Comment 42 Karolin Seeger 2020-06-19 11:12:49 UTC
Planned release date Thursday July 2nd
Opening bug report for vendors.
Comment 45 Douglas Bagnall 2020-06-23 22:33:32 UTC
Created attachment 16068 [details]
Advisory v5

Fix the release number for 4.11.
Comment 46 Gary Lockyer 2020-06-25 22:02:28 UTC
Created attachment 16090 [details]
Patch for V4.5
Comment 47 Gary Lockyer 2020-06-26 03:18:26 UTC
Created attachment 16093 [details]
Patch for V4.5 (v2)
Comment 48 Andrew Bartlett 2020-06-29 22:36:01 UTC
Created attachment 16099 [details]
updated advisory to indicate AD DC only

A revised analysis is that this issue does not impact nmbd and so the versions have changed to 4.0.0 and later (the first production use of the AD DC) and the text changed to indicate that this is an AD DC only issue.

We apologise for the alarm and will note here if there is any further change.
Comment 49 Jeremy Allison 2020-06-30 00:16:27 UTC
Comment on attachment 16099 [details]
updated advisory to indicate AD DC only

LGTM. Thanks for the clarification Andrew.
Comment 50 Karolin Seeger 2020-07-02 08:52:46 UTC
Samba 4.12.4, 4.11.11 and 4.10.17 have been shipped to address this defect.
Comment 51 Karolin Seeger 2020-07-02 08:56:56 UTC
Pushed to autobuild-master.
Comment 52 Karolin Seeger 2020-07-02 09:07:37 UTC
Merged into v4-{12,11,10}-test.
Comment 53 Karolin Seeger 2020-07-03 09:20:02 UTC
Pushed to master.
Closing out bug report.

Thanks!
Comment 54 Andrew Bartlett 2020-07-21 07:59:14 UTC
Opening to the public and removing the samba-vendor alias from CC.  

Vendors: CC individually if you wish to follow along.
Comment 55 Andrew Bartlett 2020-07-21 19:50:08 UTC
Comment on attachment 16037 [details]
extra patches for master v4

Douglas,

Can you look into the feedback on this in comment 18 and get this to apply to current master?  I would be good to wrap this up.

Thanks!