From 8d94dee73b35110ec75ec7f7555654166a1cbfc8 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 1 May 2020 23:11:47 +1200 Subject: [PATCH 1/6] CVE-2020-10745: pytests: hand-rolled invalid dns/nbt packet tests The client libraries don't allow us to make packets that are broken in certain ways, so we need to construct them as byte strings. These tests all fail at present, proving the server is rendered unresponsive, which is the crux of CVE-2020-10745. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 Signed-off-by: Douglas Bagnall --- python/samba/tests/dns_packet.py | 193 +++++++++++++++++++++++++++++++ selftest/knownfail.d/dns_packet | 4 + source4/selftest/tests.py | 10 ++ 3 files changed, 207 insertions(+) create mode 100644 python/samba/tests/dns_packet.py create mode 100644 selftest/knownfail.d/dns_packet diff --git a/python/samba/tests/dns_packet.py b/python/samba/tests/dns_packet.py new file mode 100644 index 00000000000..2b3c37a8ab6 --- /dev/null +++ b/python/samba/tests/dns_packet.py @@ -0,0 +1,193 @@ +"""Sanity tests for DNS and NBT server parsing. + +We don't use a proper client library so we can make improper packets. +""" + +import os +import struct +import socket +import select +from samba.dcerpc import dns, nbt + +from samba.tests import TestCase + + +def _msg_id(): + while True: + for i in range(1, 0xffff): + yield i + + +SERVER = os.environ['SERVER_IP'] +SERVER_NAME = f"{os.environ['SERVER']}.{os.environ['REALM']}" +TIMEOUT = 0.5 + + +def encode_netbios_bytes(chars): + """Even RFC 1002 uses distancing quotes when calling this "compression".""" + out = [] + chars = (chars + b' ')[:16] + for c in chars: + out.append((c >> 4) + 65) + out.append((c & 15) + 65) + return bytes(out) + + +class TestDnsPacketBase(TestCase): + msg_id = _msg_id() + + 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") + + def decode_reply(self, data): + header = data[:12] + id, flags, n_q, n_a, n_rec, n_exta = struct.unpack('!6H', + header) + return { + 'rcode': flags & 0xf + } + + def construct_query(self, names): + """Create a query packet containing one query record. + + *names* is either a single string name in the usual dotted + form, or a list of names. In the latter case, each name can + be a dotted string or a list of byte components, which allows + dots in components. Where I say list, I mean non-string + iterable. + + Examples: + + # these 3 are all the same + "example.com" + ["example.com"] + [[b"example", b"com"]] + + # this is three names in the same request + ["example.com", + [b"example", b"com", b"..!"], + (b"first component", b" 2nd component")] + """ + header = struct.pack('!6H', + next(self.msg_id), + 0x0100, # query, with recursion + len(names), # number of queries + 0x0000, # no answers + 0x0000, # no records + 0x0000, # no extra records + ) + tail = struct.pack('!BHH', + 0x00, # root node + self.qtype, + 0x0001, # class IN-ternet + ) + encoded_bits = [] + for name in names: + if isinstance(name, str): + bits = name.encode('utf8').split(b'.') + else: + bits = name + + for b in bits: + encoded_bits.append(b'%c%s' % (len(b), b)) + encoded_bits.append(tail) + + 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) + 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") + + def _known_good_query(self): + if self.server[1] == 53: + name = SERVER_NAME + expected_rcode = dns.DNS_RCODE_OK + else: + name = [encode_netbios_bytes(b'nxdomain'), b'nxdomain'] + expected_rcode = nbt.NBT_RCODE_NAM + + packet = self.construct_query([name]) + s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) + s.sendto(packet, self.server) + r, _, _ = select.select([s], [], [], TIMEOUT) + if r != [s]: + s.close() + return False + + data, addr = s.recvfrom(4096) + s.close() + rcode = self.decode_reply(data)['rcode'] + return expected_rcode == rcode + + +class TestDnsPackets(TestDnsPacketBase): + server = (SERVER, 53) + qtype = 1 # dns type A + + def _test_many_repeated_components(self, label, n, expected_rcode=None): + name = [label] * n + 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) + + def test_127_half_dotty_components(self): + label = b'x.' * 31 + b'x' + self._test_many_repeated_components(label, 127) + + +class TestNbtPackets(TestDnsPacketBase): + server = (SERVER, 137) + qtype = 0x20 # NBT_QTYPE_NETBIOS + + def _test_nbt_encode_query(self, names, *args, **kwargs): + if isinstance(names, str): + names = [names] + + nbt_names = [] + for name in names: + if isinstance(name, str): + bits = name.encode('utf8').split(b'.') + else: + bits = name + + encoded = [encode_netbios_bytes(bits[0])] + encoded.extend(bits[1:]) + nbt_names.append(encoded) + + 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) + + def test_127_half_dotty_components(self): + label = b'x.' * 31 + b'x' + self._test_many_repeated_components(label, 127) diff --git a/selftest/knownfail.d/dns_packet b/selftest/knownfail.d/dns_packet new file mode 100644 index 00000000000..fedca760912 --- /dev/null +++ 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.TestNbtPackets.test_127_half_dotty_components +samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_127_very_dotty_components diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py index 211a56e689a..ddb8a12b8d6 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -454,6 +454,16 @@ plantestsuite_loadlist("samba.tests.dns_wildcard", "ad_dc", [python, os.path.joi 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, + '-msamba.subunit.run', + '$LOADLIST', + "$LISTOPT" + "samba.tests.dns_packet" + ]) + + for t in smbtorture4_testsuites("dns_internal."): plansmbtorture4testsuite(t, "ad_dc_default:local", '//$SERVER/whavever') -- 2.17.1 From b66454c9ec117463b54ad8da5e88598f48b51a1d Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sat, 25 Apr 2020 11:02:08 +1200 Subject: [PATCH 2/6] CVE-2020-10745: ndr_dns: move ndr_push_dns_string core into sharable function This is because ndr_nbt.c does almost exactly the same thing with almost exactly the same code, and they both do it wrong. Soon they will both be using the better version that this will become. Though in this patch we just move the code, not fix it. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 Signed-off-by: Douglas Bagnall --- librpc/ndr/ndr_dns.c | 79 +++------------------------------- librpc/ndr/ndr_dns_utils.c | 88 ++++++++++++++++++++++++++++++++++++++ librpc/ndr/ndr_dns_utils.h | 5 +++ librpc/wscript_build | 2 +- 4 files changed, 99 insertions(+), 75 deletions(-) create mode 100644 librpc/ndr/ndr_dns_utils.c create mode 100644 librpc/ndr/ndr_dns_utils.h diff --git a/librpc/ndr/ndr_dns.c b/librpc/ndr/ndr_dns.c index d37c8cc2ece..68a3c9de782 100644 --- a/librpc/ndr/ndr_dns.c +++ b/librpc/ndr/ndr_dns.c @@ -33,6 +33,7 @@ #include "librpc/gen_ndr/ndr_dnsp.h" #include "system/locale.h" #include "lib/util/util_net.h" +#include "ndr_dns_utils.h" /* don't allow an unlimited number of name components */ #define MAX_COMPONENTS 128 @@ -159,80 +160,10 @@ _PUBLIC_ enum ndr_err_code ndr_push_dns_string(struct ndr_push *ndr, int ndr_flags, const char *s) { - if (!(ndr_flags & NDR_SCALARS)) { - return NDR_ERR_SUCCESS; - } - - while (s && *s) { - enum ndr_err_code ndr_err; - char *compname; - size_t complen; - uint32_t offset; - - if (!(ndr->flags & LIBNDR_FLAG_NO_COMPRESSION)) { - /* see if we have pushed the remaining string already, - * if so we use a label pointer to this string - */ - ndr_err = ndr_token_retrieve_cmp_fn(&ndr->dns_string_list, s, - &offset, - (comparison_fn_t)strcmp, - false); - if (NDR_ERR_CODE_IS_SUCCESS(ndr_err)) { - uint8_t b[2]; - - if (offset > 0x3FFF) { - return ndr_push_error(ndr, NDR_ERR_STRING, - "offset for dns string " \ - "label pointer " \ - "%u[%08X] > 0x00003FFF", - offset, offset); - } - - b[0] = 0xC0 | (offset>>8); - b[1] = (offset & 0xFF); - - return ndr_push_bytes(ndr, b, 2); - } - } - - complen = strcspn(s, "."); - - /* we need to make sure the length fits into 6 bytes */ - if (complen > 0x3F) { - return ndr_push_error(ndr, NDR_ERR_STRING, - "component length %u[%08X] > " \ - "0x0000003F", - (unsigned)complen, - (unsigned)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 - */ - if (!(ndr->flags & LIBNDR_FLAG_NO_COMPRESSION)) { - NDR_CHECK(ndr_token_store(ndr, &ndr->dns_string_list, s, - ndr->offset)); - } - - /* push just this component into the blob */ - NDR_CHECK(ndr_push_bytes(ndr, (const uint8_t *)compname, - complen+1)); - talloc_free(compname); - - s += complen; - if (*s == '.') s++; - } - - /* if we reach the end of the string and have pushed the last component - * without using a label pointer, we need to terminate the string - */ - return ndr_push_bytes(ndr, (const uint8_t *)"", 1); + return ndr_push_dns_string_list(ndr, + &ndr->dns_string_list, + ndr_flags, + s); } _PUBLIC_ enum ndr_err_code ndr_pull_dns_txt_record(struct ndr_pull *ndr, int ndr_flags, struct dns_txt_record *r) diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c new file mode 100644 index 00000000000..2d9b5f1bc1e --- /dev/null +++ b/librpc/ndr/ndr_dns_utils.c @@ -0,0 +1,88 @@ +#include "includes.h" +#include "../librpc/ndr/libndr.h" +#include "ndr_dns_utils.h" + + +/** + push a dns/nbt string list to the wire +*/ +enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, + struct ndr_token_list *string_list, + int ndr_flags, + const char *s) +{ + if (!(ndr_flags & NDR_SCALARS)) { + return NDR_ERR_SUCCESS; + } + + while (s && *s) { + enum ndr_err_code ndr_err; + char *compname; + size_t complen; + uint32_t offset; + + if (!(ndr->flags & LIBNDR_FLAG_NO_COMPRESSION)) { + /* see if we have pushed the remaining string already, + * if so we use a label pointer to this string + */ + ndr_err = ndr_token_retrieve_cmp_fn(string_list, s, + &offset, + (comparison_fn_t)strcmp, + false); + if (NDR_ERR_CODE_IS_SUCCESS(ndr_err)) { + uint8_t b[2]; + + if (offset > 0x3FFF) { + return ndr_push_error(ndr, NDR_ERR_STRING, + "offset for dns string " \ + "label pointer " \ + "%u[%08X] > 0x00003FFF", + offset, offset); + } + + b[0] = 0xC0 | (offset>>8); + b[1] = (offset & 0xFF); + + return ndr_push_bytes(ndr, b, 2); + } + } + + complen = strcspn(s, "."); + + /* we need to make sure the length fits into 6 bytes */ + if (complen > 0x3F) { + return ndr_push_error(ndr, NDR_ERR_STRING, + "component length %u[%08X] > " \ + "0x0000003F", + (unsigned)complen, + (unsigned)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 + */ + if (!(ndr->flags & LIBNDR_FLAG_NO_COMPRESSION)) { + NDR_CHECK(ndr_token_store(ndr, string_list, s, + ndr->offset)); + } + + /* push just this component into the blob */ + NDR_CHECK(ndr_push_bytes(ndr, (const uint8_t *)compname, + complen+1)); + talloc_free(compname); + + s += complen; + if (*s == '.') s++; + } + + /* if we reach the end of the string and have pushed the last component + * without using a label pointer, we need to terminate the string + */ + return ndr_push_bytes(ndr, (const uint8_t *)"", 1); +} diff --git a/librpc/ndr/ndr_dns_utils.h b/librpc/ndr/ndr_dns_utils.h new file mode 100644 index 00000000000..823e3201112 --- /dev/null +++ b/librpc/ndr/ndr_dns_utils.h @@ -0,0 +1,5 @@ + +enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, + struct ndr_token_list *string_list, + int ndr_flags, + const char *s); diff --git a/librpc/wscript_build b/librpc/wscript_build index aa112890a7a..c5f676cd3b6 100644 --- a/librpc/wscript_build +++ b/librpc/wscript_build @@ -31,7 +31,7 @@ bld.SAMBA_SUBSYSTEM('NDR_DNSSERVER', ) bld.SAMBA_SUBSYSTEM('NDR_DNS', - source='gen_ndr/ndr_dns.c ndr/ndr_dns.c', + source='gen_ndr/ndr_dns.c ndr/ndr_dns.c ndr/ndr_dns_utils.c', public_deps='ndr NDR_DNSP' ) -- 2.17.1 From ec8b7529e4bddf22012b38e91c4492b8c8bb2876 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sat, 25 Apr 2020 11:10:18 +1200 Subject: [PATCH 3/6] CVE-2020-10745: ndr_dns: do not allow consecutive dots The empty subdomain component is reserved for the root domain, which we should only (and always) see at the end of the list. That is, we expect "example.com.", but never "example..com". BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 Signed-off-by: Douglas Bagnall --- librpc/ndr/ndr_dns_utils.c | 6 ++++++ selftest/knownfail.d/dns_packet | 1 - 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c index 2d9b5f1bc1e..67cada217a7 100644 --- a/librpc/ndr/ndr_dns_utils.c +++ b/librpc/ndr/ndr_dns_utils.c @@ -58,6 +58,12 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, (unsigned)complen); } + if (complen == 0 && s[complen] == '.') { + return ndr_push_error(ndr, NDR_ERR_STRING, + "component length is 0 " + "(consecutive dots)"); + } + compname = talloc_asprintf(ndr, "%c%*.*s", (unsigned char)complen, (unsigned char)complen, diff --git a/selftest/knownfail.d/dns_packet b/selftest/knownfail.d/dns_packet index fedca760912..bb176f72d5d 100644 --- a/selftest/knownfail.d/dns_packet +++ b/selftest/knownfail.d/dns_packet @@ -1,4 +1,3 @@ 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.TestNbtPackets.test_127_half_dotty_components samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_127_very_dotty_components -- 2.17.1 From e8baf247325789fc832520854437fc28cda34ca4 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 15 May 2020 00:06:08 +1200 Subject: [PATCH 4/6] CVE-2020-10745: dns_util/push: forbid names longer than 255 bytes As per RFC 1035. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 Signed-off-by: Douglas Bagnall --- librpc/ndr/ndr_dns_utils.c | 10 +++++++++- selftest/knownfail.d/dns_packet | 1 - 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c index 67cada217a7..ab59c9853f5 100644 --- a/librpc/ndr/ndr_dns_utils.c +++ b/librpc/ndr/ndr_dns_utils.c @@ -11,6 +11,8 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, int ndr_flags, const char *s) { + const char *start = s; + if (!(ndr_flags & NDR_SCALARS)) { return NDR_ERR_SUCCESS; } @@ -84,7 +86,13 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, talloc_free(compname); s += complen; - if (*s == '.') s++; + if (*s == '.') { + s++; + } + if (s - start > 255) { + return ndr_push_error(ndr, NDR_ERR_STRING, + "name > 255 character long"); + } } /* if we reach the end of the string and have pushed the last component diff --git a/selftest/knownfail.d/dns_packet b/selftest/knownfail.d/dns_packet index bb176f72d5d..da7347d5af3 100644 --- a/selftest/knownfail.d/dns_packet +++ b/selftest/knownfail.d/dns_packet @@ -1,3 +1,2 @@ -samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_127_half_dotty_components samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_127_half_dotty_components samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_127_very_dotty_components -- 2.17.1 From 32f73624f7de08517b0bad79147eba5f24ba0ae9 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 15 May 2020 10:52:45 +1200 Subject: [PATCH 5/6] CVE-2020-10745: ndr/dns-utils: prepare for NBT compatibility NBT has a funny thing where it sometimes needs to send a trailing dot as part of the last component, because the string representation is a user name. In DNS, "example.com", and "example.com." are the same, both having three components ("example", "com", ""); in NBT, we want to treat them differently, with the second form having the three components ("example", "com.", ""). This retains the logic of e6e2ec0001fe3c010445e26cc0efddbc1f73416b. Also DNS compression cannot be turned off for NBT. 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 | 21 ++++++++++++++++++--- librpc/ndr/ndr_dns_utils.h | 3 ++- 3 files changed, 22 insertions(+), 5 deletions(-) diff --git a/librpc/ndr/ndr_dns.c b/librpc/ndr/ndr_dns.c index 68a3c9de782..966e0b59786 100644 --- a/librpc/ndr/ndr_dns.c +++ b/librpc/ndr/ndr_dns.c @@ -163,7 +163,8 @@ _PUBLIC_ enum ndr_err_code ndr_push_dns_string(struct ndr_push *ndr, return ndr_push_dns_string_list(ndr, &ndr->dns_string_list, ndr_flags, - s); + s, + false); } _PUBLIC_ enum ndr_err_code ndr_pull_dns_txt_record(struct ndr_pull *ndr, int ndr_flags, struct dns_txt_record *r) diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c index ab59c9853f5..eb0c9ab9026 100644 --- a/librpc/ndr/ndr_dns_utils.c +++ b/librpc/ndr/ndr_dns_utils.c @@ -9,9 +9,16 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, struct ndr_token_list *string_list, int ndr_flags, - const char *s) + const char *s, + bool is_nbt) { const char *start = s; + bool use_compression; + if (is_nbt) { + use_compression = true; + } else { + use_compression = !(ndr->flags & LIBNDR_FLAG_NO_COMPRESSION); + } if (!(ndr_flags & NDR_SCALARS)) { return NDR_ERR_SUCCESS; @@ -23,7 +30,7 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, size_t complen; uint32_t offset; - if (!(ndr->flags & LIBNDR_FLAG_NO_COMPRESSION)) { + if (use_compression) { /* see if we have pushed the remaining string already, * if so we use a label pointer to this string */ @@ -66,6 +73,14 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, "(consecutive dots)"); } + if (is_nbt && s[complen] == '.' && s[complen + 1] == '\0') { + /* nbt names are sometimes usernames, and we need to + * keep a trailing dot to ensure it is byte-identical, + * (not just semantically identical given DNS + * semantics). */ + complen++; + } + compname = talloc_asprintf(ndr, "%c%*.*s", (unsigned char)complen, (unsigned char)complen, @@ -75,7 +90,7 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, /* remember the current component + the rest of the string * so it can be reused later */ - if (!(ndr->flags & LIBNDR_FLAG_NO_COMPRESSION)) { + if (use_compression) { NDR_CHECK(ndr_token_store(ndr, string_list, s, ndr->offset)); } diff --git a/librpc/ndr/ndr_dns_utils.h b/librpc/ndr/ndr_dns_utils.h index 823e3201112..71a65433bbb 100644 --- a/librpc/ndr/ndr_dns_utils.h +++ b/librpc/ndr/ndr_dns_utils.h @@ -2,4 +2,5 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, struct ndr_token_list *string_list, int ndr_flags, - const char *s); + const char *s, + bool is_nbt); -- 2.17.1 From bb15db6abf811e71ee319359893f6ce0f99c0a6a Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 15 May 2020 11:48:27 +1200 Subject: [PATCH 6/6] CVE-2020-10745: ndr/nbt_push_string: use dns_utils common function BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 Signed-off-by: Douglas Bagnall --- librpc/ndr/ndr_nbt.c | 72 ++++----------------------------- librpc/wscript_build | 2 +- selftest/knownfail.d/dns_packet | 2 - 3 files changed, 8 insertions(+), 68 deletions(-) delete mode 100644 selftest/knownfail.d/dns_packet diff --git a/librpc/ndr/ndr_nbt.c b/librpc/ndr/ndr_nbt.c index b15dc5c06e5..8ed9f0a5f05 100644 --- a/librpc/ndr/ndr_nbt.c +++ b/librpc/ndr/ndr_nbt.c @@ -25,6 +25,8 @@ #include "includes.h" #include "../libcli/nbt/libnbt.h" #include "../libcli/netlogon/netlogon.h" +#include "ndr_dns_utils.h" + /* don't allow an unlimited number of name components */ #define MAX_COMPONENTS 128 @@ -141,71 +143,11 @@ _PUBLIC_ enum ndr_err_code ndr_pull_nbt_string(struct ndr_pull *ndr, int ndr_fla */ _PUBLIC_ enum ndr_err_code ndr_push_nbt_string(struct ndr_push *ndr, int ndr_flags, const char *s) { - if (!(ndr_flags & NDR_SCALARS)) { - return NDR_ERR_SUCCESS; - } - - while (s && *s) { - enum ndr_err_code ndr_err; - char *compname; - size_t complen; - uint32_t offset; - - /* see if we have pushed the remaining string already, - * if so we use a label pointer to this string - */ - ndr_err = ndr_token_retrieve_cmp_fn(&ndr->nbt_string_list, s, &offset, (comparison_fn_t)strcmp, false); - if (NDR_ERR_CODE_IS_SUCCESS(ndr_err)) { - uint8_t b[2]; - - if (offset > 0x3FFF) { - return ndr_push_error(ndr, NDR_ERR_STRING, - "offset for nbt string label pointer %u[%08X] > 0x00003FFF", - offset, offset); - } - - b[0] = 0xC0 | (offset>>8); - b[1] = (offset & 0xFF); - - return ndr_push_bytes(ndr, b, 2); - } - - complen = strcspn(s, "."); - - /* we need to make sure the length fits into 6 bytes */ - if (complen > 0x3F) { - return ndr_push_error(ndr, NDR_ERR_STRING, - "component length %u[%08X] > 0x0000003F", - (unsigned)complen, (unsigned)complen); - } - - if (s[complen] == '.' && s[complen+1] == '\0') { - 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 componemt + the rest of the string - * so it can be reused later - */ - NDR_CHECK(ndr_token_store(ndr, &ndr->nbt_string_list, s, ndr->offset)); - - /* push just this component into the blob */ - NDR_CHECK(ndr_push_bytes(ndr, (const uint8_t *)compname, complen+1)); - talloc_free(compname); - - s += complen; - if (*s == '.') s++; - } - - /* if we reach the end of the string and have pushed the last component - * without using a label pointer, we need to terminate the string - */ - return ndr_push_bytes(ndr, (const uint8_t *)"", 1); + return ndr_push_dns_string_list(ndr, + &ndr->dns_string_list, + ndr_flags, + s, + true); } diff --git a/librpc/wscript_build b/librpc/wscript_build index c5f676cd3b6..3ab24da19c7 100644 --- a/librpc/wscript_build +++ b/librpc/wscript_build @@ -415,7 +415,7 @@ bld.SAMBA_SUBSYSTEM('NDR_NBT', bld.SAMBA_LIBRARY('ndr_nbt', source='gen_ndr/ndr_nbt.c ndr/ndr_nbt.c', - public_deps='ndr NDR_NBT_BUF NDR_SECURITY', + public_deps='ndr NDR_NBT_BUF NDR_SECURITY NDR_DNS', public_headers='gen_ndr/nbt.h gen_ndr/ndr_nbt.h ndr/ndr_nbt.h', header_path=[ ('gen_ndr*', 'gen_ndr'), ('ndr*', 'ndr')], pc_files='ndr_nbt.pc', diff --git a/selftest/knownfail.d/dns_packet b/selftest/knownfail.d/dns_packet deleted file mode 100644 index da7347d5af3..00000000000 --- a/selftest/knownfail.d/dns_packet +++ /dev/null @@ -1,2 +0,0 @@ -samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_127_half_dotty_components -samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_127_very_dotty_components -- 2.17.1