Attachment 15926 Details for Bug 14331 – Patch for V4.11 (v2)

[patch] Patch for V4.11 (v2)

CVE-2020-10700-4.11-V2.patch (text/plain), 13.82 KB, created by Gary Lockyer on 2020-04-17 02:57:49 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Gary Lockyer

Created: 2020-04-17 02:57:49 UTC

Size: 13.82 KB

patch

obsolete

>From fe59a3fa1e31b15ed6fce194fc85ac105220d506 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Mon, 30 Mar 2020 09:44:20 +0000
>Subject: [PATCH 1/3] CVE-2020-10700: dsdb: Add test for ASQ and ASQ in
> combination with paged_results
>
>Thanks to Andrei Popa <andrei.popa@next-gen.ro> for finding,
>reporting and working with us to diagnose this issue!
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14331
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Gary Lockyer <gary@catalyst.net.nz>
>---
> selftest/knownfail.d/asq         |   1 +
> source4/dsdb/tests/python/asq.py | 171 +++++++++++++++++++++++++++++++
> source4/selftest/tests.py        |   1 +
> 3 files changed, 173 insertions(+)
> create mode 100644 selftest/knownfail.d/asq
> create mode 100644 source4/dsdb/tests/python/asq.py
>
>diff --git a/selftest/knownfail.d/asq b/selftest/knownfail.d/asq
>new file mode 100644
>index 00000000000..eb0e3e0aba1
>--- /dev/null
>+++ b/selftest/knownfail.d/asq
>@@ -0,0 +1 @@
>+samba4.asq.python$ad_dc_default$.__main__.ASQLDAPTest.test_asq_paged
>\ No newline at end of file
>diff --git a/source4/dsdb/tests/python/asq.py b/source4/dsdb/tests/python/asq.py
>new file mode 100644
>index 00000000000..a32c9f40cd3
>--- /dev/null
>+++ b/source4/dsdb/tests/python/asq.py
>@@ -0,0 +1,171 @@
>+#!/usr/bin/env python3
>+#
>+# Test ASQ LDAP control behaviour in Samba
>+# Copyright (C) Andrew Bartlett 2019-2020
>+#
>+# Based on Unit tests for the notification control
>+# Copyright (C) Stefan Metzmacher 2016
>+#
>+# 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/>.
>+
>+import optparse
>+import sys
>+import os
>+import random
>+
>+sys.path.insert(0, "bin/python")
>+import samba
>+from samba.tests.subunitrun import SubunitOptions, TestProgram
>+
>+import samba.getopt as options
>+
>+from samba.auth import system_session
>+from samba import ldb
>+from samba.samdb import SamDB
>+from samba.ndr import ndr_unpack
>+from samba import gensec
>+from samba.credentials import Credentials
>+import samba.tests
>+
>+from ldb import SCOPE_SUBTREE, SCOPE_ONELEVEL, SCOPE_BASE, LdbError
>+from ldb import ERR_TIME_LIMIT_EXCEEDED, ERR_ADMIN_LIMIT_EXCEEDED, ERR_UNWILLING_TO_PERFORM
>+from ldb import Message
>+
>+parser = optparse.OptionParser("large_ldap.py [options] <host>")
>+sambaopts = options.SambaOptions(parser)
>+parser.add_option_group(sambaopts)
>+parser.add_option_group(options.VersionOptions(parser))
>+# use command line creds if available
>+credopts = options.CredentialsOptions(parser)
>+parser.add_option_group(credopts)
>+subunitopts = SubunitOptions(parser)
>+parser.add_option_group(subunitopts)
>+opts, args = parser.parse_args()
>+
>+if len(args) < 1:
>+    parser.print_usage()
>+    sys.exit(1)
>+
>+url = args[0]
>+
>+lp = sambaopts.get_loadparm()
>+creds = credopts.get_credentials(lp)
>+
>+
>+class ASQLDAPTest(samba.tests.TestCase):
>+
>+    def setUp(self):
>+        super(ASQLDAPTest, self).setUp()
>+        self.ldb = samba.Ldb(url, credentials=creds, session_info=system_session(lp), lp=lp)
>+        self.base_dn = self.ldb.get_default_basedn()
>+        self.NAME_ASQ="asq_" + format(random.randint(0, 99999), "05")
>+        self.OU_NAME_ASQ= self.NAME_ASQ + "_ou"
>+        self.ou_dn = ldb.Dn(self.ldb, "ou=" + self.OU_NAME_ASQ + "," + str(self.base_dn))
>+
>+        samba.tests.delete_force(self.ldb, self.ou_dn,
>+                                 controls=['tree_delete:1'])
>+
>+        self.ldb.add({
>+            "dn": self.ou_dn,
>+            "objectclass": "organizationalUnit",
>+            "ou": self.OU_NAME_ASQ})
>+
>+        self.members = []
>+        self.members2 = []
>+
>+        for x in range(20):
>+            name = self.NAME_ASQ + "_" + str(x)
>+            dn = ldb.Dn(self.ldb,
>+                        "cn=" + name + "," + str(self.ou_dn))
>+            self.members.append(dn)
>+            self.ldb.add({
>+                "dn": dn,
>+                "objectclass": "group"})
>+
>+        for x in range(20):
>+            name = self.NAME_ASQ + "_" + str(x + 20)
>+            dn = ldb.Dn(self.ldb,
>+                        "cn=" + name + "," + str(self.ou_dn))
>+            self.members2.append(dn)
>+            self.ldb.add({
>+                "dn": dn,
>+                "objectclass": "group",
>+                "member": [str(x) for x in self.members]})
>+
>+        name = self.NAME_ASQ + "_" + str(x + 40)
>+        self.top_dn = ldb.Dn(self.ldb,
>+                             "cn=" + name + "," + str(self.ou_dn))
>+        self.ldb.add({
>+            "dn": self.top_dn,
>+            "objectclass": "group",
>+            "member": [str(x) for x in self.members2]})
>+
>+    def tearDown(self):
>+        samba.tests.delete_force(self.ldb, self.ou_dn,
>+                                 controls=['tree_delete:1'])
>+
>+    def test_asq(self):
>+        """Testing ASQ behaviour.
>+
>+        ASQ is very strange, it turns a BASE search into a search for
>+        all the objects pointed to by the specified attribute,
>+        returning multiple entries!
>+
>+        """
>+
>+        msgs = self.ldb.search(base=self.top_dn,
>+                               scope=ldb.SCOPE_BASE,
>+                               attrs=["objectGUID", "cn", "member"],
>+                               controls=["asq:1:member"])
>+
>+        self.assertEqual(len(msgs), 20)
>+
>+        for msg in msgs:
>+            self.assertNotEqual(msg.dn, self.top_dn)
>+            self.assertIn(msg.dn, self.members2)
>+            for group in msg["member"]:
>+                self.assertIn(ldb.Dn(self.ldb, str(group)),
>+                              self.members)
>+
>+    def test_asq_paged(self):
>+        """Testing ASQ behaviour with paged_results set.
>+
>+        ASQ is very strange, it turns a BASE search into a search for
>+        all the objects pointed to by the specified attribute,
>+        returning multiple entries!
>+
>+        """
>+
>+        msgs = self.ldb.search(base=self.top_dn,
>+                               scope=ldb.SCOPE_BASE,
>+                               attrs=["objectGUID", "cn", "member"],
>+                               controls=["asq:1:member",
>+                                         "paged_results:1:1024"])
>+
>+        self.assertEqual(len(msgs), 20)
>+
>+        for msg in msgs:
>+            self.assertNotEqual(msg.dn, self.top_dn)
>+            self.assertIn(msg.dn, self.members2)
>+            for group in msg["member"]:
>+                self.assertIn(ldb.Dn(self.ldb, str(group)),
>+                              self.members)
>+
>+if "://" not in url:
>+    if os.path.isfile(url):
>+        url = "tdb://%s" % url
>+    else:
>+        url = "ldap://%s" % url
>+
>+TestProgram(module=__name__, opts=subunitopts)
>diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py
>index ae2b10ae659..52db18a872b 100755
>--- a/source4/selftest/tests.py
>+++ b/source4/selftest/tests.py
>@@ -885,6 +885,7 @@ plantestsuite_loadlist("samba4.tokengroups.krb5.python(ad_dc_default)", "ad_dc_d
> plantestsuite_loadlist("samba4.tokengroups.ntlm.python(ad_dc_default)", "ad_dc_default:local", [python, os.path.join(DSDB_PYTEST_DIR, "token_group.py"), '$SERVER', '-U"$USERNAME%$PASSWORD"', '--workgroup=$DOMAIN', '-k', 'no', '$LOADLIST', '$LISTOPT'])
> plantestsuite("samba4.sam.python(fl2008r2dc)", "fl2008r2dc", [python, os.path.join(DSDB_PYTEST_DIR, "sam.py"), '$SERVER', '-U"$USERNAME%$PASSWORD"', '--workgroup=$DOMAIN'])
> plantestsuite("samba4.sam.python(ad_dc_default)", "ad_dc_default", [python, os.path.join(DSDB_PYTEST_DIR, "sam.py"), '$SERVER', '-U"$USERNAME%$PASSWORD"', '--workgroup=$DOMAIN'])
>+plantestsuite("samba4.asq.python(ad_dc_default)", "ad_dc_default", [python, os.path.join(DSDB_PYTEST_DIR, "asq.py"), '$SERVER', '-U"$USERNAME%$PASSWORD"', '--workgroup=$DOMAIN'])
> plantestsuite("samba4.user_account_control.python(ad_dc_default)", "ad_dc_default", [python, os.path.join(DSDB_PYTEST_DIR, "user_account_control.py"), '$SERVER', '-U"$USERNAME%$PASSWORD"', '--workgroup=$DOMAIN'])
> 
> for env in ['ad_dc_default:local', 'schema_dc:local']:
>-- 
>2.17.1
>
>
>From 48c22901b7a1461098437bb28c85e9193b3e3e93 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Wed, 11 Mar 2020 16:41:34 +1300
>Subject: [PATCH 2/3] CVE-2020-10700: ldb: Always use ldb_next_request() in ASQ
> module
>
>We want to keep going down the module stack, and not start from the top again.
>
>ASQ is above the ACL modules, but below paged_results and we do not wish to
>re-trigger that work.
>
>Thanks to Andrei Popa <andrei.popa@next-gen.ro> for finding,
>reporting and working with us to diagnose this issue!
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14331
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Gary Lockyer <gary@catalyst.net.nz>
>---
> lib/ldb/modules/asq.c | 12 +++---------
> 1 file changed, 3 insertions(+), 9 deletions(-)
>
>diff --git a/lib/ldb/modules/asq.c b/lib/ldb/modules/asq.c
>index 7482de826f0..4eba941ae0b 100644
>--- a/lib/ldb/modules/asq.c
>+++ b/lib/ldb/modules/asq.c
>@@ -311,12 +311,9 @@ static int asq_build_multiple_requests(struct asq_context *ac, bool *terminated)
> 
> static int asq_search_continue(struct asq_context *ac)
> {
>-	struct ldb_context *ldb;
> 	bool terminated = false;
> 	int ret;
> 
>-	ldb = ldb_module_get_ctx(ac->module);
>-
> 	switch (ac->step) {
> 	case ASQ_SEARCH_BASE:
> 
>@@ -328,7 +325,7 @@ static int asq_search_continue(struct asq_context *ac)
> 
> 		ac->step = ASQ_SEARCH_MULTI;
> 
>-		return ldb_request(ldb, ac->reqs[ac->cur_req]);
>+		return ldb_next_request(ac->module, ac->reqs[ac->cur_req]);
> 
> 	case ASQ_SEARCH_MULTI:
> 
>@@ -339,7 +336,7 @@ static int asq_search_continue(struct asq_context *ac)
> 			return asq_search_terminate(ac);
> 		}
> 
>-		return ldb_request(ldb, ac->reqs[ac->cur_req]);
>+		return ldb_next_request(ac->module, ac->reqs[ac->cur_req]);
> 	}
> 
> 	return LDB_ERR_OPERATIONS_ERROR;
>@@ -347,14 +344,11 @@ static int asq_search_continue(struct asq_context *ac)
> 
> static int asq_search(struct ldb_module *module, struct ldb_request *req)
> {
>-	struct ldb_context *ldb;
> 	struct ldb_request *base_req;
> 	struct ldb_control *control;
> 	struct asq_context *ac;
> 	int ret;
> 
>-	ldb = ldb_module_get_ctx(module);
>-
> 	/* check if there's an ASQ control */
> 	control = ldb_request_get_control(req, LDB_CONTROL_ASQ_OID);
> 	if (control == NULL) {
>@@ -385,7 +379,7 @@ static int asq_search(struct ldb_module *module, struct ldb_request *req)
> 
> 	ac->step = ASQ_SEARCH_BASE;
> 
>-	return ldb_request(ldb, base_req);
>+	return ldb_next_request(ac->module, base_req);
> }
> 
> static int asq_init(struct ldb_module *module)
>-- 
>2.17.1
>
>
>From a3d8f09acfb540522fc72727bfdc549c5f030287 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Wed, 11 Mar 2020 16:43:31 +1300
>Subject: [PATCH 3/3] CVE-2020-10700: dsdb: Do not permit the ASQ control for
> the GUID search in paged_results
>
>ASQ is a very strange control and a BASE search can return multiple results
>that are NOT the requested DN, but the DNs pointed to by it!
>
>Thanks to Andrei Popa <andrei.popa@next-gen.ro> for finding,
>reporting and working with us to diagnose this issue!
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14331
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Gary Lockyer <gary@catalyst.net.nz>
>---
> selftest/knownfail.d/asq                       |  1 -
> source4/dsdb/samdb/ldb_modules/paged_results.c | 18 +++++++++++++-----
> 2 files changed, 13 insertions(+), 6 deletions(-)
> delete mode 100644 selftest/knownfail.d/asq
>
>diff --git a/selftest/knownfail.d/asq b/selftest/knownfail.d/asq
>deleted file mode 100644
>index eb0e3e0aba1..00000000000
>--- a/selftest/knownfail.d/asq
>+++ /dev/null
>@@ -1 +0,0 @@
>-samba4.asq.python$ad_dc_default$.__main__.ASQLDAPTest.test_asq_paged
>\ No newline at end of file
>diff --git a/source4/dsdb/samdb/ldb_modules/paged_results.c b/source4/dsdb/samdb/ldb_modules/paged_results.c
>index 940d2254fb0..dc211dd18ce 100644
>--- a/source4/dsdb/samdb/ldb_modules/paged_results.c
>+++ b/source4/dsdb/samdb/ldb_modules/paged_results.c
>@@ -483,8 +483,14 @@ paged_results_copy_down_controls(TALLOC_CTX *mem_ctx,
> 		if (control->oid == NULL) {
> 			continue;
> 		}
>-		if (strncmp(control->oid, LDB_CONTROL_PAGED_RESULTS_OID,
>-		    sizeof(LDB_CONTROL_PAGED_RESULTS_OID)) == 0) {
>+		if (strcmp(control->oid, LDB_CONTROL_PAGED_RESULTS_OID) == 0) {
>+			continue;
>+		}
>+		/*
>+		 * ASQ changes everything, do not copy it down for the
>+		 * per-GUID search
>+		 */
>+		if (strcmp(control->oid, LDB_CONTROL_ASQ_OID) == 0) {
> 			continue;
> 		}
> 		new_controls[j] = talloc_steal(new_controls, control);
>@@ -534,21 +540,23 @@ static bool paged_controls_same(struct ldb_request *req,
> 
> 	num_non_null_req_controls = 0;
> 	for (i=0; req->controls[i] != NULL; i++) {
>-		if (req->controls[i]->oid != NULL) {
>+		if (req->controls[i]->oid != NULL &&
>+		    strcmp(req->controls[i]->oid,
>+			   LDB_CONTROL_ASQ_OID) != 0) {
> 			num_non_null_req_controls++;
> 		}
> 	}
> 
> 	/* At this point we have the number of non-null entries for both
> 	 * control lists and we know that:
>-	 * 1. down_controls does not contain the paged control
>+	 * 1. down_controls does not contain the paged control or ASQ
> 	 * 	(because paged_results_copy_down_controls excludes it)
> 	 * 2. req->controls does contain the paged control
> 	 * 	(because this function is only called if this is true)
> 	 * 3. down_controls is a subset of non-null controls in req->controls
> 	 * 	(checked above)
> 	 * So to confirm that the two lists are identical except for the paged
>-	 * control, all we need to check is: */
>+	 * control and possibly ASQ, all we need to check is: */
> 	if (num_non_null_req_controls == num_down_controls + 1) {
> 		return true;
> 	}
>-- 
>2.17.1
>

Flags:

dbagnall: review+

Actions: View

Attachments on bug 14331: 15882 | 15884 | 15885 | 15890 | 15891 | 15892 | 15921 | 15924 | 15925 | 15926 | 15927 | 15929 | 15930 | 15931 | 15934