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 <gary@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>

3 files changed, 321 insertions, 2 deletions

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 <http://www.gnu.org/licenses/>.
+ *
+ */
+
+/*
+ * from cmocka.c:
+ * These headers or their equivalents should be included prior to
+ * including
+ * this header file.
+ *
+ * #include <stdarg.h>
+ * #include <stddef.h>
+ * #include <setjmp.h>
+ *
+ * This allows test applications to use custom definitions of C standard
+ * library functions and types.
+ *
+ */
+
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include <cmocka.h>
+
+
+#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 dc5807247ab..972f65307d8 100755
--- a/source4/selftest/tests.py
+++ b/source4/selftest/tests.py
@@ -1237,6 +1237,8 @@ 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")])
 
 # process restart and limit tests, these break the environment so need to run
 # in their own specific environment