From 3ca77e3edc0ba2c9dd3f2c0394f8c2f799d989b9 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 c745a495415d77fd3ebfb8a658a1dad7cce114a5 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 9d4b98d75ea6a283afc45e9e4aef91fdfb95a189 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 80144c53d5546314f929b48f40c704f7cff083a8 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 ca2be7c95bb3a76c125cc816a8fe23a5557b6e63 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 bc07a53d575..ce7aa213c36 100644 --- a/source4/dsdb/tests/python/vlv.py +++ b/source4/dsdb/tests/python/vlv.py @@ -1644,6 +1644,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 04f059fa7ec543122e72f77d3fbd6e9cc45f6947 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 dff247a9a55..c442a967792 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 fff5a35ba4ee43c50691fc178c386dd2d9e460e7 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 c442a967792..e9221099325 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 9c5c2c845caf7f03a6b47abcd5e1bc0092d628c2 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 e9221099325..c4b538f2208 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); } @@ -769,7 +794,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 d400d7d8ac66ab889c3f1a6572fec64f20f0861e 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 d6848d2f3f281956d8401f11ed1a6b609f802e21 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 44a487ba987..090d41dde69 100644 --- a/lib/ldb/common/ldb.c +++ b/lib/ldb/common/ldb.c @@ -1009,6 +1009,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); @@ -1023,7 +1030,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 ec11a9adbf989add9a77c68fe8175bd587400d4b Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Fri, 22 May 2020 10:53:29 +1200 Subject: [PATCH 11/22] ldb: Bump version to 1.5.8 BUG: https://bugzilla.samba.org/show_bug.cgi?id=14364 Signed-off-by: Gary Lockyer Reviewed-by: Andrew Bartlett --- lib/ldb/ABI/ldb-1.5.8.sigs | 281 ++++++++++++++++++++++++++ lib/ldb/ABI/pyldb-util-1.5.8.sigs | 2 + lib/ldb/ABI/pyldb-util.py3-1.5.8.sigs | 2 + lib/ldb/wscript | 2 +- 4 files changed, 286 insertions(+), 1 deletion(-) create mode 100644 lib/ldb/ABI/ldb-1.5.8.sigs create mode 100644 lib/ldb/ABI/pyldb-util-1.5.8.sigs create mode 100644 lib/ldb/ABI/pyldb-util.py3-1.5.8.sigs diff --git a/lib/ldb/ABI/ldb-1.5.8.sigs b/lib/ldb/ABI/ldb-1.5.8.sigs new file mode 100644 index 00000000000..9bf06ce6e93 --- /dev/null +++ b/lib/ldb/ABI/ldb-1.5.8.sigs @@ -0,0 +1,281 @@ +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_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_find: const char *(struct ldb_context *, const char **, const char *) +ldb_pack_data: int (struct ldb_context *, const struct ldb_message *, struct ldb_val *) +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_only_attr_list: int (struct ldb_context *, const struct ldb_val *, struct ldb_message *, const char * const *, unsigned int, unsigned int *) +ldb_unpack_data_only_attr_list_flags: int (struct ldb_context *, const struct ldb_val *, struct ldb_message *, const char * const *, unsigned int, unsigned int, 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-1.5.8.sigs b/lib/ldb/ABI/pyldb-util-1.5.8.sigs new file mode 100644 index 00000000000..74d6719d2bc --- /dev/null +++ b/lib/ldb/ABI/pyldb-util-1.5.8.sigs @@ -0,0 +1,2 @@ +pyldb_Dn_FromDn: PyObject *(struct ldb_dn *) +pyldb_Object_AsDn: bool (TALLOC_CTX *, PyObject *, struct ldb_context *, struct ldb_dn **) diff --git a/lib/ldb/ABI/pyldb-util.py3-1.5.8.sigs b/lib/ldb/ABI/pyldb-util.py3-1.5.8.sigs new file mode 100644 index 00000000000..74d6719d2bc --- /dev/null +++ b/lib/ldb/ABI/pyldb-util.py3-1.5.8.sigs @@ -0,0 +1,2 @@ +pyldb_Dn_FromDn: PyObject *(struct ldb_dn *) +pyldb_Object_AsDn: bool (TALLOC_CTX *, PyObject *, struct ldb_context *, struct ldb_dn **) diff --git a/lib/ldb/wscript b/lib/ldb/wscript index 0f760a9bc80..58240222d5f 100644 --- a/lib/ldb/wscript +++ b/lib/ldb/wscript @@ -1,7 +1,7 @@ #!/usr/bin/env python APPNAME = 'ldb' -VERSION = '1.5.7' +VERSION = '1.5.8' import sys, os -- 2.17.1 From 6253d590d2330eadd740fdde79551f9a50f4b52f 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 (backported from patch for master) [abartlet@samba.org: f"" strings are not in Python 3.4 and bytes cannot be formatted in python 3.4] Signed-off-by: Andrew Bartlett --- python/samba/tests/dns_packet.py | 210 +++++++++++++++++++++++++++++++ selftest/knownfail.d/dns_packet | 2 + source4/selftest/tests.py | 10 ++ 3 files changed, 222 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..a9996664e57 --- /dev/null +++ b/python/samba/tests/dns_packet.py @@ -0,0 +1,210 @@ +# 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 = 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("the server is STILL unresponsive after %d seconds" % (40 * TIMEOUT)) + + 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(bytes([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, "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 19c81b46ea4..23416df221c 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -416,6 +416,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_ntvfs:local", '//$SERVER/whavever') -- 2.17.1 From b9ba23d41e0bc5f781e8a391f71b3be5f8017155 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 | 13 ++ selftest/knownfail.d/ndr_dns_nbt | 4 + source4/selftest/tests.py | 2 + 4 files changed, 255 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 8e113c422b2..788efcf57a6 100644 --- a/librpc/wscript_build +++ b/librpc/wscript_build @@ -756,3 +756,16 @@ bld.SAMBA_SUBSYSTEM('NDR_FSRVP_STATE', source='gen_ndr/ndr_fsrvp_state.c', public_deps='ndr' ) +# +# Cmocka tests +# + +bld.SAMBA_BINARY('test_ndr_dns_nbt', + source='tests/test_ndr_dns_nbt.c', + deps=''' + cmocka + ndr + ndr_nbt + NDR_DNS + ''', + install=False) 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 23416df221c..8cf54841e86 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -1303,6 +1303,8 @@ plantestsuite("samba4.dsdb.samdb.ldb_modules.group_audit.errors", "none", [os.path.join(bindir(), "test_group_audit_errors")]) plantestsuite("samba4.dcerpc.dnsserver.dnsutils", "none", [os.path.join(bindir(), "test_rpc_dns_server_dnsutils")]) +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 b8788a04ebd6ca9c2f9f72e11049d770ff54afac 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 788efcf57a6..62200ea0524 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 0c6bbd701926dfc16754fc5ea523d6f21d0ea740 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 dbde3431f70ec0cf9c0da7abe7bc53fd4e5d3a63 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 d266802a3fd75b91848b41f2b347de2e27fee5f9 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 21a449f491be33f7cc2dd54491abf17dae041c21 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 838f947a168..e8dd7549a53 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 62200ea0524..b560a08a7e2 100644 --- a/librpc/wscript_build +++ b/librpc/wscript_build @@ -411,7 +411,7 @@ bld.SAMBA_SUBSYSTEM('NDR_SCHANNEL', 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', @@ -766,6 +766,5 @@ bld.SAMBA_BINARY('test_ndr_dns_nbt', cmocka ndr ndr_nbt - NDR_DNS ''', install=False) 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 9773231e3a53291214a914ed168065f5ed5ea1e6 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 (backported from master commit) [abartlet@samba.org: Remove f"" format string not supported in Python 3.4] --- 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 a9996664e57..68e4d154cad 100644 --- a/python/samba/tests/dns_packet.py +++ b/python/samba/tests/dns_packet.py @@ -155,6 +155,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, "the server is unresponsive") + class TestDnsPackets(TestDnsPacketBase): server = (SERVER, 53) @@ -173,6 +186,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) @@ -208,3 +224,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 2e190d5c766d3487223ccdd4dc1e2ad0e160bb3f 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 33d53fba993..8aecaf73247 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 4bc0ada8d99425ad8d8933e0c3b1abecaa185edf 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 c4b538f2208..bc4996880e0 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 ca38b0eecddbfab0ff1b80d7e588b19beb299084 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. (backported from master patch) [abartlet@samba.org: sort=True parameter on test_paged_delete_during_search is not in 4.10] 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 ce7aa213c36..acaf64d0faa 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.ou = "ou=vlv,%s" % self.base_dn if opts.delete_in_setup: @@ -195,8 +195,60 @@ class TestsWithUserOU(samba.tests.TestCase): self.ldb.delete(self.ou, ['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.""" @@ -1077,31 +1129,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)" @@ -1214,11 +1241,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 @@ -1231,6 +1258,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): expr = "(objectClass=*)" @@ -1611,39 +1685,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