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