From c61a5255d9efaa1e63e33810f5236ea21b283c40 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 5 May 2020 12:54:59 +1200 Subject: [PATCH 01/22] 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 dd53faf44463705f83e6f25e53a5682eb0dc0e1b Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 5 May 2020 12:55:57 +1200 Subject: [PATCH 02/22] 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 98a67d491375b9ccbf55e4cedd517a1678010a91 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 5 May 2020 13:16:48 +1200 Subject: [PATCH 03/22] 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 337f3e42841fb5c592b93285d37fb9ea238ee3ec Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 5 May 2020 16:34:11 +1200 Subject: [PATCH 04/22] 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 d6329e8ace7f332492201f224359fd7f2e0f6937 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 6 May 2020 16:19:01 +1200 Subject: [PATCH 05/22] 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 5b88e5abe49..5c1ef5c7fcf 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 8103df54b18ec7ae0fc3876c826da06385663776 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 6 May 2020 17:05:30 +1200 Subject: [PATCH 06/22] 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 b1f72d2ce5e7cf3dc6877040a0fd086bd5d7c2c9 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 6 May 2020 16:18:19 +1200 Subject: [PATCH 07/22] 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 c29319e91d0764c7078ea59af78a16dbaf0b6824 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Mon, 18 May 2020 12:36:57 +1200 Subject: [PATCH 08/22] 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 Reviewed-by: Andrew Bartlett --- .../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 b2658b9432c6a6a03b9da269da6ede701c8322fb Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Mon, 18 May 2020 12:37:39 +1200 Subject: [PATCH 09/22] 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 Reviewed-by: Andrew Bartlett --- .../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 9dd458956d7af1b4bbe505ba2ab72235e81c27d0 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Wed, 13 May 2020 10:56:56 +1200 Subject: [PATCH 10/22] 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 Reviewed-by: Andrew Bartlett --- 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 From 7cc5a5a35e93e5f5bfcafe767a07b3c87192a34f Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Fri, 22 May 2020 09:11:14 +1200 Subject: [PATCH 11/22] ldb: Bump version to 2.1.4 BUG: https://bugzilla.samba.org/show_bug.cgi?id=14364 Signed-off-by: Gary Lockyer Reviewed-by: Andrew Bartlett --- lib/ldb/ABI/ldb-2.1.4.sigs | 283 ++++++++++++++++++++++++++++++ lib/ldb/ABI/pyldb-util-2.1.4.sigs | 3 + lib/ldb/wscript | 2 +- 3 files changed, 287 insertions(+), 1 deletion(-) create mode 100644 lib/ldb/ABI/ldb-2.1.4.sigs create mode 100644 lib/ldb/ABI/pyldb-util-2.1.4.sigs diff --git a/lib/ldb/ABI/ldb-2.1.4.sigs b/lib/ldb/ABI/ldb-2.1.4.sigs new file mode 100644 index 00000000000..5049dc64ce1 --- /dev/null +++ b/lib/ldb/ABI/ldb-2.1.4.sigs @@ -0,0 +1,283 @@ +ldb_add: int (struct ldb_context *, const struct ldb_message *) +ldb_any_comparison: int (struct ldb_context *, void *, ldb_attr_handler_t, const struct ldb_val *, const struct ldb_val *) +ldb_asprintf_errstring: void (struct ldb_context *, const char *, ...) +ldb_attr_casefold: char *(TALLOC_CTX *, const char *) +ldb_attr_dn: int (const char *) +ldb_attr_in_list: int (const char * const *, const char *) +ldb_attr_list_copy: const char **(TALLOC_CTX *, const char * const *) +ldb_attr_list_copy_add: const char **(TALLOC_CTX *, const char * const *, const char *) +ldb_base64_decode: int (char *) +ldb_base64_encode: char *(TALLOC_CTX *, const char *, int) +ldb_binary_decode: struct ldb_val (TALLOC_CTX *, const char *) +ldb_binary_encode: char *(TALLOC_CTX *, struct ldb_val) +ldb_binary_encode_string: char *(TALLOC_CTX *, const char *) +ldb_build_add_req: int (struct ldb_request **, struct ldb_context *, TALLOC_CTX *, const struct ldb_message *, struct ldb_control **, void *, ldb_request_callback_t, struct ldb_request *) +ldb_build_del_req: int (struct ldb_request **, struct ldb_context *, TALLOC_CTX *, struct ldb_dn *, struct ldb_control **, void *, ldb_request_callback_t, struct ldb_request *) +ldb_build_extended_req: int (struct ldb_request **, struct ldb_context *, TALLOC_CTX *, const char *, void *, struct ldb_control **, void *, ldb_request_callback_t, struct ldb_request *) +ldb_build_mod_req: int (struct ldb_request **, struct ldb_context *, TALLOC_CTX *, const struct ldb_message *, struct ldb_control **, void *, ldb_request_callback_t, struct ldb_request *) +ldb_build_rename_req: int (struct ldb_request **, struct ldb_context *, TALLOC_CTX *, struct ldb_dn *, struct ldb_dn *, struct ldb_control **, void *, ldb_request_callback_t, struct ldb_request *) +ldb_build_search_req: int (struct ldb_request **, struct ldb_context *, TALLOC_CTX *, struct ldb_dn *, enum ldb_scope, const char *, const char * const *, struct ldb_control **, void *, ldb_request_callback_t, struct ldb_request *) +ldb_build_search_req_ex: int (struct ldb_request **, struct ldb_context *, TALLOC_CTX *, struct ldb_dn *, enum ldb_scope, struct ldb_parse_tree *, const char * const *, struct ldb_control **, void *, ldb_request_callback_t, struct ldb_request *) +ldb_casefold: char *(struct ldb_context *, TALLOC_CTX *, const char *, size_t) +ldb_casefold_default: char *(void *, TALLOC_CTX *, const char *, size_t) +ldb_check_critical_controls: int (struct ldb_control **) +ldb_comparison_binary: int (struct ldb_context *, void *, const struct ldb_val *, const struct ldb_val *) +ldb_comparison_fold: int (struct ldb_context *, void *, const struct ldb_val *, const struct ldb_val *) +ldb_connect: int (struct ldb_context *, const char *, unsigned int, const char **) +ldb_control_to_string: char *(TALLOC_CTX *, const struct ldb_control *) +ldb_controls_except_specified: struct ldb_control **(struct ldb_control **, TALLOC_CTX *, struct ldb_control *) +ldb_debug: void (struct ldb_context *, enum ldb_debug_level, const char *, ...) +ldb_debug_add: void (struct ldb_context *, const char *, ...) +ldb_debug_end: void (struct ldb_context *, enum ldb_debug_level) +ldb_debug_set: void (struct ldb_context *, enum ldb_debug_level, const char *, ...) +ldb_delete: int (struct ldb_context *, struct ldb_dn *) +ldb_dn_add_base: bool (struct ldb_dn *, struct ldb_dn *) +ldb_dn_add_base_fmt: bool (struct ldb_dn *, const char *, ...) +ldb_dn_add_child: bool (struct ldb_dn *, struct ldb_dn *) +ldb_dn_add_child_fmt: bool (struct ldb_dn *, const char *, ...) +ldb_dn_add_child_val: bool (struct ldb_dn *, const char *, struct ldb_val) +ldb_dn_alloc_casefold: char *(TALLOC_CTX *, struct ldb_dn *) +ldb_dn_alloc_linearized: char *(TALLOC_CTX *, struct ldb_dn *) +ldb_dn_canonical_ex_string: char *(TALLOC_CTX *, struct ldb_dn *) +ldb_dn_canonical_string: char *(TALLOC_CTX *, struct ldb_dn *) +ldb_dn_check_local: bool (struct ldb_module *, struct ldb_dn *) +ldb_dn_check_special: bool (struct ldb_dn *, const char *) +ldb_dn_compare: int (struct ldb_dn *, struct ldb_dn *) +ldb_dn_compare_base: int (struct ldb_dn *, struct ldb_dn *) +ldb_dn_copy: struct ldb_dn *(TALLOC_CTX *, struct ldb_dn *) +ldb_dn_escape_value: char *(TALLOC_CTX *, struct ldb_val) +ldb_dn_extended_add_syntax: int (struct ldb_context *, unsigned int, const struct ldb_dn_extended_syntax *) +ldb_dn_extended_filter: void (struct ldb_dn *, const char * const *) +ldb_dn_extended_syntax_by_name: const struct ldb_dn_extended_syntax *(struct ldb_context *, const char *) +ldb_dn_from_ldb_val: struct ldb_dn *(TALLOC_CTX *, struct ldb_context *, const struct ldb_val *) +ldb_dn_get_casefold: const char *(struct ldb_dn *) +ldb_dn_get_comp_num: int (struct ldb_dn *) +ldb_dn_get_component_name: const char *(struct ldb_dn *, unsigned int) +ldb_dn_get_component_val: const struct ldb_val *(struct ldb_dn *, unsigned int) +ldb_dn_get_extended_comp_num: int (struct ldb_dn *) +ldb_dn_get_extended_component: const struct ldb_val *(struct ldb_dn *, const char *) +ldb_dn_get_extended_linearized: char *(TALLOC_CTX *, struct ldb_dn *, int) +ldb_dn_get_ldb_context: struct ldb_context *(struct ldb_dn *) +ldb_dn_get_linearized: const char *(struct ldb_dn *) +ldb_dn_get_parent: struct ldb_dn *(TALLOC_CTX *, struct ldb_dn *) +ldb_dn_get_rdn_name: const char *(struct ldb_dn *) +ldb_dn_get_rdn_val: const struct ldb_val *(struct ldb_dn *) +ldb_dn_has_extended: bool (struct ldb_dn *) +ldb_dn_is_null: bool (struct ldb_dn *) +ldb_dn_is_special: bool (struct ldb_dn *) +ldb_dn_is_valid: bool (struct ldb_dn *) +ldb_dn_map_local: struct ldb_dn *(struct ldb_module *, void *, struct ldb_dn *) +ldb_dn_map_rebase_remote: struct ldb_dn *(struct ldb_module *, void *, struct ldb_dn *) +ldb_dn_map_remote: struct ldb_dn *(struct ldb_module *, void *, struct ldb_dn *) +ldb_dn_minimise: bool (struct ldb_dn *) +ldb_dn_new: struct ldb_dn *(TALLOC_CTX *, struct ldb_context *, const char *) +ldb_dn_new_fmt: struct ldb_dn *(TALLOC_CTX *, struct ldb_context *, const char *, ...) +ldb_dn_remove_base_components: bool (struct ldb_dn *, unsigned int) +ldb_dn_remove_child_components: bool (struct ldb_dn *, unsigned int) +ldb_dn_remove_extended_components: void (struct ldb_dn *) +ldb_dn_replace_components: bool (struct ldb_dn *, struct ldb_dn *) +ldb_dn_set_component: int (struct ldb_dn *, int, const char *, const struct ldb_val) +ldb_dn_set_extended_component: int (struct ldb_dn *, const char *, const struct ldb_val *) +ldb_dn_update_components: int (struct ldb_dn *, const struct ldb_dn *) +ldb_dn_validate: bool (struct ldb_dn *) +ldb_dump_results: void (struct ldb_context *, struct ldb_result *, FILE *) +ldb_error_at: int (struct ldb_context *, int, const char *, const char *, int) +ldb_errstring: const char *(struct ldb_context *) +ldb_extended: int (struct ldb_context *, const char *, void *, struct ldb_result **) +ldb_extended_default_callback: int (struct ldb_request *, struct ldb_reply *) +ldb_filter_attrs: int (struct ldb_context *, const struct ldb_message *, const char * const *, struct ldb_message *) +ldb_filter_from_tree: char *(TALLOC_CTX *, const struct ldb_parse_tree *) +ldb_get_config_basedn: struct ldb_dn *(struct ldb_context *) +ldb_get_create_perms: unsigned int (struct ldb_context *) +ldb_get_default_basedn: struct ldb_dn *(struct ldb_context *) +ldb_get_event_context: struct tevent_context *(struct ldb_context *) +ldb_get_flags: unsigned int (struct ldb_context *) +ldb_get_opaque: void *(struct ldb_context *, const char *) +ldb_get_root_basedn: struct ldb_dn *(struct ldb_context *) +ldb_get_schema_basedn: struct ldb_dn *(struct ldb_context *) +ldb_global_init: int (void) +ldb_handle_get_event_context: struct tevent_context *(struct ldb_handle *) +ldb_handle_new: struct ldb_handle *(TALLOC_CTX *, struct ldb_context *) +ldb_handle_use_global_event_context: void (struct ldb_handle *) +ldb_handler_copy: int (struct ldb_context *, void *, const struct ldb_val *, struct ldb_val *) +ldb_handler_fold: int (struct ldb_context *, void *, const struct ldb_val *, struct ldb_val *) +ldb_init: struct ldb_context *(TALLOC_CTX *, struct tevent_context *) +ldb_ldif_message_redacted_string: char *(struct ldb_context *, TALLOC_CTX *, enum ldb_changetype, const struct ldb_message *) +ldb_ldif_message_string: char *(struct ldb_context *, TALLOC_CTX *, enum ldb_changetype, const struct ldb_message *) +ldb_ldif_parse_modrdn: int (struct ldb_context *, const struct ldb_ldif *, TALLOC_CTX *, struct ldb_dn **, struct ldb_dn **, bool *, struct ldb_dn **, struct ldb_dn **) +ldb_ldif_read: struct ldb_ldif *(struct ldb_context *, int (*)(void *), void *) +ldb_ldif_read_file: struct ldb_ldif *(struct ldb_context *, FILE *) +ldb_ldif_read_file_state: struct ldb_ldif *(struct ldb_context *, struct ldif_read_file_state *) +ldb_ldif_read_free: void (struct ldb_context *, struct ldb_ldif *) +ldb_ldif_read_string: struct ldb_ldif *(struct ldb_context *, const char **) +ldb_ldif_write: int (struct ldb_context *, int (*)(void *, const char *, ...), void *, const struct ldb_ldif *) +ldb_ldif_write_file: int (struct ldb_context *, FILE *, const struct ldb_ldif *) +ldb_ldif_write_redacted_trace_string: char *(struct ldb_context *, TALLOC_CTX *, const struct ldb_ldif *) +ldb_ldif_write_string: char *(struct ldb_context *, TALLOC_CTX *, const struct ldb_ldif *) +ldb_load_modules: int (struct ldb_context *, const char **) +ldb_map_add: int (struct ldb_module *, struct ldb_request *) +ldb_map_delete: int (struct ldb_module *, struct ldb_request *) +ldb_map_init: int (struct ldb_module *, const struct ldb_map_attribute *, const struct ldb_map_objectclass *, const char * const *, const char *, const char *) +ldb_map_modify: int (struct ldb_module *, struct ldb_request *) +ldb_map_rename: int (struct ldb_module *, struct ldb_request *) +ldb_map_search: int (struct ldb_module *, struct ldb_request *) +ldb_match_message: int (struct ldb_context *, const struct ldb_message *, const struct ldb_parse_tree *, enum ldb_scope, bool *) +ldb_match_msg: int (struct ldb_context *, const struct ldb_message *, const struct ldb_parse_tree *, struct ldb_dn *, enum ldb_scope) +ldb_match_msg_error: int (struct ldb_context *, const struct ldb_message *, const struct ldb_parse_tree *, struct ldb_dn *, enum ldb_scope, bool *) +ldb_match_msg_objectclass: int (const struct ldb_message *, const char *) +ldb_mod_register_control: int (struct ldb_module *, const char *) +ldb_modify: int (struct ldb_context *, const struct ldb_message *) +ldb_modify_default_callback: int (struct ldb_request *, struct ldb_reply *) +ldb_module_call_chain: char *(struct ldb_request *, TALLOC_CTX *) +ldb_module_connect_backend: int (struct ldb_context *, const char *, const char **, struct ldb_module **) +ldb_module_done: int (struct ldb_request *, struct ldb_control **, struct ldb_extended *, int) +ldb_module_flags: uint32_t (struct ldb_context *) +ldb_module_get_ctx: struct ldb_context *(struct ldb_module *) +ldb_module_get_name: const char *(struct ldb_module *) +ldb_module_get_ops: const struct ldb_module_ops *(struct ldb_module *) +ldb_module_get_private: void *(struct ldb_module *) +ldb_module_init_chain: int (struct ldb_context *, struct ldb_module *) +ldb_module_load_list: int (struct ldb_context *, const char **, struct ldb_module *, struct ldb_module **) +ldb_module_new: struct ldb_module *(TALLOC_CTX *, struct ldb_context *, const char *, const struct ldb_module_ops *) +ldb_module_next: struct ldb_module *(struct ldb_module *) +ldb_module_popt_options: struct poptOption **(struct ldb_context *) +ldb_module_send_entry: int (struct ldb_request *, struct ldb_message *, struct ldb_control **) +ldb_module_send_referral: int (struct ldb_request *, char *) +ldb_module_set_next: void (struct ldb_module *, struct ldb_module *) +ldb_module_set_private: void (struct ldb_module *, void *) +ldb_modules_hook: int (struct ldb_context *, enum ldb_module_hook_type) +ldb_modules_list_from_string: const char **(struct ldb_context *, TALLOC_CTX *, const char *) +ldb_modules_load: int (const char *, const char *) +ldb_msg_add: int (struct ldb_message *, const struct ldb_message_element *, int) +ldb_msg_add_empty: int (struct ldb_message *, const char *, int, struct ldb_message_element **) +ldb_msg_add_fmt: int (struct ldb_message *, const char *, const char *, ...) +ldb_msg_add_linearized_dn: int (struct ldb_message *, const char *, struct ldb_dn *) +ldb_msg_add_steal_string: int (struct ldb_message *, const char *, char *) +ldb_msg_add_steal_value: int (struct ldb_message *, const char *, struct ldb_val *) +ldb_msg_add_string: int (struct ldb_message *, const char *, const char *) +ldb_msg_add_value: int (struct ldb_message *, const char *, const struct ldb_val *, struct ldb_message_element **) +ldb_msg_canonicalize: struct ldb_message *(struct ldb_context *, const struct ldb_message *) +ldb_msg_check_string_attribute: int (const struct ldb_message *, const char *, const char *) +ldb_msg_copy: struct ldb_message *(TALLOC_CTX *, const struct ldb_message *) +ldb_msg_copy_attr: int (struct ldb_message *, const char *, const char *) +ldb_msg_copy_shallow: struct ldb_message *(TALLOC_CTX *, const struct ldb_message *) +ldb_msg_diff: struct ldb_message *(struct ldb_context *, struct ldb_message *, struct ldb_message *) +ldb_msg_difference: int (struct ldb_context *, TALLOC_CTX *, struct ldb_message *, struct ldb_message *, struct ldb_message **) +ldb_msg_element_compare: int (struct ldb_message_element *, struct ldb_message_element *) +ldb_msg_element_compare_name: int (struct ldb_message_element *, struct ldb_message_element *) +ldb_msg_element_equal_ordered: bool (const struct ldb_message_element *, const struct ldb_message_element *) +ldb_msg_find_attr_as_bool: int (const struct ldb_message *, const char *, int) +ldb_msg_find_attr_as_dn: struct ldb_dn *(struct ldb_context *, TALLOC_CTX *, const struct ldb_message *, const char *) +ldb_msg_find_attr_as_double: double (const struct ldb_message *, const char *, double) +ldb_msg_find_attr_as_int: int (const struct ldb_message *, const char *, int) +ldb_msg_find_attr_as_int64: int64_t (const struct ldb_message *, const char *, int64_t) +ldb_msg_find_attr_as_string: const char *(const struct ldb_message *, const char *, const char *) +ldb_msg_find_attr_as_uint: unsigned int (const struct ldb_message *, const char *, unsigned int) +ldb_msg_find_attr_as_uint64: uint64_t (const struct ldb_message *, const char *, uint64_t) +ldb_msg_find_common_values: int (struct ldb_context *, TALLOC_CTX *, struct ldb_message_element *, struct ldb_message_element *, uint32_t) +ldb_msg_find_duplicate_val: int (struct ldb_context *, TALLOC_CTX *, const struct ldb_message_element *, struct ldb_val **, uint32_t) +ldb_msg_find_element: struct ldb_message_element *(const struct ldb_message *, const char *) +ldb_msg_find_ldb_val: const struct ldb_val *(const struct ldb_message *, const char *) +ldb_msg_find_val: struct ldb_val *(const struct ldb_message_element *, struct ldb_val *) +ldb_msg_new: struct ldb_message *(TALLOC_CTX *) +ldb_msg_normalize: int (struct ldb_context *, TALLOC_CTX *, const struct ldb_message *, struct ldb_message **) +ldb_msg_remove_attr: void (struct ldb_message *, const char *) +ldb_msg_remove_element: void (struct ldb_message *, struct ldb_message_element *) +ldb_msg_rename_attr: int (struct ldb_message *, const char *, const char *) +ldb_msg_sanity_check: int (struct ldb_context *, const struct ldb_message *) +ldb_msg_sort_elements: void (struct ldb_message *) +ldb_next_del_trans: int (struct ldb_module *) +ldb_next_end_trans: int (struct ldb_module *) +ldb_next_init: int (struct ldb_module *) +ldb_next_prepare_commit: int (struct ldb_module *) +ldb_next_read_lock: int (struct ldb_module *) +ldb_next_read_unlock: int (struct ldb_module *) +ldb_next_remote_request: int (struct ldb_module *, struct ldb_request *) +ldb_next_request: int (struct ldb_module *, struct ldb_request *) +ldb_next_start_trans: int (struct ldb_module *) +ldb_op_default_callback: int (struct ldb_request *, struct ldb_reply *) +ldb_options_copy: const char **(TALLOC_CTX *, const char **) +ldb_options_find: const char *(struct ldb_context *, const char **, const char *) +ldb_options_get: const char **(struct ldb_context *) +ldb_pack_data: int (struct ldb_context *, const struct ldb_message *, struct ldb_val *, uint32_t) +ldb_parse_control_from_string: struct ldb_control *(struct ldb_context *, TALLOC_CTX *, const char *) +ldb_parse_control_strings: struct ldb_control **(struct ldb_context *, TALLOC_CTX *, const char **) +ldb_parse_tree: struct ldb_parse_tree *(TALLOC_CTX *, const char *) +ldb_parse_tree_attr_replace: void (struct ldb_parse_tree *, const char *, const char *) +ldb_parse_tree_copy_shallow: struct ldb_parse_tree *(TALLOC_CTX *, const struct ldb_parse_tree *) +ldb_parse_tree_walk: int (struct ldb_parse_tree *, int (*)(struct ldb_parse_tree *, void *), void *) +ldb_qsort: void (void * const, size_t, size_t, void *, ldb_qsort_cmp_fn_t) +ldb_register_backend: int (const char *, ldb_connect_fn, bool) +ldb_register_extended_match_rule: int (struct ldb_context *, const struct ldb_extended_match_rule *) +ldb_register_hook: int (ldb_hook_fn) +ldb_register_module: int (const struct ldb_module_ops *) +ldb_rename: int (struct ldb_context *, struct ldb_dn *, struct ldb_dn *) +ldb_reply_add_control: int (struct ldb_reply *, const char *, bool, void *) +ldb_reply_get_control: struct ldb_control *(struct ldb_reply *, const char *) +ldb_req_get_custom_flags: uint32_t (struct ldb_request *) +ldb_req_is_untrusted: bool (struct ldb_request *) +ldb_req_location: const char *(struct ldb_request *) +ldb_req_mark_trusted: void (struct ldb_request *) +ldb_req_mark_untrusted: void (struct ldb_request *) +ldb_req_set_custom_flags: void (struct ldb_request *, uint32_t) +ldb_req_set_location: void (struct ldb_request *, const char *) +ldb_request: int (struct ldb_context *, struct ldb_request *) +ldb_request_add_control: int (struct ldb_request *, const char *, bool, void *) +ldb_request_done: int (struct ldb_request *, int) +ldb_request_get_control: struct ldb_control *(struct ldb_request *, const char *) +ldb_request_get_status: int (struct ldb_request *) +ldb_request_replace_control: int (struct ldb_request *, const char *, bool, void *) +ldb_request_set_state: void (struct ldb_request *, int) +ldb_reset_err_string: void (struct ldb_context *) +ldb_save_controls: int (struct ldb_control *, struct ldb_request *, struct ldb_control ***) +ldb_schema_attribute_add: int (struct ldb_context *, const char *, unsigned int, const char *) +ldb_schema_attribute_add_with_syntax: int (struct ldb_context *, const char *, unsigned int, const struct ldb_schema_syntax *) +ldb_schema_attribute_by_name: const struct ldb_schema_attribute *(struct ldb_context *, const char *) +ldb_schema_attribute_fill_with_syntax: int (struct ldb_context *, TALLOC_CTX *, const char *, unsigned int, const struct ldb_schema_syntax *, struct ldb_schema_attribute *) +ldb_schema_attribute_remove: void (struct ldb_context *, const char *) +ldb_schema_attribute_remove_flagged: void (struct ldb_context *, unsigned int) +ldb_schema_attribute_set_override_handler: void (struct ldb_context *, ldb_attribute_handler_override_fn_t, void *) +ldb_schema_set_override_GUID_index: void (struct ldb_context *, const char *, const char *) +ldb_schema_set_override_indexlist: void (struct ldb_context *, bool) +ldb_search: int (struct ldb_context *, TALLOC_CTX *, struct ldb_result **, struct ldb_dn *, enum ldb_scope, const char * const *, const char *, ...) +ldb_search_default_callback: int (struct ldb_request *, struct ldb_reply *) +ldb_sequence_number: int (struct ldb_context *, enum ldb_sequence_type, uint64_t *) +ldb_set_create_perms: void (struct ldb_context *, unsigned int) +ldb_set_debug: int (struct ldb_context *, void (*)(void *, enum ldb_debug_level, const char *, va_list), void *) +ldb_set_debug_stderr: int (struct ldb_context *) +ldb_set_default_dns: void (struct ldb_context *) +ldb_set_errstring: void (struct ldb_context *, const char *) +ldb_set_event_context: void (struct ldb_context *, struct tevent_context *) +ldb_set_flags: void (struct ldb_context *, unsigned int) +ldb_set_modules_dir: void (struct ldb_context *, const char *) +ldb_set_opaque: int (struct ldb_context *, const char *, void *) +ldb_set_require_private_event_context: void (struct ldb_context *) +ldb_set_timeout: int (struct ldb_context *, struct ldb_request *, int) +ldb_set_timeout_from_prev_req: int (struct ldb_context *, struct ldb_request *, struct ldb_request *) +ldb_set_utf8_default: void (struct ldb_context *) +ldb_set_utf8_fns: void (struct ldb_context *, void *, char *(*)(void *, void *, const char *, size_t)) +ldb_setup_wellknown_attributes: int (struct ldb_context *) +ldb_should_b64_encode: int (struct ldb_context *, const struct ldb_val *) +ldb_standard_syntax_by_name: const struct ldb_schema_syntax *(struct ldb_context *, const char *) +ldb_strerror: const char *(int) +ldb_string_to_time: time_t (const char *) +ldb_string_utc_to_time: time_t (const char *) +ldb_timestring: char *(TALLOC_CTX *, time_t) +ldb_timestring_utc: char *(TALLOC_CTX *, time_t) +ldb_transaction_cancel: int (struct ldb_context *) +ldb_transaction_cancel_noerr: int (struct ldb_context *) +ldb_transaction_commit: int (struct ldb_context *) +ldb_transaction_prepare_commit: int (struct ldb_context *) +ldb_transaction_start: int (struct ldb_context *) +ldb_unpack_data: int (struct ldb_context *, const struct ldb_val *, struct ldb_message *) +ldb_unpack_data_flags: int (struct ldb_context *, const struct ldb_val *, struct ldb_message *, unsigned int) +ldb_unpack_get_format: int (const struct ldb_val *, uint32_t *) +ldb_val_dup: struct ldb_val (TALLOC_CTX *, const struct ldb_val *) +ldb_val_equal_exact: int (const struct ldb_val *, const struct ldb_val *) +ldb_val_map_local: struct ldb_val (struct ldb_module *, void *, const struct ldb_map_attribute *, const struct ldb_val *) +ldb_val_map_remote: struct ldb_val (struct ldb_module *, void *, const struct ldb_map_attribute *, const struct ldb_val *) +ldb_val_string_cmp: int (const struct ldb_val *, const char *) +ldb_val_to_time: int (const struct ldb_val *, time_t *) +ldb_valid_attr_name: int (const char *) +ldb_vdebug: void (struct ldb_context *, enum ldb_debug_level, const char *, va_list) +ldb_wait: int (struct ldb_handle *, enum ldb_wait_type) diff --git a/lib/ldb/ABI/pyldb-util-2.1.4.sigs b/lib/ldb/ABI/pyldb-util-2.1.4.sigs new file mode 100644 index 00000000000..164a806b2ff --- /dev/null +++ b/lib/ldb/ABI/pyldb-util-2.1.4.sigs @@ -0,0 +1,3 @@ +pyldb_Dn_FromDn: PyObject *(struct ldb_dn *) +pyldb_Object_AsDn: bool (TALLOC_CTX *, PyObject *, struct ldb_context *, struct ldb_dn **) +pyldb_check_type: bool (PyObject *, const char *) diff --git a/lib/ldb/wscript b/lib/ldb/wscript index 86b83c1b5cc..9eb3ddad0c0 100644 --- a/lib/ldb/wscript +++ b/lib/ldb/wscript @@ -1,7 +1,7 @@ #!/usr/bin/env python APPNAME = 'ldb' -VERSION = '2.1.3' +VERSION = '2.1.4' import sys, os -- 2.17.1 From 6e04e0b4944b394cd1ba1879367af2b6e53b9882 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Thu, 11 Jun 2020 17:38:51 +1200 Subject: [PATCH 12/22] CVE-2020-10745: pytests: hand-rolled invalid dns/nbt packet tests The client libraries don't allow us to make packets that are broken in certain ways, so we need to construct them as byte strings. These tests all fail at present, proving the server is rendered unresponsive, which is the crux of CVE-2020-10745. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 Signed-off-by: Douglas Bagnall --- python/samba/tests/dns_packet.py | 211 +++++++++++++++++++++++++++++++ selftest/knownfail.d/dns_packet | 2 + source4/selftest/tests.py | 10 ++ 3 files changed, 223 insertions(+) create mode 100644 python/samba/tests/dns_packet.py create mode 100644 selftest/knownfail.d/dns_packet diff --git a/python/samba/tests/dns_packet.py b/python/samba/tests/dns_packet.py new file mode 100644 index 00000000000..c4f843eb613 --- /dev/null +++ b/python/samba/tests/dns_packet.py @@ -0,0 +1,211 @@ +# Tests of malformed DNS packets +# Copyright (C) Catalyst.NET ltd +# +# written by Douglas Bagnall +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . + +"""Sanity tests for DNS and NBT server parsing. + +We don't use a proper client library so we can make improper packets. +""" + +import os +import struct +import socket +import select +from samba.dcerpc import dns, nbt + +from samba.tests import TestCase + + +def _msg_id(): + while True: + for i in range(1, 0xffff): + yield i + + +SERVER = os.environ['SERVER_IP'] +SERVER_NAME = f"{os.environ['SERVER']}.{os.environ['REALM']}" +TIMEOUT = 0.5 + + +def encode_netbios_bytes(chars): + """Even RFC 1002 uses distancing quotes when calling this "compression".""" + out = [] + chars = (chars + b' ')[:16] + for c in chars: + out.append((c >> 4) + 65) + out.append((c & 15) + 65) + return bytes(out) + + +class TestDnsPacketBase(TestCase): + msg_id = _msg_id() + + def tearDown(self): + # we need to ensure the DNS server is responsive before + # continuing. + for i in range(40): + ok = self._known_good_query() + if ok: + return + print(f"the server is STILL unresponsive after {40 * TIMEOUT} seconds") + + def decode_reply(self, data): + header = data[:12] + id, flags, n_q, n_a, n_rec, n_exta = struct.unpack('!6H', + header) + return { + 'rcode': flags & 0xf + } + + def construct_query(self, names): + """Create a query packet containing one query record. + + *names* is either a single string name in the usual dotted + form, or a list of names. In the latter case, each name can + be a dotted string or a list of byte components, which allows + dots in components. Where I say list, I mean non-string + iterable. + + Examples: + + # these 3 are all the same + "example.com" + ["example.com"] + [[b"example", b"com"]] + + # this is three names in the same request + ["example.com", + [b"example", b"com", b"..!"], + (b"first component", b" 2nd component")] + """ + header = struct.pack('!6H', + next(self.msg_id), + 0x0100, # query, with recursion + len(names), # number of queries + 0x0000, # no answers + 0x0000, # no records + 0x0000, # no extra records + ) + tail = struct.pack('!BHH', + 0x00, # root node + self.qtype, + 0x0001, # class IN-ternet + ) + encoded_bits = [] + for name in names: + if isinstance(name, str): + bits = name.encode('utf8').split(b'.') + else: + bits = name + + for b in bits: + encoded_bits.append(b'%c%s' % (len(b), b)) + encoded_bits.append(tail) + + return header + b''.join(encoded_bits) + + def _test_query(self, names=(), expected_rcode=None): + + if isinstance(names, str): + names = [names] + + packet = self.construct_query(names) + s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) + s.sendto(packet, self.server) + r, _, _ = select.select([s], [], [], TIMEOUT) + s.close() + # It is reasonable to not reply to these packets (Windows + # doesn't), but it is not reasonable to render the server + # unresponsive. + if r != [s]: + ok = self._known_good_query() + self.assertTrue(ok, f"the server is unresponsive") + + def _known_good_query(self): + if self.server[1] == 53: + name = SERVER_NAME + expected_rcode = dns.DNS_RCODE_OK + else: + name = [encode_netbios_bytes(b'nxdomain'), b'nxdomain'] + expected_rcode = nbt.NBT_RCODE_NAM + + packet = self.construct_query([name]) + s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) + s.sendto(packet, self.server) + r, _, _ = select.select([s], [], [], TIMEOUT) + if r != [s]: + s.close() + return False + + data, addr = s.recvfrom(4096) + s.close() + rcode = self.decode_reply(data)['rcode'] + return expected_rcode == rcode + + +class TestDnsPackets(TestDnsPacketBase): + server = (SERVER, 53) + qtype = 1 # dns type A + + def _test_many_repeated_components(self, label, n, expected_rcode=None): + name = [label] * n + self._test_query([name], + expected_rcode=expected_rcode) + + def test_127_very_dotty_components(self): + label = b'.' * 63 + self._test_many_repeated_components(label, 127) + + def test_127_half_dotty_components(self): + label = b'x.' * 31 + b'x' + self._test_many_repeated_components(label, 127) + + +class TestNbtPackets(TestDnsPacketBase): + server = (SERVER, 137) + qtype = 0x20 # NBT_QTYPE_NETBIOS + + def _test_nbt_encode_query(self, names, *args, **kwargs): + if isinstance(names, str): + names = [names] + + nbt_names = [] + for name in names: + if isinstance(name, str): + bits = name.encode('utf8').split(b'.') + else: + bits = name + + encoded = [encode_netbios_bytes(bits[0])] + encoded.extend(bits[1:]) + nbt_names.append(encoded) + + self._test_query(nbt_names, *args, **kwargs) + + def _test_many_repeated_components(self, label, n, expected_rcode=None): + name = [label] * n + name[0] = encode_netbios_bytes(label) + self._test_query([name], + expected_rcode=expected_rcode) + + def test_127_very_dotty_components(self): + label = b'.' * 63 + self._test_many_repeated_components(label, 127) + + def test_127_half_dotty_components(self): + label = b'x.' * 31 + b'x' + self._test_many_repeated_components(label, 127) diff --git a/selftest/knownfail.d/dns_packet b/selftest/knownfail.d/dns_packet new file mode 100644 index 00000000000..6e2e5a699de --- /dev/null +++ b/selftest/knownfail.d/dns_packet @@ -0,0 +1,2 @@ +samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_127_very_dotty_components +samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_127_very_dotty_components diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py index cbb38f0ee62..36a2f126b33 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -431,6 +431,16 @@ plantestsuite_loadlist("samba.tests.dns_wildcard", "ad_dc", [python, os.path.joi plantestsuite_loadlist("samba.tests.dns_invalid", "ad_dc", [python, os.path.join(srcdir(), "python/samba/tests/dns_invalid.py"), '$SERVER_IP', '--machine-pass', '-U"$USERNAME%$PASSWORD"', '--workgroup=$DOMAIN', '$LOADLIST', '$LISTOPT']) +plantestsuite_loadlist("samba.tests.dns_packet", + "ad_dc", + [python, + '-msamba.subunit.run', + '$LOADLIST', + "$LISTOPT" + "samba.tests.dns_packet" + ]) + + for t in smbtorture4_testsuites("dns_internal."): plansmbtorture4testsuite(t, "ad_dc_default:local", '//$SERVER/whavever') -- 2.17.1 From ec640f93bc99e5b24409a2ea2b5fb70e7f991c53 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 12 Jun 2020 14:26:38 +1200 Subject: [PATCH 13/22] CVE-2020-10745: librpc/tests: cmocka tests of dns and ndr strings These time the push and pull function in isolation. Timing should be under 0.0001 seconds on even quite old hardware; we assert it must be under 0.2 seconds. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 (backported from master commit) [abartlet@samba.org: backported due to differences in pre-existing tests - eg test_ndr - mentioned in wscript_build and tests.py] Signed-off-by: Douglas Bagnall --- librpc/tests/test_ndr_dns_nbt.c | 236 +++++++++++++++++++++++++++++++ librpc/wscript_build | 10 ++ selftest/knownfail.d/ndr_dns_nbt | 4 + source4/selftest/tests.py | 2 + 4 files changed, 252 insertions(+) create mode 100644 librpc/tests/test_ndr_dns_nbt.c create mode 100644 selftest/knownfail.d/ndr_dns_nbt diff --git a/librpc/tests/test_ndr_dns_nbt.c b/librpc/tests/test_ndr_dns_nbt.c new file mode 100644 index 00000000000..1e2ef45c10d --- /dev/null +++ b/librpc/tests/test_ndr_dns_nbt.c @@ -0,0 +1,236 @@ +/* + * Tests for librpc ndr functions + * + * Copyright (C) Catalyst.NET Ltd 2020 + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + */ + +#include "replace.h" +#include +#include + +#include "includes.h" +#include "librpc/ndr/libndr.h" +#include "librpc/gen_ndr/ndr_dns.h" +#include "librpc/gen_ndr/ndr_nbt.h" +#include "lib/util/time.h" + +#define NBT_NAME "EOGFGLGPCACACACACACACACACACACACA" /* "neko" */ + + +static DATA_BLOB generate_obnoxious_dns_name(TALLOC_CTX *mem_ctx, + size_t n_labels, + size_t dot_every, + bool is_nbt) +{ + size_t i, j; + char *s; + DATA_BLOB name = data_blob_talloc(mem_ctx, NULL, 64 * n_labels + 1); + assert_non_null(name.data); + + s = (char*)name.data; + if (is_nbt) { + size_t len = strlen(NBT_NAME); + *s = len; + s++; + memcpy(s, NBT_NAME, len); + s += len; + n_labels--; + } + + for (i = 0; i < n_labels; i++) { + *s = 63; + s++; + for (j = 0; j < 63; j++) { + if (j % dot_every == (dot_every - 1)) { + *s = '.'; + } else { + *s = 'x'; + } + s++; + } + } + *s = 0; + s++; + name.length = s - (char*)name.data; + return name; +} + + +static char *_test_ndr_pull_dns_string_list(TALLOC_CTX *mem_ctx, + size_t n_labels, + size_t dot_every, + bool is_nbt) +{ + enum ndr_err_code ndr_err; + DATA_BLOB blob = generate_obnoxious_dns_name(mem_ctx, + n_labels, + dot_every, + is_nbt); + + char *name; + ndr_pull_flags_fn_t fn; + + if (is_nbt) { + fn = (ndr_pull_flags_fn_t)ndr_pull_nbt_string; + } else { + fn = (ndr_pull_flags_fn_t)ndr_pull_dns_string; + } + + ndr_err = ndr_pull_struct_blob(&blob, + mem_ctx, + &name, + fn); + /* Success here is not expected, but we let it go to measure timing. */ + if (ndr_err == NDR_ERR_SUCCESS) { + printf("pull succeed\n"); + } else { + assert_int_equal(ndr_err, NDR_ERR_STRING); + } + + TALLOC_FREE(blob.data); + return name; +} + + +static void _test_ndr_push_dns_string_list(TALLOC_CTX *mem_ctx, + char *name, + bool is_nbt) +{ + DATA_BLOB blob; + enum ndr_err_code ndr_err; + ndr_push_flags_fn_t fn; + + if (is_nbt) { + fn = (ndr_push_flags_fn_t)ndr_push_nbt_string; + } else { + fn = (ndr_push_flags_fn_t)ndr_push_dns_string; + } + + ndr_err = ndr_push_struct_blob(&blob, + mem_ctx, + name, + fn); + + /* Success here is not expected, but we let it go to measure timing. */ + if (ndr_err == NDR_ERR_SUCCESS) { + printf("push succeed\n"); + } else { + assert_int_equal(ndr_err, NDR_ERR_STRING); + } +} + + +static uint64_t elapsed_time(struct timespec start, const char *print) +{ + struct timespec end; + unsigned long long microsecs; + clock_gettime_mono(&end); + end.tv_sec -= start.tv_sec; + if (end.tv_nsec < start.tv_nsec) { + /* we need to borrow */ + end.tv_nsec += 1000 * 1000 * 1000; + end.tv_sec -= 1; + } + end.tv_nsec -= start.tv_nsec; + microsecs = end.tv_sec * 1000000; + microsecs += end.tv_nsec / 1000; + + if (print != NULL) { + printf(" %s: %llu microseconds\n", print, microsecs); + } + return microsecs; +} + + +static void test_ndr_dns_string_half_dots(void **state) +{ + TALLOC_CTX *mem_ctx = talloc_new(NULL); + char *name; + struct timespec start; + uint64_t elapsed; + + clock_gettime_mono(&start); + name =_test_ndr_pull_dns_string_list(mem_ctx, 127, 2, false); + elapsed_time(start, "pull"); + _test_ndr_push_dns_string_list(mem_ctx, name, false); + elapsed = elapsed_time(start, "total"); + assert_in_range(elapsed, 0, 200000); + talloc_free(mem_ctx); +} + +static void test_ndr_nbt_string_half_dots(void **state) +{ + TALLOC_CTX *mem_ctx = talloc_new(NULL); + char *name; + struct timespec start; + uint64_t elapsed; + + clock_gettime_mono(&start); + name =_test_ndr_pull_dns_string_list(mem_ctx, 127, 2, true); + elapsed_time(start, "pull"); + _test_ndr_push_dns_string_list(mem_ctx, name, true); + elapsed = elapsed_time(start, "total"); + assert_in_range(elapsed, 0, 200000); + talloc_free(mem_ctx); +} + +static void test_ndr_dns_string_all_dots(void **state) +{ + TALLOC_CTX *mem_ctx = talloc_new(NULL); + char *name; + struct timespec start; + uint64_t elapsed; + + clock_gettime_mono(&start); + name =_test_ndr_pull_dns_string_list(mem_ctx, 127, 1, false); + elapsed_time(start, "pull"); + _test_ndr_push_dns_string_list(mem_ctx, name, false); + elapsed = elapsed_time(start, "total"); + assert_in_range(elapsed, 0, 200000); + talloc_free(mem_ctx); +} + +static void test_ndr_nbt_string_all_dots(void **state) +{ + TALLOC_CTX *mem_ctx = talloc_new(NULL); + char *name; + struct timespec start; + uint64_t elapsed; + + clock_gettime_mono(&start); + name =_test_ndr_pull_dns_string_list(mem_ctx, 127, 1, true); + elapsed_time(start, "pull"); + _test_ndr_push_dns_string_list(mem_ctx, name, true); + elapsed = elapsed_time(start, "total"); + assert_in_range(elapsed, 0, 200000); + talloc_free(mem_ctx); +} + + + +int main(int argc, const char **argv) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test(test_ndr_nbt_string_half_dots), + cmocka_unit_test(test_ndr_dns_string_half_dots), + cmocka_unit_test(test_ndr_nbt_string_all_dots), + cmocka_unit_test(test_ndr_dns_string_all_dots), + }; + + cmocka_set_message_output(CM_OUTPUT_SUBUNIT); + return cmocka_run_group_tests(tests, NULL, NULL); +} diff --git a/librpc/wscript_build b/librpc/wscript_build index 5eb78e6010a..b56ccf10acd 100644 --- a/librpc/wscript_build +++ b/librpc/wscript_build @@ -698,3 +698,13 @@ bld.SAMBA_BINARY('test_ndr_string', ndr ''', for_selftest=True) + +bld.SAMBA_BINARY('test_ndr_dns_nbt', + source='tests/test_ndr_dns_nbt.c', + deps=''' + cmocka + ndr + ndr_nbt + NDR_DNS + ''', + for_selftest=True) diff --git a/selftest/knownfail.d/ndr_dns_nbt b/selftest/knownfail.d/ndr_dns_nbt new file mode 100644 index 00000000000..f30217c4033 --- /dev/null +++ b/selftest/knownfail.d/ndr_dns_nbt @@ -0,0 +1,4 @@ +librpc.ndr.ndr_dns_nbt.test_ndr_dns_string_all_dots +librpc.ndr.ndr_dns_nbt.test_ndr_dns_string_half_dots +librpc.ndr.ndr_dns_nbt.test_ndr_nbt_string_all_dots +librpc.ndr.ndr_dns_nbt.test_ndr_nbt_string_half_dots diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py index 36a2f126b33..726f2c01f7e 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -1350,6 +1350,8 @@ plantestsuite("libcli.drsuapi.repl_decrypt", "none", [os.path.join(bindir(), "test_repl_decrypt")]) plantestsuite("librpc.ndr.ndr_string", "none", [os.path.join(bindir(), "test_ndr_string")]) +plantestsuite("librpc.ndr.ndr_dns_nbt", "none", + [os.path.join(bindir(), "test_ndr_dns_nbt")]) plantestsuite("libcli.ldap.ldap_message", "none", [os.path.join(bindir(), "test_ldap_message")]) -- 2.17.1 From a80dd6d12718a69aa18d0c429f4c6b2c18dc6662 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sat, 25 Apr 2020 11:02:08 +1200 Subject: [PATCH 14/22] CVE-2020-10745: ndr_dns: move ndr_push_dns_string core into sharable function This is because ndr_nbt.c does almost exactly the same thing with almost exactly the same code, and they both do it wrong. Soon they will both be using the better version that this will become. Though in this patch we just move the code, not fix it. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 Signed-off-by: Douglas Bagnall --- librpc/ndr/ndr_dns.c | 79 +++------------------------------- librpc/ndr/ndr_dns_utils.c | 88 ++++++++++++++++++++++++++++++++++++++ librpc/ndr/ndr_dns_utils.h | 5 +++ librpc/wscript_build | 2 +- 4 files changed, 99 insertions(+), 75 deletions(-) create mode 100644 librpc/ndr/ndr_dns_utils.c create mode 100644 librpc/ndr/ndr_dns_utils.h diff --git a/librpc/ndr/ndr_dns.c b/librpc/ndr/ndr_dns.c index d37c8cc2ece..68a3c9de782 100644 --- a/librpc/ndr/ndr_dns.c +++ b/librpc/ndr/ndr_dns.c @@ -33,6 +33,7 @@ #include "librpc/gen_ndr/ndr_dnsp.h" #include "system/locale.h" #include "lib/util/util_net.h" +#include "ndr_dns_utils.h" /* don't allow an unlimited number of name components */ #define MAX_COMPONENTS 128 @@ -159,80 +160,10 @@ _PUBLIC_ enum ndr_err_code ndr_push_dns_string(struct ndr_push *ndr, int ndr_flags, const char *s) { - if (!(ndr_flags & NDR_SCALARS)) { - return NDR_ERR_SUCCESS; - } - - while (s && *s) { - enum ndr_err_code ndr_err; - char *compname; - size_t complen; - uint32_t offset; - - if (!(ndr->flags & LIBNDR_FLAG_NO_COMPRESSION)) { - /* see if we have pushed the remaining string already, - * if so we use a label pointer to this string - */ - ndr_err = ndr_token_retrieve_cmp_fn(&ndr->dns_string_list, s, - &offset, - (comparison_fn_t)strcmp, - false); - if (NDR_ERR_CODE_IS_SUCCESS(ndr_err)) { - uint8_t b[2]; - - if (offset > 0x3FFF) { - return ndr_push_error(ndr, NDR_ERR_STRING, - "offset for dns string " \ - "label pointer " \ - "%u[%08X] > 0x00003FFF", - offset, offset); - } - - b[0] = 0xC0 | (offset>>8); - b[1] = (offset & 0xFF); - - return ndr_push_bytes(ndr, b, 2); - } - } - - complen = strcspn(s, "."); - - /* we need to make sure the length fits into 6 bytes */ - if (complen > 0x3F) { - return ndr_push_error(ndr, NDR_ERR_STRING, - "component length %u[%08X] > " \ - "0x0000003F", - (unsigned)complen, - (unsigned)complen); - } - - compname = talloc_asprintf(ndr, "%c%*.*s", - (unsigned char)complen, - (unsigned char)complen, - (unsigned char)complen, s); - NDR_ERR_HAVE_NO_MEMORY(compname); - - /* remember the current component + the rest of the string - * so it can be reused later - */ - if (!(ndr->flags & LIBNDR_FLAG_NO_COMPRESSION)) { - NDR_CHECK(ndr_token_store(ndr, &ndr->dns_string_list, s, - ndr->offset)); - } - - /* push just this component into the blob */ - NDR_CHECK(ndr_push_bytes(ndr, (const uint8_t *)compname, - complen+1)); - talloc_free(compname); - - s += complen; - if (*s == '.') s++; - } - - /* if we reach the end of the string and have pushed the last component - * without using a label pointer, we need to terminate the string - */ - return ndr_push_bytes(ndr, (const uint8_t *)"", 1); + return ndr_push_dns_string_list(ndr, + &ndr->dns_string_list, + ndr_flags, + s); } _PUBLIC_ enum ndr_err_code ndr_pull_dns_txt_record(struct ndr_pull *ndr, int ndr_flags, struct dns_txt_record *r) diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c new file mode 100644 index 00000000000..2d9b5f1bc1e --- /dev/null +++ b/librpc/ndr/ndr_dns_utils.c @@ -0,0 +1,88 @@ +#include "includes.h" +#include "../librpc/ndr/libndr.h" +#include "ndr_dns_utils.h" + + +/** + push a dns/nbt string list to the wire +*/ +enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, + struct ndr_token_list *string_list, + int ndr_flags, + const char *s) +{ + if (!(ndr_flags & NDR_SCALARS)) { + return NDR_ERR_SUCCESS; + } + + while (s && *s) { + enum ndr_err_code ndr_err; + char *compname; + size_t complen; + uint32_t offset; + + if (!(ndr->flags & LIBNDR_FLAG_NO_COMPRESSION)) { + /* see if we have pushed the remaining string already, + * if so we use a label pointer to this string + */ + ndr_err = ndr_token_retrieve_cmp_fn(string_list, s, + &offset, + (comparison_fn_t)strcmp, + false); + if (NDR_ERR_CODE_IS_SUCCESS(ndr_err)) { + uint8_t b[2]; + + if (offset > 0x3FFF) { + return ndr_push_error(ndr, NDR_ERR_STRING, + "offset for dns string " \ + "label pointer " \ + "%u[%08X] > 0x00003FFF", + offset, offset); + } + + b[0] = 0xC0 | (offset>>8); + b[1] = (offset & 0xFF); + + return ndr_push_bytes(ndr, b, 2); + } + } + + complen = strcspn(s, "."); + + /* we need to make sure the length fits into 6 bytes */ + if (complen > 0x3F) { + return ndr_push_error(ndr, NDR_ERR_STRING, + "component length %u[%08X] > " \ + "0x0000003F", + (unsigned)complen, + (unsigned)complen); + } + + compname = talloc_asprintf(ndr, "%c%*.*s", + (unsigned char)complen, + (unsigned char)complen, + (unsigned char)complen, s); + NDR_ERR_HAVE_NO_MEMORY(compname); + + /* remember the current component + the rest of the string + * so it can be reused later + */ + if (!(ndr->flags & LIBNDR_FLAG_NO_COMPRESSION)) { + NDR_CHECK(ndr_token_store(ndr, string_list, s, + ndr->offset)); + } + + /* push just this component into the blob */ + NDR_CHECK(ndr_push_bytes(ndr, (const uint8_t *)compname, + complen+1)); + talloc_free(compname); + + s += complen; + if (*s == '.') s++; + } + + /* if we reach the end of the string and have pushed the last component + * without using a label pointer, we need to terminate the string + */ + return ndr_push_bytes(ndr, (const uint8_t *)"", 1); +} diff --git a/librpc/ndr/ndr_dns_utils.h b/librpc/ndr/ndr_dns_utils.h new file mode 100644 index 00000000000..823e3201112 --- /dev/null +++ b/librpc/ndr/ndr_dns_utils.h @@ -0,0 +1,5 @@ + +enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, + struct ndr_token_list *string_list, + int ndr_flags, + const char *s); diff --git a/librpc/wscript_build b/librpc/wscript_build index b56ccf10acd..3b4599f3211 100644 --- a/librpc/wscript_build +++ b/librpc/wscript_build @@ -31,7 +31,7 @@ bld.SAMBA_SUBSYSTEM('NDR_DNSSERVER', ) bld.SAMBA_SUBSYSTEM('NDR_DNS', - source='gen_ndr/ndr_dns.c ndr/ndr_dns.c', + source='gen_ndr/ndr_dns.c ndr/ndr_dns.c ndr/ndr_dns_utils.c', public_deps='ndr NDR_DNSP' ) -- 2.17.1 From be20d15af1349217ada57b6651ba063218301aa5 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sat, 25 Apr 2020 11:03:30 +1200 Subject: [PATCH 15/22] CVE-2020-10745: ndr/dns_utils: correct a comment BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 Signed-off-by: Douglas Bagnall --- librpc/ndr/ndr_dns_utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c index 2d9b5f1bc1e..2ce300863bc 100644 --- a/librpc/ndr/ndr_dns_utils.c +++ b/librpc/ndr/ndr_dns_utils.c @@ -49,7 +49,7 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, complen = strcspn(s, "."); - /* we need to make sure the length fits into 6 bytes */ + /* the length must fit into 6 bits (i.e. <= 63) */ if (complen > 0x3F) { return ndr_push_error(ndr, NDR_ERR_STRING, "component length %u[%08X] > " \ -- 2.17.1 From a19660d1fea66937a29dde85af41d911822aa2f2 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Sat, 25 Apr 2020 11:10:18 +1200 Subject: [PATCH 16/22] CVE-2020-10745: ndr_dns: do not allow consecutive dots The empty subdomain component is reserved for the root domain, which we should only (and always) see at the end of the list. That is, we expect "example.com.", but never "example..com". BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 Signed-off-by: Douglas Bagnall --- librpc/ndr/ndr_dns_utils.c | 6 ++++++ selftest/knownfail.d/dns_packet | 1 - selftest/knownfail.d/ndr_dns_nbt | 1 - 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c index 2ce300863bc..6931dac422d 100644 --- a/librpc/ndr/ndr_dns_utils.c +++ b/librpc/ndr/ndr_dns_utils.c @@ -58,6 +58,12 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, (unsigned)complen); } + if (complen == 0 && s[complen] == '.') { + return ndr_push_error(ndr, NDR_ERR_STRING, + "component length is 0 " + "(consecutive dots)"); + } + compname = talloc_asprintf(ndr, "%c%*.*s", (unsigned char)complen, (unsigned char)complen, diff --git a/selftest/knownfail.d/dns_packet b/selftest/knownfail.d/dns_packet index 6e2e5a699de..0662266f689 100644 --- a/selftest/knownfail.d/dns_packet +++ b/selftest/knownfail.d/dns_packet @@ -1,2 +1 @@ -samba.tests.dns_packet.samba.tests.dns_packet.TestDnsPackets.test_127_very_dotty_components samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_127_very_dotty_components diff --git a/selftest/knownfail.d/ndr_dns_nbt b/selftest/knownfail.d/ndr_dns_nbt index f30217c4033..e11c121b7a7 100644 --- a/selftest/knownfail.d/ndr_dns_nbt +++ b/selftest/knownfail.d/ndr_dns_nbt @@ -1,4 +1,3 @@ -librpc.ndr.ndr_dns_nbt.test_ndr_dns_string_all_dots librpc.ndr.ndr_dns_nbt.test_ndr_dns_string_half_dots librpc.ndr.ndr_dns_nbt.test_ndr_nbt_string_all_dots librpc.ndr.ndr_dns_nbt.test_ndr_nbt_string_half_dots -- 2.17.1 From 0afc9f777a609d0677bc5de83b35fb9313ea8466 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 15 May 2020 00:06:08 +1200 Subject: [PATCH 17/22] CVE-2020-10745: dns_util/push: forbid names longer than 255 bytes As per RFC 1035. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 Signed-off-by: Douglas Bagnall --- librpc/ndr/ndr_dns_utils.c | 10 +++++++++- selftest/knownfail.d/ndr_dns_nbt | 1 - 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c index 6931dac422d..b7f11dbab4e 100644 --- a/librpc/ndr/ndr_dns_utils.c +++ b/librpc/ndr/ndr_dns_utils.c @@ -11,6 +11,8 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, int ndr_flags, const char *s) { + const char *start = s; + if (!(ndr_flags & NDR_SCALARS)) { return NDR_ERR_SUCCESS; } @@ -84,7 +86,13 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, talloc_free(compname); s += complen; - if (*s == '.') s++; + if (*s == '.') { + s++; + } + if (s - start > 255) { + return ndr_push_error(ndr, NDR_ERR_STRING, + "name > 255 character long"); + } } /* if we reach the end of the string and have pushed the last component diff --git a/selftest/knownfail.d/ndr_dns_nbt b/selftest/knownfail.d/ndr_dns_nbt index e11c121b7a7..603395c8c50 100644 --- a/selftest/knownfail.d/ndr_dns_nbt +++ b/selftest/knownfail.d/ndr_dns_nbt @@ -1,3 +1,2 @@ -librpc.ndr.ndr_dns_nbt.test_ndr_dns_string_half_dots librpc.ndr.ndr_dns_nbt.test_ndr_nbt_string_all_dots librpc.ndr.ndr_dns_nbt.test_ndr_nbt_string_half_dots -- 2.17.1 From 3b98e807eb9879e89fffd1bbf6c5a0feaa99d121 Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Fri, 15 May 2020 10:52:45 +1200 Subject: [PATCH 18/22] CVE-2020-10745: ndr/dns-utils: prepare for NBT compatibility NBT has a funny thing where it sometimes needs to send a trailing dot as part of the last component, because the string representation is a user name. In DNS, "example.com", and "example.com." are the same, both having three components ("example", "com", ""); in NBT, we want to treat them differently, with the second form having the three components ("example", "com.", ""). This retains the logic of e6e2ec0001fe3c010445e26cc0efddbc1f73416b. Also DNS compression cannot be turned off for NBT. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14378 Signed-off-by: Douglas Bagnall --- librpc/ndr/ndr_dns.c | 3 +- librpc/ndr/ndr_dns_utils.c | 42 ++++++++++++++++--- librpc/ndr/ndr_dns_utils.h | 3 +- librpc/ndr/ndr_nbt.c | 72 ++++---------------------------- librpc/wscript_build | 3 +- selftest/knownfail.d/dns_packet | 1 - selftest/knownfail.d/ndr_dns_nbt | 2 - 7 files changed, 49 insertions(+), 77 deletions(-) delete mode 100644 selftest/knownfail.d/ndr_dns_nbt diff --git a/librpc/ndr/ndr_dns.c b/librpc/ndr/ndr_dns.c index 68a3c9de782..966e0b59786 100644 --- a/librpc/ndr/ndr_dns.c +++ b/librpc/ndr/ndr_dns.c @@ -163,7 +163,8 @@ _PUBLIC_ enum ndr_err_code ndr_push_dns_string(struct ndr_push *ndr, return ndr_push_dns_string_list(ndr, &ndr->dns_string_list, ndr_flags, - s); + s, + false); } _PUBLIC_ enum ndr_err_code ndr_pull_dns_txt_record(struct ndr_pull *ndr, int ndr_flags, struct dns_txt_record *r) diff --git a/librpc/ndr/ndr_dns_utils.c b/librpc/ndr/ndr_dns_utils.c index b7f11dbab4e..325d9c68bea 100644 --- a/librpc/ndr/ndr_dns_utils.c +++ b/librpc/ndr/ndr_dns_utils.c @@ -9,9 +9,32 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, struct ndr_token_list *string_list, int ndr_flags, - const char *s) + const char *s, + bool is_nbt) { const char *start = s; + bool use_compression; + size_t max_length; + if (is_nbt) { + use_compression = true; + /* + * Max length is longer in NBT/Wins, because Windows counts + * the semi-decompressed size of the netbios name (16 bytes) + * rather than the wire size of 32, which is what you'd expect + * if it followed RFC1002 (it uses the short form in + * [MS-WINSRA]). In other words the maximum size of the + * "scope" is 237, not 221. + * + * We make the size limit slightly larger than 255 + 16, + * because the 237 scope limit is already enforced in the + * winsserver code with a specific return value; bailing out + * here would muck with that. + */ + max_length = 274; + } else { + use_compression = !(ndr->flags & LIBNDR_FLAG_NO_COMPRESSION); + max_length = 255; + } if (!(ndr_flags & NDR_SCALARS)) { return NDR_ERR_SUCCESS; @@ -23,7 +46,7 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, size_t complen; uint32_t offset; - if (!(ndr->flags & LIBNDR_FLAG_NO_COMPRESSION)) { + if (use_compression) { /* see if we have pushed the remaining string already, * if so we use a label pointer to this string */ @@ -66,6 +89,14 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, "(consecutive dots)"); } + if (is_nbt && s[complen] == '.' && s[complen + 1] == '\0') { + /* nbt names are sometimes usernames, and we need to + * keep a trailing dot to ensure it is byte-identical, + * (not just semantically identical given DNS + * semantics). */ + complen++; + } + compname = talloc_asprintf(ndr, "%c%*.*s", (unsigned char)complen, (unsigned char)complen, @@ -75,7 +106,7 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, /* remember the current component + the rest of the string * so it can be reused later */ - if (!(ndr->flags & LIBNDR_FLAG_NO_COMPRESSION)) { + if (use_compression) { NDR_CHECK(ndr_token_store(ndr, string_list, s, ndr->offset)); } @@ -89,9 +120,10 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, if (*s == '.') { s++; } - if (s - start > 255) { + if (s - start > max_length) { return ndr_push_error(ndr, NDR_ERR_STRING, - "name > 255 character long"); + "name > %zu character long", + max_length); } } diff --git a/librpc/ndr/ndr_dns_utils.h b/librpc/ndr/ndr_dns_utils.h index 823e3201112..71a65433bbb 100644 --- a/librpc/ndr/ndr_dns_utils.h +++ b/librpc/ndr/ndr_dns_utils.h @@ -2,4 +2,5 @@ enum ndr_err_code ndr_push_dns_string_list(struct ndr_push *ndr, struct ndr_token_list *string_list, int ndr_flags, - const char *s); + const char *s, + bool is_nbt); diff --git a/librpc/ndr/ndr_nbt.c b/librpc/ndr/ndr_nbt.c index b15dc5c06e5..8ed9f0a5f05 100644 --- a/librpc/ndr/ndr_nbt.c +++ b/librpc/ndr/ndr_nbt.c @@ -25,6 +25,8 @@ #include "includes.h" #include "../libcli/nbt/libnbt.h" #include "../libcli/netlogon/netlogon.h" +#include "ndr_dns_utils.h" + /* don't allow an unlimited number of name components */ #define MAX_COMPONENTS 128 @@ -141,71 +143,11 @@ _PUBLIC_ enum ndr_err_code ndr_pull_nbt_string(struct ndr_pull *ndr, int ndr_fla */ _PUBLIC_ enum ndr_err_code ndr_push_nbt_string(struct ndr_push *ndr, int ndr_flags, const char *s) { - if (!(ndr_flags & NDR_SCALARS)) { - return NDR_ERR_SUCCESS; - } - - while (s && *s) { - enum ndr_err_code ndr_err; - char *compname; - size_t complen; - uint32_t offset; - - /* see if we have pushed the remaining string already, - * if so we use a label pointer to this string - */ - ndr_err = ndr_token_retrieve_cmp_fn(&ndr->nbt_string_list, s, &offset, (comparison_fn_t)strcmp, false); - if (NDR_ERR_CODE_IS_SUCCESS(ndr_err)) { - uint8_t b[2]; - - if (offset > 0x3FFF) { - return ndr_push_error(ndr, NDR_ERR_STRING, - "offset for nbt string label pointer %u[%08X] > 0x00003FFF", - offset, offset); - } - - b[0] = 0xC0 | (offset>>8); - b[1] = (offset & 0xFF); - - return ndr_push_bytes(ndr, b, 2); - } - - complen = strcspn(s, "."); - - /* we need to make sure the length fits into 6 bytes */ - if (complen > 0x3F) { - return ndr_push_error(ndr, NDR_ERR_STRING, - "component length %u[%08X] > 0x0000003F", - (unsigned)complen, (unsigned)complen); - } - - if (s[complen] == '.' && s[complen+1] == '\0') { - complen++; - } - - compname = talloc_asprintf(ndr, "%c%*.*s", - (unsigned char)complen, - (unsigned char)complen, - (unsigned char)complen, s); - NDR_ERR_HAVE_NO_MEMORY(compname); - - /* remember the current componemt + the rest of the string - * so it can be reused later - */ - NDR_CHECK(ndr_token_store(ndr, &ndr->nbt_string_list, s, ndr->offset)); - - /* push just this component into the blob */ - NDR_CHECK(ndr_push_bytes(ndr, (const uint8_t *)compname, complen+1)); - talloc_free(compname); - - s += complen; - if (*s == '.') s++; - } - - /* if we reach the end of the string and have pushed the last component - * without using a label pointer, we need to terminate the string - */ - return ndr_push_bytes(ndr, (const uint8_t *)"", 1); + return ndr_push_dns_string_list(ndr, + &ndr->dns_string_list, + ndr_flags, + s, + true); } diff --git a/librpc/wscript_build b/librpc/wscript_build index 3b4599f3211..30763907aaa 100644 --- a/librpc/wscript_build +++ b/librpc/wscript_build @@ -415,7 +415,7 @@ bld.SAMBA_SUBSYSTEM('NDR_NBT', bld.SAMBA_LIBRARY('ndr_nbt', source='gen_ndr/ndr_nbt.c ndr/ndr_nbt.c', - public_deps='ndr NDR_NBT_BUF NDR_SECURITY', + public_deps='ndr NDR_NBT_BUF NDR_SECURITY NDR_DNS', public_headers='gen_ndr/nbt.h gen_ndr/ndr_nbt.h ndr/ndr_nbt.h', header_path=[ ('gen_ndr*', 'gen_ndr'), ('ndr*', 'ndr')], pc_files='ndr_nbt.pc', @@ -705,6 +705,5 @@ bld.SAMBA_BINARY('test_ndr_dns_nbt', cmocka ndr ndr_nbt - NDR_DNS ''', for_selftest=True) diff --git a/selftest/knownfail.d/dns_packet b/selftest/knownfail.d/dns_packet index 0662266f689..e69de29bb2d 100644 --- a/selftest/knownfail.d/dns_packet +++ b/selftest/knownfail.d/dns_packet @@ -1 +0,0 @@ -samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_127_very_dotty_components diff --git a/selftest/knownfail.d/ndr_dns_nbt b/selftest/knownfail.d/ndr_dns_nbt deleted file mode 100644 index 603395c8c50..00000000000 --- a/selftest/knownfail.d/ndr_dns_nbt +++ /dev/null @@ -1,2 +0,0 @@ -librpc.ndr.ndr_dns_nbt.test_ndr_nbt_string_all_dots -librpc.ndr.ndr_dns_nbt.test_ndr_nbt_string_half_dots -- 2.17.1 From 795a25dc81b37f2d85264865f8a4a38559f06e2c Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Thu, 25 Jun 2020 11:59:54 +1200 Subject: [PATCH 19/22] CVE-2020-14303 Ensure an empty packet will not DoS the NBT server Signed-off-by: Andrew Bartlett --- python/samba/tests/dns_packet.py | 19 +++++++++++++++++++ selftest/knownfail.d/empty-nbt | 1 + 2 files changed, 20 insertions(+) create mode 100644 selftest/knownfail.d/empty-nbt diff --git a/python/samba/tests/dns_packet.py b/python/samba/tests/dns_packet.py index c4f843eb613..ae7bcb3ad8c 100644 --- a/python/samba/tests/dns_packet.py +++ b/python/samba/tests/dns_packet.py @@ -156,6 +156,19 @@ class TestDnsPacketBase(TestCase): rcode = self.decode_reply(data)['rcode'] return expected_rcode == rcode + def _test_empty_packet(self): + + packet = b"" + s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) + s.sendto(packet, self.server) + s.close() + + # It is reasonable not to reply to an empty packet + # but it is not reasonable to render the server + # unresponsive. + ok = self._known_good_query() + self.assertTrue(ok, f"the server is unresponsive") + class TestDnsPackets(TestDnsPacketBase): server = (SERVER, 53) @@ -174,6 +187,9 @@ class TestDnsPackets(TestDnsPacketBase): label = b'x.' * 31 + b'x' self._test_many_repeated_components(label, 127) + def test_empty_packet(self): + self._test_empty_packet() + class TestNbtPackets(TestDnsPacketBase): server = (SERVER, 137) @@ -209,3 +225,6 @@ class TestNbtPackets(TestDnsPacketBase): def test_127_half_dotty_components(self): label = b'x.' * 31 + b'x' self._test_many_repeated_components(label, 127) + + def test_empty_packet(self): + self._test_empty_packet() diff --git a/selftest/knownfail.d/empty-nbt b/selftest/knownfail.d/empty-nbt new file mode 100644 index 00000000000..e4bcccab4e5 --- /dev/null +++ b/selftest/knownfail.d/empty-nbt @@ -0,0 +1 @@ +^samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_empty_packet \ No newline at end of file -- 2.17.1 From d7a32e815bfedeb70a17ecaa7028a1587b88c6c9 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Wed, 24 Jun 2020 14:27:08 +1200 Subject: [PATCH 20/22] CVE-2020-14303: s4 nbt: fix busy loop on empty UDP packet An empty UDP packet put the nbt server into a busy loop that consumes 100% of a cpu. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14417 Signed-off-by: Gary Lockyer --- libcli/nbt/nbtsocket.c | 17 ++++++++++++++++- selftest/knownfail.d/empty-nbt | 1 - 2 files changed, 16 insertions(+), 2 deletions(-) delete mode 100644 selftest/knownfail.d/empty-nbt diff --git a/libcli/nbt/nbtsocket.c b/libcli/nbt/nbtsocket.c index 38a2192fbcd..4c6c20d4fe2 100644 --- a/libcli/nbt/nbtsocket.c +++ b/libcli/nbt/nbtsocket.c @@ -167,8 +167,23 @@ static void nbt_name_socket_recv(struct nbt_name_socket *nbtsock) return; } + /* + * Given a zero length, data_blob_talloc() returns the + * NULL blob {NULL, 0}. + * + * We only want to error return here on a real out of memory condition + * (i.e. dsize != 0, so the UDP packet has data, but the return of the + * allocation failed, so blob.data==NULL). + * + * Given an actual zero length UDP packet having blob.data == NULL + * isn't an out of memory error condition, that's the defined semantics + * of data_blob_talloc() when asked for zero bytes. + * + * We still need to continue to do the zero-length socket_recvfrom() + * read in order to clear the "read pending" condition on the socket. + */ blob = data_blob_talloc(tmp_ctx, NULL, dsize); - if (blob.data == NULL) { + if (blob.data == NULL && dsize != 0) { talloc_free(tmp_ctx); return; } diff --git a/selftest/knownfail.d/empty-nbt b/selftest/knownfail.d/empty-nbt deleted file mode 100644 index e4bcccab4e5..00000000000 --- a/selftest/knownfail.d/empty-nbt +++ /dev/null @@ -1 +0,0 @@ -^samba.tests.dns_packet.samba.tests.dns_packet.TestNbtPackets.test_empty_packet \ No newline at end of file -- 2.17.1 From a6a3a9f59d429ae01dc05aa412eab251cc842dcf Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Fri, 5 Jun 2020 22:14:48 +1200 Subject: [PATCH 21/22] CVE-2020-10760 dsdb: Ensure a proper talloc tree for saved controls Otherwise a paged search on the GC port will fail as the ->data was not kept around for the second page of searches. An example command to produce this is bin/ldbsearch --paged -H ldap://$SERVER:3268 -U$USERNAME%$PASSWORD This shows up later in the partition module as: ERROR: AddressSanitizer: heap-use-after-free on address 0x60b00151ef20 at pc 0x7fec3f801aac bp 0x7ffe8472c270 sp 0x7ffe8472c260 READ of size 4 at 0x60b00151ef20 thread T0 (ldap(0)) #0 0x7fec3f801aab in talloc_chunk_from_ptr ../../lib/talloc/talloc.c:526 #1 0x7fec3f801aab in __talloc_get_name ../../lib/talloc/talloc.c:1559 #2 0x7fec3f801aab in talloc_check_name ../../lib/talloc/talloc.c:1582 #3 0x7fec1b86b2e1 in partition_search ../../source4/dsdb/samdb/ldb_modules/partition.c:780 or smb_panic_default: PANIC (pid 13287): Bad talloc magic value - unknown value (from source4/dsdb/samdb/ldb_modules/partition.c:780) BUG: https://bugzilla.samba.org/show_bug.cgi?id=14402 Signed-off-by: Andrew Bartlett --- source4/dsdb/samdb/ldb_modules/paged_results.c | 8 ++++++++ source4/dsdb/samdb/ldb_modules/vlv_pagination.c | 7 +++++++ 2 files changed, 15 insertions(+) diff --git a/source4/dsdb/samdb/ldb_modules/paged_results.c b/source4/dsdb/samdb/ldb_modules/paged_results.c index 735883e8802..3eea3236e7d 100644 --- a/source4/dsdb/samdb/ldb_modules/paged_results.c +++ b/source4/dsdb/samdb/ldb_modules/paged_results.c @@ -523,6 +523,14 @@ paged_results_copy_down_controls(TALLOC_CTX *mem_ctx, continue; } new_controls[j] = talloc_steal(new_controls, control); + + /* + * Sadly the caller is not obliged to make this a + * proper talloc tree, so we do so here. + */ + if (control->data) { + talloc_steal(control, control->data); + } j++; } new_controls[j] = NULL; diff --git a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c index b103bda5f52..d6d6039e849 100644 --- a/source4/dsdb/samdb/ldb_modules/vlv_pagination.c +++ b/source4/dsdb/samdb/ldb_modules/vlv_pagination.c @@ -746,6 +746,13 @@ vlv_copy_down_controls(TALLOC_CTX *mem_ctx, struct ldb_control **controls) continue; } new_controls[j] = talloc_steal(new_controls, control); + /* + * Sadly the caller is not obliged to make this a + * proper talloc tree, so we do so here. + */ + if (control->data) { + talloc_steal(control, control->data); + } j++; } new_controls[j] = NULL; -- 2.17.1 From 565811f4f5f16e072644a1a7c625ad1e60c89b10 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 8 Jun 2020 16:32:14 +1200 Subject: [PATCH 22/22] CVE-2020-10760 dsdb: Add tests for paged_results and VLV over the Global Catalog port This should avoid a regression. Signed-off-by: Andrew Bartlett --- selftest/knownfail.d/vlv | 2 +- source4/dsdb/tests/python/vlv.py | 171 +++++++++++++++++++------------ 2 files changed, 107 insertions(+), 66 deletions(-) diff --git a/selftest/knownfail.d/vlv b/selftest/knownfail.d/vlv index f187a2ed55e..7ae02baf17b 100644 --- a/selftest/knownfail.d/vlv +++ b/selftest/knownfail.d/vlv @@ -1,2 +1,2 @@ samba4.ldap.vlv.python.*__main__.VLVTests.test_vlv_change_search_expr -samba4.ldap.vlv.python.*__main__.PagedResultsTests.test_paged_cant_change_controls_data +samba4.ldap.vlv.python.*__main__.PagedResultsTestsRW.test_paged_cant_change_controls_data diff --git a/source4/dsdb/tests/python/vlv.py b/source4/dsdb/tests/python/vlv.py index 5c1ef5c7fcf..5caebafb94b 100644 --- a/source4/dsdb/tests/python/vlv.py +++ b/source4/dsdb/tests/python/vlv.py @@ -152,7 +152,7 @@ class TestsWithUserOU(samba.tests.TestCase): super(TestsWithUserOU, self).setUp() self.ldb = SamDB(host, credentials=creds, session_info=system_session(lp), lp=lp) - + self.ldb_ro = self.ldb self.base_dn = self.ldb.domain_dn() self.tree_dn = "ou=vlvtesttree,%s" % self.base_dn self.ou = "ou=vlvou,%s" % self.tree_dn @@ -199,8 +199,60 @@ class TestsWithUserOU(samba.tests.TestCase): self.ldb.delete(self.tree_dn, ['tree_delete:1']) -class VLVTests(TestsWithUserOU): +class VLVTestsBase(TestsWithUserOU): + + # Run a vlv search and return important fields of the response control + def vlv_search(self, attr, expr, cookie="", after_count=0, offset=1): + sort_ctrl = "server_sort:1:0:%s" % attr + ctrl = "vlv:1:0:%d:%d:0" % (after_count, offset) + if cookie: + ctrl += ":" + cookie + + res = self.ldb_ro.search(self.ou, + expression=expr, + scope=ldb.SCOPE_ONELEVEL, + attrs=[attr], + controls=[ctrl, sort_ctrl]) + results = [str(x[attr][0]) for x in res] + + ctrls = [str(c) for c in res.controls if + str(c).startswith('vlv')] + self.assertEqual(len(ctrls), 1) + + spl = ctrls[0].rsplit(':') + cookie = "" + if len(spl) == 6: + cookie = spl[-1] + + return results, cookie + + +class VLVTestsRO(VLVTestsBase): + def test_vlv_simple_double_run(self): + """Do the simplest possible VLV query to confirm if VLV + works at all. Useful for showing VLV as a whole works + on Global Catalog (for example)""" + attr = 'roomNumber' + expr = "(objectclass=user)" + # Start new search + full_results, cookie = self.vlv_search(attr, expr, + after_count=len(self.users)) + + results, cookie = self.vlv_search(attr, expr, cookie=cookie, + after_count=len(self.users)) + expected_results = full_results + self.assertEqual(results, expected_results) + + +class VLVTestsGC(VLVTestsRO): + def setUp(self): + super(VLVTestsRO, self).setUp() + self.ldb_ro = SamDB(host + ":3268", credentials=creds, + session_info=system_session(lp), lp=lp) + + +class VLVTests(VLVTestsBase): def get_full_list(self, attr, include_cn=False): """Fetch the whole list sorted on the attribute, using the VLV. This way you get a VLV cookie.""" @@ -1081,31 +1133,6 @@ class VLVTests(TestsWithUserOU): controls=[sort_control, "vlv:0:1:1:1:0:%s" % vlv_cookies[-1]]) - # Run a vlv search and return important fields of the response control - def vlv_search(self, attr, expr, cookie="", after_count=0, offset=1): - sort_ctrl = "server_sort:1:0:%s" % attr - ctrl = "vlv:1:0:%d:%d:0" % (after_count, offset) - if cookie: - ctrl += ":" + cookie - - res = self.ldb.search(self.ou, - expression=expr, - scope=ldb.SCOPE_ONELEVEL, - attrs=[attr], - controls=[ctrl, sort_ctrl]) - results = [str(x[attr][0]) for x in res] - - ctrls = [str(c) for c in res.controls if - str(c).startswith('vlv')] - self.assertEqual(len(ctrls), 1) - - spl = ctrls[0].rsplit(':') - cookie = "" - if len(spl) == 6: - cookie = spl[-1] - - return results, cookie - def test_vlv_modify_during_view(self): attr = 'roomNumber' expr = "(objectclass=user)" @@ -1219,11 +1246,11 @@ class PagedResultsTests(TestsWithUserOU): if subtree: scope = ldb.SCOPE_SUBTREE - res = self.ldb.search(ou, - expression=expr, - scope=scope, - controls=controls, - **kwargs) + res = self.ldb_ro.search(ou, + expression=expr, + scope=scope, + controls=controls, + **kwargs) results = [str(r['cn'][0]) for r in res] ctrls = [str(c) for c in res.controls if @@ -1236,6 +1263,53 @@ class PagedResultsTests(TestsWithUserOU): cookie = spl[-1] return results, cookie + +class PagedResultsTestsRO(PagedResultsTests): + + def test_paged_search_lockstep(self): + expr = "(objectClass=*)" + ps = 3 + + all_results, _ = self.paged_search(expr, page_size=len(self.users)+1) + + # Run two different but overlapping paged searches simultaneously. + set_1_index = int((len(all_results))//3) + set_2_index = int((2*len(all_results))//3) + set_1 = all_results[set_1_index:] + set_2 = all_results[:set_2_index+1] + set_1_expr = "(cn>=%s)" % (all_results[set_1_index]) + set_2_expr = "(cn<=%s)" % (all_results[set_2_index]) + + results, cookie1 = self.paged_search(set_1_expr, page_size=ps) + self.assertEqual(results, set_1[:ps]) + results, cookie2 = self.paged_search(set_2_expr, page_size=ps) + self.assertEqual(results, set_2[:ps]) + + results, cookie1 = self.paged_search(set_1_expr, cookie=cookie1, + page_size=ps) + self.assertEqual(results, set_1[ps:ps*2]) + results, cookie2 = self.paged_search(set_2_expr, cookie=cookie2, + page_size=ps) + self.assertEqual(results, set_2[ps:ps*2]) + + results, _ = self.paged_search(set_1_expr, cookie=cookie1, + page_size=len(self.users)) + self.assertEqual(results, set_1[ps*2:]) + results, _ = self.paged_search(set_2_expr, cookie=cookie2, + page_size=len(self.users)) + self.assertEqual(results, set_2[ps*2:]) + + +class PagedResultsTestsGC(PagedResultsTestsRO): + + def setUp(self): + super(PagedResultsTestsRO, self).setUp() + self.ldb_ro = SamDB(host + ":3268", credentials=creds, + session_info=system_session(lp), lp=lp) + + +class PagedResultsTestsRW(PagedResultsTests): + def test_paged_delete_during_search(self, sort=True): expr = "(objectClass=*)" @@ -1637,39 +1711,6 @@ class PagedResultsTests(TestsWithUserOU): cookie, attrs=changed_attrs, extra_ctrls=[]) - def test_paged_search_lockstep(self): - expr = "(objectClass=*)" - ps = 3 - - all_results, _ = self.paged_search(expr, page_size=len(self.users)+1) - - # Run two different but overlapping paged searches simultaneously. - set_1_index = int((len(all_results))//3) - set_2_index = int((2*len(all_results))//3) - set_1 = all_results[set_1_index:] - set_2 = all_results[:set_2_index+1] - set_1_expr = "(cn>=%s)" % (all_results[set_1_index]) - set_2_expr = "(cn<=%s)" % (all_results[set_2_index]) - - results, cookie1 = self.paged_search(set_1_expr, page_size=ps) - self.assertEqual(results, set_1[:ps]) - results, cookie2 = self.paged_search(set_2_expr, page_size=ps) - self.assertEqual(results, set_2[:ps]) - - results, cookie1 = self.paged_search(set_1_expr, cookie=cookie1, - page_size=ps) - self.assertEqual(results, set_1[ps:ps*2]) - results, cookie2 = self.paged_search(set_2_expr, cookie=cookie2, - page_size=ps) - self.assertEqual(results, set_2[ps:ps*2]) - - results, _ = self.paged_search(set_1_expr, cookie=cookie1, - page_size=len(self.users)) - self.assertEqual(results, set_1[ps*2:]) - results, _ = self.paged_search(set_2_expr, cookie=cookie2, - 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. -- 2.17.1