From b63bcca94c42e3ca22fe5fad398d18b7b81009b4 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Tue, 6 Nov 2018 12:10:07 +1300 Subject: [PATCH 1/7] CVE-2018-16852 dcerpc dnsserver: Verification tests Tests to verify Bug 13669 - (CVE-2018-16852) NULL pointer de-reference in Samba AD DC DNS management The presence of the ZONE_MASTER_SERVERS property or the ZONE_SCAVENGING_SERVERS property in a zone record causes the server to follow a null pointer and terminate. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13669 Signed-off-by: Gary Lockyer Reviewed-by: Andrew Bartlett --- selftest/knownfail.d/bug13669 | 4 + .../tests/rpc_dns_server_dnsutils_test.c | 304 +++++++++++++++++++++ source4/rpc_server/wscript_build | 16 +- source4/selftest/tests.py | 2 + 4 files changed, 324 insertions(+), 2 deletions(-) create mode 100644 selftest/knownfail.d/bug13669 create mode 100644 source4/rpc_server/tests/rpc_dns_server_dnsutils_test.c diff --git a/selftest/knownfail.d/bug13669 b/selftest/knownfail.d/bug13669 new file mode 100644 index 00000000000..74c8c130674 --- /dev/null +++ b/selftest/knownfail.d/bug13669 @@ -0,0 +1,4 @@ +^samba4.dcerpc.dnsserver.dnsutils.test_dnsserver_init_zoneinfo_master_servers_empty +^samba4.dcerpc.dnsserver.dnsutils.test_dnsserver_init_zoneinfo_master_servers +^samba4.dcerpc.dnsserver.dnsutils.test_dnsserver_init_zoneinfo_scavenging_servers_empty +^samba4.dcerpc.dnsserver.dnsutils.test_dnsserver_init_zoneinfo_scavenging_servers diff --git a/source4/rpc_server/tests/rpc_dns_server_dnsutils_test.c b/source4/rpc_server/tests/rpc_dns_server_dnsutils_test.c new file mode 100644 index 00000000000..89721135658 --- /dev/null +++ b/source4/rpc_server/tests/rpc_dns_server_dnsutils_test.c @@ -0,0 +1,304 @@ +/* + * Unit tests for source4/rpc_server/dnsserver/dnsutils.c + * + * Copyright (C) Catalyst.NET Ltd 2018 + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + */ + +/* + * from cmocka.c: + * These headers or their equivalents should be included prior to + * including + * this header file. + * + * #include + * #include + * #include + * + * This allows test applications to use custom definitions of C standard + * library functions and types. + * + */ + +#include +#include +#include +#include + + +#include "../dnsserver/dnsutils.c" + + +/* + * Test setting of an empty ZONE_MASTER_SERVERS property + */ +static void test_dnsserver_init_zoneinfo_master_servers_empty(void **state) +{ + struct dnsserver_zone *zone = NULL; + struct dnsserver_serverinfo *serverinfo = NULL; + struct dnsserver_zoneinfo *zoneinfo = NULL; + struct dnsp_DnsProperty *property = NULL; + + TALLOC_CTX *ctx = talloc_new(NULL); + + /* + * Setup the zone data + */ + zone = talloc_zero(ctx, struct dnsserver_zone); + assert_non_null(zone); + zone->name = "test"; + + /* + * Set up an empty ZONE_MASTER_SERVERS property + */ + property = talloc_zero_array(ctx, struct dnsp_DnsProperty, 1); + assert_non_null(property); + property->id = DSPROPERTY_ZONE_MASTER_SERVERS; + property->data.master_servers.addrCount = 0; + property->data.master_servers.addr = NULL; + + zone->tmp_props = property; + zone->num_props = 1; + + + /* + * Setup the server info + */ + serverinfo = talloc_zero(ctx, struct dnsserver_serverinfo); + assert_non_null(serverinfo); + + /* + * call dnsserver_init_zoneinfo + */ + zoneinfo = dnsserver_init_zoneinfo(zone, serverinfo); + + /* + * Check results + */ + assert_non_null(zoneinfo); + assert_non_null(zoneinfo->aipLocalMasters); + assert_int_equal(zoneinfo->aipLocalMasters->AddrCount, 0); + assert_null(zoneinfo->aipLocalMasters->AddrArray); + + TALLOC_FREE(ctx); +} + +/* + * Test setting of a non empty ZONE_MASTER_SERVERS property + */ +static void test_dnsserver_init_zoneinfo_master_servers(void **state) +{ + struct dnsserver_zone *zone = NULL; + struct dnsserver_serverinfo *serverinfo = NULL; + struct dnsserver_zoneinfo *zoneinfo = NULL; + struct dnsp_DnsProperty *property = NULL; + + TALLOC_CTX *ctx = talloc_new(NULL); + + /* + * Setup the zone data + */ + zone = talloc_zero(ctx, struct dnsserver_zone); + assert_non_null(zone); + zone->name = "test"; + + /* + * Set up an empty ZONE_MASTER_SERVERS property + */ + property = talloc_zero_array(ctx, struct dnsp_DnsProperty, 1); + assert_non_null(property); + property->id = DSPROPERTY_ZONE_MASTER_SERVERS; + property->data.master_servers.addrCount = 4; + property->data.master_servers.addr = + talloc_zero_array(ctx, uint32_t, 4); + property->data.master_servers.addr[0] = 1000; + property->data.master_servers.addr[1] = 1001; + property->data.master_servers.addr[2] = 1002; + property->data.master_servers.addr[3] = 1003; + + zone->tmp_props = property; + zone->num_props = 1; + + + /* + * Setup the server info + */ + serverinfo = talloc_zero(ctx, struct dnsserver_serverinfo); + assert_non_null(serverinfo); + + /* + * call dnsserver_init_zoneinfo + */ + zoneinfo = dnsserver_init_zoneinfo(zone, serverinfo); + + /* + * Check results + */ + assert_non_null(zoneinfo); + assert_non_null(zoneinfo->aipLocalMasters); + assert_int_equal(zoneinfo->aipLocalMasters->AddrCount, 4); + assert_non_null(zoneinfo->aipLocalMasters->AddrArray); + assert_int_equal(zoneinfo->aipLocalMasters->AddrArray[0], 1000); + assert_int_equal(zoneinfo->aipLocalMasters->AddrArray[1], 1001); + assert_int_equal(zoneinfo->aipLocalMasters->AddrArray[2], 1002); + assert_int_equal(zoneinfo->aipLocalMasters->AddrArray[3], 1003); + + /* + * Ensure that we're working with a copy of the property data + * and not a reference. + * The pointers should be different, and we should be able to change + * the values in the property without affecting the zoneinfo data + */ + assert_true(zoneinfo->aipLocalMasters->AddrArray != + property->data.master_servers.addr); + property->data.master_servers.addr[0] = 0; + property->data.master_servers.addr[1] = 1; + property->data.master_servers.addr[2] = 2; + property->data.master_servers.addr[3] = 3; + assert_int_equal(zoneinfo->aipLocalMasters->AddrArray[0], 1000); + assert_int_equal(zoneinfo->aipLocalMasters->AddrArray[1], 1001); + assert_int_equal(zoneinfo->aipLocalMasters->AddrArray[2], 1002); + assert_int_equal(zoneinfo->aipLocalMasters->AddrArray[3], 1003); + + TALLOC_FREE(ctx); +} + +/* + * Test setting of an empty ZONE_SCAVENGING_SERVERS property + */ +static void test_dnsserver_init_zoneinfo_scavenging_servers_empty(void **state) +{ + struct dnsserver_zone *zone = NULL; + struct dnsserver_serverinfo *serverinfo = NULL; + struct dnsserver_zoneinfo *zoneinfo = NULL; + struct dnsp_DnsProperty *property = NULL; + + TALLOC_CTX *ctx = talloc_new(NULL); + + /* + * Setup the zone data + */ + zone = talloc_zero(ctx, struct dnsserver_zone); + assert_non_null(zone); + zone->name = "test"; + + property = talloc_zero_array(ctx, struct dnsp_DnsProperty, 1); + assert_non_null(property); + property->id = DSPROPERTY_ZONE_SCAVENGING_SERVERS; + property->data.servers.addrCount = 0; + property->data.servers.addr = NULL; + + zone->tmp_props = property; + zone->num_props = 1; + + + serverinfo = talloc_zero(ctx, struct dnsserver_serverinfo); + assert_non_null(serverinfo); + + zoneinfo = dnsserver_init_zoneinfo(zone, serverinfo); + + assert_non_null(zoneinfo); + assert_non_null(zoneinfo->aipScavengeServers); + assert_int_equal(zoneinfo->aipScavengeServers->AddrCount, 0); + assert_null(zoneinfo->aipScavengeServers->AddrArray); + + TALLOC_FREE(ctx); +} + +/* + * Test setting of a non empty ZONE_SCAVENGING_SERVERS property + */ +static void test_dnsserver_init_zoneinfo_scavenging_servers(void **state) +{ + struct dnsserver_zone *zone = NULL; + struct dnsserver_serverinfo *serverinfo = NULL; + struct dnsserver_zoneinfo *zoneinfo = NULL; + struct dnsp_DnsProperty *property = NULL; + + TALLOC_CTX *ctx = talloc_new(NULL); + + /* + * Setup the zone data + */ + zone = talloc_zero(ctx, struct dnsserver_zone); + assert_non_null(zone); + zone->name = "test"; + + property = talloc_zero_array(ctx, struct dnsp_DnsProperty, 1); + assert_non_null(property); + property->id = DSPROPERTY_ZONE_SCAVENGING_SERVERS; + property->data.servers.addrCount = 4; + property->data.servers.addr = talloc_zero_array(ctx, uint32_t, 4); + property->data.servers.addr[0] = 1000; + property->data.servers.addr[1] = 1001; + property->data.servers.addr[2] = 1002; + property->data.servers.addr[3] = 1003; + + zone->tmp_props = property; + zone->num_props = 1; + + + serverinfo = talloc_zero(ctx, struct dnsserver_serverinfo); + assert_non_null(serverinfo); + + zoneinfo = dnsserver_init_zoneinfo(zone, serverinfo); + + assert_non_null(zoneinfo); + assert_non_null(zoneinfo->aipScavengeServers); + assert_int_equal(zoneinfo->aipScavengeServers->AddrCount, 4); + assert_non_null(zoneinfo->aipScavengeServers->AddrArray); + assert_non_null(zoneinfo->aipScavengeServers->AddrArray); + assert_int_equal(zoneinfo->aipScavengeServers->AddrArray[0], 1000); + assert_int_equal(zoneinfo->aipScavengeServers->AddrArray[1], 1001); + assert_int_equal(zoneinfo->aipScavengeServers->AddrArray[2], 1002); + assert_int_equal(zoneinfo->aipScavengeServers->AddrArray[3], 1003); + + /* + * Ensure that we're working with a copy of the property data + * and not a reference. + * The pointers should be different, and we should be able to change + * the values in the property without affecting the zoneinfo data + */ + assert_true(zoneinfo->aipScavengeServers->AddrArray != + property->data.servers.addr); + property->data.servers.addr[0] = 0; + property->data.servers.addr[1] = 1; + property->data.servers.addr[2] = 2; + property->data.servers.addr[3] = 3; + assert_int_equal(zoneinfo->aipScavengeServers->AddrArray[0], 1000); + assert_int_equal(zoneinfo->aipScavengeServers->AddrArray[1], 1001); + assert_int_equal(zoneinfo->aipScavengeServers->AddrArray[2], 1002); + assert_int_equal(zoneinfo->aipScavengeServers->AddrArray[3], 1003); + + + TALLOC_FREE(ctx); +} +int main(int argc, const char **argv) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test( + test_dnsserver_init_zoneinfo_master_servers_empty), + cmocka_unit_test( + test_dnsserver_init_zoneinfo_master_servers), + cmocka_unit_test( + test_dnsserver_init_zoneinfo_scavenging_servers_empty), + cmocka_unit_test( + test_dnsserver_init_zoneinfo_scavenging_servers), + }; + + cmocka_set_message_output(CM_OUTPUT_SUBUNIT); + return cmocka_run_group_tests(tests, NULL, NULL); +} diff --git a/source4/rpc_server/wscript_build b/source4/rpc_server/wscript_build index 8e05eb8a0c3..80dee974aac 100644 --- a/source4/rpc_server/wscript_build +++ b/source4/rpc_server/wscript_build @@ -3,7 +3,7 @@ bld.SAMBA_SUBSYSTEM('DCERPC_SHARE', source='common/share_info.c', autoproto='common/share.h', - deps='ldb', + deps='ldb share', enabled=bld.CONFIG_SET('WITH_NTVFS_FILESERVER'), ) @@ -163,7 +163,7 @@ bld.SAMBA_MODULE('dcerpc_dnsserver', source='dnsserver/dcerpc_dnsserver.c dnsserver/dnsutils.c dnsserver/dnsdata.c dnsserver/dnsdb.c', subsystem='dcerpc_server', init_function='dcerpc_server_dnsserver_init', - deps='DCERPC_COMMON dnsserver_common' + deps='DCERPC_COMMON dnsserver_common netif' ) @@ -176,3 +176,15 @@ bld.SAMBA_MODULE('service_dcerpc', deps='dcerpc_server' ) +if bld.CONFIG_GET('ENABLE_SELFTEST'): + bld.SAMBA_BINARY( + 'test_rpc_dns_server_dnsutils', + source='tests/rpc_dns_server_dnsutils_test.c', + deps=''' + dnsserver_common + DCERPC_COMMON + cmocka + talloc + ''', + install=False + ) diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py index 24817e40fb5..d1289a5c516 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -1231,3 +1231,5 @@ plantestsuite("samba4.dsdb.samdb.ldb_modules.group_audit", "none", [os.path.join(bindir(), "test_group_audit")]) 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")]) -- 2.11.0 From b75a262ef8f30eb38e019c34ab02a55e35a1cc59 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Tue, 6 Nov 2018 12:16:30 +1300 Subject: [PATCH 2/7] CVE-2018-16852 dcerpc dnsserver: Ensure properties are handled correctly Fixes for Bug 13669 - (CVE-2018-16852) NULL pointer de-reference in Samba AD DC DNS management The presence of the ZONE_MASTER_SERVERS property or the ZONE_SCAVENGING_SERVERS property in a zone record causes the server to follow a null pointer and terminate. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13669 Signed-off-by: Gary Lockyer Reviewed-by: Andrew Bartlett --- selftest/knownfail.d/bug13669 | 4 -- source4/rpc_server/dnsserver/dnsutils.c | 65 +++++++++++++++++++++++++++++---- 2 files changed, 57 insertions(+), 12 deletions(-) delete mode 100644 selftest/knownfail.d/bug13669 diff --git a/selftest/knownfail.d/bug13669 b/selftest/knownfail.d/bug13669 deleted file mode 100644 index 74c8c130674..00000000000 --- a/selftest/knownfail.d/bug13669 +++ /dev/null @@ -1,4 +0,0 @@ -^samba4.dcerpc.dnsserver.dnsutils.test_dnsserver_init_zoneinfo_master_servers_empty -^samba4.dcerpc.dnsserver.dnsutils.test_dnsserver_init_zoneinfo_master_servers -^samba4.dcerpc.dnsserver.dnsutils.test_dnsserver_init_zoneinfo_scavenging_servers_empty -^samba4.dcerpc.dnsserver.dnsutils.test_dnsserver_init_zoneinfo_scavenging_servers diff --git a/source4/rpc_server/dnsserver/dnsutils.c b/source4/rpc_server/dnsserver/dnsutils.c index b3d8949f8ab..2db1e0fedda 100644 --- a/source4/rpc_server/dnsserver/dnsutils.c +++ b/source4/rpc_server/dnsserver/dnsutils.c @@ -209,6 +209,46 @@ struct dnsserver_serverinfo *dnsserver_init_serverinfo(TALLOC_CTX *mem_ctx, } +/* + * Helper function to copy a dnsp_ip4_array struct to an IP4_ARRAY struct. + * The new structure and it's data are allocated on the supplied talloc context + */ +static struct IP4_ARRAY *copy_ip4_array( + TALLOC_CTX *ctx, + const char *name, + struct dnsp_ip4_array array) { + + struct IP4_ARRAY *ip4_array = NULL; + unsigned int i; + + ip4_array = talloc_zero(ctx, struct IP4_ARRAY); + if (ip4_array == NULL) { + DBG_ERR("Out of memory copying property [%s]\n", + name); + return NULL; + } + + ip4_array->AddrCount = array.addrCount; + if (ip4_array->AddrCount == 0) { + return ip4_array; + } + + ip4_array->AddrArray = talloc_array(ip4_array, uint32_t, + ip4_array->AddrCount); + if (ip4_array->AddrArray == NULL) { + TALLOC_FREE(ip4_array); + DBG_ERR("Out of memory copying property [%s] values\n", + name); + return NULL; + } + + for (i = 0; i < ip4_array->AddrCount; i++) { + ip4_array->AddrArray[i] = array.addr[i]; + } + + return ip4_array; +} + struct dnsserver_zoneinfo *dnsserver_init_zoneinfo(struct dnsserver_zone *zone, struct dnsserver_serverinfo *serverinfo) { @@ -309,20 +349,29 @@ struct dnsserver_zoneinfo *dnsserver_init_zoneinfo(struct dnsserver_zone *zone, prop->aging_enabled; break; case DSPROPERTY_ZONE_SCAVENGING_SERVERS: - zoneinfo->aipScavengeServers->AddrCount = - prop->servers.addrCount; - zoneinfo->aipScavengeServers->AddrArray = - prop->servers.addr; + zoneinfo->aipScavengeServers = NULL; + zoneinfo->aipScavengeServers = + copy_ip4_array(zoneinfo, + "ZONE_SCAVENGING_SERVERS", + prop->servers); + if (zoneinfo->aipScavengeServers == NULL) { + TALLOC_FREE(zoneinfo); + return NULL; + } break; case DSPROPERTY_ZONE_AGING_ENABLED_TIME: zoneinfo->dwAvailForScavengeTime = prop->next_scavenging_cycle_hours; break; case DSPROPERTY_ZONE_MASTER_SERVERS: - zoneinfo->aipLocalMasters->AddrCount = - prop->master_servers.addrCount; - zoneinfo->aipLocalMasters->AddrArray = - prop->master_servers.addr; + zoneinfo->aipLocalMasters = + copy_ip4_array(zoneinfo, + "ZONE_MASTER_SERVERS", + prop->master_servers); + if (zoneinfo->aipLocalMasters == NULL) { + TALLOC_FREE(zoneinfo); + return NULL; + } break; case DSPROPERTY_ZONE_EMPTY: case DSPROPERTY_ZONE_SECURE_TIME: -- 2.11.0 From 1c5bb31df1c667049064685ea1ac501648b87f41 Mon Sep 17 00:00:00 2001 From: Aaron Haslett Date: Tue, 23 Oct 2018 11:52:07 +1300 Subject: [PATCH 3/7] dns: prevent self-referencing CNAME Stops the user from adding a self-referencing CNAME over RPC, which is an easy mistake to make with samba-tool. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13600 Signed-off-by: Aaron Haslett Reviewed-by: Andrew Bartlett Reviewed-by: Garming Sam --- python/samba/tests/dns.py | 42 +++++++++++++++++++++++++ source4/rpc_server/dnsserver/dcerpc_dnsserver.c | 40 +++++++++++++++++++++++ 2 files changed, 82 insertions(+) diff --git a/python/samba/tests/dns.py b/python/samba/tests/dns.py index 12cfb86c254..d0c2484c018 100644 --- a/python/samba/tests/dns.py +++ b/python/samba/tests/dns.py @@ -1481,6 +1481,48 @@ class TestRPCRoundtrip(DNSTest): def tearDown(self): super(TestRPCRoundtrip, self).tearDown() + def rpc_update(self, fqn=None, data=None, wType=None, delete=False): + fqn = fqn or ("rpctestrec." + self.get_dns_domain()) + + rec = data_to_dns_record(wType, data) + add_rec_buf = dnsserver.DNS_RPC_RECORD_BUF() + add_rec_buf.rec = rec + + add_arg = add_rec_buf + del_arg = None + if delete: + add_arg = None + del_arg = add_rec_buf + + self.rpc_conn.DnssrvUpdateRecord2( + dnsserver.DNS_CLIENT_VERSION_LONGHORN, + 0, + self.server_ip, + self.get_dns_domain(), + fqn, + add_arg, + del_arg) + + def test_rpc_self_referencing_cname(self): + cname = "cnametest2_unqual_rec_loop" + cname_fqn = "%s.%s" % (cname, self.get_dns_domain()) + + try: + self.rpc_update(fqn=cname, data=cname_fqn, + wType=dnsp.DNS_TYPE_CNAME, delete=True) + except WERRORError as e: + if e.args[0] != werror.WERR_DNS_ERROR_RECORD_DOES_NOT_EXIST: + raise e + + try: + self.rpc_update(fqn=cname, wType=dnsp.DNS_TYPE_CNAME, data=cname_fqn) + except WERRORError as e: + if e.args[0] != werror.WERR_DNS_ERROR_CNAME_LOOP: + raise e + return + + self.fail("RPC DNS allowed insertion of self referencing CNAME") + def test_update_add_txt_rpc_to_dns(self): prefix, txt = 'rpctextrec', ['"This is a test"'] diff --git a/source4/rpc_server/dnsserver/dcerpc_dnsserver.c b/source4/rpc_server/dnsserver/dcerpc_dnsserver.c index b42d7c549d1..e3ca106a075 100644 --- a/source4/rpc_server/dnsserver/dcerpc_dnsserver.c +++ b/source4/rpc_server/dnsserver/dcerpc_dnsserver.c @@ -25,6 +25,7 @@ #include "dsdb/samdb/samdb.h" #include "lib/util/dlinklist.h" #include "librpc/gen_ndr/ndr_dnsserver.h" +#include "dns_server/dnsserver_common.h" #include "dnsserver.h" #define DCESRV_INTERFACE_DNSSERVER_BIND(call, iface) \ @@ -1868,6 +1869,38 @@ static WERROR dnsserver_enumerate_records(struct dnsserver_state *dsstate, return WERR_OK; } +/* + * Check str1 + '.' + str2 = name, for example: + * ("dc0", "example.com", "dc0.example.com") = true + */ +static bool cname_self_reference(const char* node_name, + const char* zone_name, + struct DNS_RPC_NAME name) { + int node_len, zone_len; + + if (node_name == NULL || zone_name == NULL) { + return false; + } + + node_len = strlen(node_name); + zone_len = strlen(zone_name); + + if (node_len <= 0 || + zone_len <= 0 || + (name.len != node_len + zone_len + 1)) { + return false; + } + + if (strncmp(node_name, name.str, node_len) == 0 && + name.str[node_len] == '.' && + strncmp(zone_name, name.str + node_len + 1, zone_len) == 0) { + return true; + } + + return false; + +} + /* dnsserver update function */ static WERROR dnsserver_update_record(struct dnsserver_state *dsstate, @@ -1895,6 +1928,13 @@ static WERROR dnsserver_update_record(struct dnsserver_state *dsstate, } W_ERROR_HAVE_NO_MEMORY_AND_FREE(name, tmp_ctx); + /* CNAMEs can't point to themselves */ + if (add_buf != NULL && add_buf->rec.wType == DNS_TYPE_CNAME) { + if (cname_self_reference(node_name, z->name, add_buf->rec.data.name)) { + return WERR_DNS_ERROR_CNAME_LOOP; + } + } + if (add_buf != NULL) { if (del_buf == NULL) { /* Add record */ -- 2.11.0 From 7b6d4511f922a33c68c74116fddb0545b9524f73 Mon Sep 17 00:00:00 2001 From: Aaron Haslett Date: Tue, 23 Oct 2018 17:25:51 +1300 Subject: [PATCH 4/7] CVE-2018-14629 dns: CNAME loop prevention using counter Count number of answers generated by internal DNS query routine and stop at 20 to match Microsoft's loop prevention mechanism. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13600 Signed-off-by: Aaron Haslett Reviewed-by: Andrew Bartlett Reviewed-by: Garming Sam --- python/samba/tests/dns.py | 22 ++++++++++++++++++++++ source4/dns_server/dns_query.c | 6 ++++++ 2 files changed, 28 insertions(+) diff --git a/python/samba/tests/dns.py b/python/samba/tests/dns.py index d0c2484c018..e2d9bb23ed0 100644 --- a/python/samba/tests/dns.py +++ b/python/samba/tests/dns.py @@ -846,6 +846,28 @@ class TestComplexQueries(DNSTest): self.assertEquals(response.answers[1].name, name2) self.assertEquals(response.answers[1].rdata, name0) + def test_cname_loop(self): + cname1 = "cnamelooptestrec." + self.get_dns_domain() + cname2 = "cnamelooptestrec2." + self.get_dns_domain() + cname3 = "cnamelooptestrec3." + self.get_dns_domain() + self.make_dns_update(cname1, cname2, dnsp.DNS_TYPE_CNAME) + self.make_dns_update(cname2, cname3, dnsp.DNS_TYPE_CNAME) + self.make_dns_update(cname3, cname1, dnsp.DNS_TYPE_CNAME) + + p = self.make_name_packet(dns.DNS_OPCODE_QUERY) + questions = [] + + q = self.make_name_question(cname1, + dns.DNS_QTYPE_A, + dns.DNS_QCLASS_IN) + questions.append(q) + self.finish_name_packet(p, questions) + + (response, response_packet) =\ + self.dns_transaction_udp(p, host=self.server_ip) + + max_recursion_depth = 20 + self.assertEquals(len(response.answers), max_recursion_depth) class TestInvalidQueries(DNSTest): def setUp(self): diff --git a/source4/dns_server/dns_query.c b/source4/dns_server/dns_query.c index 923f7233eb9..65faeac3b6a 100644 --- a/source4/dns_server/dns_query.c +++ b/source4/dns_server/dns_query.c @@ -40,6 +40,7 @@ #undef DBGC_CLASS #define DBGC_CLASS DBGC_DNS +#define MAX_Q_RECURSION_DEPTH 20 struct forwarder_string { const char *forwarder; @@ -419,6 +420,11 @@ static struct tevent_req *handle_dnsrpcrec_send( state->answers = answers; state->nsrecs = nsrecs; + if (talloc_array_length(*answers) >= MAX_Q_RECURSION_DEPTH) { + tevent_req_done(req); + return tevent_req_post(req, ev); + } + resolve_cname = ((rec->wType == DNS_TYPE_CNAME) && ((question->question_type == DNS_QTYPE_A) || (question->question_type == DNS_QTYPE_AAAA))); -- 2.11.0 From 1a59601dbb9eb948648306e932985a98b63cc3b4 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 23 Oct 2018 17:33:46 +1300 Subject: [PATCH 5/7] CVE-2018-16841 heimdal: Fix segfault on PKINIT with mis-matching principal In Heimdal KRB5_KDC_ERR_CLIENT_NAME_MISMATCH is an enum, so we tried to double-free mem_ctx. This was introduced in 9a0263a7c316112caf0265237bfb2cfb3a3d370d for the MIT KDC effort. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13628 Signed-off-by: Andrew Bartlett Reviewed-by: Gary Lockyer --- source4/kdc/db-glue.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source4/kdc/db-glue.c b/source4/kdc/db-glue.c index acd24ec0c83..969f4f6b556 100644 --- a/source4/kdc/db-glue.c +++ b/source4/kdc/db-glue.c @@ -2610,10 +2610,10 @@ samba_kdc_check_pkinit_ms_upn_match(krb5_context context, * comparison */ if (!(orig_sid && target_sid && dom_sid_equal(orig_sid, target_sid))) { talloc_free(mem_ctx); -#ifdef KRB5_KDC_ERR_CLIENT_NAME_MISMATCH /* Heimdal */ - return KRB5_KDC_ERR_CLIENT_NAME_MISMATCH; -#elif defined(KRB5KDC_ERR_CLIENT_NAME_MISMATCH) /* MIT */ +#if defined(KRB5KDC_ERR_CLIENT_NAME_MISMATCH) /* MIT */ return KRB5KDC_ERR_CLIENT_NAME_MISMATCH; +#else /* Heimdal (where this is an enum) */ + return KRB5_KDC_ERR_CLIENT_NAME_MISMATCH; #endif } -- 2.11.0 From 98635563a6ac3bd6b25ac50e1881765c93aafa9f Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 24 Oct 2018 15:41:28 +1300 Subject: [PATCH 6/7] CVE-2018-16841 selftest: Check for mismatching principal in certficate compared with principal in AS-REQ BUG: https://bugzilla.samba.org/show_bug.cgi?id=13628 Signed-off-by: Andrew Bartlett Reviewed-by: Gary Lockyer --- testprogs/blackbox/test_pkinit_heimdal.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/testprogs/blackbox/test_pkinit_heimdal.sh b/testprogs/blackbox/test_pkinit_heimdal.sh index 0a13aa293e7..0912e0dbfe8 100755 --- a/testprogs/blackbox/test_pkinit_heimdal.sh +++ b/testprogs/blackbox/test_pkinit_heimdal.sh @@ -75,10 +75,18 @@ testit "STEP1 kinit with pkinit (name specified) " $samba4kinit $enctype --reque testit "STEP1 kinit renew ticket (name specified)" $samba4kinit --request-pac -R || failed=`expr $failed + 1` test_smbclient "STEP1 Test login with kerberos ccache (name specified)" 'ls' "$unc" -k yes || failed=`expr $failed + 1` +testit_expect_failure "STEP1 kinit with pkinit (wrong name specified) " $samba4kinit $enctype --request-pac --renewable $PKUSER not$USERNAME@$REALM || failed=`expr $failed + 1` + +testit_expect_failure "STEP1 kinit with pkinit (wrong name specified 2) " $samba4kinit $enctype --request-pac --renewable $PKUSER $SERVER@$REALM || failed=`expr $failed + 1` + testit "STEP1 kinit with pkinit (enterprise name specified)" $samba4kinit $enctype --request-pac --renewable $PKUSER --enterprise $USERNAME@$REALM || failed=`expr $failed + 1` testit "STEP1 kinit renew ticket (enterprise name specified)" $samba4kinit --request-pac -R || failed=`expr $failed + 1` test_smbclient "STEP1 Test login with kerberos ccache (enterprise name specified)" 'ls' "$unc" -k yes || failed=`expr $failed + 1` +testit_expect_failure "STEP1 kinit with pkinit (wrong enterprise name specified) " $samba4kinit $enctype --request-pac --renewable $PKUSER --enterprise not$USERNAME@$REALM || failed=`expr $failed + 1` + +testit_expect_failure "STEP1 kinit with pkinit (wrong enterprise name specified 2) " $samba4kinit $enctype --request-pac --renewable $PKUSER --enterprise $SERVER$@$REALM || failed=`expr $failed + 1` + testit "STEP1 kinit with pkinit (enterprise name in cert)" $samba4kinit $enctype --request-pac --renewable $PKUSER --pk-enterprise || failed=`expr $failed + 1` testit "STEP1 kinit renew ticket (enterprise name in cert)" $samba4kinit --request-pac -R || failed=`expr $failed + 1` test_smbclient "STEP1 Test login with kerberos ccache (enterprise name in cert)" 'ls' "$unc" -k yes || failed=`expr $failed + 1` -- 2.11.0 From 5d2389669bc9ef4398c021d956f71a29184b4542 Mon Sep 17 00:00:00 2001 From: Garming Sam Date: Mon, 5 Nov 2018 16:18:18 +1300 Subject: [PATCH 7/7] CVE-2018-16851 ldap_server: Check ret before manipulating blob In the case of hitting the talloc ~256MB limit, this causes a crash in the server. Note that you would actually need to load >256MB of data into the LDAP. Although there is some generated/hidden data which would help you reach that limit (descriptors and RMD blobs). BUG: https://bugzilla.samba.org/show_bug.cgi?id=13674 Signed-off-by: Garming Sam Reviewed-by: Andrew Bartlett --- source4/ldap_server/ldap_server.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source4/ldap_server/ldap_server.c b/source4/ldap_server/ldap_server.c index b5251e3623e..bc2f54bc146 100644 --- a/source4/ldap_server/ldap_server.c +++ b/source4/ldap_server/ldap_server.c @@ -690,13 +690,13 @@ static void ldapsrv_call_writev_start(struct ldapsrv_call *call) ret = data_blob_append(call, &blob, b.data, b.length); data_blob_free(&b); - talloc_set_name_const(blob.data, "Outgoing, encoded LDAP packet"); - if (!ret) { ldapsrv_terminate_connection(conn, "data_blob_append failed"); return; } + talloc_set_name_const(blob.data, "Outgoing, encoded LDAP packet"); + DLIST_REMOVE(call->replies, call->replies); } -- 2.11.0