From 0b9da247534f735fa96141e9285fd22e0f2bb442 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Date: Wed, 22 May 2019 12:58:01 +1200
Subject: [PATCH 1/3] CVE-2019-12435 rpc/dns: avoid NULL deference if zone not
 found in DnssrvOperation

We still want to return DOES_NOT_EXIST when request_filter is not 0.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13922

Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
---
 python/samba/tests/dcerpc/dnsserver.py        | 25 +++++++++++++++++++
 .../rpc_server/dnsserver/dcerpc_dnsserver.c   |  7 +++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/python/samba/tests/dcerpc/dnsserver.py b/python/samba/tests/dcerpc/dnsserver.py
index 8e485c540dd..bfe86323e0c 100644
--- a/python/samba/tests/dcerpc/dnsserver.py
+++ b/python/samba/tests/dcerpc/dnsserver.py
@@ -28,6 +28,7 @@ from samba.dcerpc import dnsp, dnsserver, security
 from samba.tests import RpcInterfaceTestCase, env_get_var_value
 from samba.netcmd.dns import ARecord, AAAARecord, PTRRecord, CNameRecord, NSRecord, MXRecord, SRVRecord, TXTRecord
 from samba import sd_utils, descriptor
+from samba import WERRORError, werror
 
 
 class DnsserverTests(RpcInterfaceTestCase):
@@ -707,6 +708,30 @@ class DnsserverTests(RpcInterfaceTestCase):
                                                 'ServerInfo')
         self.assertEquals(dnsserver.DNSSRV_TYPEID_SERVER_INFO, typeid)
 
+
+    # This test is to confirm that we do not support multizone operations,
+    # which are designated by a non-zero dwContext value (the 3rd argument
+    # to DnssrvOperation).
+    def test_operation_invalid(self):
+        non_zone = 'a-zone-that-does-not-exist'
+        typeid = dnsserver.DNSSRV_TYPEID_NAME_AND_PARAM
+        name_and_param = dnsserver.DNS_RPC_NAME_AND_PARAM()
+        name_and_param.pszNodeName = 'AllowUpdate'
+        name_and_param.dwParam = dnsp.DNS_ZONE_UPDATE_SECURE
+        try:
+            res = self.conn.DnssrvOperation(self.server,
+                                            non_zone,
+                                            1,
+                                            'ResetDwordProperty',
+                                            typeid,
+                                            name_and_param)
+        except WERRORError as e:
+            if e.args[0] == werror.WERR_DNS_ERROR_ZONE_DOES_NOT_EXIST:
+                return
+
+        # We should always encounter a DOES_NOT_EXIST error.
+        self.fail()
+
     def test_operation2(self):
         client_version = dnsserver.DNS_CLIENT_VERSION_LONGHORN
         rev_zone = '1.168.192.in-addr.arpa'
diff --git a/source4/rpc_server/dnsserver/dcerpc_dnsserver.c b/source4/rpc_server/dnsserver/dcerpc_dnsserver.c
index 841557814a0..bdf894634ce 100644
--- a/source4/rpc_server/dnsserver/dcerpc_dnsserver.c
+++ b/source4/rpc_server/dnsserver/dcerpc_dnsserver.c
@@ -2018,7 +2018,12 @@ static WERROR dcesrv_DnssrvOperation(struct dcesrv_call_state *dce_call, TALLOC_
 						&r->in.pData);
 	} else {
 		z = dnsserver_find_zone(dsstate->zones, r->in.pszZone);
-		if (z == NULL && request_filter == 0) {
+		/*
+		 * In the case that request_filter is not 0 and z is NULL,
+		 * the request is for a multizone operation, which we do not
+		 * yet support, so just error on NULL zone name.
+		 */
+		if (z == NULL) {
 			return WERR_DNS_ERROR_ZONE_DOES_NOT_EXIST;
 		}
 
-- 
2.17.1


From d32b96aeff0022c7a9052f15adbc7cd36643ca22 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Date: Wed, 22 May 2019 13:23:25 +1200
Subject: [PATCH 2/3] CVE-2019-12435 rpc/dns: avoid NULL deference if zone not
 found in DnssrvOperation2

We still want to return DOES_NOT_EXIST when request_filter is not 0.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13922

Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
---
 python/samba/tests/dcerpc/dnsserver.py        | 26 +++++++++++++++++++
 .../rpc_server/dnsserver/dcerpc_dnsserver.c   |  7 ++++-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/python/samba/tests/dcerpc/dnsserver.py b/python/samba/tests/dcerpc/dnsserver.py
index bfe86323e0c..0da9614d066 100644
--- a/python/samba/tests/dcerpc/dnsserver.py
+++ b/python/samba/tests/dcerpc/dnsserver.py
@@ -732,6 +732,32 @@ class DnsserverTests(RpcInterfaceTestCase):
         # We should always encounter a DOES_NOT_EXIST error.
         self.fail()
 
+    # This test is to confirm that we do not support multizone operations,
+    # which are designated by a non-zero dwContext value (the 5th argument
+    # to DnssrvOperation2).
+    def test_operation2_invalid(self):
+        client_version = dnsserver.DNS_CLIENT_VERSION_LONGHORN
+        non_zone = 'a-zone-that-does-not-exist'
+        typeid = dnsserver.DNSSRV_TYPEID_NAME_AND_PARAM
+        name_and_param = dnsserver.DNS_RPC_NAME_AND_PARAM()
+        name_and_param.pszNodeName = 'AllowUpdate'
+        name_and_param.dwParam = dnsp.DNS_ZONE_UPDATE_SECURE
+        try:
+            res = self.conn.DnssrvOperation2(client_version,
+                                             0,
+                                             self.server,
+                                             non_zone,
+                                             1,
+                                             'ResetDwordProperty',
+                                             typeid,
+                                             name_and_param)
+        except WERRORError as e:
+            if e.args[0] == werror.WERR_DNS_ERROR_ZONE_DOES_NOT_EXIST:
+                return
+
+        # We should always encounter a DOES_NOT_EXIST error.
+        self.fail()
+
     def test_operation2(self):
         client_version = dnsserver.DNS_CLIENT_VERSION_LONGHORN
         rev_zone = '1.168.192.in-addr.arpa'
diff --git a/source4/rpc_server/dnsserver/dcerpc_dnsserver.c b/source4/rpc_server/dnsserver/dcerpc_dnsserver.c
index bdf894634ce..f8a8f0bae61 100644
--- a/source4/rpc_server/dnsserver/dcerpc_dnsserver.c
+++ b/source4/rpc_server/dnsserver/dcerpc_dnsserver.c
@@ -2230,7 +2230,12 @@ static WERROR dcesrv_DnssrvOperation2(struct dcesrv_call_state *dce_call, TALLOC
 						&r->in.pData);
 	} else {
 		z = dnsserver_find_zone(dsstate->zones, r->in.pszZone);
-		if (z == NULL && request_filter == 0) {
+		/*
+		 * In the case that request_filter is not 0 and z is NULL,
+		 * the request is for a multizone operation, which we do not
+		 * yet support, so just error on NULL zone name.
+		 */
+		if (z == NULL) {
 			return WERR_DNS_ERROR_ZONE_DOES_NOT_EXIST;
 		}
 
-- 
2.17.1


From c48920093da7f5f6cbbca42d516b86b9cf51eea6 Mon Sep 17 00:00:00 2001
From: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Date: Fri, 17 May 2019 14:42:24 +1200
Subject: [PATCH 3/3] CVE-2019-12436 dsdb/paged_results: ignore successful
 results without messages

So that we don't dereference result->msgs[0] when it doesn't exist.
This can happen when the object has changed in such a way that it no
longer matches the original search query.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=13951

Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
---
 .../dsdb/samdb/ldb_modules/paged_results.c    |  3 +-
 source4/dsdb/tests/python/vlv.py              | 50 ++++++++++++++++++-
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/source4/dsdb/samdb/ldb_modules/paged_results.c b/source4/dsdb/samdb/ldb_modules/paged_results.c
index 78ad44f6601..5cad398ab61 100644
--- a/source4/dsdb/samdb/ldb_modules/paged_results.c
+++ b/source4/dsdb/samdb/ldb_modules/paged_results.c
@@ -266,7 +266,8 @@ static int paged_results(struct paged_context *ac)
 		ret = paged_search_by_dn_guid(ac->module, ac, &result, guid,
 					    ac->req->op.search.attrs,
 					    ac->store->expr);
-		if (ret == LDAP_NO_SUCH_OBJECT /* TODO or no result */) {
+		if (ret == LDAP_NO_SUCH_OBJECT ||
+		    (ret == LDB_SUCCESS && result->count == 0)) {
 			/* The thing isn't there TODO, which we quietly
 			   ignore and go on to send an extra one
 			   instead. */
diff --git a/source4/dsdb/tests/python/vlv.py b/source4/dsdb/tests/python/vlv.py
index 8550a38e287..bc07a53d575 100644
--- a/source4/dsdb/tests/python/vlv.py
+++ b/source4/dsdb/tests/python/vlv.py
@@ -105,6 +105,7 @@ class TestsWithUserOU(samba.tests.TestCase):
             'givenName': "abcdefghijklmnopqrstuvwxyz"[i % 26],
             "roomNumber": "%sbc" % (n - i),
             "carLicense": "后来经",
+            "facsimileTelephoneNumber": name,
             "employeeNumber": "%s%sx" % (abs(i * (99 - i)), '\n' * (i & 255)),
             "accountExpires": "%s" % (10 ** 9 + 1000000 * i),
             "msTSExpireDate4": "19%02d0101010000.0Z" % (i % 100),
@@ -1334,7 +1335,7 @@ class PagedResultsTests(TestsWithUserOU):
 
         self.assertEqual(results, expected_results)
 
-    def test_paged_modify_during_search(self):
+    def test_paged_rename_during_search(self):
         expr = "(objectClass=*)"
 
         # Start new search
@@ -1421,6 +1422,53 @@ class PagedResultsTests(TestsWithUserOU):
 
         self.assertEqual(results, expected_results)
 
+    def test_paged_modify_one_during_search(self):
+        prefix = "change_during_search_"
+        num_users = 5
+        users = [self.create_user(i, num_users, prefix=prefix)
+                 for i in range(num_users)]
+        expr = "(&(objectClass=user)(facsimileTelephoneNumber=%s*))" % (prefix)
+
+        # Get the first page, then change the searched attribute and
+        # try for the second page.
+        results, cookie = self.paged_search(expr, page_size=1)
+        self.assertEqual(len(results), 1)
+        unwalked_users = [u for u in users if u['cn'] != results[0]]
+        self.assertEqual(len(unwalked_users), num_users-1)
+
+        mod_dn = unwalked_users[0]['dn']
+        self.ldb.modify_ldif("dn: %s\n"
+                             "changetype: modify\n"
+                             "replace: facsimileTelephoneNumber\n"
+                             "facsimileTelephoneNumber: 123" % mod_dn)
+
+        results, _ = self.paged_search(expr, cookie=cookie,
+                                       page_size=len(self.users))
+        expected_cns = {u['cn'] for u in unwalked_users if u['dn'] != mod_dn}
+        self.assertEqual(set(results), expected_cns)
+
+    def test_paged_modify_all_during_search(self):
+        prefix = "change_during_search_"
+        num_users = 5
+        users = [self.create_user(i, num_users, prefix=prefix)
+                 for i in range(num_users)]
+        expr = "(&(objectClass=user)(facsimileTelephoneNumber=%s*))" % (prefix)
+
+        # Get the first page, then change the searched attribute and
+        # try for the second page.
+        results, cookie = self.paged_search(expr, page_size=1)
+        unwalked_users = [u for u in users if u['cn'] != results[0]]
+
+        for u in users:
+            self.ldb.modify_ldif("dn: %s\n"
+                                 "changetype: modify\n"
+                                 "replace: facsimileTelephoneNumber\n"
+                                 "facsimileTelephoneNumber: 123" % u['dn'])
+
+        results, _ = self.paged_search(expr, cookie=cookie,
+                                       page_size=len(self.users))
+        self.assertEqual(results, [])
+
     def assertPagedSearchRaises(self, err_num, expr, cookie, attrs=None,
                                 extra_ctrls=None):
         try:
-- 
2.17.1