From 9bb0c71f1b9b0ed1a47fbe8b9dd15dbfdf53ac40 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 7 Aug 2023 11:55:55 +1200 Subject: [PATCH 1/7] CVE-2023-4154 dsdb/tests: Do not run SimpleDirsyncTests twice To re-use setup code, the super-class must have no test_*() methods otherwise these will be run as well as the class-local tests. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andrew Bartlett --- source4/dsdb/tests/python/dirsync.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/source4/dsdb/tests/python/dirsync.py b/source4/dsdb/tests/python/dirsync.py index ca0947e2d21..8ac5333f945 100755 --- a/source4/dsdb/tests/python/dirsync.py +++ b/source4/dsdb/tests/python/dirsync.py @@ -585,9 +585,6 @@ class SimpleDirsyncTests(DirsyncBaseTests): expression="(&(objectClass=organizationalUnit)(!(isDeleted=*)))", controls=controls) - -class ExtendedDirsyncTests(SimpleDirsyncTests): - def test_dirsync_linkedattributes_range(self): self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) res = self.ldb_admin.search(self.base_dn, -- 2.25.1 From 73ea11f1443a94ec34595c5f06642b900921b462 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 7 Aug 2023 13:15:40 +1200 Subject: [PATCH 2/7] CVE-2023-4154 dsdb/tests: Use self.addCleanup() and delete_force() Thie helps ensure this test is reliable even in spite of errors while running. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andrew Bartlett --- source4/dsdb/tests/python/confidential_attr.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/source4/dsdb/tests/python/confidential_attr.py b/source4/dsdb/tests/python/confidential_attr.py index 8ca56bd1023..3997848f8f9 100755 --- a/source4/dsdb/tests/python/confidential_attr.py +++ b/source4/dsdb/tests/python/confidential_attr.py @@ -98,7 +98,9 @@ class ConfidentialAttrCommon(samba.tests.TestCase): userou = "OU=conf-attr-test" self.ou = "{0},{1}".format(userou, self.base_dn) + samba.tests.delete_force(self.ldb_admin, self.ou, controls=['tree_delete:1']) self.ldb_admin.create_ou(self.ou) + self.addCleanup(samba.tests.delete_force, self.ldb_admin, self.ou, controls=['tree_delete:1']) # use a common username prefix, so we can use sAMAccountName=CATC-* as # a search filter to only return the users we're interested in @@ -139,10 +141,6 @@ class ConfidentialAttrCommon(samba.tests.TestCase): "{0} searchFlags already {1}".format(self.conf_attr, search_flags)) - def tearDown(self): - super(ConfidentialAttrCommon, self).tearDown() - self.ldb_admin.delete(self.ou, ["tree_delete:1"]) - def add_attr(self, dn, attr, value): m = Message() m.dn = Dn(self.ldb_admin, dn) -- 2.25.1 From ec01914e7fdd2370810f0ce2dddf654a2cf90d85 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 7 Aug 2023 14:44:28 +1200 Subject: [PATCH 3/7] CVE-2023-4154 dsdb/tests: Force the test attribute to be not-confidential at the start Rather than fail, if the last run failed to reset things, just force the DC into the required state. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andrew Bartlett --- source4/dsdb/tests/python/confidential_attr.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/source4/dsdb/tests/python/confidential_attr.py b/source4/dsdb/tests/python/confidential_attr.py index 3997848f8f9..ee7f554a008 100755 --- a/source4/dsdb/tests/python/confidential_attr.py +++ b/source4/dsdb/tests/python/confidential_attr.py @@ -136,10 +136,12 @@ class ConfidentialAttrCommon(samba.tests.TestCase): # sanity-check the flag is not already set (this'll cause problems if # previous test run didn't clean up properly) - search_flags = self.get_attr_search_flags(self.attr_dn) - self.assertEqual(0, int(search_flags) & SEARCH_FLAG_CONFIDENTIAL, - "{0} searchFlags already {1}".format(self.conf_attr, - search_flags)) + search_flags = int(self.get_attr_search_flags(self.attr_dn)) + if search_flags & SEARCH_FLAG_CONFIDENTIAL: + self.set_attr_search_flags(self.attr_dn, str(search_flags &~ SEARCH_FLAG_CONFIDENTIAL)) + search_flags = int(self.get_attr_search_flags(self.attr_dn)) + self.assertEqual(0, search_flags & SEARCH_FLAG_CONFIDENTIAL, + f"{self.conf_attr} searchFlags did not reset to omit SEARCH_FLAG_CONFIDENTIAL ({search_flags})") def add_attr(self, dn, attr, value): m = Message() -- 2.25.1 From 4dc5f8a40dd905f697d6f12632ff251b4d52d3f9 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 7 Aug 2023 11:56:56 +1200 Subject: [PATCH 4/7] CVE-2023-4154 dsdb/tests: Check that secret attributes are not visible with DirSync ever. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andrew Bartlett --- source4/dsdb/tests/python/dirsync.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/source4/dsdb/tests/python/dirsync.py b/source4/dsdb/tests/python/dirsync.py index 8ac5333f945..d07dc707e7d 100755 --- a/source4/dsdb/tests/python/dirsync.py +++ b/source4/dsdb/tests/python/dirsync.py @@ -746,6 +746,19 @@ class SimpleDirsyncTests(DirsyncBaseTests): self.assertEqual(guid2, guid) self.assertEqual(str(res[0].dn), "") + def test_dirsync_unicodePwd(self): + self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) + res = self.ldb_admin.search(self.base_dn, + attrs=["unicodePwd", "supplementalCredentials", "cn"], + expression="(samAccountName=krbtgt)", + controls=["dirsync:1:0:0"]) + + self.assertTrue(len(res) == 1) + # This form ensures this is a case insensitive comparison + self.assertTrue(res[0].get("cn")) + self.assertTrue(res[0].get("unicodePwd") is None) + self.assertTrue(res[0].get("supplementalCredentials") is None) + if not getattr(opts, "listtests", False): lp = sambaopts.get_loadparm() -- 2.25.1 From 9d2604929fed912bc54cdd6ff0c25dd998006830 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 8 Aug 2023 11:18:46 +1200 Subject: [PATCH 5/7] CVE-2023-4154 dsdb/tests: Speed up DirSync test by only checking positive matches once When we (expect to) get back a result, do not waste time against a potentially slow server confirming we also get back results for all the other attribute combinations. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andrew Bartlett --- source4/dsdb/tests/python/confidential_attr.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/source4/dsdb/tests/python/confidential_attr.py b/source4/dsdb/tests/python/confidential_attr.py index ee7f554a008..678a5a82948 100755 --- a/source4/dsdb/tests/python/confidential_attr.py +++ b/source4/dsdb/tests/python/confidential_attr.py @@ -742,7 +742,13 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon): # want to weed out results from any previous test runs search = "(&{0}{1})".format(expr, self.extra_filter) - for attr in self.attr_filters: + # If we expect to return multiple results, only check the first + if expected_num > 0: + attr_filters = [self.attr_filters[0]] + else: + attr_filters = self.attr_filters + + for attr in attr_filters: res = samdb.search(base_dn, expression=search, scope=SCOPE_SUBTREE, attrs=attr, controls=self.dirsync) self.assertEqual(len(res), expected_num, -- 2.25.1 From c53f4c21a0f7ecb4d773edac7d7b090d5739ed20 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 8 Aug 2023 14:30:19 +1200 Subject: [PATCH 6/7] CVE-2023-4154 dsdb/tests: Add test for SEARCH_FLAG_RODC_ATTRIBUTE behaviour SEARCH_FLAG_RODC_ATTRIBUTE should be like SEARCH_FLAG_CONFIDENTIAL, but for DirSync and DRS replication. Accounts with GUID_DRS_GET_CHANGES rights should not be able to read this attribute. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andrew Bartlett --- .../dsdb/tests/python/confidential_attr.py | 45 ++++++++++++++++--- 1 file changed, 40 insertions(+), 5 deletions(-) diff --git a/source4/dsdb/tests/python/confidential_attr.py b/source4/dsdb/tests/python/confidential_attr.py index 678a5a82948..df342fafc3d 100755 --- a/source4/dsdb/tests/python/confidential_attr.py +++ b/source4/dsdb/tests/python/confidential_attr.py @@ -30,13 +30,15 @@ import time from samba.tests.subunitrun import SubunitOptions, TestProgram import samba.getopt as options from ldb import SCOPE_BASE, SCOPE_SUBTREE -from samba.dsdb import SEARCH_FLAG_CONFIDENTIAL, SEARCH_FLAG_PRESERVEONDELETE +from samba.dsdb import SEARCH_FLAG_CONFIDENTIAL, SEARCH_FLAG_RODC_ATTRIBUTE, SEARCH_FLAG_PRESERVEONDELETE from ldb import Message, MessageElement, Dn from ldb import FLAG_MOD_REPLACE, FLAG_MOD_ADD from samba.auth import system_session from samba import gensec, sd_utils from samba.samdb import SamDB from samba.credentials import Credentials, DONT_USE_KERBEROS +from samba.dcerpc import security + import samba.tests import samba.dsdb @@ -137,11 +139,11 @@ class ConfidentialAttrCommon(samba.tests.TestCase): # sanity-check the flag is not already set (this'll cause problems if # previous test run didn't clean up properly) search_flags = int(self.get_attr_search_flags(self.attr_dn)) - if search_flags & SEARCH_FLAG_CONFIDENTIAL: - self.set_attr_search_flags(self.attr_dn, str(search_flags &~ SEARCH_FLAG_CONFIDENTIAL)) + if search_flags & SEARCH_FLAG_CONFIDENTIAL|SEARCH_FLAG_RODC_ATTRIBUTE: + self.set_attr_search_flags(self.attr_dn, str(search_flags &~ (SEARCH_FLAG_CONFIDENTIAL|SEARCH_FLAG_RODC_ATTRIBUTE))) search_flags = int(self.get_attr_search_flags(self.attr_dn)) - self.assertEqual(0, search_flags & SEARCH_FLAG_CONFIDENTIAL, - f"{self.conf_attr} searchFlags did not reset to omit SEARCH_FLAG_CONFIDENTIAL ({search_flags})") + self.assertEqual(0, search_flags & (SEARCH_FLAG_CONFIDENTIAL|SEARCH_FLAG_RODC_ATTRIBUTE), + f"{self.conf_attr} searchFlags did not reset to omit SEARCH_FLAG_CONFIDENTIAL and SEARCH_FLAG_RODC_ATTRIBUTE ({search_flags})") def add_attr(self, dn, attr, value): m = Message() @@ -1098,5 +1100,38 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon): user_matching, user_non_matching = time_searches(self.ldb_user) assertRangesOverlap(user_matching, user_non_matching) +# Check that using the dirsync controls doesn't reveal confidential +# "RODC filtered attribute" values to users with only +# GUID_DRS_GET_CHANGES. The tests is so similar to the Confidential +# attribute test we base it on that. +class RodcFilteredAttrDirsync(ConfidentialAttrTestDirsync): + + def setUp(self): + super().setUp() + self.dirsync = ["dirsync:1:0:1000"] + + user_sid = self.sd_utils.get_object_sid(self.get_user_dn(self.user)) + mod = "(OA;;CR;%s;;%s)" % (security.GUID_DRS_GET_CHANGES, + str(user_sid)) + self.sd_utils.dacl_add_ace(self.base_dn, mod) + + self.ldb_user = self.get_ldb_connection(self.user, self.user_pass) + + self.addCleanup(self.sd_utils.dacl_del_ace, self.base_dn, mod) + + def make_attr_confidential(self): + """Marks the attribute under test as being confidential AND RODC + filtered (which should mean it is not visible with only + GUID_DRS_GET_CHANGES) + """ + + # work out the original 'searchFlags' value before we overwrite it + old_value = self.get_attr_search_flags(self.attr_dn) + + self.set_attr_search_flags(self.attr_dn, str(SEARCH_FLAG_RODC_ATTRIBUTE|SEARCH_FLAG_CONFIDENTIAL)) + + # reset the value after the test completes + self.addCleanup(self.set_attr_search_flags, self.attr_dn, old_value) + TestProgram(module=__name__, opts=subunitopts) -- 2.25.1 From 6cd82b81e5e042f3a55f082f4ef91a02a785507c Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 8 Aug 2023 17:58:27 +1200 Subject: [PATCH 7/7] CVE-2023-4154: Unimplement the original DirSync behaviour without LDAP_DIRSYNC_OBJECT_SECURITY This makes LDAP_DIRSYNC_OBJECT_SECURITY the only behaviour provided by Samba. Having a second access control system withing the LDAP stack is unsafe and this layer is incomplete. The current system gives all accounts that have been given the GUID_DRS_GET_CHANGES extended right SYSTEM access. Currently in Samba this equates to full access to passwords as well as "RODC Filtered attributes" (often used with confidential attributes). Rather than attempting to correctly filter for secrets (passwords) and these filtered attributes, as well as preventing search expressions for both, we leave this complexity to the acl_read module which has this facility already well tested. The implication is that callers will only see and filter by attribute in DirSync that they could without DirSync. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andrew Bartlett --- source4/dsdb/samdb/ldb_modules/dirsync.c | 67 +----------------------- 1 file changed, 1 insertion(+), 66 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/dirsync.c b/source4/dsdb/samdb/ldb_modules/dirsync.c index 9261758ec7b..b48d3faad04 100644 --- a/source4/dsdb/samdb/ldb_modules/dirsync.c +++ b/source4/dsdb/samdb/ldb_modules/dirsync.c @@ -998,7 +998,6 @@ static int dirsync_search_callback(struct ldb_request *req, struct ldb_reply *ar static int dirsync_ldb_search(struct ldb_module *module, struct ldb_request *req) { struct ldb_control *control; - struct ldb_result *acl_res; struct ldb_dirsync_control *dirsync_ctl; struct ldb_control *extended = NULL; struct ldb_request *down_req; @@ -1052,72 +1051,8 @@ static int dirsync_ldb_search(struct ldb_module *module, struct ldb_request *req } if (ldb_dn_compare(dsc->nc_root, req->op.search.base) != 0) { - if (dirsync_ctl->flags & LDAP_DIRSYNC_OBJECT_SECURITY) { - return ldb_error(ldb, LDB_ERR_UNWILLING_TO_PERFORM, + return ldb_error(ldb, LDB_ERR_UNWILLING_TO_PERFORM, "DN is not one of the naming context"); - } - else { - return ldb_error(ldb, LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS, - "dN is not one of the naming context"); - } - } - - if (!(dirsync_ctl->flags & LDAP_DIRSYNC_OBJECT_SECURITY)) { - struct dom_sid *sid; - struct security_descriptor *sd = NULL; - const char *acl_attrs[] = { "nTSecurityDescriptor", "objectSid", "objectClass", NULL }; - const struct dsdb_schema *schema = NULL; - const struct dsdb_class *objectclass = NULL; - /* - * If we don't have the flag and if we have the "replicate directory change" granted - * then we upgrade ourself to system to not be blocked by the acl - */ - /* FIXME we won't check the replicate directory change filtered attribute set - * it should be done so that if attr is not empty then we check that the user - * has also this right - */ - - /* - * First change to system to get the SD of the root of current NC - * if we don't the acl_read will forbid us the right to read it ... - */ - ret = dsdb_module_search_dn(module, dsc, &acl_res, - req->op.search.base, - acl_attrs, - DSDB_FLAG_NEXT_MODULE|DSDB_FLAG_AS_SYSTEM, req); - - if (ret != LDB_SUCCESS) { - return ret; - } - - sid = samdb_result_dom_sid(dsc, acl_res->msgs[0], "objectSid"); - /* sid can be null ... */ - ret = dsdb_get_sd_from_ldb_message(ldb_module_get_ctx(module), acl_res, acl_res->msgs[0], &sd); - - if (ret != LDB_SUCCESS) { - return ret; - } - schema = dsdb_get_schema(ldb, req); - if (!schema) { - return LDB_ERR_OPERATIONS_ERROR; - } - objectclass = dsdb_get_structural_oc_from_msg(schema, acl_res->msgs[0]); - ret = acl_check_extended_right(dsc, module, req, objectclass, - sd, acl_user_token(module), - GUID_DRS_GET_CHANGES, SEC_ADS_CONTROL_ACCESS, sid); - - if (ret == LDB_ERR_INSUFFICIENT_ACCESS_RIGHTS) { - return ret; - } - dsc->assystem = true; - ret = ldb_request_add_control(req, LDB_CONTROL_AS_SYSTEM_OID, false, NULL); - - if (ret != LDB_SUCCESS) { - return ret; - } - talloc_free(acl_res); - } else if (ret != LDB_SUCCESS) { - return ret; } dsc->functional_level = dsdb_functional_level(ldb); -- 2.25.1