Attachment 15998 Details for Bug 14364 – Proposed patch for master V3

[patch] Proposed patch for master V3

CVE-2020-10730-V3.patch (text/plain), 23.72 KB, created by Gary Lockyer on 2020-05-21 02:51:45 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Gary Lockyer

Created: 2020-05-21 02:51:45 UTC

Size: 23.72 KB

patch

obsolete

>From aab5401d1cea36234399b44d2861ac7f89548ff9 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 9fb66c359d463876bc977f15b48628b82e441b63 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 cd0cb8fd7bf422b79c0a791c3b4f3a1444b9c0dc 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 c58d796ad42187a178f17094651715053ef30bdb 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 8d523547fa86eeeaf3d7ca6581c24954775f93eb 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 9430b1464ab788a74942c3a80968030999c4ef42 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 029a22b7aa9ddad9c4c21471eaa56bff814d2471 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 e13ecd25821d2a406a2f3339698bfdaadde34ddb 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>
>---
> .../dsdb/samdb/ldb_modules/paged_results.c    | 43 +++++++++++++++----
> 1 file changed, 34 insertions(+), 9 deletions(-)
>
>diff --git a/source4/dsdb/samdb/ldb_modules/paged_results.c b/source4/dsdb/samdb/ldb_modules/paged_results.c
>index aa49a6e4aa5..735883e8802 100644
>--- a/source4/dsdb/samdb/ldb_modules/paged_results.c
>+++ b/source4/dsdb/samdb/ldb_modules/paged_results.c
>@@ -237,14 +237,16 @@ static int paged_search_by_dn_guid(struct ldb_module *module,
> 	return ret;
> }
> 
>-static int paged_results(struct paged_context *ac)
>+static int paged_results(struct paged_context *ac, struct ldb_reply *ares)
> {
> 	struct ldb_paged_control *paged;
> 	unsigned int i, num_ctrls;
> 	int ret;
> 
> 	if (ac->store == NULL) {
>-		return LDB_ERR_OPERATIONS_ERROR;
>+		ret = LDB_ERR_OPERATIONS_ERROR;
>+		return ldb_module_done(
>+			ac->req, ac->controls, ares->response, ret);
> 	}
> 
> 	while (ac->store->last_i < ac->store->num_entries && ac->size > 0) {
>@@ -273,12 +275,17 @@ static int paged_results(struct paged_context *ac)
> 			   instead. */
> 			continue;
> 		} else if (ret != LDB_SUCCESS) {
>-			return ret;
>+			return ldb_module_done(
>+				ac->req, ac->controls, ares->response, ret);
> 		}
> 
> 		ret = ldb_module_send_entry(ac->req, result->msgs[0],
> 					    NULL);
> 		if (ret != LDB_SUCCESS) {
>+			/*
>+			 * ldb_module_send_entry will have called
>+			 * ldb_module_done if an error occurred.
>+			 */
> 			return ret;
> 		}
> 	}
>@@ -289,6 +296,10 @@ static int paged_results(struct paged_context *ac)
> 		*/
> 		ret = send_referrals(ac->store, ac->req);
> 		if (ret != LDB_SUCCESS) {
>+			/*
>+			 * send_referrals will have called ldb_module_done
>+			 * if an error occurred.
>+			 */
> 			return ret;
> 		}
> 	}
>@@ -305,7 +316,9 @@ static int paged_results(struct paged_context *ac)
> 
> 	ac->controls = talloc_array(ac, struct ldb_control *, num_ctrls +1);
> 	if (ac->controls == NULL) {
>-		return LDB_ERR_OPERATIONS_ERROR;
>+		ret = LDB_ERR_OPERATIONS_ERROR;
>+		return ldb_module_done(
>+			ac->req, ac->controls, ares->response, ret);
> 	}
> 	ac->controls[num_ctrls] = NULL;
> 
>@@ -316,20 +329,26 @@ static int paged_results(struct paged_context *ac)
> 
> 	ac->controls[i] = talloc(ac->controls, struct ldb_control);
> 	if (ac->controls[i] == NULL) {
>-		return LDB_ERR_OPERATIONS_ERROR;
>+		ret = LDB_ERR_OPERATIONS_ERROR;
>+		return ldb_module_done(
>+			ac->req, ac->controls, ares->response, ret);
> 	}
> 
> 	ac->controls[i]->oid = talloc_strdup(ac->controls[i],
> 						LDB_CONTROL_PAGED_RESULTS_OID);
> 	if (ac->controls[i]->oid == NULL) {
>-		return LDB_ERR_OPERATIONS_ERROR;
>+		ret = LDB_ERR_OPERATIONS_ERROR;
>+		return ldb_module_done(
>+			ac->req, ac->controls, ares->response, ret);
> 	}
> 
> 	ac->controls[i]->critical = 0;
> 
> 	paged = talloc(ac->controls[i], struct ldb_paged_control);
> 	if (paged == NULL) {
>-		return LDB_ERR_OPERATIONS_ERROR;
>+		ret = LDB_ERR_OPERATIONS_ERROR;
>+		return ldb_module_done(
>+			ac->req, ac->controls, ares->response, ret);
> 	}
> 
> 	ac->controls[i]->data = paged;
>@@ -456,7 +475,13 @@ static int paged_search_callback(struct ldb_request *req,
> 		store->result_array_size = store->num_entries;
> 
> 		ac->store->controls = talloc_move(ac->store, &ares->controls);
>-		ret = paged_results(ac);
>+		ret = paged_results(ac, ares);
>+		if (ret != LDB_SUCCESS) {
>+			/* paged_results will have called ldb_module_done
>+			 * if an error occurred
>+			 */
>+			return ret;
>+		}
> 		return ldb_module_done(ac->req, ac->controls,
> 					ares->response, ret);
> 	}
>@@ -768,7 +793,7 @@ static int paged_search(struct ldb_module *module, struct ldb_request *req)
> 								LDB_SUCCESS);
> 		}
> 
>-		ret = paged_results(ac);
>+		ret = paged_results(ac, NULL);
> 		if (ret != LDB_SUCCESS) {
> 			return ldb_module_done(req, NULL, NULL, ret);
> 		}
>-- 
>2.17.1
>
>
>From 61799aa27b180caca65544ec823441e8f8dd2ae2 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>
>---
> .../dsdb/samdb/ldb_modules/vlv_pagination.c   | 61 +++++++++++++++----
> 1 file changed, 49 insertions(+), 12 deletions(-)
>
>diff --git a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c
>index 720b5e95638..b103bda5f52 100644
>--- a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c
>+++ b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c
>@@ -387,7 +387,7 @@ static int vlv_calc_real_offset(int offset, int denominator, int n_entries)
>    has been prepared earlier and saved -- or by vlv_search_callback() when a
>    search has just been completed. */
> 
>-static int vlv_results(struct vlv_context *ac)
>+static int vlv_results(struct vlv_context *ac, struct ldb_reply *ares)
> {
> 	struct ldb_vlv_resp_control *vlv;
> 	unsigned int num_ctrls;
>@@ -397,7 +397,9 @@ static int vlv_results(struct vlv_context *ac)
> 	int target = 0;
> 
> 	if (ac->store == NULL) {
>-		return LDB_ERR_OPERATIONS_ERROR;
>+		ret = LDB_ERR_OPERATIONS_ERROR;
>+		return ldb_module_done(
>+			ac->req, ac->controls, ares->response, ret);
> 	}
> 
> 	if (ac->store->first_ref) {
>@@ -406,6 +408,10 @@ static int vlv_results(struct vlv_context *ac)
> 		*/
> 		ret = send_referrals(ac->store, ac->req);
> 		if (ret != LDB_SUCCESS) {
>+			/*
>+			 * send_referrals will have called ldb_module_done
>+			 * if there was an error.
>+			 */
> 			return ret;
> 		}
> 	}
>@@ -419,14 +425,23 @@ static int vlv_results(struct vlv_context *ac)
> 						    vlv_details,
> 						    sort_details, &ret);
> 			if (ret != LDB_SUCCESS) {
>-				return ret;
>+				return ldb_module_done(
>+					ac->req,
>+					ac->controls,
>+					ares->response,
>+					ret);
> 			}
> 		} else {
> 			target = vlv_calc_real_offset(vlv_details->match.byOffset.offset,
> 						      vlv_details->match.byOffset.contentCount,
> 						      ac->store->num_entries);
> 			if (target == -1) {
>-				return LDB_ERR_OPERATIONS_ERROR;
>+				ret = LDB_ERR_OPERATIONS_ERROR;
>+				return ldb_module_done(
>+					ac->req,
>+					ac->controls,
>+					ares->response,
>+					ret);
> 			}
> 		}
> 
>@@ -462,12 +477,20 @@ static int vlv_results(struct vlv_context *ac)
> 				}
> 				continue;
> 			} else if (ret != LDB_SUCCESS) {
>-				return ret;
>+				return ldb_module_done(
>+					ac->req,
>+					ac->controls,
>+					ares->response,
>+					ret);
> 			}
> 
> 			ret = ldb_module_send_entry(ac->req, result->msgs[0],
> 						    NULL);
> 			if (ret != LDB_SUCCESS) {
>+				/*
>+				 * ldb_module_send_entry will have called
>+				 * ldb_module_done if there was an error
>+				 */
> 				return ret;
> 			}
> 		}
>@@ -488,7 +511,9 @@ static int vlv_results(struct vlv_context *ac)
> 
> 	ac->controls = talloc_array(ac, struct ldb_control *, num_ctrls + 1);
> 	if (ac->controls == NULL) {
>-		return LDB_ERR_OPERATIONS_ERROR;
>+		ret = LDB_ERR_OPERATIONS_ERROR;
>+		return ldb_module_done(
>+			ac->req, ac->controls, ares->response, ret);
> 	}
> 	ac->controls[num_ctrls] = NULL;
> 
>@@ -498,20 +523,26 @@ static int vlv_results(struct vlv_context *ac)
> 
> 	ac->controls[i] = talloc(ac->controls, struct ldb_control);
> 	if (ac->controls[i] == NULL) {
>-		return LDB_ERR_OPERATIONS_ERROR;
>+		ret = LDB_ERR_OPERATIONS_ERROR;
>+		return ldb_module_done(
>+			ac->req, ac->controls, ares->response, ret);
> 	}
> 
> 	ac->controls[i]->oid = talloc_strdup(ac->controls[i],
> 					     LDB_CONTROL_VLV_RESP_OID);
> 	if (ac->controls[i]->oid == NULL) {
>-		return LDB_ERR_OPERATIONS_ERROR;
>+		ret = LDB_ERR_OPERATIONS_ERROR;
>+		return ldb_module_done(
>+			ac->req, ac->controls, ares->response, ret);
> 	}
> 
> 	ac->controls[i]->critical = 0;
> 
> 	vlv = talloc(ac->controls[i], struct ldb_vlv_resp_control);
> 	if (vlv == NULL) {
>-		return LDB_ERR_OPERATIONS_ERROR;
>+		ret = LDB_ERR_OPERATIONS_ERROR;
>+		return ldb_module_done(
>+			ac->req, ac->controls, ares->response, ret);
> 	}
> 	ac->controls[i]->data = vlv;
> 
>@@ -600,7 +631,13 @@ static int vlv_search_callback(struct ldb_request *req, struct ldb_reply *ares)
> 		store->result_array_size = store->num_entries;
> 
> 		ac->store->controls = talloc_move(ac->store, &ares->controls);
>-		ret = vlv_results(ac);
>+		ret = vlv_results(ac, ares);
>+		if (ret != LDB_SUCCESS) {
>+			/* vlv_results will have called ldb_module_done
>+			 * if there was an error.
>+			 */
>+			return ret;
>+		}
> 		return ldb_module_done(ac->req, ac->controls,
> 					ares->response, ret);
> 	}
>@@ -845,9 +882,9 @@ static int vlv_search(struct ldb_module *module, struct ldb_request *req)
> 			return ret;
> 		}
> 
>-		ret = vlv_results(ac);
>+		ret = vlv_results(ac, NULL);
> 		if (ret != LDB_SUCCESS) {
>-			return ldb_module_done(req, NULL, NULL, ret);
>+			return ret;
> 		}
> 		return ldb_module_done(req, ac->controls, NULL,
> 				       LDB_SUCCESS);
>-- 
>2.17.1
>
>
>From af17e088b306e29e929be36a6b66e4b6a2cb0a17 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
>

Flags:

abartlet: review+

Actions: View

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