The Samba-Bugzilla – Attachment 16016 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 v2
CVE-2020-10745-extra-for-master-v2.patch (text/plain), 55.33 KB, created by
Douglas Bagnall
on 2020-06-03 05:06:48 UTC
(
hide
)
Description:
additional patches for master v2
Filename:
MIME Type:
Creator:
Douglas Bagnall
Created:
2020-06-03 05:06:48 UTC
Size:
55.33 KB
patch
obsolete
>From ab5dbeceb9f60a35bba071742e8513a478f54583 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 01/11] 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 | 247 ++++++++++++++++++++++++++++--- > selftest/knownfail.d/dns_packet | 13 ++ > 2 files changed, 241 insertions(+), 19 deletions(-) > create mode 100644 selftest/knownfail.d/dns_packet > >diff --git a/python/samba/tests/dns_packet.py b/python/samba/tests/dns_packet.py >index 2b3c37a8ab6..16e6c38277e 100644 >--- a/python/samba/tests/dns_packet.py >+++ b/python/samba/tests/dns_packet.py >@@ -4,11 +4,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 > > >@@ -20,8 +23,15 @@ 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".""" >@@ -38,12 +48,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] >@@ -101,21 +110,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: >@@ -148,6 +205,65 @@ class TestDnsPackets(TestDnsPacketBase): > self._test_query([name], > expected_rcode=expected_rcode) > >+ def test_a_query_dots(self): >+ name = '.' * 127 >+ # With Win2012r2, sometimes we get no reply, sometimes we get >+ # rcode 1 (almost 50/50). >+ self._test_query(name, >+ expected_rcode={None, dns.DNS_RCODE_FORMERR}) >+ >+ def test_a_query_dots_with_good_domain(self): >+ dots = '.' * 127 >+ names = [dots, SERVER_NAME] >+ self._test_query(names, expected_rcode=None) >+ >+ def test_a_query_dots_with_nxdomain(self): >+ dots = '.' * 127 >+ names = ["nxdomain.nxdomain.nxdomain", >+ dots, >+ SERVER_NAME] >+ self._test_query(names, expected_rcode=None) >+ >+ def test_a_query_many_long_components(self): >+ name = '.'.join(['z' * 63] * 127) >+ self._test_query(name, expected_rcode=None) >+ >+ def test_a_query_few_long_components(self): >+ name = '.'.join(['z' * 63] * 5) >+ self._test_query(name, expected_rcode=None) >+ >+ def test_a_query_many_long_components_3(self): >+ names = ['.'.join(['z' * 63] * i) for i in range(3, 6)] >+ self._test_query(names, expected_rcode=None) >+ >+ def test_a_query_too_many_dots(self): >+ # more dots than can possibly be allowed given the implicit >+ # limit on components (~max address length /2), but not more >+ # than the maximum overall address length. >+ name = '.' * 130 >+ self._test_query(name, expected_rcode=None) >+ >+ def test_a_query_dots_in_component(self): >+ # more dots than can possibly be allowed given the implicit >+ # limit on components (~max address length /2), but not more >+ # than the maximum overall address length. >+ name = [b'.' * 130, b"example", b"com"] >+ self._test_query([name], expected_rcode=None) >+ >+ def test_a_query_much_too_many_dots(self): >+ name = '.' * 1000 >+ self._test_query(name, expected_rcode=None) >+ >+ def test_simple_nxdomain(self): >+ name = 'nx.nx.nxdomain' >+ self._test_query(name, >+ expected_rcode=dns.DNS_RCODE_NXDOMAIN) >+ >+ def test_simple_good_domain(self): >+ name = SERVER_NAME >+ self._test_query(name, >+ expected_rcode=OK) >+ > def test_127_very_dotty_components(self): > label = b'.' * 63 > self._test_many_repeated_components(label, 127) >@@ -161,6 +277,12 @@ 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] >@@ -178,11 +300,98 @@ 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) >+ def test_nbt_dots_in_components(self): >+ name = [NBT_NAME.encode('utf8')] >+ name += [b'x.x' * 6] * 3 >+ self._test_query([name], expected_rcode=nbt.NBT_RCODE_NAM) >+ >+ def test_nbt_dots_in_components_nbt_enc(self): >+ name = [b'x.x' * 6] * 3 >+ self._test_nbt_encode_query([name], expected_rcode=nbt.NBT_RCODE_NAM) >+ >+ def test_dots_and_nuls_in_components(self): >+ name = [b'a.\x00b' * 3] * 12 > self._test_query([name], >- expected_rcode=expected_rcode) >+ expected_rcode=nbt.NBT_RCODE_NAM) >+ >+ def test_dots_and_nuls_in_components_nbt_enc(self): >+ name = [b'a.\x00b' * 3] * 12 >+ self._test_nbt_encode_query([name], >+ expected_rcode=nbt.NBT_RCODE_NAM) >+ >+ def test_dot_in_otherwise_reasonable_component(self): >+ name = [b'example.com'] >+ self._test_query([name], expected_rcode=None) >+ >+ def test_dot_in_otherwise_reasonable_component_nbt_enc(self): >+ name = [b'example.com'] >+ self._test_nbt_encode_query([name], expected_rcode=nbt.NBT_RCODE_NAM) >+ >+ def test_all_dots_in_nbt_enc(self): >+ name = [b'............'] >+ self._test_nbt_encode_query([name], expected_rcode=nbt.NBT_RCODE_NAM) >+ >+ def test_bad_chars_in_nbt_enc(self): >+ name = [b'......\x00......', b'example'] >+ self._test_nbt_encode_query([name], expected_rcode=nbt.NBT_RCODE_NAM) >+ >+ def test_unexpected_chars(self): >+ name = 'let us include spaces. And, for example, Ä@!#!$\n.com' >+ self._test_query(name, expected_rcode=None) >+ >+ def test_unexpected_chars_nbt_enc(self): >+ name = 'let us include spaces. And, for example, Ä@!#!$\n.com' >+ self._test_nbt_encode_query(name, expected_rcode=nbt.NBT_RCODE_NAM) >+ >+ def test_nbt_dots_in_components_long_name(self): >+ name = [NBT_NAME.encode('utf8')] >+ name += [b'.' * 63] * 126 >+ self._test_query([name], expected_rcode=None) >+ >+ def test_too_many_components(self): >+ name = 'x.' * 200 + 'x' >+ self._test_query(name, expected_rcode=None) >+ >+ def test_too_many_components_100(self): >+ c = 'x.' * 31 + 'x' >+ names = [f'{c}.x{i}' for i in range(100)] >+ self._test_query(names, expected_rcode=None) >+ >+ def test_nbt_simple(self): >+ name = NBT_NAME + '.pale.green.pants' >+ self._test_query(name, expected_rcode=nbt.NBT_RCODE_NAM) >+ >+ def test_nbt_non_nbt_first_component(self): >+ name = 'we.dont.know' >+ self._test_query(name, expected_rcode=None) >+ >+ def test_nbt_many_components(self): >+ # 112 components is about the limit >+ # (32 + 2 * 111 = 254) >+ name = NBT_NAME + '.x' * 111 >+ self._test_query(name, expected_rcode=nbt.NBT_RCODE_NAM) >+ >+ def test_nbt_long_query(self): >+ # malformed, because the maximum overall length is < 255 >+ name = NBT_NAME + ('.' + 'z' * 63) * 126 >+ self._test_query(name, expected_rcode=None) >+ >+ def test_nbt_too_many_components(self): >+ name = NBT_NAME + '.x' * 1000 >+ self._test_query(name, expected_rcode=None) >+ >+ def test_nbt_too_many_components_10(self): >+ name = NBT_NAME + '.x' * 1000 >+ names = [name] * 10 >+ self._test_query(names, expected_rcode=None) >+ >+ def test_nbt_many_dots(self): >+ name = NBT_NAME + '.' * 200 >+ self._test_query(name, expected_rcode=nbt.NBT_RCODE_NAM) >+ >+ def test_nbt_two_dots(self): >+ name = NBT_NAME + 'example..com' >+ self._test_query(name, expected_rcode=None) > > def test_127_very_dotty_components(self): > label = b'.' * 63 >diff --git a/selftest/knownfail.d/dns_packet b/selftest/knownfail.d/dns_packet >new file mode 100644 >index 00000000000..ac34eedd6fc >--- /dev/null >+++ b/selftest/knownfail.d/dns_packet >@@ -0,0 +1,13 @@ >+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_a_query_dots_in_component >+samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_dots_with_good_domain >+samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_dots_with_nxdomain >+samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_few_long_components >+samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_many_long_components >+samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_many_long_components_3 >+samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_much_too_many_dots >+samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_too_many_dots >+samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_dots_and_nuls_in_components >+samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_dots_and_nuls_in_components_nbt_enc >+samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_nbt_many_dots >-- >2.17.1 > > >From fb0eacfc4176d5d5a5305b4874c0e0dd7acfc38b Mon Sep 17 00:00:00 2001 >From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >Date: Sat, 25 Apr 2020 11:03:30 +1200 >Subject: [PATCH 02/11] CVE-2020-10745: ndr/dns_utils: correct a comment > >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 | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c >index eb0c9ab9026..b054091c46c 100644 >--- a/librpc/ndr/ndr_dns_utils.c >+++ b/librpc/ndr/ndr_dns_utils.c >@@ -58,7 +58,7 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, > > complen = strcspn(s, "."); > >- /* we need to make sure the length fits into 6 bytes */ >+ /* the length must fit into 6 bits (i.e. <= 63) */ > if (complen > 0x3F) { > return ndr_push_error(ndr, NDR_ERR_STRING, > "component length %u[%08X] > " \ >-- >2.17.1 > > >From f1bd247a43106b058e9c0a1cb9fb0705c81635c5 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 03/11] CVE-2020-10745: 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). > >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 b054091c46c..6509f101145 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.17.1 > > >From bcee6f4efba578db766efeb2ce4d0781ffb12828 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 04/11] CVE-2020-10745: 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 6509f101145..41b4ee8e0ea 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.17.1 > > >From 446f26eb6c1475c7f6c14dea8f4e9d685924c63a 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 05/11] CVE-2020-10745: 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.c | 2 +- > librpc/ndr/ndr_dns_utils.c | 17 +++-------------- > librpc/ndr/ndr_dns_utils.h | 2 +- > librpc/ndr/ndr_nbt.c | 2 +- > 4 files changed, 6 insertions(+), 17 deletions(-) > >diff --git a/librpc/ndr/ndr_dns.c b/librpc/ndr/ndr_dns.c >index de168c82e7d..15aba4ddb5c 100644 >--- a/librpc/ndr/ndr_dns.c >+++ b/librpc/ndr/ndr_dns.c >@@ -57,7 +57,7 @@ _PUBLIC_ enum ndr_err_code ndr_pull_dns_string(struct ndr_pull *ndr, > return ndr_pull_dns_string_list(ndr, > ndr_flags, > s, >- false); >+ "DNS"); > } > > >diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c >index 41b4ee8e0ea..898e91de12c 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; >@@ -80,22 +79,12 @@ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, > enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, > int ndr_flags, > const char **s, >- bool is_nbt) >+ const char *err_name) > { > 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; >@@ -109,7 +98,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 +112,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, >diff --git a/librpc/ndr/ndr_dns_utils.h b/librpc/ndr/ndr_dns_utils.h >index 834fa9138f0..6604c24e2c6 100644 >--- a/librpc/ndr/ndr_dns_utils.h >+++ b/librpc/ndr/ndr_dns_utils.h >@@ -2,7 +2,7 @@ > enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, > int ndr_flags, > const char **s, >- bool is_nbt); >+ const char *err_name); > > 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 48f51ebbc23..2d0ab4711c4 100644 >--- a/librpc/ndr/ndr_nbt.c >+++ b/librpc/ndr/ndr_nbt.c >@@ -44,7 +44,7 @@ _PUBLIC_ enum ndr_err_code ndr_pull_nbt_string(struct ndr_pull *ndr, int ndr_fla > return ndr_pull_dns_string_list(ndr, > ndr_flags, > s, >- true); >+ "NBT"); > } > > /** >-- >2.17.1 > > >From cde5fd9b0e948ca0fde7b03148578d020918ded6 Mon Sep 17 00:00:00 2001 >From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >Date: Wed, 20 May 2020 12:18:26 +1200 >Subject: [PATCH 06/11] CVE-2020-10745: 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 | 56 ++++++++++++++++++++++----------- > selftest/knownfail.d/dns_packet | 3 +- > 2 files changed, 38 insertions(+), 21 deletions(-) > >diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c >index 898e91de12c..97d5a200882 100644 >--- a/librpc/ndr/ndr_dns_utils.c >+++ b/librpc/ndr/ndr_dns_utils.c >@@ -12,7 +12,8 @@ > 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) >@@ -29,7 +30,7 @@ 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; >+ *component_len = 0; > 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,41 +86,58 @@ 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; > > if (!(ndr_flags & NDR_SCALARS)) { > return NDR_ERR_SUCCESS; > } > >- name = talloc_strdup(ndr->current_mem_ctx, ""); >+ name = talloc_array(ndr->current_mem_ctx, char, 256); >+ 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; > 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 > 253) { >+ /* >+ * 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. >+ */ >+ return ndr_pull_error(ndr, NDR_ERR_STRING, >+ "BAD %s NAME too long", >+ err_name); > } >- NDR_ERR_HAVE_NO_MEMORY(name); >- TALLOC_FREE(component); >+ if (name_len > 0) { >+ name[name_len] = '.'; >+ name_len++; >+ } >+ >+ memcpy(name + name_len, component, component_len); >+ name_len += component_len; > } >+ > if (num_components == MAX_COMPONENTS) { > 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 ac34eedd6fc..14dddebddc0 100644 >--- a/selftest/knownfail.d/dns_packet >+++ b/selftest/knownfail.d/dns_packet >@@ -8,6 +8,5 @@ samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_many_l > samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_many_long_components_3 > samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_much_too_many_dots > samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_too_many_dots >-samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_dots_and_nuls_in_components >-samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_dots_and_nuls_in_components_nbt_enc >+samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_dots_and_nuls_in_components[^_] > samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_nbt_many_dots >-- >2.17.1 > > >From c3f7e052cb38e9cd835ae9e68fac6d8669c6f552 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 07/11] CVE-2020-10745: 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. > >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 | 33 ++++++++++++++++++++++++++++++++- > selftest/knownfail.d/dns_packet | 3 +++ > 2 files changed, 35 insertions(+), 1 deletion(-) > >diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c >index 97d5a200882..dd7b2ce7f45 100644 >--- a/librpc/ndr/ndr_dns_utils.c >+++ b/librpc/ndr/ndr_dns_utils.c >@@ -8,6 +8,27 @@ > */ > #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' || c == '.') { >+ ret = NULL; >+ } >+ } >+ return ret; >+} >+ > /* > pull one component of a dns/nbt string > */ >@@ -20,6 +41,7 @@ static enum ndr_err_code ndr_pull_component(struct ndr_pull *ndr, > { > uint8_t len; > unsigned int loops = 0; >+ uint8_t *valid_comp; > while (loops < 5) { > if (*offset >= ndr->data_size) { > return ndr_pull_error(ndr, NDR_ERR_STRING, >@@ -61,7 +83,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); >diff --git a/selftest/knownfail.d/dns_packet b/selftest/knownfail.d/dns_packet >index 14dddebddc0..5364394166f 100644 >--- a/selftest/knownfail.d/dns_packet >+++ b/selftest/knownfail.d/dns_packet >@@ -9,4 +9,7 @@ samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_many_l > samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_much_too_many_dots > samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_too_many_dots > samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_dots_and_nuls_in_components[^_] >+samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_dots_and_nuls_in_components_nbt_enc > samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_nbt_many_dots >+samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_nbt_dots_in_components[^_] >+samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_nbt_dots_in_components_nbt_enc >-- >2.17.1 > > >From d254a044bb3ba3636fe632c5a0ab9778923724fd Mon Sep 17 00:00:00 2001 >From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >Date: Wed, 20 May 2020 12:18:49 +1200 >Subject: [PATCH 08/11] CVE-2020-10745: ndr: pull_dns_string: remove dead > branch > >Because we now keep track of the name length (which must be < 254), >there is no way for the number of components to exceed 127, so this >branch is never seen. > >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 | 6 ------ > 1 file changed, 6 deletions(-) > >diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c >index dd7b2ce7f45..3d05961c089 100644 >--- a/librpc/ndr/ndr_dns_utils.c >+++ b/librpc/ndr/ndr_dns_utils.c >@@ -162,12 +162,6 @@ enum ndr_err_code ndr_pull_dns_string_list(struct ndr_pull *ndr, > name_len += component_len; > } > >- if (num_components == MAX_COMPONENTS) { >- 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; >-- >2.17.1 > > >From ffe39ecaf1f534b048bac3378b1b600ff4fad931 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 09/11] CVE-2020-10745: 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 3d05961c089..6abd0970cfe 100644 >--- a/librpc/ndr/ndr_dns_utils.c >+++ b/librpc/ndr/ndr_dns_utils.c >@@ -193,7 +193,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; > >@@ -248,12 +248,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 > */ >@@ -263,9 +257,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.17.1 > > >From 2fc69ef6af9eb743e6ef08e8e22a71d6f10d439d Mon Sep 17 00:00:00 2001 >From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >Date: Sat, 23 May 2020 14:09:22 +1200 >Subject: [PATCH 10/11] dns_server: do not reply to badly formed packets > >We have been replying to ill-formed packets with the FORMATERROR >rcode, but it turns out that Windows prefers to just not reply. >To match Windows, we won't reply either. > >This should have no effect on legitimate clients, but erroneous or >malicious clients will be left hanging onto their sockets for a while. > >Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >--- > selftest/knownfail.d/dns_packet | 15 ++++----------- > source4/dns_server/dns_server.c | 7 +++++++ > 2 files changed, 11 insertions(+), 11 deletions(-) > >diff --git a/selftest/knownfail.d/dns_packet b/selftest/knownfail.d/dns_packet >index 5364394166f..97760da0a5d 100644 >--- a/selftest/knownfail.d/dns_packet >+++ b/selftest/knownfail.d/dns_packet >@@ -1,15 +1,8 @@ >-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_a_query_dots_in_component >-samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_dots_with_good_domain >-samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_dots_with_nxdomain >-samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_few_long_components >-samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_many_long_components >-samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_many_long_components_3 >-samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_much_too_many_dots >-samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_a_query_too_many_dots >+# Windows will often not reply to an NBT request with an invalid name, >+# where Samba replies with format error. >+ > samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_dots_and_nuls_in_components[^_] > samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_dots_and_nuls_in_components_nbt_enc >-samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_nbt_many_dots > samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_nbt_dots_in_components[^_] > samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_nbt_dots_in_components_nbt_enc >+samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_nbt_many_dots >diff --git a/source4/dns_server/dns_server.c b/source4/dns_server/dns_server.c >index 43ea88158ce..e67663ac815 100644 >--- a/source4/dns_server/dns_server.c >+++ b/source4/dns_server/dns_server.c >@@ -276,6 +276,13 @@ static WERROR dns_process_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx, > return WERR_OK; > > drop: >+ if (state->dns_err == DNS_RCODE_FORMERR) { >+ /* >+ * Windows tends not to reply when a DNS name is badly >+ * formed. Here we do the same. >+ */ >+ return WERR_BAD_FORMAT; >+ } > *out = data_blob_talloc(mem_ctx, state->in->data, state->in->length); > if (out->data == NULL) { > return WERR_NOT_ENOUGH_MEMORY; >-- >2.17.1 > > >From 87c71baf491f517f017b3b3435da6b682e43bf7f Mon Sep 17 00:00:00 2001 >From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >Date: Sun, 24 May 2020 00:29:06 +0000 >Subject: [PATCH 11/11] dns tests: remove dns_invalid > >The single test in this file purported to "check the server refuses >invalid characters in the query name", by making a name with control >and non-ascii characters. > >However, the assertion it makes is that this name returns NXDOMAIN, >which is exactly what you expect if these "invalid" characters were >allowed but this name did not exist [the name does not, in fact, >exist]. If the characters were forbidden we should see something like >FORMERROR or (following Windows) get no response at all. As it >happens, in conformant DNS there are no invalid characters, though in >Samba we do not allow '.' and '\0' in components for internal reasons >(and general sanity), and -- this is the origin of the test -- ldb is >stricter on what it allows in zone so as not to crash on casefold. >This test used to crash ldb. > >The python samba.dcerpc.dns interface uses the same code as the DNS >server, so there is no way for a test based on it to include >characters the server won't accept. > >So we have a test that proves exactly the opposite of its supposed >premise, but the point it accidentally made is now well made by other >tests (e.g. dns_packet). > >There is no rescuing this; let's be rid if it. > >Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >--- > python/samba/tests/dns_invalid.py | 80 ------------------------------- > source4/selftest/tests.py | 2 - > 2 files changed, 82 deletions(-) > delete mode 100644 python/samba/tests/dns_invalid.py > >diff --git a/python/samba/tests/dns_invalid.py b/python/samba/tests/dns_invalid.py >deleted file mode 100644 >index 0234798091d..00000000000 >--- a/python/samba/tests/dns_invalid.py >+++ /dev/null >@@ -1,80 +0,0 @@ >-# Unix SMB/CIFS implementation. >-# Copyright (C) Kai Blin <kai@samba.org> 2018 >-# >-# This program is free software; you can redistribute it and/or modify >-# it under the terms of the GNU General Public License as published by >-# the Free Software Foundation; either version 3 of the License, or >-# (at your option) any later version. >-# >-# This program is distributed in the hope that it will be useful, >-# but WITHOUT ANY WARRANTY; without even the implied warranty of >-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >-# GNU General Public License for more details. >-# >-# You should have received a copy of the GNU General Public License >-# along with this program. If not, see <http://www.gnu.org/licenses/>. >-# >- >-import sys >-from samba import credentials >-from samba.dcerpc import dns >-from samba.tests.subunitrun import SubunitOptions, TestProgram >-from samba.tests.dns_base import DNSTest >-import samba.getopt as options >-import optparse >- >-parser = optparse.OptionParser("dns_invalid.py <server ip> [options]") >-sambaopts = options.SambaOptions(parser) >-parser.add_option_group(sambaopts) >- >-# This timeout only has relevance when testing against Windows >-# Format errors tend to return patchy responses, so a timeout is needed. >-parser.add_option("--timeout", type="int", dest="timeout", >- help="Specify timeout for DNS requests") >- >-# use command line creds if available >-credopts = options.CredentialsOptions(parser) >-parser.add_option_group(credopts) >-subunitopts = SubunitOptions(parser) >-parser.add_option_group(subunitopts) >- >-opts, args = parser.parse_args() >- >-lp = sambaopts.get_loadparm() >-creds = credopts.get_credentials(lp) >- >-timeout = opts.timeout >- >-if len(args) < 1: >- parser.print_usage() >- sys.exit(1) >- >-server_ip = args[0] >-creds.set_krb_forwardable(credentials.NO_KRB_FORWARDABLE) >- >- >-class TestBrokenQueries(DNSTest): >- def setUp(self): >- super(TestBrokenQueries, self).setUp() >- global server, server_ip, lp, creds, timeout >- self.server_ip = server_ip >- self.lp = lp >- self.creds = creds >- self.timeout = timeout >- >- def test_invalid_chars_in_name(self): >- """Check the server refuses invalid characters in the query name""" >- p = self.make_name_packet(dns.DNS_OPCODE_QUERY) >- questions = [] >- >- name = "\x10\x11\x05\xa8.%s" % self.get_dns_domain() >- q = self.make_name_question(name, dns.DNS_QTYPE_A, dns.DNS_QCLASS_IN) >- print("asking for %s" % (q.name)) >- questions.append(q) >- >- self.finish_name_packet(p, questions) >- (response, response_packet) = self.dns_transaction_udp(p, host=server_ip) >- self.assert_dns_rcode_equals(response, dns.DNS_RCODE_NXDOMAIN) >- >- >-TestProgram(module=__name__, opts=subunitopts) >diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py >index ddb8a12b8d6..887002b81c9 100755 >--- a/source4/selftest/tests.py >+++ b/source4/selftest/tests.py >@@ -452,8 +452,6 @@ plantestsuite_loadlist("samba.tests.dns_forwarder", "fl2003dc:local", [python, o > plantestsuite_loadlist("samba.tests.dns_tkey", "fl2008r2dc", [python, os.path.join(srcdir(), "python/samba/tests/dns_tkey.py"), '$SERVER', '$SERVER_IP', '--machine-pass', '-U"$USERNAME%$PASSWORD"', '--workgroup=$DOMAIN', '$LOADLIST', '$LISTOPT']) > plantestsuite_loadlist("samba.tests.dns_wildcard", "ad_dc", [python, os.path.join(srcdir(), "python/samba/tests/dns_wildcard.py"), '$SERVER', '$SERVER_IP', '--machine-pass', '-U"$USERNAME%$PASSWORD"', '--workgroup=$DOMAIN', '$LOADLIST', '$LISTOPT']) > >-plantestsuite_loadlist("samba.tests.dns_invalid", "ad_dc", [python, os.path.join(srcdir(), "python/samba/tests/dns_invalid.py"), '$SERVER_IP', '--machine-pass', '-U"$USERNAME%$PASSWORD"', '--workgroup=$DOMAIN', '$LOADLIST', '$LISTOPT']) >- > plantestsuite_loadlist("samba.tests.dns_packet", > "ad_dc", > [python, >-- >2.17.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