From bd84c0c626aa0792a3d40270c284cf4cc7a7ebbd Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 6 Nov 2018 13:32:05 +1300 Subject: [PATCH 01/10] build: The Samba AD DC, when build with MIT Kerberos is experimental This matches https://wiki.samba.org/index.php/Running_a_Samba_AD_DC_with_MIT_Kerberos_KDC BUG: https://bugzilla.samba.org/show_bug.cgi?id=13678 Signed-off-by: Andrew Bartlett --- wscript | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/wscript b/wscript index c5d8e5bdd7d..e9d8a834aa2 100644 --- a/wscript +++ b/wscript @@ -55,6 +55,14 @@ def options(opt): help='build Samba with system MIT Kerberos. ' + 'You may specify list of paths where Kerberos is installed (e.g. /usr/local /usr/kerberos) to search krb5-config', action='callback', callback=system_mitkrb5_callback, dest='with_system_mitkrb5', default=False) + + opt.add_option('--with-experimental-mit-ad-dc', + help='Enable the experimental MIT Kerberos-backed AD DC. ' + + 'Note that security patches are not issued for this configuration', + action='store_true', + dest='with_experimental_mit_ad_dc', + default=False) + opt.add_option('--with-system-mitkdc', help=('Specify the path to the krb5kdc binary from MIT Kerberos'), type="string", @@ -214,7 +222,16 @@ def configure(conf): conf.DEFINE('AD_DC_BUILD_IS_ENABLED', 1) if Options.options.with_system_mitkrb5: + if not Options.options.with_experimental_mit_ad_dc and \ + not Options.options.without_ad_dc: + raise Utils.WafError('The MIT Kerberos build of Samba as an AD DC ' + + 'is experimental. Therefore ' + '--with-system-mitkrb5 requires either ' + + '--with-experimental-mit-ad-dc or ' + + '--without-ad-dc') + conf.PROCESS_SEPARATE_RULE('system_mitkrb5') + if not (Options.options.without_ad_dc or Options.options.with_system_mitkrb5): conf.DEFINE('AD_DC_BUILD_IS_ENABLED', 1) -- 2.17.1 From 5de4d8079f1188747c0aa80a90aac1ba17871f97 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 6 Nov 2018 13:40:48 +1300 Subject: [PATCH 02/10] WHATSNEW: The Samba AD DC, when build with MIT Kerberos is experimental BUG: https://bugzilla.samba.org/show_bug.cgi?id=13678 Signed-off-by: Andrew Bartlett --- WHATSNEW.txt | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/WHATSNEW.txt b/WHATSNEW.txt index 7a7a88d4152..fbe625038bf 100644 --- a/WHATSNEW.txt +++ b/WHATSNEW.txt @@ -36,6 +36,16 @@ forked for the KDC when the pre-fork process model is selected for samba. REMOVED FEATURES ================ +MIT Kerberos build of the AD DC +------------------------------- + +While not removed, the MIT Kerberos build of the Samba AD DC is still +considered experimental. Because Samba will not issue security +patches for this configuration, such builds now require the explicit +configure option: --with-experimental-mit-ad-dc + +For further details see +https://wiki.samba.org/index.php/Running_a_Samba_AD_DC_with_MIT_Kerberos_KDC smb.conf changes ================ -- 2.17.1 From 7d95ca0f9f198496f880268d4caa15900bd58496 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Tue, 6 Nov 2018 12:10:07 +1300 Subject: [PATCH 03/10] 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 | 17 +- source4/selftest/tests.py | 2 + 4 files changed, 325 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..14b68c7ce0f 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,16 @@ 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, + enabled=bld.AD_DC_BUILD_IS_ENABLED() + ) diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py index c4b7d18444c..4544d16ea01 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -1232,3 +1232,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.17.1 From 71f991cae29feeff6cd803dd009ca979f3e29995 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Tue, 6 Nov 2018 12:16:30 +1300 Subject: [PATCH 04/10] 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.17.1 From 7c7b16c74ab6fdafb20e1d1d89971afc69e32c91 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Wed, 7 Nov 2018 15:08:04 +1300 Subject: [PATCH 05/10] CVE-2018-16852 dcerpc dnsserver: refactor common properties handling dnsserver_common.c and dnsutils.c both share similar code to process zone properties. This patch extracts the common code and moves it to dnsserver_common.c. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13669 Signed-off-by: Gary Lockyer --- source4/dns_server/dnsserver_common.c | 130 +++++++++++++++++------- source4/dns_server/dnsserver_common.h | 3 + source4/rpc_server/dnsserver/dnsutils.c | 108 ++------------------ 3 files changed, 105 insertions(+), 136 deletions(-) diff --git a/source4/dns_server/dnsserver_common.c b/source4/dns_server/dnsserver_common.c index 1a032b4aa9f..02997f0b433 100644 --- a/source4/dns_server/dnsserver_common.c +++ b/source4/dns_server/dnsserver_common.c @@ -742,6 +742,95 @@ bool dns_name_is_static(struct dnsp_DnssrvRpcRecord *records, return false; } +/* + * 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; +} + +bool dns_zoneinfo_load_zone_property(struct dnsserver_zoneinfo *zoneinfo, + struct dnsp_DnsProperty *prop) +{ + switch (prop->id) { + case DSPROPERTY_ZONE_TYPE: + zoneinfo->dwZoneType = prop->data.zone_type; + break; + case DSPROPERTY_ZONE_ALLOW_UPDATE: + zoneinfo->fAllowUpdate = prop->data.allow_update_flag; + break; + case DSPROPERTY_ZONE_NOREFRESH_INTERVAL: + zoneinfo->dwNoRefreshInterval = prop->data.norefresh_hours; + break; + case DSPROPERTY_ZONE_REFRESH_INTERVAL: + zoneinfo->dwRefreshInterval = prop->data.refresh_hours; + break; + case DSPROPERTY_ZONE_AGING_STATE: + zoneinfo->fAging = prop->data.aging_enabled; + break; + case DSPROPERTY_ZONE_SCAVENGING_SERVERS: + zoneinfo->aipScavengeServers = NULL; + zoneinfo->aipScavengeServers = copy_ip4_array( + zoneinfo, "ZONE_SCAVENGING_SERVERS", prop->data.servers); + if (zoneinfo->aipScavengeServers == NULL) { + return false; + } + break; + case DSPROPERTY_ZONE_AGING_ENABLED_TIME: + zoneinfo->dwAvailForScavengeTime = + prop->data.next_scavenging_cycle_hours; + break; + case DSPROPERTY_ZONE_MASTER_SERVERS: + zoneinfo->aipLocalMasters = copy_ip4_array( + zoneinfo, "ZONE_MASTER_SERVERS", prop->data.master_servers); + if (zoneinfo->aipLocalMasters == NULL) { + return false; + } + break; + case DSPROPERTY_ZONE_EMPTY: + case DSPROPERTY_ZONE_SECURE_TIME: + case DSPROPERTY_ZONE_DELETED_FROM_HOSTNAME: + case DSPROPERTY_ZONE_AUTO_NS_SERVERS: + case DSPROPERTY_ZONE_DCPROMO_CONVERT: + case DSPROPERTY_ZONE_SCAVENGING_SERVERS_DA: + case DSPROPERTY_ZONE_MASTER_SERVERS_DA: + case DSPROPERTY_ZONE_NS_SERVERS_DA: + case DSPROPERTY_ZONE_NODE_DBFLAGS: + break; + } + return true; +} WERROR dns_get_zone_properties(struct ldb_context *samdb, TALLOC_CTX *mem_ctx, struct ldb_dn *zone_dn, @@ -774,6 +863,7 @@ WERROR dns_get_zone_properties(struct ldb_context *samdb, } for (i = 0; i < element->num_values; i++) { + bool valid_property; prop = talloc_zero(mem_ctx, struct dnsp_DnsProperty); if (prop == NULL) { return WERR_NOT_ENOUGH_MEMORY; @@ -787,42 +877,10 @@ WERROR dns_get_zone_properties(struct ldb_context *samdb, return DNS_ERR(SERVER_FAILURE); } - switch (prop->id) { - case DSPROPERTY_ZONE_AGING_STATE: - zoneinfo->fAging = prop->data.aging_enabled; - break; - case DSPROPERTY_ZONE_NOREFRESH_INTERVAL: - zoneinfo->dwNoRefreshInterval = - prop->data.norefresh_hours; - break; - case DSPROPERTY_ZONE_REFRESH_INTERVAL: - zoneinfo->dwRefreshInterval = prop->data.refresh_hours; - break; - case DSPROPERTY_ZONE_ALLOW_UPDATE: - zoneinfo->fAllowUpdate = prop->data.allow_update_flag; - break; - case DSPROPERTY_ZONE_AGING_ENABLED_TIME: - zoneinfo->dwAvailForScavengeTime = - prop->data.next_scavenging_cycle_hours; - break; - case DSPROPERTY_ZONE_SCAVENGING_SERVERS: - zoneinfo->aipScavengeServers->AddrCount = - prop->data.servers.addrCount; - zoneinfo->aipScavengeServers->AddrArray = - prop->data.servers.addr; - break; - case DSPROPERTY_ZONE_EMPTY: - case DSPROPERTY_ZONE_TYPE: - case DSPROPERTY_ZONE_SECURE_TIME: - case DSPROPERTY_ZONE_DELETED_FROM_HOSTNAME: - case DSPROPERTY_ZONE_MASTER_SERVERS: - case DSPROPERTY_ZONE_AUTO_NS_SERVERS: - case DSPROPERTY_ZONE_DCPROMO_CONVERT: - case DSPROPERTY_ZONE_SCAVENGING_SERVERS_DA: - case DSPROPERTY_ZONE_MASTER_SERVERS_DA: - case DSPROPERTY_ZONE_NS_SERVERS_DA: - case DSPROPERTY_ZONE_NODE_DBFLAGS: - break; + valid_property = + dns_zoneinfo_load_zone_property(zoneinfo, prop); + if (!valid_property) { + return DNS_ERR(SERVER_FAILURE); } } diff --git a/source4/dns_server/dnsserver_common.h b/source4/dns_server/dnsserver_common.h index 380f61b8dbc..60ecde4fa91 100644 --- a/source4/dns_server/dnsserver_common.h +++ b/source4/dns_server/dnsserver_common.h @@ -87,4 +87,7 @@ NTSTATUS dns_common_zones(struct ldb_context *samdb, TALLOC_CTX *mem_ctx, struct ldb_dn *base_dn, struct dns_server_zone **zones_ret); + +bool dns_zoneinfo_load_zone_property(struct dnsserver_zoneinfo *zoneinfo, + struct dnsp_DnsProperty *prop); #endif /* __DNSSERVER_COMMON_H__ */ diff --git a/source4/rpc_server/dnsserver/dnsutils.c b/source4/rpc_server/dnsserver/dnsutils.c index 2db1e0fedda..ea1eca596a1 100644 --- a/source4/rpc_server/dnsserver/dnsutils.c +++ b/source4/rpc_server/dnsserver/dnsutils.c @@ -25,6 +25,7 @@ #include "dsdb/samdb/samdb.h" #include "lib/socket/netif.h" #include "lib/util/util_net.h" +#include "dnsserver_common.h" static struct DNS_ADDR_ARRAY *fill_dns_addr_array(TALLOC_CTX *mem_ctx, struct loadparm_context *lp_ctx, @@ -208,47 +209,6 @@ struct dnsserver_serverinfo *dnsserver_init_serverinfo(TALLOC_CTX *mem_ctx, return serverinfo; } - -/* - * 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) { @@ -257,8 +217,7 @@ struct dnsserver_zoneinfo *dnsserver_init_zoneinfo(struct dnsserver_zone *zone, const char *revzone = "in-addr.arpa"; const char *revzone6 = "ip6.arpa"; int len1, len2; - union dnsPropertyData *prop = NULL; - int i=0; + unsigned int i = 0; zoneinfo = talloc_zero(zone, struct dnsserver_zoneinfo); if (zoneinfo == NULL) { @@ -326,63 +285,12 @@ struct dnsserver_zoneinfo *dnsserver_init_zoneinfo(struct dnsserver_zone *zone, zoneinfo->dwLastXfrResult = 0; for(i=0; inum_props; i++){ - prop=&(zone->tmp_props[i].data); - switch (zone->tmp_props[i].id) { - case DSPROPERTY_ZONE_TYPE: - zoneinfo->dwZoneType = - prop->zone_type; - break; - case DSPROPERTY_ZONE_ALLOW_UPDATE: - zoneinfo->fAllowUpdate = - prop->allow_update_flag; - break; - case DSPROPERTY_ZONE_NOREFRESH_INTERVAL: - zoneinfo->dwNoRefreshInterval = - prop->norefresh_hours; - break; - case DSPROPERTY_ZONE_REFRESH_INTERVAL: - zoneinfo->dwRefreshInterval = - prop->refresh_hours; - break; - case DSPROPERTY_ZONE_AGING_STATE: - zoneinfo->fAging = - prop->aging_enabled; - break; - case DSPROPERTY_ZONE_SCAVENGING_SERVERS: - 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 = - 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: - case DSPROPERTY_ZONE_DELETED_FROM_HOSTNAME: - case DSPROPERTY_ZONE_AUTO_NS_SERVERS: - case DSPROPERTY_ZONE_DCPROMO_CONVERT: - case DSPROPERTY_ZONE_SCAVENGING_SERVERS_DA: - case DSPROPERTY_ZONE_MASTER_SERVERS_DA: - case DSPROPERTY_ZONE_NS_SERVERS_DA: - case DSPROPERTY_ZONE_NODE_DBFLAGS: - break; + bool valid_property; + valid_property = dns_zoneinfo_load_zone_property( + zoneinfo, &zone->tmp_props[i]); + if (!valid_property) { + TALLOC_FREE(zoneinfo); + return NULL; } } -- 2.17.1 From 3bdf2c6feaf31a40af73276e71ab3bdba09daff4 Mon Sep 17 00:00:00 2001 From: Garming Sam Date: Mon, 5 Nov 2018 16:18:18 +1300 Subject: [PATCH 06/10] 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 f6329f05c5a..783823043dc 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.17.1 From a65fbaadc162433096df15ff38abbe1a5ed7788c Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 23 Oct 2018 17:33:46 +1300 Subject: [PATCH 07/10] 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.17.1 From a7ee7c3c17a36bad22600d3a27b03d4df39235dc Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 24 Oct 2018 15:41:28 +1300 Subject: [PATCH 08/10] 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.17.1 From 3096c17bbcfc8c639ce8bfc26a937af9bd51857f Mon Sep 17 00:00:00 2001 From: Aaron Haslett Date: Tue, 23 Oct 2018 11:52:07 +1300 Subject: [PATCH 09/10] 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 | 44 +++++++++++++++++++ selftest/knownfail.d/dns | 1 + .../rpc_server/dnsserver/dcerpc_dnsserver.c | 40 +++++++++++++++++ 3 files changed, 85 insertions(+) diff --git a/python/samba/tests/dns.py b/python/samba/tests/dns.py index 12cfb86c254..56609769a4f 100644 --- a/python/samba/tests/dns.py +++ b/python/samba/tests/dns.py @@ -1481,6 +1481,50 @@ 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: + self.fail("RPC DNS gaven wrong error on pre-test cleanup " + "for self referencing CNAME: %s" % e.args[0]) + + 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: + self.fail("RPC DNS gaven wrong error on insertion of " + "self referencing CNAME: %s" % e.args[0]) + 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/selftest/knownfail.d/dns b/selftest/knownfail.d/dns index ca18b4334c1..0f6caba8916 100644 --- a/selftest/knownfail.d/dns +++ b/selftest/knownfail.d/dns @@ -13,6 +13,7 @@ samba.tests.dns.__main__.TestRPCRoundtrip.test_update_add_null_char_txt_record\( samba.tests.dns.__main__.TestRPCRoundtrip.test_update_add_null_padded_txt_record\(rodc:local\) samba.tests.dns.__main__.TestRPCRoundtrip.test_update_add_slash_txt_record\(rodc:local\) samba.tests.dns.__main__.TestRPCRoundtrip.test_update_add_two_txt_records\(rodc:local\) +samba.tests.dns.__main__.TestRPCRoundtrip.test_rpc_self_referencing_cname\(rodc:local\) samba.tests.dns.__main__.TestDNSUpdates.test_delete_record\(vampire_dc:local\) samba.tests.dns.__main__.TestDNSUpdates.test_readd_record\(vampire_dc:local\) samba.tests.dns.__main__.TestDNSUpdates.test_update_add_mx_record\(vampire_dc:local\) 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.17.1 From 18cb544da4e8b22438519156b0584b4b92bf335a Mon Sep 17 00:00:00 2001 From: Aaron Haslett Date: Tue, 23 Oct 2018 17:25:51 +1300 Subject: [PATCH 10/10] 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 ++++++++++++++++++++++ selftest/knownfail.d/dns | 6 ++++++ source4/dns_server/dns_query.c | 6 ++++++ 3 files changed, 34 insertions(+) diff --git a/python/samba/tests/dns.py b/python/samba/tests/dns.py index 56609769a4f..e35cded7b8c 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/selftest/knownfail.d/dns b/selftest/knownfail.d/dns index 0f6caba8916..39b337ed6bb 100644 --- a/selftest/knownfail.d/dns +++ b/selftest/knownfail.d/dns @@ -71,3 +71,9 @@ samba.tests.dns.__main__.TestSimpleQueries.test_qtype_all_query\(rodc:local\) # The SOA override should not pass against the RODC, it must not overstamp samba.tests.dns.__main__.TestSimpleQueries.test_one_SOA_query\(rodc:local\) + +# +# rodc and vampire_dc require signed dns updates, so the test setup +# fails, but the test does run on fl2003dc +^samba.tests.dns.__main__.TestComplexQueries.test_cname_loop\(rodc:local\) +^samba.tests.dns.__main__.TestComplexQueries.test_cname_loop\(vampire_dc:local\) 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.17.1