Attachment 15983 Details for Bug 14364 – Proposed patch for master V1

[patch] Proposed patch for master V1

CVE-2020-10730-master-V1.patch (text/plain), 14.55 KB, created by Gary Lockyer on 2020-05-15 02:22:06 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Gary Lockyer

Created: 2020-05-15 02:22:06 UTC

Size: 14.55 KB

patch

obsolete

>From afbc19b3412db8aa7381076420c285ccb86d2aed Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Tue, 5 May 2020 12:54:59 +1200
>Subject: [PATCH 1/8] CVE-2020-10730: vlv: Use strcmp(), not strncmp() checking
> the NULL terminated control OIDs
>
>The end result is the same, as sizeof() includes the trailing NUL, but this
>avoids having to think about that.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14364
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Gary Lockyer <gary@catalyst.net.nz>
>---
> source4/dsdb/samdb/ldb_modules/vlv_pagination.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c
>index 980177cb05e..31e64b4bd78 100644
>--- a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c
>+++ b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c
>@@ -682,8 +682,8 @@ vlv_copy_down_controls(TALLOC_CTX *mem_ctx, struct ldb_control **controls)
> 		if (control->oid == NULL) {
> 			break;
> 		}
>-		if (strncmp(control->oid, LDB_CONTROL_VLV_REQ_OID, sizeof(LDB_CONTROL_VLV_REQ_OID)) == 0 ||
>-		    strncmp(control->oid, LDB_CONTROL_SERVER_SORT_OID, sizeof(LDB_CONTROL_SERVER_SORT_OID)) == 0) {
>+		if (strcmp(control->oid, LDB_CONTROL_VLV_REQ_OID) == 0 ||
>+		    strcmp(control->oid, LDB_CONTROL_SERVER_SORT_OID) == 0) {
> 			continue;
> 		}
> 		new_controls[j] = talloc_steal(new_controls, control);
>-- 
>2.17.1
>
>
>From 9063ca2eeb949cef93fc8c7e23a3457f59186d95 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Tue, 5 May 2020 12:55:57 +1200
>Subject: [PATCH 2/8] CVE-2020-10730: vlv: Do not re-ASQ search the results of
> an ASQ search with VLV
>
>This is a silly combination, but at least try and keep the results sensible
>and avoid a double-dereference.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14364
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Gary Lockyer <gary@catalyst.net.nz>
>---
> source4/dsdb/samdb/ldb_modules/vlv_pagination.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
>diff --git a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c
>index 31e64b4bd78..d58a62482c9 100644
>--- a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c
>+++ b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c
>@@ -682,10 +682,21 @@ vlv_copy_down_controls(TALLOC_CTX *mem_ctx, struct ldb_control **controls)
> 		if (control->oid == NULL) {
> 			break;
> 		}
>+		/*
>+		 * Do not re-use VLV, nor the server-sort, both are
>+		 * already handled here.
>+		 */
> 		if (strcmp(control->oid, LDB_CONTROL_VLV_REQ_OID) == 0 ||
> 		    strcmp(control->oid, LDB_CONTROL_SERVER_SORT_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);
> 		j++;
> 	}
>-- 
>2.17.1
>
>
>From 35ded89cc73dad658897f89d90506d7de153c583 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Tue, 5 May 2020 13:16:48 +1200
>Subject: [PATCH 3/8] CVE-2020-10730: selftest: Add test to confirm VLV
> interaction with ASQ
>
>Tested against Windows 1709.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14364
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Gary Lockyer <gary@catalyst.net.nz>
>---
> source4/dsdb/tests/python/asq.py | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
>diff --git a/source4/dsdb/tests/python/asq.py b/source4/dsdb/tests/python/asq.py
>index a32c9f40cd3..1c93a45f131 100644
>--- a/source4/dsdb/tests/python/asq.py
>+++ b/source4/dsdb/tests/python/asq.py
>@@ -162,6 +162,33 @@ class ASQLDAPTest(samba.tests.TestCase):
>                 self.assertIn(ldb.Dn(self.ldb, str(group)),
>                               self.members)
> 
>+    def test_asq_vlv(self):
>+        """Testing ASQ behaviour with VLV 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!
>+
>+        """
>+
>+        sort_control = "server_sort:1:0:cn"
>+
>+        msgs = self.ldb.search(base=self.top_dn,
>+                               scope=ldb.SCOPE_BASE,
>+                               attrs=["objectGUID", "cn", "member"],
>+                               controls=["asq:1:member",
>+                                         sort_control,
>+                                         "vlv:1:20:20:11:0"])
>+
>+        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
>-- 
>2.17.1
>
>
>From baecf1cb9c23e2e2eecbf75378c03417f3a7963d Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Tue, 5 May 2020 16:34:11 +1200
>Subject: [PATCH 4/8] CVE-2020-10730: vlv: Another workaround for mixing ASQ
> and VLV
>
>This is essentially an alternative patch, but without the correct
>behaviour.  Instead this just avoids a segfault.
>
>Included in case we have something simialr again in
>another module.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14364
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Gary Lockyer <gary@catalyst.net.nz>
>---
> .../dsdb/samdb/ldb_modules/vlv_pagination.c   | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
>diff --git a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c
>index d58a62482c9..720b5e95638 100644
>--- a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c
>+++ b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c
>@@ -442,10 +442,21 @@ static int vlv_results(struct vlv_context *ac)
> 			ret = vlv_search_by_dn_guid(ac->module, ac, &result, guid,
> 						    ac->req->op.search.attrs);
> 
>-			if (ret == LDAP_NO_SUCH_OBJECT) {
>-				/* The thing isn't there, which we quietly
>-				   ignore and go on to send an extra one
>-				   instead. */
>+			if (ret == LDAP_NO_SUCH_OBJECT
>+			    || result->count != 1) {
>+				/*
>+				 * The thing isn't there, which we quietly
>+				 * ignore and go on to send an extra one
>+				 * instead.
>+				 *
>+				 * result->count == 0 or > 1 can only
>+				 * happen if ASQ (which breaks all the
>+				 * rules) is somehow invoked (as this
>+				 * is a BASE search).
>+				 *
>+				 * (We skip the ASQ cookie for the
>+				 * GUID searches)
>+				 */
> 				if (last_i < ac->store->num_entries - 1) {
> 					last_i++;
> 				}
>-- 
>2.17.1
>
>
>From 6d69426bc794a73ba501a41d3eb1865548289837 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Wed, 6 May 2020 16:19:01 +1200
>Subject: [PATCH 5/8] CVE-2020-10730: selftest: Add test to show that VLV and
> paged_results are incompatible
>
>As tested against Windows Server 1709
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14364
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Gary Lockyer <gary@catalyst.net.nz>
>---
> source4/dsdb/tests/python/asq.py | 27 +++++++++++++++++++++++++++
> source4/dsdb/tests/python/vlv.py | 23 +++++++++++++++++++++++
> 2 files changed, 50 insertions(+)
>
>diff --git a/source4/dsdb/tests/python/asq.py b/source4/dsdb/tests/python/asq.py
>index 1c93a45f131..33973d66c37 100644
>--- a/source4/dsdb/tests/python/asq.py
>+++ b/source4/dsdb/tests/python/asq.py
>@@ -189,6 +189,33 @@ class ASQLDAPTest(samba.tests.TestCase):
>                 self.assertIn(ldb.Dn(self.ldb, str(group)),
>                               self.members)
> 
>+    def test_asq_vlv_paged(self):
>+        """Testing ASQ behaviour with VLV and 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!
>+
>+        Thankfully combining both of these gives
>+        unavailable-critical-extension against Windows 1709
>+
>+        """
>+
>+        sort_control = "server_sort:1:0:cn"
>+
>+        try:
>+            msgs = self.ldb.search(base=self.top_dn,
>+                                   scope=ldb.SCOPE_BASE,
>+                                   attrs=["objectGUID", "cn", "member"],
>+                                   controls=["asq:1:member",
>+                                             sort_control,
>+                                             "vlv:1:20:20:11:0",
>+                                             "paged_results:1:1024"])
>+            self.fail("should have failed with LDAP_UNAVAILABLE_CRITICAL_EXTENSION")
>+        except ldb.LdbError as e:
>+            (enum, estr) = e.args
>+            self.assertEqual(enum, ldb.ERR_UNSUPPORTED_CRITICAL_EXTENSION)
>+
> if "://" not in url:
>     if os.path.isfile(url):
>         url = "tdb://%s" % url
>diff --git a/source4/dsdb/tests/python/vlv.py b/source4/dsdb/tests/python/vlv.py
>index 47c6d0610c7..0adde9799f0 100644
>--- a/source4/dsdb/tests/python/vlv.py
>+++ b/source4/dsdb/tests/python/vlv.py
>@@ -1670,6 +1670,29 @@ class PagedResultsTests(TestsWithUserOU):
>                                        page_size=len(self.users))
>         self.assertEqual(results, set_2[ps*2:])
> 
>+    def test_vlv_paged(self):
>+        """Testing behaviour with VLV and paged_results set.
>+
>+        A strange combination, certainly
>+
>+        Thankfully combining both of these gives
>+        unavailable-critical-extension against Windows 1709
>+
>+        """
>+        sort_control = "server_sort:1:0:cn"
>+
>+        try:
>+            msgs = self.ldb.search(base=self.base_dn,
>+                                   scope=ldb.SCOPE_SUBTREE,
>+                                   attrs=["objectGUID", "cn", "member"],
>+                                   controls=["vlv:1:20:20:11:0",
>+                                             sort_control,
>+                                             "paged_results:1:1024"])
>+            self.fail("should have failed with LDAP_UNAVAILABLE_CRITICAL_EXTENSION")
>+        except ldb.LdbError as e:
>+            (enum, estr) = e.args
>+            self.assertEqual(enum, ldb.ERR_UNSUPPORTED_CRITICAL_EXTENSION)
>+
> 
> if "://" not in host:
>     if os.path.isfile(host):
>-- 
>2.17.1
>
>
>From 942111a1ed2e5d5dc8881bcf5c54ac15ece10f40 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Wed, 6 May 2020 17:05:30 +1200
>Subject: [PATCH 6/8] CVE-2020-10730: dsdb: Fix crash when vlv and
> paged_results are combined
>
>The GUID is not returned in the DN for some reason in this (to be banned)
>combination.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14364
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Gary Lockyer <gary@catalyst.net.nz>
>---
> source4/dsdb/samdb/ldb_modules/paged_results.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/source4/dsdb/samdb/ldb_modules/paged_results.c b/source4/dsdb/samdb/ldb_modules/paged_results.c
>index dc211dd18ce..f720a2e4337 100644
>--- a/source4/dsdb/samdb/ldb_modules/paged_results.c
>+++ b/source4/dsdb/samdb/ldb_modules/paged_results.c
>@@ -416,6 +416,10 @@ static int paged_search_callback(struct ldb_request *req,
> 
> 		guid_blob = ldb_dn_get_extended_component(ares->message->dn,
> 							  "GUID");
>+		if (guid_blob == NULL) {
>+			return ldb_module_done(ac->req, NULL, NULL,
>+					       LDB_ERR_OPERATIONS_ERROR);
>+		}
> 		status = GUID_from_ndr_blob(guid_blob, &guid);
> 		if (!NT_STATUS_IS_OK(status)) {
> 			return ldb_module_done(ac->req, NULL, NULL,
>-- 
>2.17.1
>
>
>From 26ff5503e6d0d2c8bb35bc3f63808987a600cf69 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Wed, 6 May 2020 16:18:19 +1200
>Subject: [PATCH 7/8] CVE-2020-10730: dsdb: Ban the combination of
> paged_results and VLV
>
>This (two different paging controls) makes no sense and fails against
>Windows Server 1709.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14364
>
>Signed-off-by: Andrew Bartlett <abartlet@samba.org>
>Reviewed-by: Gary Lockyer <gary@catalyst.net.nz>
>---
> source4/dsdb/samdb/ldb_modules/paged_results.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
>diff --git a/source4/dsdb/samdb/ldb_modules/paged_results.c b/source4/dsdb/samdb/ldb_modules/paged_results.c
>index f720a2e4337..aa49a6e4aa5 100644
>--- a/source4/dsdb/samdb/ldb_modules/paged_results.c
>+++ b/source4/dsdb/samdb/ldb_modules/paged_results.c
>@@ -589,6 +589,7 @@ static int paged_search(struct ldb_module *module, struct ldb_request *req)
> {
> 	struct ldb_context *ldb;
> 	struct ldb_control *control;
>+	struct ldb_control *vlv_control;
> 	struct private_data *private_data;
> 	struct ldb_paged_control *paged_ctrl;
> 	struct ldb_request *search_req;
>@@ -612,6 +613,15 @@ static int paged_search(struct ldb_module *module, struct ldb_request *req)
> 	private_data = talloc_get_type(ldb_module_get_private(module),
> 					struct private_data);
> 
>+	vlv_control = ldb_request_get_control(req, LDB_CONTROL_VLV_REQ_OID);
>+	if (vlv_control != NULL) {
>+		/*
>+		 * VLV and paged_results are not allowed at the same
>+		 * time
>+		 */
>+		return LDB_ERR_UNSUPPORTED_CRITICAL_EXTENSION;
>+	}
>+
> 	ac = talloc_zero(req, struct paged_context);
> 	if (ac == NULL) {
> 		ldb_set_errstring(ldb, "Out of Memory");
>-- 
>2.17.1
>
>
>From a0aec860eca3f4056a1d3eeb4bc8e53a25db8305 Mon Sep 17 00:00:00 2001
>From: Gary Lockyer <gary@catalyst.net.nz>
>Date: Wed, 13 May 2020 10:56:56 +1200
>Subject: [PATCH 8/8] CVE-2020-10730: lib ldb: Check if
> ldb_lock_backend_callback called twice
>
>Prevent use after free issues if ldb_lock_backend_callback is called
>twice, usually due to ldb_module_done being called twice. This can happen if a
>module ignores the return value from function a function that calls
>ldb_module_done as part of it's error handling.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14364
>
>Signed-off-by: Gary Lockyer <gary@catalyst.net.nz>
>---
> lib/ldb/common/ldb.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
>diff --git a/lib/ldb/common/ldb.c b/lib/ldb/common/ldb.c
>index 8c86dca45a1..0fec89a52a8 100644
>--- a/lib/ldb/common/ldb.c
>+++ b/lib/ldb/common/ldb.c
>@@ -1018,6 +1018,13 @@ static int ldb_lock_backend_callback(struct ldb_request *req,
> 	struct ldb_db_lock_context *lock_context;
> 	int ret;
> 
>+	if (req->context == NULL) {
>+		/*
>+		 * The usual way to get here is to ignore the return codes
>+		 * and continuing processing after an error.
>+		 */
>+		abort();
>+	}
> 	lock_context = talloc_get_type(req->context,
> 				       struct ldb_db_lock_context);
> 
>@@ -1032,7 +1039,7 @@ static int ldb_lock_backend_callback(struct ldb_request *req,
> 		 * If this is a LDB_REPLY_DONE or an error, unlock the
> 		 * DB by calling the destructor on this context
> 		 */
>-		talloc_free(lock_context);
>+		TALLOC_FREE(req->context);
> 		return ret;
> 	}
> 
>-- 
>2.17.1
>

Actions: View

Attachments on bug 14364: 15951 | 15952 | 15983 | 15987 | 15998 | 16002 | 16003 | 16004 | 16005 | 16007 | 16008 | 16009 | 16056 | 16067 | 16089