From 1d9f02d821499324a2561c0c3123398cbb6822e7 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 11 Jun 2020 17:38:51 +1200 Subject: [PATCH 1/7] 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 | 211 +++++++++++++++++++++++++++++++ selftest/knownfail.d/dns_packet | 2 + source4/selftest/tests.py | 10 ++ 3 files changed, 223 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..c4f843eb613 --- /dev/null +++ b/python/samba/tests/dns_packet.py @@ -0,0 +1,211 @@ +# Tests of malformed DNS packets +# Copyright (C) Catalyst.NET ltd +# +# written by Douglas Bagnall +# +# 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 . + +"""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..6e2e5a699de --- /dev/null +++ b/selftest/knownfail.d/dns_packet @@ -0,0 +1,2 @@ +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_very_dotty_components diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py index 19c81b46ea4..23416df221c 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -416,6 +416,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_ntvfs:local", '//$SERVER/whavever') -- 2.17.1 From 5b89b6e7d28d87587626eb063d168a398e7c4161 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 12 Jun 2020 14:26:38 +1200 Subject: [PATCH 2/7] CVE-2020-10745: librpc/tests: cmocka tests of dns and ndr strings These time the push and pull function in isolation. Timing should be under 0.0001 seconds on even quite old hardware; we assert it must be under 0.2 seconds. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 (backported from master commit) [abartlet@samba.org: backported due to differences in pre-existing tests - eg test_ndr - mentioned in wscript_build and tests.py] Signed-off-by: Douglas Bagnall --- librpc/tests/test_ndr_dns_nbt.c | 236 +++++++++++++++++++++++++++++++ librpc/wscript_build | 13 ++ selftest/knownfail.d/ndr_dns_nbt | 4 + source4/selftest/tests.py | 2 + 4 files changed, 255 insertions(+) create mode 100644 librpc/tests/test_ndr_dns_nbt.c create mode 100644 selftest/knownfail.d/ndr_dns_nbt diff --git a/librpc/tests/test_ndr_dns_nbt.c b/librpc/tests/test_ndr_dns_nbt.c new file mode 100644 index 00000000000..1e2ef45c10d --- /dev/null +++ b/librpc/tests/test_ndr_dns_nbt.c @@ -0,0 +1,236 @@ +/* + * Tests for librpc ndr functions + * + * Copyright (C) Catalyst.NET Ltd 2020 + * + * 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 . + * + */ + +#include "replace.h" +#include +#include + +#include "includes.h" +#include "librpc/ndr/libndr.h" +#include "librpc/gen_ndr/ndr_dns.h" +#include "librpc/gen_ndr/ndr_nbt.h" +#include "lib/util/time.h" + +#define NBT_NAME "EOGFGLGPCACACACACACACACACACACACA" /* "neko" */ + + +static DATA_BLOB generate_obnoxious_dns_name(TALLOC_CTX *mem_ctx, + size_t n_labels, + size_t dot_every, + bool is_nbt) +{ + size_t i, j; + char *s; + DATA_BLOB name = data_blob_talloc(mem_ctx, NULL, 64 * n_labels + 1); + assert_non_null(name.data); + + s = (char*)name.data; + if (is_nbt) { + size_t len = strlen(NBT_NAME); + *s = len; + s++; + memcpy(s, NBT_NAME, len); + s += len; + n_labels--; + } + + for (i = 0; i < n_labels; i++) { + *s = 63; + s++; + for (j = 0; j < 63; j++) { + if (j % dot_every == (dot_every - 1)) { + *s = '.'; + } else { + *s = 'x'; + } + s++; + } + } + *s = 0; + s++; + name.length = s - (char*)name.data; + return name; +} + + +static char *_test_ndr_pull_dns_string_list(TALLOC_CTX *mem_ctx, + size_t n_labels, + size_t dot_every, + bool is_nbt) +{ + enum ndr_err_code ndr_err; + DATA_BLOB blob = generate_obnoxious_dns_name(mem_ctx, + n_labels, + dot_every, + is_nbt); + + char *name; + ndr_pull_flags_fn_t fn; + + if (is_nbt) { + fn = (ndr_pull_flags_fn_t)ndr_pull_nbt_string; + } else { + fn = (ndr_pull_flags_fn_t)ndr_pull_dns_string; + } + + ndr_err = ndr_pull_struct_blob(&blob, + mem_ctx, + &name, + fn); + /* Success here is not expected, but we let it go to measure timing. */ + if (ndr_err == NDR_ERR_SUCCESS) { + printf("pull succeed\n"); + } else { + assert_int_equal(ndr_err, NDR_ERR_STRING); + } + + TALLOC_FREE(blob.data); + return name; +} + + +static void _test_ndr_push_dns_string_list(TALLOC_CTX *mem_ctx, + char *name, + bool is_nbt) +{ + DATA_BLOB blob; + enum ndr_err_code ndr_err; + ndr_push_flags_fn_t fn; + + if (is_nbt) { + fn = (ndr_push_flags_fn_t)ndr_push_nbt_string; + } else { + fn = (ndr_push_flags_fn_t)ndr_push_dns_string; + } + + ndr_err = ndr_push_struct_blob(&blob, + mem_ctx, + name, + fn); + + /* Success here is not expected, but we let it go to measure timing. */ + if (ndr_err == NDR_ERR_SUCCESS) { + printf("push succeed\n"); + } else { + assert_int_equal(ndr_err, NDR_ERR_STRING); + } +} + + +static uint64_t elapsed_time(struct timespec start, const char *print) +{ + struct timespec end; + unsigned long long microsecs; + clock_gettime_mono(&end); + end.tv_sec -= start.tv_sec; + if (end.tv_nsec < start.tv_nsec) { + /* we need to borrow */ + end.tv_nsec += 1000 * 1000 * 1000; + end.tv_sec -= 1; + } + end.tv_nsec -= start.tv_nsec; + microsecs = end.tv_sec * 1000000; + microsecs += end.tv_nsec / 1000; + + if (print != NULL) { + printf(" %s: %llu microseconds\n", print, microsecs); + } + return microsecs; +} + + +static void test_ndr_dns_string_half_dots(void **state) +{ + TALLOC_CTX *mem_ctx = talloc_new(NULL); + char *name; + struct timespec start; + uint64_t elapsed; + + clock_gettime_mono(&start); + name =_test_ndr_pull_dns_string_list(mem_ctx, 127, 2, false); + elapsed_time(start, "pull"); + _test_ndr_push_dns_string_list(mem_ctx, name, false); + elapsed = elapsed_time(start, "total"); + assert_in_range(elapsed, 0, 200000); + talloc_free(mem_ctx); +} + +static void test_ndr_nbt_string_half_dots(void **state) +{ + TALLOC_CTX *mem_ctx = talloc_new(NULL); + char *name; + struct timespec start; + uint64_t elapsed; + + clock_gettime_mono(&start); + name =_test_ndr_pull_dns_string_list(mem_ctx, 127, 2, true); + elapsed_time(start, "pull"); + _test_ndr_push_dns_string_list(mem_ctx, name, true); + elapsed = elapsed_time(start, "total"); + assert_in_range(elapsed, 0, 200000); + talloc_free(mem_ctx); +} + +static void test_ndr_dns_string_all_dots(void **state) +{ + TALLOC_CTX *mem_ctx = talloc_new(NULL); + char *name; + struct timespec start; + uint64_t elapsed; + + clock_gettime_mono(&start); + name =_test_ndr_pull_dns_string_list(mem_ctx, 127, 1, false); + elapsed_time(start, "pull"); + _test_ndr_push_dns_string_list(mem_ctx, name, false); + elapsed = elapsed_time(start, "total"); + assert_in_range(elapsed, 0, 200000); + talloc_free(mem_ctx); +} + +static void test_ndr_nbt_string_all_dots(void **state) +{ + TALLOC_CTX *mem_ctx = talloc_new(NULL); + char *name; + struct timespec start; + uint64_t elapsed; + + clock_gettime_mono(&start); + name =_test_ndr_pull_dns_string_list(mem_ctx, 127, 1, true); + elapsed_time(start, "pull"); + _test_ndr_push_dns_string_list(mem_ctx, name, true); + elapsed = elapsed_time(start, "total"); + assert_in_range(elapsed, 0, 200000); + talloc_free(mem_ctx); +} + + + +int main(int argc, const char **argv) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test(test_ndr_nbt_string_half_dots), + cmocka_unit_test(test_ndr_dns_string_half_dots), + cmocka_unit_test(test_ndr_nbt_string_all_dots), + cmocka_unit_test(test_ndr_dns_string_all_dots), + }; + + cmocka_set_message_output(CM_OUTPUT_SUBUNIT); + return cmocka_run_group_tests(tests, NULL, NULL); +} diff --git a/librpc/wscript_build b/librpc/wscript_build index 8e113c422b2..0e700d629df 100644 --- a/librpc/wscript_build +++ b/librpc/wscript_build @@ -756,3 +756,16 @@ bld.SAMBA_SUBSYSTEM('NDR_FSRVP_STATE', source='gen_ndr/ndr_fsrvp_state.c', public_deps='ndr' ) +# +# Cmocka tests +# + +bld.SAMBA_BINARY('test_ndr_dns_nbt', + source='tests/test_ndr_dns_nbt.c', + deps=''' + cmocka + ndr + ndr_nbt + NDR_DNS + ''', + for_selftest=True) diff --git a/selftest/knownfail.d/ndr_dns_nbt b/selftest/knownfail.d/ndr_dns_nbt new file mode 100644 index 00000000000..f30217c4033 --- /dev/null +++ b/selftest/knownfail.d/ndr_dns_nbt @@ -0,0 +1,4 @@ +librpc.ndr.ndr_dns_nbt.test_ndr_dns_string_all_dots +librpc.ndr.ndr_dns_nbt.test_ndr_dns_string_half_dots +librpc.ndr.ndr_dns_nbt.test_ndr_nbt_string_all_dots +librpc.ndr.ndr_dns_nbt.test_ndr_nbt_string_half_dots diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py index 23416df221c..8cf54841e86 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -1303,6 +1303,8 @@ plantestsuite("samba4.dsdb.samdb.ldb_modules.group_audit.errors", "none", [os.path.join(bindir(), "test_group_audit_errors")]) plantestsuite("samba4.dcerpc.dnsserver.dnsutils", "none", [os.path.join(bindir(), "test_rpc_dns_server_dnsutils")]) +plantestsuite("librpc.ndr.ndr_dns_nbt", "none", + [os.path.join(bindir(), "test_ndr_dns_nbt")]) plantestsuite("libcli.ldap.ldap_message", "none", [os.path.join(bindir(), "test_ldap_message")]) -- 2.17.1 From 2cde5a15d1b365bd25af83d5991b4a84344d8c8e Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sat, 25 Apr 2020 11:02:08 +1200 Subject: [PATCH 3/7] 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 0e700d629df..83cf967a4ec 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 58601cf061506f4b1805b855b4d1ad71dfcdb20a Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sat, 25 Apr 2020 11:03:30 +1200 Subject: [PATCH 4/7] CVE-2020-10745: ndr/dns_utils: correct a comment BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 Signed-off-by: Douglas Bagnall --- 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 2d9b5f1bc1e..2ce300863bc 100644 --- a/librpc/ndr/ndr_dns_utils.c +++ b/librpc/ndr/ndr_dns_utils.c @@ -49,7 +49,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 0e08476b5a0041a8113011dfebe485a39d9766c2 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sat, 25 Apr 2020 11:10:18 +1200 Subject: [PATCH 5/7] 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 - selftest/knownfail.d/ndr_dns_nbt | 1 - 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c index 2ce300863bc..6931dac422d 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 6e2e5a699de..0662266f689 100644 --- a/selftest/knownfail.d/dns_packet +++ b/selftest/knownfail.d/dns_packet @@ -1,2 +1 @@ -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_very_dotty_components diff --git a/selftest/knownfail.d/ndr_dns_nbt b/selftest/knownfail.d/ndr_dns_nbt index f30217c4033..e11c121b7a7 100644 --- a/selftest/knownfail.d/ndr_dns_nbt +++ b/selftest/knownfail.d/ndr_dns_nbt @@ -1,4 +1,3 @@ -librpc.ndr.ndr_dns_nbt.test_ndr_dns_string_all_dots librpc.ndr.ndr_dns_nbt.test_ndr_dns_string_half_dots librpc.ndr.ndr_dns_nbt.test_ndr_nbt_string_all_dots librpc.ndr.ndr_dns_nbt.test_ndr_nbt_string_half_dots -- 2.17.1 From 5be27a37adf088a2cbeaa0ea64be051d0459a185 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 15 May 2020 00:06:08 +1200 Subject: [PATCH 6/7] 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/ndr_dns_nbt | 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 6931dac422d..b7f11dbab4e 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/ndr_dns_nbt b/selftest/knownfail.d/ndr_dns_nbt index e11c121b7a7..603395c8c50 100644 --- a/selftest/knownfail.d/ndr_dns_nbt +++ b/selftest/knownfail.d/ndr_dns_nbt @@ -1,3 +1,2 @@ -librpc.ndr.ndr_dns_nbt.test_ndr_dns_string_half_dots librpc.ndr.ndr_dns_nbt.test_ndr_nbt_string_all_dots librpc.ndr.ndr_dns_nbt.test_ndr_nbt_string_half_dots -- 2.17.1 From 9fd907c0baf8a24b8fb524823bf6078347e62fc3 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 15 May 2020 10:52:45 +1200 Subject: [PATCH 7/7] 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 | 42 ++++++++++++++++--- librpc/ndr/ndr_dns_utils.h | 3 +- librpc/ndr/ndr_nbt.c | 72 ++++---------------------------- librpc/wscript_build | 3 +- selftest/knownfail.d/dns_packet | 1 - selftest/knownfail.d/ndr_dns_nbt | 2 - 7 files changed, 49 insertions(+), 77 deletions(-) delete mode 100644 selftest/knownfail.d/ndr_dns_nbt 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 b7f11dbab4e..2c3ddf08bb2 100644 --- a/librpc/ndr/ndr_dns_utils.c +++ b/librpc/ndr/ndr_dns_utils.c @@ -9,9 +9,32 @@ 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; + size_t max_length; + if (is_nbt) { + use_compression = true; + /* + * Max length is longer in NBT/Wins, because Windows counts + * the semi-decompressed size of the netbios name (16 bytes) + * rather than the wire size of 32, which is what you'd expect + * if it followed RFC1002 (it uses the short form in + * [MS-WINSRA]). In other words the maximum size of the + * "scope" is 237, not 221. + * + * We make the size limit slightly larger than 255 + 32, + * because the 237 scope limit is already enforced in the + * winsserver code with a specific return value; bailing out + * here would muck with that. + */ + max_length = 274; + } else { + use_compression = !(ndr->flags & LIBNDR_FLAG_NO_COMPRESSION); + max_length = 255; + } if (!(ndr_flags & NDR_SCALARS)) { return NDR_ERR_SUCCESS; @@ -23,7 +46,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 +89,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 +106,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)); } @@ -89,9 +120,10 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, if (*s == '.') { s++; } - if (s - start > 255) { + if (s - start > max_length) { return ndr_push_error(ndr, NDR_ERR_STRING, - "name > 255 character long"); + "name > %zu character long", + max_length); } } 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); diff --git a/librpc/ndr/ndr_nbt.c b/librpc/ndr/ndr_nbt.c index 838f947a168..e8dd7549a53 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 83cf967a4ec..76ca2a6e7cb 100644 --- a/librpc/wscript_build +++ b/librpc/wscript_build @@ -411,7 +411,7 @@ bld.SAMBA_SUBSYSTEM('NDR_SCHANNEL', 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', @@ -766,6 +766,5 @@ bld.SAMBA_BINARY('test_ndr_dns_nbt', cmocka ndr ndr_nbt - NDR_DNS ''', for_selftest=True) diff --git a/selftest/knownfail.d/dns_packet b/selftest/knownfail.d/dns_packet index 0662266f689..e69de29bb2d 100644 --- a/selftest/knownfail.d/dns_packet +++ b/selftest/knownfail.d/dns_packet @@ -1 +0,0 @@ -samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_127_very_dotty_components diff --git a/selftest/knownfail.d/ndr_dns_nbt b/selftest/knownfail.d/ndr_dns_nbt deleted file mode 100644 index 603395c8c50..00000000000 --- a/selftest/knownfail.d/ndr_dns_nbt +++ /dev/null @@ -1,2 +0,0 @@ -librpc.ndr.ndr_dns_nbt.test_ndr_nbt_string_all_dots -librpc.ndr.ndr_dns_nbt.test_ndr_nbt_string_half_dots -- 2.17.1