From 96b5043d1914696f8da07efd718d50e0c526a59e Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 11 Mar 2020 16:41:34 +1300 Subject: [PATCH 1/7] CVE-2020-10700: ldb: Always use ldb_next_request() in ASQ module We want to keep going down the module stack, and not start from the top again. ASQ is above the ACL modules, but below paged_results and we do not wish to re-trigger that work. Thanks to Andrei Popa for finding, reporting and working with us to diagnose this issue! BUG: https://bugzilla.samba.org/show_bug.cgi?id=14331 Signed-off-by: Andrew Bartlett Reviewed-by: Gary Lockyer --- lib/ldb/modules/asq.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/lib/ldb/modules/asq.c b/lib/ldb/modules/asq.c index 7482de826f0..4eba941ae0b 100644 --- a/lib/ldb/modules/asq.c +++ b/lib/ldb/modules/asq.c @@ -311,12 +311,9 @@ static int asq_build_multiple_requests(struct asq_context *ac, bool *terminated) static int asq_search_continue(struct asq_context *ac) { - struct ldb_context *ldb; bool terminated = false; int ret; - ldb = ldb_module_get_ctx(ac->module); - switch (ac->step) { case ASQ_SEARCH_BASE: @@ -328,7 +325,7 @@ static int asq_search_continue(struct asq_context *ac) ac->step = ASQ_SEARCH_MULTI; - return ldb_request(ldb, ac->reqs[ac->cur_req]); + return ldb_next_request(ac->module, ac->reqs[ac->cur_req]); case ASQ_SEARCH_MULTI: @@ -339,7 +336,7 @@ static int asq_search_continue(struct asq_context *ac) return asq_search_terminate(ac); } - return ldb_request(ldb, ac->reqs[ac->cur_req]); + return ldb_next_request(ac->module, ac->reqs[ac->cur_req]); } return LDB_ERR_OPERATIONS_ERROR; @@ -347,14 +344,11 @@ static int asq_search_continue(struct asq_context *ac) static int asq_search(struct ldb_module *module, struct ldb_request *req) { - struct ldb_context *ldb; struct ldb_request *base_req; struct ldb_control *control; struct asq_context *ac; int ret; - ldb = ldb_module_get_ctx(module); - /* check if there's an ASQ control */ control = ldb_request_get_control(req, LDB_CONTROL_ASQ_OID); if (control == NULL) { @@ -385,7 +379,7 @@ static int asq_search(struct ldb_module *module, struct ldb_request *req) ac->step = ASQ_SEARCH_BASE; - return ldb_request(ldb, base_req); + return ldb_next_request(ac->module, base_req); } static int asq_init(struct ldb_module *module) -- 2.17.1 From 0191c7f6893b675317a6865911dddc39de2cac00 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 5 May 2020 12:54:59 +1200 Subject: [PATCH 2/7] 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 bd8df7def53..fe48e6729ba 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 41f899316f2b092c7e4f94d8acdc264869978a5e Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 5 May 2020 12:55:57 +1200 Subject: [PATCH 3/7] 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 fe48e6729ba..ae5842afc81 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 5dfd19e953e9c3289e9323281cd698c1a8ac214c Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 5 May 2020 16:34:11 +1200 Subject: [PATCH 4/7] 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 ae5842afc81..d6bad4bbd4a 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 1a96a0d938119f550281fd3474172fbd4fadf124 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Mon, 18 May 2020 12:37:39 +1200 Subject: [PATCH 5/7] 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 d6bad4bbd4a..99cff4ac642 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 e0bf6966028d0b5bd11160861d2bf8dcb6244f7f Mon Sep 17 00:00:00 2001 From: Douglas Bagnall Date: Wed, 7 Jun 2017 17:44:25 +1200 Subject: [PATCH 6/7] python/test: delete_force() passes on command line args This allows you to use e.g.: delete_force(self.ldb, ou, controls=['tree_delete:1']) Only in tests of course. Signed-off-by: Douglas Bagnall Reviewed-by: Andrew Bartlett --- python/samba/tests/__init__.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/python/samba/tests/__init__.py b/python/samba/tests/__init__.py index 6960c0ca3ae..9801496e406 100644 --- a/python/samba/tests/__init__.py +++ b/python/samba/tests/__init__.py @@ -901,8 +901,9 @@ def connect_samdb_env(env_url, env_username, env_password, lp=None): return connect_samdb(samdb_url, credentials=creds, lp=lp) -def delete_force(samdb, dn): +def delete_force(samdb, dn, **kwargs): try: - samdb.delete(dn) - except ldb.LdbError, (num, errstr): + samdb.delete(dn, **kwargs) + except ldb.LdbError as error: + (num, errstr) = error.args assert num == ldb.ERR_NO_SUCH_OBJECT, "ldb.delete() failed: %s" % errstr -- 2.17.1 From 0f06373cb31ea69e3baa7954e3278d46de92f908 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Mon, 25 May 2020 10:53:27 +1200 Subject: [PATCH 7/7] CVE-2020-10730: selftest: Back port ASQ tests Backport the ASQ tests added for CVE-2020-10700. The paged_result tests were removed as that functionality is not in Samba 4.5 BUG: https://bugzilla.samba.org/show_bug.cgi?id=14364 Signed-off-by: Gary Lockyer Reviewed-by: Andrew Bartlett --- source4/dsdb/tests/python/asq.py | 177 +++++++++++++++++++++++++++++++ source4/selftest/tests.py | 9 ++ 2 files changed, 186 insertions(+) create mode 100644 source4/dsdb/tests/python/asq.py diff --git a/source4/dsdb/tests/python/asq.py b/source4/dsdb/tests/python/asq.py new file mode 100644 index 00000000000..3049533f532 --- /dev/null +++ b/source4/dsdb/tests/python/asq.py @@ -0,0 +1,177 @@ +#!/usr/bin/env python +# +# Test ASQ LDAP control behaviour in Samba +# Copyright (C) Andrew Bartlett 2019-2020 +# +# Based on Unit tests for the notification control +# Copyright (C) Stefan Metzmacher 2016 +# +# 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 . + +import optparse +import sys +import os +import random + +sys.path.insert(0, "bin/python") +import samba +from samba.tests.subunitrun import SubunitOptions, TestProgram + +import samba.getopt as options + +from samba.auth import system_session +from samba import ldb +from samba.samdb import SamDB +from samba.ndr import ndr_unpack +from samba import gensec +from samba.credentials import Credentials +import samba.tests + +from ldb import SCOPE_SUBTREE, SCOPE_ONELEVEL, SCOPE_BASE, LdbError +from ldb import ERR_TIME_LIMIT_EXCEEDED, ERR_ADMIN_LIMIT_EXCEEDED, ERR_UNWILLING_TO_PERFORM +from ldb import Message + +parser = optparse.OptionParser("asq.py [options] ") +sambaopts = options.SambaOptions(parser) +parser.add_option_group(sambaopts) +parser.add_option_group(options.VersionOptions(parser)) +# use command line creds if available +credopts = options.CredentialsOptions(parser) +parser.add_option_group(credopts) +subunitopts = SubunitOptions(parser) +parser.add_option_group(subunitopts) +opts, args = parser.parse_args() + +if len(args) < 1: + parser.print_usage() + sys.exit(1) + +url = args[0] + +lp = sambaopts.get_loadparm() +creds = credopts.get_credentials(lp) + + +class ASQLDAPTest(samba.tests.TestCase): + + def setUp(self): + super(ASQLDAPTest, self).setUp() + self.ldb = samba.Ldb( + url, credentials=creds, session_info=system_session(lp), lp=lp) + self.base_dn = self.ldb.get_default_basedn() + self.NAME_ASQ = "asq_" + format(random.randint(0, 99999), "05") + self.OU_NAME_ASQ = self.NAME_ASQ + "_ou" + self.ou_dn = ldb.Dn( + self.ldb, "ou=" + self.OU_NAME_ASQ + "," + str(self.base_dn)) + + samba.tests.delete_force( + self.ldb, self.ou_dn, controls=['tree_delete:1']) + + self.ldb.add({ + "dn": self.ou_dn, + "objectclass": "organizationalUnit", + "ou": self.OU_NAME_ASQ}) + + self.members = [] + self.members2 = [] + + for x in range(20): + name = self.NAME_ASQ + "_" + str(x) + dn = ldb.Dn(self.ldb, + "cn=" + name + "," + str(self.ou_dn)) + self.members.append(dn) + self.ldb.add({ + "dn": dn, + "objectclass": "group"}) + + for x in range(20): + name = self.NAME_ASQ + "_" + str(x + 20) + dn = ldb.Dn(self.ldb, + "cn=" + name + "," + str(self.ou_dn)) + self.members2.append(dn) + self.ldb.add({ + "dn": dn, + "objectclass": "group", + "member": [str(x) for x in self.members]}) + + name = self.NAME_ASQ + "_41" + self.top_dn = ldb.Dn(self.ldb, + "cn=" + name + "," + str(self.ou_dn)) + self.ldb.add({ + "dn": self.top_dn, + "objectclass": "group", + "member": [str(x) for x in self.members2]}) + + def tearDown(self): + samba.tests.delete_force( + self.ldb, self.ou_dn, controls=['tree_delete:1']) + + def test_asq(self): + """Testing ASQ behaviour. + + 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! + + """ + + msgs = self.ldb.search(base=self.top_dn, + scope=ldb.SCOPE_BASE, + attrs=["objectGUID", "cn", "member"], + controls=["asq:1:member"]) + + 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) + + 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 + else: + url = "ldap://%s" % url + +TestProgram(module=__name__, opts=subunitopts) diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py index 966fb0397fc..ee76c561b0e 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -581,6 +581,15 @@ planoldpythontestsuite("ad_dc", "samba.tests.dcerpc.raw_protocol", extra_args=[' plantestsuite_loadlist("samba4.ldap.python(ad_dc_ntvfs)", "ad_dc_ntvfs", [python, os.path.join(samba4srcdir, "dsdb/tests/python/ldap.py"), '$SERVER', '-U"$USERNAME%$PASSWORD"', '--workgroup=$DOMAIN', '$LOADLIST', '$LISTOPT']) plantestsuite_loadlist("samba4.tokengroups.python(ad_dc_ntvfs)", "ad_dc_ntvfs:local", [python, os.path.join(samba4srcdir, "dsdb/tests/python/token_group.py"), '$SERVER', '-U"$USERNAME%$PASSWORD"', '--workgroup=$DOMAIN', '$LOADLIST', '$LISTOPT']) plantestsuite("samba4.sam.python(ad_dc_ntvfs)", "ad_dc_ntvfs", [python, os.path.join(samba4srcdir, "dsdb/tests/python/sam.py"), '$SERVER', '-U"$USERNAME%$PASSWORD"', '--workgroup=$DOMAIN']) +plantestsuite( + "samba4.asq.python(ad_dc_ntvfs)", + "ad_dc_ntvfs", + [ + python, + os.path.join(samba4srcdir, "dsdb/tests/python/asq.py"), + '$SERVER', + '-U"$USERNAME%$PASSWORD"', + '--workgroup=$DOMAIN']) plantestsuite("samba4.user_account_control.python(ad_dc_ntvfs)", "ad_dc_ntvfs", [python, os.path.join(samba4srcdir, "dsdb/tests/python/user_account_control.py"), '$SERVER', '-U"$USERNAME%$PASSWORD"', '--workgroup=$DOMAIN']) planoldpythontestsuite("ad_dc_ntvfs", "dsdb_schema_info", extra_path=[os.path.join(samba4srcdir, 'dsdb/tests/python')], -- 2.17.1