From afbc19b3412db8aa7381076420c285ccb86d2aed Mon Sep 17 00:00:00 2001 From: Andrew Bartlett 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 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 9063ca2eeb949cef93fc8c7e23a3457f59186d95 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett 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 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 35ded89cc73dad658897f89d90506d7de153c583 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett 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 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 baecf1cb9c23e2e2eecbf75378c03417f3a7963d Mon Sep 17 00:00:00 2001 From: Andrew Bartlett 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 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 6d69426bc794a73ba501a41d3eb1865548289837 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett 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 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 942111a1ed2e5d5dc8881bcf5c54ac15ece10f40 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett 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 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 26ff5503e6d0d2c8bb35bc3f63808987a600cf69 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett 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 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 a0aec860eca3f4056a1d3eeb4bc8e53a25db8305 Mon Sep 17 00:00:00 2001 From: Gary Lockyer 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 --- 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