The Samba-Bugzilla – Attachment 16035 Details for
Bug 14378
CVE-2020-10745 [SECURITY] invalid DNS or NBT queries containing dots use several seconds of CPU each
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
additional patches for master
extra-for-master-v3.patch (text/plain), 39.45 KB, created by
Douglas Bagnall
on 2020-06-12 05:13:02 UTC
(
hide
)
Description:
additional patches for master
Filename:
MIME Type:
Creator:
Douglas Bagnall
Created:
2020-06-12 05:13:02 UTC
Size:
39.45 KB
patch
obsolete
>From 729cdd5f609670528e9ee9259ab4027e536f0114 Mon Sep 17 00:00:00 2001 >From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >Date: Wed, 3 Jun 2020 14:42:41 +1200 >Subject: [PATCH 1/7] pytests: dns_packet tests check rcodes match Windows > >the dns_packet tests originally checked only for a particular DoS >situation (CVE-2020-10745) but now we widen them to ensure Samba's >replies to invalid packets resembles those of Windows (in particular, >Windows 2012r2). We want Samba to reply only when Windows replies, and >with the same rcode. > >At present we fail a lot of these tests. > >The original CVE-2020-10745 test is retained and widened indirectly -- >any test that leaves the server unable to respond within 0.5 seconds >will count as a failure. > >Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >--- > python/samba/tests/dns_packet.py | 170 ++++++++++++++++++++++++++----- > selftest/knownfail.d/dns_packet | 4 + > 2 files changed, 148 insertions(+), 26 deletions(-) > >diff --git a/python/samba/tests/dns_packet.py b/python/samba/tests/dns_packet.py >index c4f843eb613..4a44c04afed 100644 >--- a/python/samba/tests/dns_packet.py >+++ b/python/samba/tests/dns_packet.py >@@ -22,11 +22,14 @@ We don't use a proper client library so we can make improper packets. > """ > > import os >+import sys > import struct > import socket >+import itertools > import select >+import time > from samba.dcerpc import dns, nbt >- >+from collections import Counter > from samba.tests import TestCase > > >@@ -38,11 +41,18 @@ def _msg_id(): > > SERVER = os.environ['SERVER_IP'] > SERVER_NAME = f"{os.environ['SERVER']}.{os.environ['REALM']}" >+NBT_NAME = 'EOGFGLGPCACACACACACACACACACACACA' # "neko" > TIMEOUT = 0.5 > >+VERBOSE = '-v' in sys.argv >+ >+# We use OK for rcode assertions when we don't known whether the query >+# is DNS or NBT (not exactly coincidentally, OK is 0 in both cases). >+OK = dns.DNS_RCODE_OK >+ > > def encode_netbios_bytes(chars): >- """Even RFC 1002 uses distancing quotes when calling this "compression".""" >+ """RFC 1002 calls this "first-level encoding".""" > out = [] > chars = (chars + b' ')[:16] > for c in chars: >@@ -56,12 +66,11 @@ class TestDnsPacketBase(TestCase): > > def tearDown(self): > # we need to ensure the DNS server is responsive before >- # continuing. >- for i in range(40): >- ok = self._known_good_query() >- if ok: >- return >- print(f"the server is STILL unresponsive after {40 * TIMEOUT} seconds") >+ # continuing. This will catch the return of any DoS problems >+ # like CVE-2020-10745. >+ ok = self._known_good_query() >+ if not ok: >+ self.fail(f"the server is STILL unresponsive") > > def decode_reply(self, data): > header = data[:12] >@@ -119,21 +128,69 @@ class TestDnsPacketBase(TestCase): > return header + b''.join(encoded_bits) > > def _test_query(self, names=(), expected_rcode=None): >- > if isinstance(names, str): > names = [names] > > packet = self.construct_query(names) >+ start = time.time() > s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) > s.sendto(packet, self.server) > r, _, _ = select.select([s], [], [], TIMEOUT) >- s.close() >- # It is reasonable to not reply to these packets (Windows >- # doesn't), but it is not reasonable to render the server >- # unresponsive. >- if r != [s]: >- ok = self._known_good_query() >- self.assertTrue(ok, f"the server is unresponsive") >+ >+ # For some queries Windows varies its response. In these >+ # cases, we accept any of the known replies, or noreply if it >+ # is an option. >+ if isinstance(expected_rcode, set) and None in expected_rcode and r == []: >+ expected_rcode = None >+ >+ if expected_rcode is None: >+ # we don't think the server should answer (the packet is >+ # rubbish), but we also want to be sure that the reason >+ # the server is not answering is not that it is stuck >+ # doing work on the packet (c.f. CVE-2020-10745). So we >+ # wait for a while, then immediately send another, easy >+ # packet. If the reply to the easy packet comes back >+ # quickly, we say the server is good. >+ >+ elapsed = time.time() - start >+ if r: >+ data, addr = s.recvfrom(16 * 1024) >+ s.close() >+ >+ rcode = self.decode_reply(data)['rcode'] >+ #print(names) >+ self.fail( >+ (f"an answer was not expected " >+ f"(timeout {TIMEOUT}, elapsed {elapsed:.2f}, rcode {rcode})")) >+ >+ s.close() >+ >+ self.assertTrue(self._known_good_query(), >+ "The query timed out (good), but the server is " >+ "still unresponsive.") >+ >+ else: >+ if r != [s]: >+ s.close() >+ self.fail(f"an answer was expected within {TIMEOUT} seconds)") >+ >+ data, addr = s.recvfrom(16 * 1024) >+ s.close() >+ rcode = self.decode_reply(data)['rcode'] >+ >+ if isinstance(expected_rcode, set): >+ self.assertIn(rcode, >+ expected_rcode, >+ f"expected RCODE {expected_rcode}, got {rcode}") >+ else: >+ self.assertEqual(rcode, >+ expected_rcode, >+ f"expected RCODE {expected_rcode}, got {rcode}") >+ >+ if VERBOSE: >+ print(data, len(data)) >+ if VERBOSE: >+ print("succeeded in %f seconds" % (time.time() - start)) > > def _known_good_query(self): > if self.server[1] == 53: >@@ -156,6 +213,17 @@ class TestDnsPacketBase(TestCase): > rcode = self.decode_reply(data)['rcode'] > return expected_rcode == rcode > >+ def _make_long_name(self, length, first_component=None): >+ name = [] >+ if first_component is not None: >+ name.append(first_component) >+ length -= len(first_component) + 1 >+ while length > 33: >+ name.append("x" * 30) >+ length -= 31 >+ name.append("x" * length) >+ return '.'.join(name) >+ > > class TestDnsPackets(TestDnsPacketBase): > server = (SERVER, 53) >@@ -168,17 +236,40 @@ class TestDnsPackets(TestDnsPacketBase): > > def test_127_very_dotty_components(self): > label = b'.' * 63 >- self._test_many_repeated_components(label, 127) >+ # Windows will refuse to reply, but we also accept format error >+ self._test_many_repeated_components(label, 127, >+ expected_rcode={None, >+ nbt.NBT_RCODE_FMT}) > > def test_127_half_dotty_components(self): > label = b'x.' * 31 + b'x' >- self._test_many_repeated_components(label, 127) >+ # Windows will refuse to reply >+ self._test_many_repeated_components(label, 127, >+ expected_rcode={None, >+ nbt.NBT_RCODE_FMT}) >+ >+ def test_253_bytes(self): >+ name = self._make_long_name(253) >+ self._test_query(name, >+ expected_rcode=dns.DNS_RCODE_NXDOMAIN) >+ >+ def test_254_bytes(self): >+ name = self._make_long_name(254) >+ self._test_query(name, >+ expected_rcode=dns.DNS_RCODE_FORMERR) > > > class TestNbtPackets(TestDnsPacketBase): > server = (SERVER, 137) > qtype = 0x20 # NBT_QTYPE_NETBIOS > >+ def _test_many_repeated_components(self, label, n, >+ expected_rcode=None): >+ name = [label] * n >+ name[0] = encode_netbios_bytes(label) >+ self._test_query([name], >+ expected_rcode=expected_rcode) >+ > def _test_nbt_encode_query(self, names, *args, **kwargs): > if isinstance(names, str): > names = [names] >@@ -196,16 +287,43 @@ class TestNbtPackets(TestDnsPacketBase): > > self._test_query(nbt_names, *args, **kwargs) > >- def _test_many_repeated_components(self, label, n, expected_rcode=None): >- name = [label] * n >- name[0] = encode_netbios_bytes(label) >- self._test_query([name], >- expected_rcode=expected_rcode) >- > def test_127_very_dotty_components(self): > label = b'.' * 63 >- self._test_many_repeated_components(label, 127) >+ # Windows will refuse to reply >+ self._test_many_repeated_components(label, 127, >+ expected_rcode={None, >+ nbt.NBT_RCODE_FMT}) > > def test_127_half_dotty_components(self): > label = b'x.' * 31 + b'x' >- self._test_many_repeated_components(label, 127) >+ # Windows will refuse to reply >+ self._test_many_repeated_components(label, 127, >+ expected_rcode={None, >+ nbt.NBT_RCODE_FMT}) >+ >+ def test_253_bytes(self): >+ name = self._make_long_name(253, NBT_NAME) >+ self._test_query(name, >+ expected_rcode=nbt.NBT_RCODE_NAM) >+ >+ def test_254_bytes(self): >+ # This works because we follow Windows, not RFC 1001/1002. >+ # (see next test for a longer explanation). >+ name = self._make_long_name(254, NBT_NAME) >+ self._test_query(name, >+ expected_rcode=nbt.NBT_RCODE_NAM) >+ >+ def test_272_bytes(self): >+ # This works because we (contra RFC, following Windows) treat >+ # the 32 byte encoded Netbios name as if it were the 16 byte >+ # un-encoded form (used in MS-WINSRA), AND add three bytes >+ # because what kind of limit is 253 anyway? (matches 2012r2) >+ name = self._make_long_name(272, NBT_NAME) >+ self._test_query(name, >+ expected_rcode=nbt.NBT_RCODE_NAM) >+ >+ def test_273_bytes(self): >+ # Finally we exhaust Windows' generosity toward long names. >+ name = self._make_long_name(273, NBT_NAME) >+ self._test_query(name, >+ expected_rcode={None, nbt.NBT_RCODE_FMT}) >diff --git a/selftest/knownfail.d/dns_packet b/selftest/knownfail.d/dns_packet >index e69de29bb2d..fa09f11adf4 100644 >--- a/selftest/knownfail.d/dns_packet >+++ b/selftest/knownfail.d/dns_packet >@@ -0,0 +1,4 @@ >+samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_127_half_dotty_components >+samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_127_very_dotty_components >+samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_254_bytes >+samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_273_bytes >-- >2.20.1 > > >From 235bf8654058e83cb999bc3ec7b806821de7bb3f Mon Sep 17 00:00:00 2001 >From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >Date: Wed, 29 Apr 2020 16:16:26 +1200 >Subject: [PATCH 2/7] ndr/dns: shift pull_dns_string to ndr_dns_util.c > >This will allow NBT to use the same function (after modifications in >the next commit). > >This is post CVE-2020-10745 hardening and optimisation. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 > >Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >--- > librpc/ndr/ndr_dns.c | 101 ++------------------------------- > librpc/ndr/ndr_dns_utils.c | 111 +++++++++++++++++++++++++++++++++++++ > librpc/ndr/ndr_dns_utils.h | 4 ++ > 3 files changed, 119 insertions(+), 97 deletions(-) > >diff --git a/librpc/ndr/ndr_dns.c b/librpc/ndr/ndr_dns.c >index 966e0b59786..6b0dbd2c067 100644 >--- a/librpc/ndr/ndr_dns.c >+++ b/librpc/ndr/ndr_dns.c >@@ -35,8 +35,6 @@ > #include "lib/util/util_net.h" > #include "ndr_dns_utils.h" > >-/* don't allow an unlimited number of name components */ >-#define MAX_COMPONENTS 128 > > /** > print a dns string >@@ -48,65 +46,6 @@ _PUBLIC_ void ndr_print_dns_string(struct ndr_print *ndr, > ndr_print_string(ndr, name, s); > } > >-/* >- pull one component of a dns_string >-*/ >-static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, >- uint8_t **component, >- uint32_t *offset, >- uint32_t *max_offset) >-{ >- uint8_t len; >- unsigned int loops = 0; >- while (loops < 5) { >- if (*offset >= ndr->data_size) { >- return ndr_pull_error(ndr, NDR_ERR_STRING, >- "BAD DNS NAME component, bad offset"); >- } >- len = ndr->data[*offset]; >- if (len == 0) { >- *offset += 1; >- *max_offset = MAX(*max_offset, *offset); >- *component = NULL; >- return NDR_ERR_SUCCESS; >- } >- if ((len & 0xC0) == 0xC0) { >- /* its a label pointer */ >- if (1 + *offset >= ndr->data_size) { >- return ndr_pull_error(ndr, NDR_ERR_STRING, >- "BAD DNS NAME component, " \ >- "bad label offset"); >- } >- *max_offset = MAX(*max_offset, *offset + 2); >- *offset = ((len&0x3F)<<8) | ndr->data[1 + *offset]; >- *max_offset = MAX(*max_offset, *offset); >- loops++; >- continue; >- } >- if ((len & 0xC0) != 0) { >- /* its a reserved length field */ >- return ndr_pull_error(ndr, NDR_ERR_STRING, >- "BAD DNS NAME component, " \ >- "reserved length field: 0x%02x", >- (len &0xC)); >- } >- if (*offset + len + 1 > ndr->data_size) { >- return ndr_pull_error(ndr, NDR_ERR_STRING, >- "BAD DNS NAME component, "\ >- "length too long"); >- } >- *component = (uint8_t*)talloc_strndup(ndr, >- (const char *)&ndr->data[1 + *offset], len); >- NDR_ERR_HAVE_NO_MEMORY(*component); >- *offset += len + 1; >- *max_offset = MAX(*max_offset, *offset); >- return NDR_ERR_SUCCESS; >- } >- >- /* too many pointers */ >- return ndr_pull_error(ndr, NDR_ERR_STRING, >- "BAD DNS NAME component, too many pointers"); >-} > > /** > pull a dns_string from the wire >@@ -115,44 +54,12 @@ _PUBLIC_ enum ndr_err_code ndr_pull_dns_string(struct ndr_pull *ndr, > int ndr_flags, > const char **s) > { >- uint32_t offset = ndr->offset; >- uint32_t max_offset = offset; >- unsigned num_components; >- char *name; >- >- if (!(ndr_flags & NDR_SCALARS)) { >- return NDR_ERR_SUCCESS; >- } >- >- name = talloc_strdup(ndr->current_mem_ctx, ""); >- >- /* break up name into a list of components */ >- for (num_components=0; num_components<MAX_COMPONENTS; >- num_components++) { >- uint8_t *component = NULL; >- NDR_CHECK(ndr_pull_component(ndr, &component, &offset, >- &max_offset)); >- if (component == NULL) break; >- if (num_components > 0) { >- name = talloc_asprintf_append_buffer(name, ".%s", >- component); >- } else { >- name = talloc_asprintf_append_buffer(name, "%s", >- component); >- } >- NDR_ERR_HAVE_NO_MEMORY(name); >- } >- if (num_components == MAX_COMPONENTS) { >- return ndr_pull_error(ndr, NDR_ERR_STRING, >- "BAD DNS NAME too many components"); >- } >- >- (*s) = name; >- ndr->offset = max_offset; >- >- return NDR_ERR_SUCCESS; >+ return ndr_pull_dns_string_list(ndr, >+ ndr_flags, >+ s); > } > >+ > /** > push a dns string to the wire > */ >diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c >index 2c3ddf08bb2..eca59c36559 100644 >--- a/librpc/ndr/ndr_dns_utils.c >+++ b/librpc/ndr/ndr_dns_utils.c >@@ -2,6 +2,117 @@ > #include "../librpc/ndr/libndr.h" > #include "ndr_dns_utils.h" > >+/* don't allow an unlimited number of name components. The string must be less >+ than 255, with at least one character between dots, so 128 components is >+ plenty. >+ */ >+#define MAX_COMPONENTS 128 >+ >+/* >+ pull one component of a dns_string >+*/ >+static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, >+ uint8_t **component, >+ uint32_t *offset, >+ uint32_t *max_offset) >+{ >+ uint8_t len; >+ unsigned int loops = 0; >+ while (loops < 5) { >+ if (*offset >= ndr->data_size) { >+ return ndr_pull_error(ndr, NDR_ERR_STRING, >+ "BAD DNS NAME component, bad offset"); >+ } >+ len = ndr->data[*offset]; >+ if (len == 0) { >+ *offset += 1; >+ *max_offset = MAX(*max_offset, *offset); >+ *component = NULL; >+ return NDR_ERR_SUCCESS; >+ } >+ if ((len & 0xC0) == 0xC0) { >+ /* its a label pointer */ >+ if (1 + *offset >= ndr->data_size) { >+ return ndr_pull_error(ndr, NDR_ERR_STRING, >+ "BAD DNS NAME component, " \ >+ "bad label offset"); >+ } >+ *max_offset = MAX(*max_offset, *offset + 2); >+ *offset = ((len&0x3F)<<8) | ndr->data[1 + *offset]; >+ *max_offset = MAX(*max_offset, *offset); >+ loops++; >+ continue; >+ } >+ if ((len & 0xC0) != 0) { >+ /* its a reserved length field */ >+ return ndr_pull_error(ndr, NDR_ERR_STRING, >+ "BAD DNS NAME component, " \ >+ "reserved length field: 0x%02x", >+ (len &0xC)); >+ } >+ if (*offset + len + 1 > ndr->data_size) { >+ return ndr_pull_error(ndr, NDR_ERR_STRING, >+ "BAD DNS NAME component, "\ >+ "length too long"); >+ } >+ *component = (uint8_t*)talloc_strndup(ndr, >+ (const char *)&ndr->data[1 + *offset], len); >+ NDR_ERR_HAVE_NO_MEMORY(*component); >+ *offset += len + 1; >+ *max_offset = MAX(*max_offset, *offset); >+ return NDR_ERR_SUCCESS; >+ } >+ >+ /* too many pointers */ >+ return ndr_pull_error(ndr, NDR_ERR_STRING, >+ "BAD DNS NAME component, too many pointers"); >+} >+ >+/** >+ pull a dns_string from the wire >+*/ >+enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, >+ int ndr_flags, >+ const char **s) >+{ >+ uint32_t offset = ndr->offset; >+ uint32_t max_offset = offset; >+ unsigned num_components; >+ char *name; >+ >+ if (!(ndr_flags & NDR_SCALARS)) { >+ return NDR_ERR_SUCCESS; >+ } >+ >+ name = talloc_strdup(ndr->current_mem_ctx, ""); >+ >+ /* break up name into a list of components */ >+ for (num_components=0; num_components<MAX_COMPONENTS; >+ num_components++) { >+ uint8_t *component = NULL; >+ NDR_CHECK(ndr_pull_component(ndr, &component, &offset, >+ &max_offset)); >+ if (component == NULL) break; >+ if (num_components > 0) { >+ name = talloc_asprintf_append_buffer(name, ".%s", >+ component); >+ } else { >+ name = talloc_asprintf_append_buffer(name, "%s", >+ component); >+ } >+ NDR_ERR_HAVE_NO_MEMORY(name); >+ } >+ if (num_components == MAX_COMPONENTS) { >+ return ndr_pull_error(ndr, NDR_ERR_STRING, >+ "BAD DNS NAME too many components"); >+ } >+ >+ (*s) = name; >+ ndr->offset = max_offset; >+ >+ return NDR_ERR_SUCCESS; >+} >+ > > /** > push a dns/nbt string list to the wire >diff --git a/librpc/ndr/ndr_dns_utils.h b/librpc/ndr/ndr_dns_utils.h >index 71a65433bbb..9d1b9c3a275 100644 >--- a/librpc/ndr/ndr_dns_utils.h >+++ b/librpc/ndr/ndr_dns_utils.h >@@ -1,4 +1,8 @@ > >+enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, >+ int ndr_flags, >+ const char **s); >+ > enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, > struct ndr_token_list *string_list, > int ndr_flags, >-- >2.20.1 > > >From f5fa0ecdaf563062bc8bd196e22d54666d9e33d9 Mon Sep 17 00:00:00 2001 >From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >Date: Tue, 19 May 2020 13:55:53 +1200 >Subject: [PATCH 3/7] ndr/nbt: use ndr_dns_utils/ndr_pull_dns_string_list > >To retain exactly the same behaviour with regard to memory contexts >and error messages, we add an is_nbt flag. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 > >Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >--- > librpc/ndr/ndr_dns.c | 3 +- > librpc/ndr/ndr_dns_utils.c | 60 ++++++++++++++++------- > librpc/ndr/ndr_dns_utils.h | 3 +- > librpc/ndr/ndr_nbt.c | 99 ++------------------------------------ > 4 files changed, 50 insertions(+), 115 deletions(-) > >diff --git a/librpc/ndr/ndr_dns.c b/librpc/ndr/ndr_dns.c >index 6b0dbd2c067..de168c82e7d 100644 >--- a/librpc/ndr/ndr_dns.c >+++ b/librpc/ndr/ndr_dns.c >@@ -56,7 +56,8 @@ _PUBLIC_ enum ndr_err_code ndr_pull_dns_string(struct ndr_pull *ndr, > { > return ndr_pull_dns_string_list(ndr, > ndr_flags, >- s); >+ s, >+ false); > } > > >diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c >index eca59c36559..4b1c85a26e1 100644 >--- a/librpc/ndr/ndr_dns_utils.c >+++ b/librpc/ndr/ndr_dns_utils.c >@@ -9,19 +9,22 @@ > #define MAX_COMPONENTS 128 > > /* >- pull one component of a dns_string >+ pull one component of a dns/nbt string > */ > static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, >+ TALLOC_CTX *mem_ctx, > uint8_t **component, > uint32_t *offset, >- uint32_t *max_offset) >+ uint32_t *max_offset, >+ const char *err_name) > { > uint8_t len; > unsigned int loops = 0; > while (loops < 5) { > if (*offset >= ndr->data_size) { > return ndr_pull_error(ndr, NDR_ERR_STRING, >- "BAD DNS NAME component, bad offset"); >+ "BAD %s NAME component, bad offset", >+ err_name); > } > len = ndr->data[*offset]; > if (len == 0) { >@@ -34,8 +37,9 @@ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, > /* its a label pointer */ > if (1 + *offset >= ndr->data_size) { > return ndr_pull_error(ndr, NDR_ERR_STRING, >- "BAD DNS NAME component, " \ >- "bad label offset"); >+ "BAD %s NAME component, " \ >+ "bad label offset", >+ err_name); > } > *max_offset = MAX(*max_offset, *offset + 2); > *offset = ((len&0x3F)<<8) | ndr->data[1 + *offset]; >@@ -46,16 +50,17 @@ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, > if ((len & 0xC0) != 0) { > /* its a reserved length field */ > return ndr_pull_error(ndr, NDR_ERR_STRING, >- "BAD DNS NAME component, " \ >+ "BAD %s NAME component, " \ > "reserved length field: 0x%02x", >- (len &0xC)); >+ err_name, (len &0xC)); > } > if (*offset + len + 1 > ndr->data_size) { > return ndr_pull_error(ndr, NDR_ERR_STRING, >- "BAD DNS NAME component, "\ >- "length too long"); >+ "BAD %s NAME component, "\ >+ "length too long", >+ err_name); > } >- *component = (uint8_t*)talloc_strndup(ndr, >+ *component = (uint8_t*)talloc_strndup(mem_ctx, > (const char *)&ndr->data[1 + *offset], len); > NDR_ERR_HAVE_NO_MEMORY(*component); > *offset += len + 1; >@@ -65,20 +70,32 @@ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, > > /* too many pointers */ > return ndr_pull_error(ndr, NDR_ERR_STRING, >- "BAD DNS NAME component, too many pointers"); >+ "BAD %s NAME component, too many pointers", >+ err_name); > } > > /** >- pull a dns_string from the wire >+ pull a dns/nbt string from the wire > */ > enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, > int ndr_flags, >- const char **s) >+ const char **s, >+ bool is_nbt) > { > uint32_t offset = ndr->offset; > uint32_t max_offset = offset; > unsigned num_components; > char *name; >+ const char *err_name = NULL; >+ TALLOC_CTX *mem_ctx = NULL; >+ >+ if (is_nbt) { >+ err_name = "NBT"; >+ mem_ctx = ndr->current_mem_ctx; >+ } else { >+ err_name = "DNS"; >+ mem_ctx = ndr; >+ } > > if (!(ndr_flags & NDR_SCALARS)) { > return NDR_ERR_SUCCESS; >@@ -87,12 +104,18 @@ enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, > name = talloc_strdup(ndr->current_mem_ctx, ""); > > /* break up name into a list of components */ >- for (num_components=0; num_components<MAX_COMPONENTS; >+ for (num_components = 0; >+ num_components < MAX_COMPONENTS; > num_components++) { > uint8_t *component = NULL; >- NDR_CHECK(ndr_pull_component(ndr, &component, &offset, >- &max_offset)); >- if (component == NULL) break; >+ NDR_CHECK(ndr_pull_component(ndr, >+ mem_ctx, >+ &component, &offset, >+ &max_offset, >+ err_name)); >+ if (component == NULL) { >+ break; >+ } > if (num_components > 0) { > name = talloc_asprintf_append_buffer(name, ".%s", > component); >@@ -104,7 +127,8 @@ enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, > } > if (num_components == MAX_COMPONENTS) { > return ndr_pull_error(ndr, NDR_ERR_STRING, >- "BAD DNS NAME too many components"); >+ "BAD %s NAME too many components", >+ err_name); > } > > (*s) = name; >diff --git a/librpc/ndr/ndr_dns_utils.h b/librpc/ndr/ndr_dns_utils.h >index 9d1b9c3a275..834fa9138f0 100644 >--- a/librpc/ndr/ndr_dns_utils.h >+++ b/librpc/ndr/ndr_dns_utils.h >@@ -1,7 +1,8 @@ > > enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, > int ndr_flags, >- const char **s); >+ const char **s, >+ bool is_nbt); > > enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, > struct ndr_token_list *string_list, >diff --git a/librpc/ndr/ndr_nbt.c b/librpc/ndr/ndr_nbt.c >index 8ed9f0a5f05..48f51ebbc23 100644 >--- a/librpc/ndr/ndr_nbt.c >+++ b/librpc/ndr/ndr_nbt.c >@@ -28,9 +28,6 @@ > #include "ndr_dns_utils.h" > > >-/* don't allow an unlimited number of name components */ >-#define MAX_COMPONENTS 128 >- > /** > print a nbt string > */ >@@ -39,103 +36,15 @@ _PUBLIC_ void ndr_print_nbt_string(struct ndr_print *ndr, const char *name, cons > ndr_print_string(ndr, name, s); > } > >-/* >- pull one component of a nbt_string >-*/ >-static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, >- uint8_t **component, >- uint32_t *offset, >- uint32_t *max_offset) >-{ >- uint8_t len; >- unsigned int loops = 0; >- while (loops < 5) { >- if (*offset >= ndr->data_size) { >- return ndr_pull_error(ndr, NDR_ERR_STRING, >- "BAD NBT NAME component"); >- } >- len = ndr->data[*offset]; >- if (len == 0) { >- *offset += 1; >- *max_offset = MAX(*max_offset, *offset); >- *component = NULL; >- return NDR_ERR_SUCCESS; >- } >- if ((len & 0xC0) == 0xC0) { >- /* its a label pointer */ >- if (1 + *offset >= ndr->data_size) { >- return ndr_pull_error(ndr, NDR_ERR_STRING, >- "BAD NBT NAME component"); >- } >- *max_offset = MAX(*max_offset, *offset + 2); >- *offset = ((len&0x3F)<<8) | ndr->data[1 + *offset]; >- *max_offset = MAX(*max_offset, *offset); >- loops++; >- continue; >- } >- if ((len & 0xC0) != 0) { >- /* its a reserved length field */ >- return ndr_pull_error(ndr, NDR_ERR_STRING, >- "BAD NBT NAME component"); >- } >- if (*offset + len + 1 > ndr->data_size) { >- return ndr_pull_error(ndr, NDR_ERR_STRING, >- "BAD NBT NAME component"); >- } >- *component = (uint8_t*)talloc_strndup( >- ndr->current_mem_ctx, >- (const char *)&ndr->data[1 + *offset], len); >- NDR_ERR_HAVE_NO_MEMORY(*component); >- *offset += len + 1; >- *max_offset = MAX(*max_offset, *offset); >- return NDR_ERR_SUCCESS; >- } >- >- /* too many pointers */ >- return ndr_pull_error(ndr, NDR_ERR_STRING, "BAD NBT NAME component"); >-} >- > /** > pull a nbt_string from the wire > */ > _PUBLIC_ enum ndr_err_code ndr_pull_nbt_string(struct ndr_pull *ndr, int ndr_flags, const char **s) > { >- uint32_t offset = ndr->offset; >- uint32_t max_offset = offset; >- unsigned num_components; >- char *name; >- >- if (!(ndr_flags & NDR_SCALARS)) { >- return NDR_ERR_SUCCESS; >- } >- >- name = NULL; >- >- /* break up name into a list of components */ >- for (num_components=0;num_components<MAX_COMPONENTS;num_components++) { >- uint8_t *component = NULL; >- NDR_CHECK(ndr_pull_component(ndr, &component, &offset, &max_offset)); >- if (component == NULL) break; >- if (name) { >- name = talloc_asprintf_append_buffer(name, ".%s", component); >- NDR_ERR_HAVE_NO_MEMORY(name); >- } else { >- name = (char *)component; >- } >- } >- if (num_components == MAX_COMPONENTS) { >- return ndr_pull_error(ndr, NDR_ERR_STRING, >- "BAD NBT NAME too many components"); >- } >- if (num_components == 0) { >- name = talloc_strdup(ndr->current_mem_ctx, ""); >- NDR_ERR_HAVE_NO_MEMORY(name); >- } >- >- (*s) = name; >- ndr->offset = max_offset; >- >- return NDR_ERR_SUCCESS; >+ return ndr_pull_dns_string_list(ndr, >+ ndr_flags, >+ s, >+ true); > } > > /** >-- >2.20.1 > > >From 59a2aee0770ca3732faec8e580a0375ae7acdc1e Mon Sep 17 00:00:00 2001 >From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >Date: Wed, 20 May 2020 19:18:14 +1200 >Subject: [PATCH 4/7] ndr: pull_dns_string: drop nbt/dns mem_ctx difference > >Until now NBT and DNS have used talloc contexts of different lifetimes >to allocate component strings. The actual talloc context doesn't >really matter -- these strings are immediately copied and can be freed >straight after. So that is what we do. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 > >Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >--- > librpc/ndr/ndr_dns_utils.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > >diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c >index 4b1c85a26e1..0778212a34e 100644 >--- a/librpc/ndr/ndr_dns_utils.c >+++ b/librpc/ndr/ndr_dns_utils.c >@@ -12,7 +12,6 @@ > pull one component of a dns/nbt string > */ > static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, >- TALLOC_CTX *mem_ctx, > uint8_t **component, > uint32_t *offset, > uint32_t *max_offset, >@@ -60,7 +59,7 @@ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, > "length too long", > err_name); > } >- *component = (uint8_t*)talloc_strndup(mem_ctx, >+ *component = (uint8_t*)talloc_strndup(ndr, > (const char *)&ndr->data[1 + *offset], len); > NDR_ERR_HAVE_NO_MEMORY(*component); > *offset += len + 1; >@@ -87,14 +86,11 @@ enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, > unsigned num_components; > char *name; > const char *err_name = NULL; >- TALLOC_CTX *mem_ctx = NULL; > > if (is_nbt) { > err_name = "NBT"; >- mem_ctx = ndr->current_mem_ctx; > } else { > err_name = "DNS"; >- mem_ctx = ndr; > } > > if (!(ndr_flags & NDR_SCALARS)) { >@@ -109,7 +105,6 @@ enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, > num_components++) { > uint8_t *component = NULL; > NDR_CHECK(ndr_pull_component(ndr, >- mem_ctx, > &component, &offset, > &max_offset, > err_name)); >@@ -124,6 +119,7 @@ enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, > component); > } > NDR_ERR_HAVE_NO_MEMORY(name); >+ TALLOC_FREE(component); > } > if (num_components == MAX_COMPONENTS) { > return ndr_pull_error(ndr, NDR_ERR_STRING, >-- >2.20.1 > > >From 808385c3b5d7d62bcd6160d4fc15d864e7accea6 Mon Sep 17 00:00:00 2001 >From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >Date: Sat, 6 Jun 2020 23:22:16 +1200 >Subject: [PATCH 5/7] ndr: pull_dns_string: check length, use buffers/memcpy > >RFC 1035 says the maximum length for a DNS name is 255 characters, and >one of the factors that allowed CVE-2020-10745 is that Samba did not >enforce that directly, enabling names around 8k long. > >We fix that by keeping track of the name length. It is easier and more >efficient to use a 64 byte buffer for the components, and this will >help us to introduce further hardening in the next commit. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 > >Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >--- > librpc/ndr/ndr_dns_utils.c | 62 +++++++++++++++++++++++---------- > selftest/knownfail.d/dns_packet | 4 --- > 2 files changed, 43 insertions(+), 23 deletions(-) > >diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c >index 0778212a34e..546e6901bef 100644 >--- a/librpc/ndr/ndr_dns_utils.c >+++ b/librpc/ndr/ndr_dns_utils.c >@@ -12,13 +12,15 @@ > pull one component of a dns/nbt string > */ > static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, >- uint8_t **component, >+ uint8_t *component, >+ size_t *component_len, > uint32_t *offset, > uint32_t *max_offset, > const char *err_name) > { > uint8_t len; > unsigned int loops = 0; >+ *component_len = 0; > while (loops < 5) { > if (*offset >= ndr->data_size) { > return ndr_pull_error(ndr, NDR_ERR_STRING, >@@ -29,7 +31,6 @@ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, > if (len == 0) { > *offset += 1; > *max_offset = MAX(*max_offset, *offset); >- *component = NULL; > return NDR_ERR_SUCCESS; > } > if ((len & 0xC0) == 0xC0) { >@@ -53,15 +54,15 @@ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, > "reserved length field: 0x%02x", > err_name, (len &0xC)); > } >- if (*offset + len + 1 > ndr->data_size) { >+ if (*offset + len + 1 > ndr->data_size || >+ len > 63 /* impossible!, but we live in fear */ ) { > return ndr_pull_error(ndr, NDR_ERR_STRING, > "BAD %s NAME component, "\ > "length too long", > err_name); > } >- *component = (uint8_t*)talloc_strndup(ndr, >- (const char *)&ndr->data[1 + *offset], len); >- NDR_ERR_HAVE_NO_MEMORY(*component); >+ memcpy(component, &ndr->data[1 + *offset], len); >+ *component_len = len; > *offset += len + 1; > *max_offset = MAX(*max_offset, *offset); > return NDR_ERR_SUCCESS; >@@ -85,48 +86,71 @@ enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, > uint32_t max_offset = offset; > unsigned num_components; > char *name; >+ size_t name_len; > const char *err_name = NULL; >+ size_t max_len; > >+ /* >+ * maximum *wire* size is 255, per RFC1035, but we compare to 253 >+ * because a) there is one more length field than there are separating >+ * dots, and b) there is a final zero-length root node, not >+ * represented in dotted form. >+ * >+ * For NBT we compare to 272 because that is roughly what Windows >+ * 2012r2 does (contra RFC 1002). >+ */ > if (is_nbt) { > err_name = "NBT"; >+ max_len = 272; > } else { > err_name = "DNS"; >+ max_len = 253; > } > > if (!(ndr_flags & NDR_SCALARS)) { > return NDR_ERR_SUCCESS; > } > >- name = talloc_strdup(ndr->current_mem_ctx, ""); >+ name = talloc_array(ndr->current_mem_ctx, char, max_len + 2); >+ NDR_ERR_HAVE_NO_MEMORY(name); >+ name_len = 0; > >- /* break up name into a list of components */ > for (num_components = 0; > num_components < MAX_COMPONENTS; > num_components++) { >- uint8_t *component = NULL; >+ uint8_t component[64]; >+ size_t component_len = 0; > NDR_CHECK(ndr_pull_component(ndr, >- &component, &offset, >+ component, >+ &component_len, >+ &offset, > &max_offset, > err_name)); >- if (component == NULL) { >+ if (component_len == 0) { > break; > } >- if (num_components > 0) { >- name = talloc_asprintf_append_buffer(name, ".%s", >- component); >- } else { >- name = talloc_asprintf_append_buffer(name, "%s", >- component); >+ if (name_len + component_len + 1 > max_len) { >+ return ndr_pull_error(ndr, NDR_ERR_STRING, >+ "BAD %s NAME too long", >+ err_name); >+ } >+ if (name_len > 0) { >+ name[name_len] = '.'; >+ name_len++; > } >- NDR_ERR_HAVE_NO_MEMORY(name); >- TALLOC_FREE(component); >+ >+ memcpy(name + name_len, component, component_len); >+ name_len += component_len; > } >+ > if (num_components == MAX_COMPONENTS) { >+ /* We should never reach this */ > return ndr_pull_error(ndr, NDR_ERR_STRING, > "BAD %s NAME too many components", > err_name); > } > >+ name[name_len] = '\0'; > (*s) = name; > ndr->offset = max_offset; > >diff --git a/selftest/knownfail.d/dns_packet b/selftest/knownfail.d/dns_packet >index fa09f11adf4..e69de29bb2d 100644 >--- a/selftest/knownfail.d/dns_packet >+++ b/selftest/knownfail.d/dns_packet >@@ -1,4 +0,0 @@ >-samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_127_half_dotty_components >-samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_127_very_dotty_components >-samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_254_bytes >-samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_273_bytes >-- >2.20.1 > > >From 0826459539c42431cf763fa27de195a2ef4fdc29 Mon Sep 17 00:00:00 2001 >From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >Date: Wed, 20 May 2020 10:05:16 +1200 >Subject: [PATCH 6/7] ndr: pull_dns_string: don't allow dots or '\0' in labels > >We use a modified version of memcpy that returns NULL if the copied >string contains the bad characters. > >As a special case, we allow a '.' as the last character, because an >NBT name with a trailing dot is sometimes used as a username, and we >need to match these exactly, even though the dotless form is >semantically the same (per RFC). > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 > >Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >--- > librpc/ndr/ndr_dns_utils.c | 44 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 43 insertions(+), 1 deletion(-) > >diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c >index 546e6901bef..22a5772d791 100644 >--- a/librpc/ndr/ndr_dns_utils.c >+++ b/librpc/ndr/ndr_dns_utils.c >@@ -8,6 +8,38 @@ > */ > #define MAX_COMPONENTS 128 > >+ >+ >+/* >+ * If the src component is "good" as a DNS component, we copy it to dest and >+ * return dest, just like memcpy. If it is bad, we do the copy anyway but >+ * return NULL. >+ */ >+static uint8_t* dns_component_copy(uint8_t *dest, uint8_t *src, size_t len) >+{ >+ size_t i; >+ uint8_t *ret = dest; >+ for (i = 0; i < len; i++) { >+ uint8_t c = src[i]; >+ dest[i] = c; >+ if (c == '\0') { >+ ret = NULL; >+ } else if (c == '.' && i != len - 1) { >+ /* >+ * As a special case, for NBT addresses that are also >+ * usernames, we allow a dot as the last character. >+ * >+ * This will not be treated symetrically on the push >+ * side unless the component is the last component of >+ * an NBT name. In other cases (DNS, non-final NBT) >+ * the trailing dot will truncate the address on push. >+ */ >+ ret = NULL; >+ } >+ } >+ return ret; >+} >+ > /* > pull one component of a dns/nbt string > */ >@@ -20,6 +52,7 @@ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, > { > uint8_t len; > unsigned int loops = 0; >+ uint8_t *valid_comp; > *component_len = 0; > while (loops < 5) { > if (*offset >= ndr->data_size) { >@@ -61,7 +94,16 @@ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, > "length too long", > err_name); > } >- memcpy(component, &ndr->data[1 + *offset], len); >+ >+ valid_comp = dns_component_copy(component, >+ &ndr->data[1 + *offset], >+ len); >+ if (valid_comp == NULL) { >+ return ndr_pull_error(ndr, NDR_ERR_STRING, >+ "BAD %s NAME component, "\ >+ "undesirable characters", >+ err_name); >+ } > *component_len = len; > *offset += len + 1; > *max_offset = MAX(*max_offset, *offset); >-- >2.20.1 > > >From b34c385e19003695949a450ab0d9564a05a60892 Mon Sep 17 00:00:00 2001 >From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >Date: Sat, 25 Apr 2020 14:56:05 +1200 >Subject: [PATCH 7/7] ndr/util push_dns_string: avoid unnecessary tallocs > >We know the components are all less than 64 bytes long. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 > >Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >--- > librpc/ndr/ndr_dns_utils.c | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > >diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c >index 22a5772d791..e358a5330b0 100644 >--- a/librpc/ndr/ndr_dns_utils.c >+++ b/librpc/ndr/ndr_dns_utils.c >@@ -239,7 +239,7 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, > > while (s && *s) { > enum ndr_err_code ndr_err; >- char *compname; >+ uint8_t compname[64]; > size_t complen; > uint32_t offset; > >@@ -294,12 +294,6 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, > complen++; > } > >- compname = talloc_asprintf(ndr, "%c%*.*s", >- (unsigned char)complen, >- (unsigned char)complen, >- (unsigned char)complen, s); >- NDR_ERR_HAVE_NO_MEMORY(compname); >- > /* remember the current component + the rest of the string > * so it can be reused later > */ >@@ -309,9 +303,9 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, > } > > /* push just this component into the blob */ >- NDR_CHECK(ndr_push_bytes(ndr, (const uint8_t *)compname, >- complen+1)); >- talloc_free(compname); >+ compname[0] = complen; >+ memcpy(compname + 1, s, complen); >+ NDR_CHECK(ndr_push_bytes(ndr, compname, complen + 1)); > > s += complen; > if (*s == '.') { >-- >2.20.1 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Actions:
View
Attachments on
bug 14378
:
15978
|
15982
|
15999
|
16000
|
16001
|
16015
|
16016
|
16034
|
16035
|
16036
|
16037
|
16038
|
16039
|
16040
|
16041
|
16042
|
16045
|
16046
|
16047
|
16048
|
16049
|
16050
|
16051
|
16052
|
16053
|
16057
|
16068
|
16090
|
16093
|
16099