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