Attachment 15987 Details for Bug 14364 – Proposes patch for master V2

[patch] Proposes patch for master V2

CVE-2020-10730-V2.patch (text/plain), 16.87 KB, created by Gary Lockyer on 2020-05-18 03:27:51 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Gary Lockyer

Created: 2020-05-18 03:27:51 UTC

Size: 16.87 KB

patch

obsolete

>From 1bef227d892896251929bc473cae787828458d7e Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Tue, 5 May 2020 12:54:59 +1200
>Subject: [PATCH 01/10] 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 c678e0e8cd8cdc59140a75536a1b4dbbd1e609b5 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Tue, 5 May 2020 12:55:57 +1200
>Subject: [PATCH 02/10] 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 4e9c63761e9b410d92996ec2e83ab70583c765c1 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Tue, 5 May 2020 13:16:48 +1200
>Subject: [PATCH 03/10] 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 35df2412d1f7ab44faee155b05c7380254a66596 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Tue, 5 May 2020 16:34:11 +1200
>Subject: [PATCH 04/10] 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 421ab074ed3a789a65bbd92913ba0069f6b1e9bd Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Wed, 6 May 2020 16:19:01 +1200
>Subject: [PATCH 05/10] 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 8934bfd70cbf26aa5630fa5c424b83b6450d3d0c Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Wed, 6 May 2020 17:05:30 +1200
>Subject: [PATCH 06/10] 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 0fa664fc434e001e79d78b363ea24fc4d356aa26 Mon Sep 17 00:00:00 2001
>From: Andrew Bartlett <abartlet@samba.org>
>Date: Wed, 6 May 2020 16:18:19 +1200
>Subject: [PATCH 07/10] 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 a42c43973ac395969dca1a3074c6b5a9c81d826f Mon Sep 17 00:00:00 2001
>From: Gary Lockyer <gary@catalyst.net.nz>
>Date: Mon, 18 May 2020 12:36:57 +1200
>Subject: [PATCH 08/10] CVE-2020-10730: s4 dsdb paged_results: Prevent repeat
> call of ldb_module_done
>
>Check the return code from paged_results, if it is not LDB_SUCCESS
>ldb_module_done has already been called, and SHOULD NOT be called again.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14364
>
>Signed-off-by: Gary Lockyer <gary@catalyst.net.nz>
>---
> source4/dsdb/samdb/ldb_modules/paged_results.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/source4/dsdb/samdb/ldb_modules/paged_results.c b/source4/dsdb/samdb/ldb_modules/paged_results.c
>index aa49a6e4aa5..b36af65d142 100644
>--- a/source4/dsdb/samdb/ldb_modules/paged_results.c
>+++ b/source4/dsdb/samdb/ldb_modules/paged_results.c
>@@ -457,6 +457,9 @@ static int paged_search_callback(struct ldb_request *req,
> 
> 		ac->store->controls = talloc_move(ac->store, &ares->controls);
> 		ret = paged_results(ac);
>+		if (ret != LDB_SUCCESS) {
>+			return ret;
>+		}
> 		return ldb_module_done(ac->req, ac->controls,
> 					ares->response, ret);
> 	}
>-- 
>2.17.1
>
>
>From f48698fc9cdac9fbe5f84018981597b611a0ceba Mon Sep 17 00:00:00 2001
>From: Gary Lockyer <gary@catalyst.net.nz>
>Date: Mon, 18 May 2020 12:37:39 +1200
>Subject: [PATCH 09/10] CVE-2020-10730: s4 dsdb vlv_pagination: Prevent repeat
> call of ldb_module_done
>
>Check the return code from vlv_results, if it is not LDB_SUCCESS
>ldb_module_done has already been called, and SHOULD NOT be called again.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14364
>
>Signed-off-by: Gary Lockyer <gary@catalyst.net.nz>
>---
> source4/dsdb/samdb/ldb_modules/vlv_pagination.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c
>index 720b5e95638..b3b6164abed 100644
>--- a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c
>+++ b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c
>@@ -601,6 +601,9 @@ static int vlv_search_callback(struct ldb_request *req, struct ldb_reply *ares)
> 
> 		ac->store->controls = talloc_move(ac->store, &ares->controls);
> 		ret = vlv_results(ac);
>+		if (ret != LDB_SUCCESS) {
>+			return ret;
>+		}
> 		return ldb_module_done(ac->req, ac->controls,
> 					ares->response, ret);
> 	}
>-- 
>2.17.1
>
>
>From 6662d9d0f11d2ef6f7e6d3c4c290ebe5f04470eb 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 10/10] 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