From 1bef227d892896251929bc473cae787828458d7e Mon Sep 17 00:00:00 2001 From: Andrew Bartlett 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 Reviewed-by: Gary Lockyer --- 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 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 Reviewed-by: Gary Lockyer --- 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 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 Reviewed-by: Gary Lockyer --- 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 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 Reviewed-by: Gary Lockyer --- .../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 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 Reviewed-by: Gary Lockyer --- 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 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 Reviewed-by: Gary Lockyer --- 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 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 Reviewed-by: Gary Lockyer --- 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 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 --- 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 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 --- 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 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 --- 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