The Samba-Bugzilla – Attachment 16093 Details for
Bug 14378
CVE-2020-10745 [SECURITY] invalid DNS or NBT queries containing dots use several seconds of CPU each
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch for V4.5 (v2)
CVE-2020-10745-V4-5-v2.patch (text/plain), 29.77 KB, created by
Gary Lockyer
on 2020-06-26 03:18:26 UTC
(
hide
)
Description:
Patch for V4.5 (v2)
Filename:
MIME Type:
Creator:
Gary Lockyer
Created:
2020-06-26 03:18:26 UTC
Size:
29.77 KB
patch
obsolete
>From a0fbd9ab5612756b3bee9c6ae0e37cb865cc4651 Mon Sep 17 00:00:00 2001 >From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >Date: Thu, 11 Jun 2020 17:38:51 +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 <douglas.bagnall@catalyst.net.nz> > >(backported from patch for master) >[abartlet@samba.org: f"" strings are not in Python 3.4 and >bytes cannot be formatted in python 3.4] >[gary@catalyst.net.nz > - cmocka tests not ported as cmocka is not in 4.5 > - fixed tests to run in python 2.7 > - knownfail.d does not exist in samba 4.5 >] > >Signed-off-by: Andrew Bartlett <abartlet@samba.org> >--- > python/samba/tests/dns_packet.py | 219 +++++++++++++++++++++++++++++++ > selftest/knownfail | 7 + > source4/selftest/tests.py | 10 ++ > 3 files changed, 236 insertions(+) > create mode 100644 python/samba/tests/dns_packet.py > >diff --git a/python/samba/tests/dns_packet.py b/python/samba/tests/dns_packet.py >new file mode 100644 >index 00000000000..c8ff2f96926 >--- /dev/null >+++ b/python/samba/tests/dns_packet.py >@@ -0,0 +1,219 @@ >+# Tests of malformed DNS packets >+# Copyright (C) Catalyst.NET ltd >+# >+# written by Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >+# >+# This program is free software; you can redistribute it and/or modify >+# it under the terms of the GNU General Public License as published by >+# the Free Software Foundation; either version 3 of the License, or >+# (at your option) any later version. >+# >+# This program is distributed in the hope that it will be useful, >+# but WITHOUT ANY WARRANTY; without even the implied warranty of >+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >+# GNU General Public License for more details. >+# >+# You should have received a copy of the GNU General Public License >+# along with this program. If not, see <http://www.gnu.org/licenses/>. >+ >+"""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 = 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((ord(c) >> 4) + 65) >+ out.append((ord(c) & 15) + 65) >+ return "".join(chr(x) for x in 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("the server is STILL unresponsive after %d seconds" % (40 * TIMEOUT)) >+ >+ 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.split(b'.') >+ else: >+ bits = name >+ >+ for b in bits: >+ encoded_bits.append(chr(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, "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) >+ >+ def test_known_good(self): >+ ok = self._known_good_query() >+ self.assertTrue(ok, "the server is unresponsive") >+ >+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) >+ >+ def test_known_good(self): >+ ok = self._known_good_query() >+ self.assertTrue(ok, "the server is unresponsive") >+ >diff --git a/selftest/knownfail b/selftest/knownfail >index 4cd6a068c5e..e2617ba2928 100644 >--- a/selftest/knownfail >+++ b/selftest/knownfail >@@ -293,3 +293,10 @@ > > ^samba4.ldap.vlv.python.*test_vlv_paged > ^samba4.asq.python.*asq_vlv_paged >+ >+# >+# >+samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_127_very_dotty_components >+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_very_dotty_components >+samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_127_half_dotty_components >diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py >index ee76c561b0e..0e0da4ec47d 100755 >--- a/source4/selftest/tests.py >+++ b/source4/selftest/tests.py >@@ -363,6 +363,16 @@ plantestsuite_loadlist("samba.tests.dns_forwarder", "fl2003dc:local", [python, o > > plantestsuite_loadlist("samba.tests.dns_tkey", "fl2008r2dc", [python, os.path.join(srcdir(), "python/samba/tests/dns_tkey.py"), '$SERVER', '$SERVER_IP', '--machine-pass', '-U"$USERNAME%$PASSWORD"', '--workgroup=$DOMAIN', '$LOADLIST', '$LISTOPT']) > >+plantestsuite_loadlist("samba.tests.dns_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 6366a818a8e3a650f37699e2a57c744dc52d81a7 Mon Sep 17 00:00:00 2001 >From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >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 <douglas.bagnall@catalyst.net.nz> >(backported from patch for master) >[gary@catalyst.net.nz > - manually add changes to ndr_push_string_list signature from commit > 70923b7521786d59b76e651d566bbd61fea024cc >] >--- > 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 4e4c556e20d..85525ed301e 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..9a0f50ad6b0 >--- /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..9e80db507bc >--- /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 d8fbf5d5005..6db272345f2 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 4a283fa1726a85ccff056f4f2325c00b428fdb6b Mon Sep 17 00:00:00 2001 >From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >Date: Sat, 25 Apr 2020 11:03:30 +1200 >Subject: [PATCH 3/6] CVE-2020-10745: ndr/dns_utils: correct a comment > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 > >Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >--- > librpc/ndr/ndr_dns_utils.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c >index 9a0f50ad6b0..9210ffc5ef4 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 1a6c6e63a337d2a3d79e84fe33f6150cfe35e51a Mon Sep 17 00:00:00 2001 >From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >Date: Sat, 25 Apr 2020 11:10:18 +1200 >Subject: [PATCH 4/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 <douglas.bagnall@catalyst.net.nz> >--- > librpc/ndr/ndr_dns_utils.c | 6 ++++++ > selftest/knownfail | 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 9210ffc5ef4..ef8e4d22230 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 b/selftest/knownfail >index e2617ba2928..c36e254c1ba 100644 >--- a/selftest/knownfail >+++ b/selftest/knownfail >@@ -296,7 +296,6 @@ > > # > # >-samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_127_very_dotty_components > 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_very_dotty_components > samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_127_half_dotty_components >-- >2.17.1 > > >From 5893beac13478a8ade80e45483f3d47b24ba7bb1 Mon Sep 17 00:00:00 2001 >From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >Date: Fri, 15 May 2020 00:06:08 +1200 >Subject: [PATCH 5/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 <douglas.bagnall@catalyst.net.nz> >--- > librpc/ndr/ndr_dns_utils.c | 10 +++++++++- > selftest/knownfail | 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 ef8e4d22230..4a8d28ae242 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 b/selftest/knownfail >index c36e254c1ba..6bb290d1ea5 100644 >--- a/selftest/knownfail >+++ b/selftest/knownfail >@@ -296,6 +296,5 @@ > > # > # >-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_very_dotty_components > samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_127_half_dotty_components >-- >2.17.1 > > >From 3460a0d065f32a83ea610b09257cdb24ffef84e7 Mon Sep 17 00:00:00 2001 >From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz> >Date: Fri, 15 May 2020 10:52:45 +1200 >Subject: [PATCH 6/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 <douglas.bagnall@catalyst.net.nz> >--- > 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 | 2 +- > selftest/knownfail | 4 --- > 6 files changed, 49 insertions(+), 77 deletions(-) > >diff --git a/librpc/ndr/ndr_dns.c b/librpc/ndr/ndr_dns.c >index 85525ed301e..5629f25c287 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 4a8d28ae242..538a0a1eb1a 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 + 16, >+ * 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 9e80db507bc..de41b6130c0 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 6db272345f2..bc43a0906bb 100644 >--- a/librpc/wscript_build >+++ b/librpc/wscript_build >@@ -401,7 +401,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', >diff --git a/selftest/knownfail b/selftest/knownfail >index 6bb290d1ea5..56a2961b1ae 100644 >--- a/selftest/knownfail >+++ b/selftest/knownfail >@@ -294,7 +294,3 @@ > ^samba4.ldap.vlv.python.*test_vlv_paged > ^samba4.asq.python.*asq_vlv_paged > >-# >-# >-samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_127_very_dotty_components >-samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_127_half_dotty_components >-- >2.17.1 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Flags:
dbagnall
:
review+
Actions:
View
Attachments on
bug 14378
:
15978
|
15982
|
15999
|
16000
|
16001
|
16015
|
16016
|
16034
|
16035
|
16036
|
16037
|
16038
|
16039
|
16040
|
16041
|
16042
|
16045
|
16046
|
16047
|
16048
|
16049
|
16050
|
16051
|
16052
|
16053
|
16057
|
16068
|
16090
| 16093 |
16099