From e43a164556b9e1d118b3c982677a32ddd9da963f Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Wed, 2 Aug 2023 10:44:32 +0200 Subject: [PATCH 1/9] CVE-2023-4154 s4:dsdb:tests: Fix code spelling BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andreas Schneider Reviewed-by: Joseph Sutton (cherry picked from commit b29793ffdee5d9b9c1c05830622e80f7faec7670) --- source4/dsdb/tests/python/acl.py | 12 ++++++------ .../dsdb/tests/python/ad_dc_search_performance.py | 2 +- source4/dsdb/tests/python/confidential_attr.py | 2 +- source4/dsdb/tests/python/dirsync.py | 8 ++++---- source4/dsdb/tests/python/ldap.py | 14 +++++++------- source4/dsdb/tests/python/ldap_modify_order.py | 4 ++-- source4/dsdb/tests/python/ldap_syntaxes.py | 4 ++-- source4/dsdb/tests/python/login_basics.py | 2 +- source4/dsdb/tests/python/password_settings.py | 4 ++-- source4/dsdb/tests/python/passwords.py | 4 ++-- source4/dsdb/tests/python/sam.py | 2 +- source4/dsdb/tests/python/sec_descriptor.py | 14 +++++++------- source4/dsdb/tests/python/token_group.py | 4 ++-- source4/dsdb/tests/python/user_account_control.py | 2 +- 14 files changed, 39 insertions(+), 39 deletions(-) diff --git a/source4/dsdb/tests/python/acl.py b/source4/dsdb/tests/python/acl.py index 13785316abb..49d9ca6383d 100755 --- a/source4/dsdb/tests/python/acl.py +++ b/source4/dsdb/tests/python/acl.py @@ -228,7 +228,7 @@ class AclAddTests(AclTests): self.assertEqual(len(res), 0) def test_add_u1(self): - """Testing OU with the rights of Doman Admin not creator of the OU """ + """Testing OU with the rights of Domain Admin not creator of the OU """ self.assert_top_ou_deleted() # Change descriptor for top level OU self.ldb_owner.create_ou("OU=test_add_ou1," + self.base_dn) @@ -241,7 +241,7 @@ class AclAddTests(AclTests): self.ldb_notowner.newgroup("test_add_group1", groupou="OU=test_add_ou2,OU=test_add_ou1", grouptype=samba.dsdb.GTYPE_DISTRIBUTION_DOMAIN_LOCAL_GROUP) # Make sure we HAVE created the two objects -- user and group - # !!! We should not be able to do that, but however beacuse of ACE ordering our inherited Deny ACE + # !!! We should not be able to do that, but however because of ACE ordering our inherited Deny ACE # !!! comes after explicit (A;;RPWPCRCCDCLCLORCWOWDSDDTSW;;;DA) that comes from somewhere res = self.ldb_admin.search(self.base_dn, expression="(distinguishedName=%s,%s)" % ("CN=test_add_user1,OU=test_add_ou2,OU=test_add_ou1", self.base_dn)) self.assertGreater(len(res), 0) @@ -302,7 +302,7 @@ class AclAddTests(AclTests): self.assertEqual(len(res), 0) def test_add_u4(self): - """ 4 Testing OU with the rights of Doman Admin creator of the OU""" + """ 4 Testing OU with the rights of Domain Admin creator of the OU""" self.assert_top_ou_deleted() self.ldb_owner.create_ou("OU=test_add_ou1," + self.base_dn) self.ldb_owner.create_ou("OU=test_add_ou2,OU=test_add_ou1," + self.base_dn) @@ -4298,7 +4298,7 @@ class AclDeleteTests(AclTests): self.assertEqual(len(res), 0) def test_delete_u3(self): - """User indentified by SID has RIGHT_DELETE to another User object""" + """User identified by SID has RIGHT_DELETE to another User object""" user_dn = self.get_user_dn("test_delete_user1") # Create user that we try to delete self.ldb_admin.newuser("test_delete_user1", self.user_pass) @@ -4589,7 +4589,7 @@ class AclCARTests(AclTests): minPwdAge = self.ldb_admin.get_minPwdAge() # Reset the "minPwdAge" as it was before self.addCleanup(self.ldb_admin.set_minPwdAge, minPwdAge) - # Set it temporarely to "0" + # Set it temporarily to "0" self.ldb_admin.set_minPwdAge("0") self.user_with_wp = "acl_car_user1" @@ -5041,7 +5041,7 @@ class AclUndeleteTests(AclTests): self.sd_utils.dacl_add_ace(self.deleted_dn2, mod) self.undelete_deleted(self.deleted_dn2, self.testuser2_dn) - # attempt undelete with simultanious addition of url, WP to which is denied + # attempt undelete with simultaneous addition of url, WP to which is denied mod = "(OD;;WP;9a9a0221-4a5b-11d1-a9c3-0000f80367c1;;%s)" % str(self.sid) self.sd_utils.dacl_add_ace(self.deleted_dn3, mod) try: diff --git a/source4/dsdb/tests/python/ad_dc_search_performance.py b/source4/dsdb/tests/python/ad_dc_search_performance.py index 0afd7a2582e..44e468097d8 100644 --- a/source4/dsdb/tests/python/ad_dc_search_performance.py +++ b/source4/dsdb/tests/python/ad_dc_search_performance.py @@ -180,7 +180,7 @@ class UserTests(samba.tests.TestCase): maybe_not = ['!(', ''] joiners = ['&', '|'] - # The number of permuations is 18432, which is not huge but + # The number of permutations is 18432, which is not huge but # would take hours to search. So we take a sample. all_permutations = list(itertools.product(joiners, classes, classes, diff --git a/source4/dsdb/tests/python/confidential_attr.py b/source4/dsdb/tests/python/confidential_attr.py index eb75da6374f..8ca56bd1023 100755 --- a/source4/dsdb/tests/python/confidential_attr.py +++ b/source4/dsdb/tests/python/confidential_attr.py @@ -722,7 +722,7 @@ class ConfidentialAttrTestDirsync(ConfidentialAttrCommon): self.attr_filters = [None, ["*"], ["name"]] - # Note dirsync behaviour is slighty different for the attribute under + # Note dirsync behaviour is slightly different for the attribute under # test - when you have full access rights, it only returns the objects # that actually have this attribute (i.e. it doesn't return an empty # message with just the DN). So we add the 'name' attribute into the diff --git a/source4/dsdb/tests/python/dirsync.py b/source4/dsdb/tests/python/dirsync.py index 1ac719e4332..ca0947e2d21 100755 --- a/source4/dsdb/tests/python/dirsync.py +++ b/source4/dsdb/tests/python/dirsync.py @@ -338,7 +338,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): self.assertEqual(len(res.msgs[0]), 3) def test_dirsync_othernc(self): - """Check that dirsync return information for entries that are normaly referrals (ie. other NCs)""" + """Check that dirsync return information for entries that are normally referrals (ie. other NCs)""" res = self.ldb_admin.search(self.base_dn, expression="(objectclass=configuration)", attrs=["name"], @@ -459,7 +459,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): delete_force(self.ldb_admin, ouname) def test_dirsync_linkedattributes(self): - """Check that dirsync returnd deleted objects too""" + """Check that dirsync returned deleted objects too""" # Let's search for members self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) res = self.ldb_simple.search(self.base_dn, @@ -541,7 +541,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): self.assertEqual(len(res[0].get("member")), 0) def test_dirsync_deleted_items(self): - """Check that dirsync returnd deleted objects too""" + """Check that dirsync returned deleted objects too""" # Let's create an OU ouname = "OU=testou3,%s" % self.base_dn self.ouname = ouname @@ -712,7 +712,7 @@ class ExtendedDirsyncTests(SimpleDirsyncTests): self.assertIn(b">; Date: Mon, 7 Aug 2023 11:55:55 +1200 Subject: [PATCH 2/9] 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. We rename tests that would otherwise have duplicate names BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andrew Bartlett --- selftest/knownfail | 2 +- source4/dsdb/tests/python/dirsync.py | 7 ++----- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/selftest/knownfail b/selftest/knownfail index 37c75d7ca33..4e34effbbd1 100644 --- a/selftest/knownfail +++ b/selftest/knownfail @@ -151,7 +151,7 @@ ^samba4.smb2.acls.*.inheritflags ^samba4.smb2.acls.*.owner ^samba4.smb2.acls.*.ACCESSBASED -^samba4.ldap.dirsync.python.ad_dc_ntvfs..__main__.ExtendedDirsyncTests.test_dirsync_deleted_items +^samba4.ldap.dirsync.python.ad_dc_ntvfs..__main__.SimpleDirsyncTests.test_dirsync_deleted_items_OBJECT_SECURITY #^samba4.ldap.dirsync.python.ad_dc_ntvfs..__main__.ExtendedDirsyncTests.* ^samba4.libsmbclient.opendir.(NT1|SMB3).opendir # This requires netbios browsing ^samba4.rpc.drsuapi.*.drsuapi.DsGetDomainControllerInfo\(.*\)$ diff --git a/source4/dsdb/tests/python/dirsync.py b/source4/dsdb/tests/python/dirsync.py index ca0947e2d21..ad136b7efee 100755 --- a/source4/dsdb/tests/python/dirsync.py +++ b/source4/dsdb/tests/python/dirsync.py @@ -458,7 +458,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): self.assertTrue(res[0].get("name") is not None) delete_force(self.ldb_admin, ouname) - def test_dirsync_linkedattributes(self): + def test_dirsync_linkedattributes_OBJECT_SECURITY(self): """Check that dirsync returned deleted objects too""" # Let's search for members self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) @@ -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, @@ -711,7 +708,7 @@ class ExtendedDirsyncTests(SimpleDirsyncTests): self.assertIn(b"; Date: Mon, 7 Aug 2023 13:15:40 +1200 Subject: [PATCH 3/9] 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 ++---- source4/dsdb/tests/python/dirsync.py | 6 +----- 2 files changed, 3 insertions(+), 9 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) diff --git a/source4/dsdb/tests/python/dirsync.py b/source4/dsdb/tests/python/dirsync.py index ad136b7efee..e06b85bc749 100755 --- a/source4/dsdb/tests/python/dirsync.py +++ b/source4/dsdb/tests/python/dirsync.py @@ -137,10 +137,6 @@ class SimpleDirsyncTests(DirsyncBaseTests): if self.ouname: delete_force(self.ldb_admin, self.ouname) self.sd_utils.modify_sd_on_dn(self.base_dn, self.desc_sddl) - try: - self.ldb_admin.deletegroup("testgroup") - except Exception: - pass # def test_dirsync_errors(self): @@ -499,6 +495,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): self.assertEqual(len(res[0].get("member")), size) self.ldb_admin.newgroup("testgroup") + self.addCleanup(self.ldb_admin.deletegroup, "testgroup") self.ldb_admin.add_remove_group_members("testgroup", [self.simple_user], add_members_operation=True) @@ -537,7 +534,6 @@ class SimpleDirsyncTests(DirsyncBaseTests): attrs=["member"], controls=[control1]) - self.ldb_admin.deletegroup("testgroup") self.assertEqual(len(res[0].get("member")), 0) def test_dirsync_deleted_items(self): -- 2.25.1 From 5e5b6bd1e143818ff75d6fcd06a62b219b79e62d Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 7 Aug 2023 14:44:28 +1200 Subject: [PATCH 4/9] 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 06208e165651a5d90aafb10ed40ccc6785d1ef94 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 7 Aug 2023 11:56:56 +1200 Subject: [PATCH 5/9] 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 --- selftest/knownfail.d/dirsync | 1 + source4/dsdb/tests/python/dirsync.py | 12 ++++++++++++ 2 files changed, 13 insertions(+) create mode 100644 selftest/knownfail.d/dirsync diff --git a/selftest/knownfail.d/dirsync b/selftest/knownfail.d/dirsync new file mode 100644 index 00000000000..9367f92e109 --- /dev/null +++ b/selftest/knownfail.d/dirsync @@ -0,0 +1 @@ +^samba4.ldap.dirsync.python\(.*\).__main__.SimpleDirsyncTests.test_dirsync_unicodePwd \ No newline at end of file diff --git a/source4/dsdb/tests/python/dirsync.py b/source4/dsdb/tests/python/dirsync.py index e06b85bc749..2cacaf01251 100755 --- a/source4/dsdb/tests/python/dirsync.py +++ b/source4/dsdb/tests/python/dirsync.py @@ -742,6 +742,18 @@ class SimpleDirsyncTests(DirsyncBaseTests): self.assertEqual(guid2, guid) self.assertEqual(str(res[0].dn), "") + def test_dirsync_unicodePwd(self): + res = self.ldb_admin.search(self.base_dn, + attrs=["unicodePwd", "supplementalCredentials", "samAccountName"], + expression="(samAccountName=krbtgt)", + controls=["dirsync:1:0:0"]) + + self.assertTrue(len(res) == 1) + # This form ensures this is a case insensitive comparison + self.assertTrue("samAccountName" in res[0]) + self.assertTrue(res[0].get("samAccountName")) + 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 f1c02852315502c5eadf790a27ed3abb912f143d Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 8 Aug 2023 11:18:46 +1200 Subject: [PATCH 6/9] 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 b5a9d803c46dc39eecd55cf046a25973ca5306e1 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 8 Aug 2023 14:30:19 +1200 Subject: [PATCH 7/9] 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..edbc9593204 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_delete_aces, 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 49564ce145334c512b5ae5b8fea4bba48a4826ed Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 22 Aug 2023 15:08:17 +1200 Subject: [PATCH 8/9] CVE-2023-4154 dsdb/tests: Extend attribute read DirSync tests The aim here is to document the expected (even if not implemented) SEARCH_FLAG_RODC_ATTRIBUTE vs SEARCH_FLAG_CONFIDENTIAL, behaviour, so that any change once CVE-2023-4154 is fixed can be noted. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15424 Signed-off-by: Andrew Bartlett --- selftest/knownfail.d/dirsync | 15 +- source4/dsdb/tests/python/dirsync.py | 456 +++++++++++++++++++++++---- 2 files changed, 414 insertions(+), 57 deletions(-) diff --git a/selftest/knownfail.d/dirsync b/selftest/knownfail.d/dirsync index 9367f92e109..db098549a08 100644 --- a/selftest/knownfail.d/dirsync +++ b/selftest/knownfail.d/dirsync @@ -1 +1,14 @@ -^samba4.ldap.dirsync.python\(.*\).__main__.SimpleDirsyncTests.test_dirsync_unicodePwd \ No newline at end of file +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_OBJECT_SECURITY_insist_on_empty_element\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_OBJ_SEC_insist_on_empty_element\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_with_GET_CHANGES\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_with_GET_CHANGES_OBJ_SEC_insist_on_empty_element\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_with_GET_CHANGES_insist_on_empty_element\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_with_GET_CHANGES_OBJECT_SECURITY_insist_on_empty_element\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_OBJECT_SECURITY_insist_on_empty_element\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_OBJECT_SECURITY_with_GET_CHANGES_insist_on_empty_element\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_with_GET_CHANGES\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_with_GET_CHANGES_attr\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_with_GET_CHANGES_insist_on_empty_element\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.FilteredDirsyncTests.test_dirsync_with_GET_CHANGES\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.FilteredDirsyncTests.test_dirsync_with_GET_CHANGES_attr\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.FilteredDirsyncTests.test_dirsync_with_GET_CHANGES_insist_on_empty_element\(.*\) diff --git a/source4/dsdb/tests/python/dirsync.py b/source4/dsdb/tests/python/dirsync.py index 2cacaf01251..a0691f0afe0 100755 --- a/source4/dsdb/tests/python/dirsync.py +++ b/source4/dsdb/tests/python/dirsync.py @@ -3,6 +3,7 @@ # Unit tests for dirsync control # Copyright (C) Matthieu Patou 2011 # Copyright (C) Jelmer Vernooij 2014 +# Copyright (C) Catalyst.Net Ltd # # 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 @@ -30,7 +31,8 @@ import base64 import ldb from ldb import LdbError, SCOPE_BASE from ldb import Message, MessageElement, Dn -from ldb import FLAG_MOD_ADD, FLAG_MOD_DELETE +from ldb import FLAG_MOD_ADD, FLAG_MOD_DELETE, FLAG_MOD_REPLACE +from samba.dsdb import SEARCH_FLAG_CONFIDENTIAL, SEARCH_FLAG_RODC_ATTRIBUTE from samba.dcerpc import security, misc, drsblobs from samba.ndr import ndr_unpack, ndr_pack @@ -60,7 +62,6 @@ if len(args) < 1: host = args.pop() if "://" not in host: ldaphost = "ldap://%s" % host - ldapshost = "ldaps://%s" % host else: ldaphost = host start = host.rindex("://") @@ -77,8 +78,8 @@ creds = credopts.get_credentials(lp) class DirsyncBaseTests(samba.tests.TestCase): def setUp(self): - super(DirsyncBaseTests, self).setUp() - self.ldb_admin = SamDB(ldapshost, credentials=creds, session_info=system_session(lp), lp=lp) + super().setUp() + self.ldb_admin = SamDB(ldaphost, credentials=creds, session_info=system_session(lp), lp=lp) self.base_dn = self.ldb_admin.domain_dn() self.domain_sid = security.dom_sid(self.ldb_admin.get_domain_sid()) self.user_pass = samba.generate_random_password(12, 16) @@ -87,63 +88,60 @@ class DirsyncBaseTests(samba.tests.TestCase): # used for anonymous login print("baseDN: %s" % self.base_dn) - def get_user_dn(self, name): - return "CN=%s,CN=Users,%s" % (name, self.base_dn) + userou = "OU=dirsync-test" + self.ou = f"{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']) - def get_ldb_connection(self, target_username, target_password): - creds_tmp = Credentials() - creds_tmp.set_username(target_username) - creds_tmp.set_password(target_password) - creds_tmp.set_domain(creds.get_domain()) - creds_tmp.set_realm(creds.get_realm()) - creds_tmp.set_workstation(creds.get_workstation()) - creds_tmp.set_gensec_features(creds_tmp.get_gensec_features() - | gensec.FEATURE_SEAL) - creds_tmp.set_kerberos_state(DONT_USE_KERBEROS) # kinit is too expensive to use in a tight loop - ldb_target = SamDB(url=ldaphost, credentials=creds_tmp, lp=lp) - return ldb_target - - -# tests on ldap add operations -class SimpleDirsyncTests(DirsyncBaseTests): - - def setUp(self): - super(SimpleDirsyncTests, self).setUp() # Regular user self.dirsync_user = "test_dirsync_user" self.simple_user = "test_simple_user" self.admin_user = "test_admin_user" - self.ouname = None + self.dirsync_pass = self.user_pass + self.simple_pass = self.user_pass + self.admin_pass = self.user_pass - self.ldb_admin.newuser(self.dirsync_user, self.user_pass) - self.ldb_admin.newuser(self.simple_user, self.user_pass) - self.ldb_admin.newuser(self.admin_user, self.user_pass) + self.ldb_admin.newuser(self.dirsync_user, self.dirsync_pass, userou=userou) + self.ldb_admin.newuser(self.simple_user, self.simple_pass, userou=userou) + self.ldb_admin.newuser(self.admin_user, self.admin_pass, userou=userou) self.desc_sddl = self.sd_utils.get_sd_as_sddl(self.base_dn) user_sid = self.sd_utils.get_object_sid(self.get_user_dn(self.dirsync_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.addCleanup(self.sd_utils.dacl_delete_aces, self.base_dn, mod) # add admins to the Domain Admins group self.ldb_admin.add_remove_group_members("Domain Admins", [self.admin_user], add_members_operation=True) - def tearDown(self): - super(SimpleDirsyncTests, self).tearDown() - delete_force(self.ldb_admin, self.get_user_dn(self.dirsync_user)) - delete_force(self.ldb_admin, self.get_user_dn(self.simple_user)) - delete_force(self.ldb_admin, self.get_user_dn(self.admin_user)) - if self.ouname: - delete_force(self.ldb_admin, self.ouname) - self.sd_utils.modify_sd_on_dn(self.base_dn, self.desc_sddl) + def get_user_dn(self, name): + return ldb.Dn(self.ldb_admin, "CN={0},{1}".format(name, self.ou)) + + def get_ldb_connection(self, target_username, target_password): + creds_tmp = Credentials() + creds_tmp.set_username(target_username) + creds_tmp.set_password(target_password) + creds_tmp.set_domain(creds.get_domain()) + creds_tmp.set_realm(creds.get_realm()) + creds_tmp.set_workstation(creds.get_workstation()) + creds_tmp.set_gensec_features(creds_tmp.get_gensec_features() + | gensec.FEATURE_SEAL) + creds_tmp.set_kerberos_state(DONT_USE_KERBEROS) # kinit is too expensive to use in a tight loop + ldb_target = SamDB(url=ldaphost, credentials=creds_tmp, lp=lp) + return ldb_target + +# tests on ldap add operations +class SimpleDirsyncTests(DirsyncBaseTests): # def test_dirsync_errors(self): def test_dirsync_supported(self): """Test the basic of the dirsync is supported""" self.ldb_dirsync = self.get_ldb_connection(self.dirsync_user, self.user_pass) - self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) + self.ldb_simple = self.get_ldb_connection(self.simple_user, self.simple_pass) res = self.ldb_admin.search(self.base_dn, expression="samaccountname=*", controls=["dirsync:1:0:1"]) res = self.ldb_dirsync.search(self.base_dn, expression="samaccountname=*", controls=["dirsync:1:0:1"]) try: @@ -169,8 +167,8 @@ class SimpleDirsyncTests(DirsyncBaseTests): def test_dirsync_errors(self): """Test if dirsync returns the correct LDAP errors in case of pb""" - self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) - self.ldb_dirsync = self.get_ldb_connection(self.dirsync_user, self.user_pass) + self.ldb_simple = self.get_ldb_connection(self.simple_user, self.simple_pass) + self.ldb_dirsync = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) try: self.ldb_simple.search(self.base_dn, expression="samaccountname=*", @@ -292,11 +290,11 @@ class SimpleDirsyncTests(DirsyncBaseTests): attrs=["parentGUID"], controls=["dirsync:1:0:1"]) self.assertEqual(len(res.msgs), 0) - ouname = "OU=testou,%s" % self.base_dn + ouname = "OU=testou,%s" % self.ou self.ouname = ouname self.ldb_admin.create_ou(ouname) delta = Message() - delta.dn = Dn(self.ldb_admin, str(ouname)) + delta.dn = Dn(self.ldb_admin, ouname) delta["cn"] = MessageElement("test ou", FLAG_MOD_ADD, "cn") @@ -457,7 +455,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): def test_dirsync_linkedattributes_OBJECT_SECURITY(self): """Check that dirsync returned deleted objects too""" # Let's search for members - self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) + self.ldb_simple = self.get_ldb_connection(self.simple_user, self.simple_pass) res = self.ldb_simple.search(self.base_dn, expression="(name=Administrators)", controls=["dirsync:1:1:1"]) @@ -582,7 +580,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): controls=controls) def test_dirsync_linkedattributes_range(self): - self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) + self.ldb_simple = self.get_ldb_connection(self.simple_user, self.simple_pass) res = self.ldb_admin.search(self.base_dn, attrs=["member;range=1-1"], expression="(name=Administrators)", @@ -594,7 +592,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): self.assertTrue(len(res[0].get("member")) > 0) def test_dirsync_linkedattributes_range_user(self): - self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) + self.ldb_simple = self.get_ldb_connection(self.simple_user, self.simple_pass) try: res = self.ldb_simple.search(self.base_dn, attrs=["member;range=1-1"], @@ -608,7 +606,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): def test_dirsync_linkedattributes(self): flag_incr_linked = 2147483648 - self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) + self.ldb_simple = self.get_ldb_connection(self.simple_user, self.simple_pass) res = self.ldb_admin.search(self.base_dn, attrs=["member"], expression="(name=Administrators)", @@ -676,7 +674,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): def test_dirsync_extended_dn(self): """Check that dirsync works together with the extended_dn control""" # Let's search for members - self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) + self.ldb_simple = self.get_ldb_connection(self.simple_user, self.simple_pass) res = self.ldb_simple.search(self.base_dn, expression="(name=Administrators)", controls=["dirsync:1:1:1"]) @@ -707,7 +705,7 @@ class SimpleDirsyncTests(DirsyncBaseTests): def test_dirsync_deleted_items_OBJECT_SECURITY(self): """Check that dirsync returned deleted objects too""" # Let's create an OU - self.ldb_simple = self.get_ldb_connection(self.simple_user, self.user_pass) + self.ldb_simple = self.get_ldb_connection(self.simple_user, self.simple_pass) ouname = "OU=testou3,%s" % self.base_dn self.ouname = ouname self.ldb_admin.create_ou(ouname) @@ -742,18 +740,364 @@ class SimpleDirsyncTests(DirsyncBaseTests): self.assertEqual(guid2, guid) self.assertEqual(str(res[0].dn), "") - def test_dirsync_unicodePwd(self): +class SpecialDirsyncTests(DirsyncBaseTests): + + def setUp(self): + super().setUp() + + self.schema_dn = self.ldb_admin.get_schema_basedn() + + # the tests work by setting the 'Confidential' or 'RODC Filtered' bit in the searchFlags + # for an existing schema attribute. This only works against Windows if + # the systemFlags does not have FLAG_SCHEMA_BASE_OBJECT set for the + # schema attribute being modified. There are only a few attributes that + # meet this criteria (most of which only apply to 'user' objects) + self.conf_attr = "homePostalAddress" + attr_cn = "CN=Address-Home" + # schemaIdGuid for homePostalAddress (used for ACE tests) + self.attr_dn = f"{attr_cn},{self.schema_dn}" + + 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']) + + # add a test object with this attribute set + self.conf_value = "abcdef" + self.conf_user = "conf-user" + self.ldb_admin.newuser(self.conf_user, self.user_pass, userou=userou) + self.conf_dn = self.get_user_dn(self.conf_user) + self.add_attr(self.conf_dn, self.conf_attr, self.conf_value) + + # 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|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|SEARCH_FLAG_RODC_ATTRIBUTE), + f"{self.conf_attr} searchFlags did not reset to omit SEARCH_FLAG_CONFIDENTIAL and SEARCH_FLAG_RODC_ATTRIBUTE ({search_flags})") + + # 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(self.flag_under_test)) + + # reset the value after the test completes + self.addCleanup(self.set_attr_search_flags, self.attr_dn, old_value) + + def add_attr(self, dn, attr, value): + m = Message() + m.dn = dn + m[attr] = MessageElement(value, FLAG_MOD_ADD, attr) + self.ldb_admin.modify(m) + + def set_attr_search_flags(self, attr_dn, flags): + """Modifies the searchFlags for an object in the schema""" + m = Message() + m.dn = Dn(self.ldb_admin, attr_dn) + m['searchFlags'] = MessageElement(flags, FLAG_MOD_REPLACE, + 'searchFlags') + self.ldb_admin.modify(m) + + # note we have to update the schema for this change to take effect (on + # Windows, at least) + self.ldb_admin.set_schema_update_now() + + def get_attr_search_flags(self, attr_dn): + res = self.ldb_admin.search(attr_dn, scope=SCOPE_BASE, + attrs=['searchFlags']) + return res[0]['searchFlags'][0] + + def find_under_current_ou(self, res): + for msg in res: + if msg.dn == self.conf_dn: + return msg + self.fail(f"Failed to find object {self.conf_dn} in {len(res)} results") + + +class ConfidentialDirsyncTests(SpecialDirsyncTests): + + def setUp(self): + self.flag_under_test = SEARCH_FLAG_CONFIDENTIAL + super().setUp() + + def test_unicodePwd_normal(self): res = self.ldb_admin.search(self.base_dn, attrs=["unicodePwd", "supplementalCredentials", "samAccountName"], - expression="(samAccountName=krbtgt)", - controls=["dirsync:1:0:0"]) + expression=f"(samAccountName={self.conf_user})") + + msg = res[0] + + self.assertTrue("samAccountName" in msg) + # This form ensures this is a case insensitive comparison + self.assertTrue(msg.get("samAccountName")) + self.assertTrue(msg.get("unicodePwd") is None) + self.assertTrue(msg.get("supplementalCredentials") is None) + + def _test_dirsync_unicodePwd(self, ldb_conn, control=None, insist_on_empty_element=False): + res = ldb_conn.search(self.base_dn, + attrs=["unicodePwd", "supplementalCredentials", "samAccountName"], + expression=f"(samAccountName={self.conf_user})", + controls=[control]) + + msg = self.find_under_current_ou(res) - self.assertTrue(len(res) == 1) + self.assertTrue("samAccountName" in msg) # This form ensures this is a case insensitive comparison - self.assertTrue("samAccountName" in res[0]) - self.assertTrue(res[0].get("samAccountName")) - self.assertTrue(res[0].get("unicodePwd") is None) - self.assertTrue(res[0].get("supplementalCredentials") is None) + self.assertTrue(msg.get("samAccountName")) + if insist_on_empty_element: + self.assertTrue(msg.get("unicodePwd") is not None) + self.assertEqual(len(msg.get("unicodePwd")), 0) + self.assertTrue(msg.get("supplementalCredentials") is not None) + self.assertEqual(len(msg.get("supplementalCredentials")), 0) + else: + self.assertTrue(msg.get("unicodePwd") is None + or len(msg.get("unicodePwd")) == 0) + self.assertTrue(msg.get("supplementalCredentials") is None + or len(msg.get("supplementalCredentials")) == 0) + + def test_dirsync_unicodePwd_OBJ_SEC(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + self._test_dirsync_unicodePwd(ldb_conn, control="dirsync:1:1:0") + + def test_dirsync_unicodePwd_OBJ_SEC_insist_on_empty_element(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + self._test_dirsync_unicodePwd(ldb_conn, control="dirsync:1:1:0", insist_on_empty_element=True) + + def test_dirsync_unicodePwd_with_GET_CHANGES_OBJ_SEC(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + self._test_dirsync_unicodePwd(ldb_conn, control="dirsync:1:1:0") + + def test_dirsync_unicodePwd_with_GET_CHANGES_OBJ_SEC_insist_on_empty_element(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + self._test_dirsync_unicodePwd(ldb_conn, control="dirsync:1:1:0", insist_on_empty_element=True) + + def test_dirsync_unicodePwd_with_GET_CHANGES(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + self._test_dirsync_unicodePwd(ldb_conn, control="dirsync:1:0:0") + + def test_dirsync_unicodePwd_with_GET_CHANGES_insist_on_empty_element(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + self._test_dirsync_unicodePwd(ldb_conn, control="dirsync:1:0:0", insist_on_empty_element=True) + + def test_normal(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + res = ldb_conn.search(self.base_dn, + attrs=[self.conf_attr, "samAccountName"], + expression=f"(samAccountName={self.conf_user})") + + msg = res[0] + self.assertTrue("samAccountName" in msg) + # This form ensures this is a case insensitive comparison + self.assertTrue(msg.get("samAccountName")) + self.assertTrue(msg.get(self.conf_attr) is None) + + def _test_dirsync_OBJECT_SECURITY(self, ldb_conn, insist_on_empty_element=False): + res = ldb_conn.search(self.base_dn, + attrs=[self.conf_attr, "samAccountName"], + expression=f"(samAccountName={self.conf_user})", + controls=["dirsync:1:1:0"]) + + msg = self.find_under_current_ou(res) + self.assertTrue("samAccountName" in msg) + # This form ensures this is a case insensitive comparison + self.assertTrue(msg.get("samAccountName")) + if insist_on_empty_element: + self.assertTrue(msg.get(self.conf_attr) is not None) + self.assertEqual(len(msg.get(self.conf_attr)), 0) + else: + self.assertTrue(msg.get(self.conf_attr) is None + or len(msg.get(self.conf_attr)) == 0) + + def test_dirsync_OBJECT_SECURITY(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn) + + def test_dirsync_OBJECT_SECURITY_insist_on_empty_element(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn, insist_on_empty_element=True) + + def test_dirsync_with_GET_CHANGES(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + res = ldb_conn.search(self.base_dn, + attrs=[self.conf_attr, "samAccountName"], + expression=f"(samAccountName={self.conf_user})", + controls=["dirsync:1:0:0"]) + + msg = self.find_under_current_ou(res) + # This form ensures this is a case insensitive comparison + self.assertTrue(msg.get("samAccountName")) + self.assertTrue(msg.get(self.conf_attr)) + self.assertEqual(len(msg.get(self.conf_attr)), 1) + + def test_dirsync_with_GET_CHANGES_OBJECT_SECURITY(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn) + + def test_dirsync_with_GET_CHANGES_OBJECT_SECURITY_insist_on_empty_element(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn, insist_on_empty_element=True) + +class FilteredDirsyncTests(SpecialDirsyncTests): + + def setUp(self): + self.flag_under_test = SEARCH_FLAG_RODC_ATTRIBUTE + super().setUp() + + def test_attr(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + res = ldb_conn.search(self.base_dn, + attrs=[self.conf_attr, "samAccountName"], + expression=f"(samAccountName={self.conf_user})") + + msg = res[0] + self.assertTrue("samAccountName" in msg) + # This form ensures this is a case insensitive comparison + self.assertTrue(msg.get("samAccountName")) + self.assertTrue(msg.get(self.conf_attr)) + self.assertEqual(len(msg.get(self.conf_attr)), 1) + + def _test_dirsync_OBJECT_SECURITY(self, ldb_conn): + res = ldb_conn.search(self.base_dn, + attrs=[self.conf_attr, "samAccountName"], + expression=f"(samAccountName={self.conf_user})", + controls=["dirsync:1:1:0"]) + + msg = self.find_under_current_ou(res) + self.assertTrue("samAccountName" in msg) + # This form ensures this is a case insensitive comparison + self.assertTrue(msg.get("samAccountName")) + self.assertTrue(msg.get(self.conf_attr)) + self.assertEqual(len(msg.get(self.conf_attr)), 1) + + def test_dirsync_OBJECT_SECURITY(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn) + + def test_dirsync_OBJECT_SECURITY_with_GET_CHANGES(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn) + + def _test_dirsync_with_GET_CHANGES(self, insist_on_empty_element=False): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + res = ldb_conn.search(self.base_dn, + expression=f"(samAccountName={self.conf_user})", + controls=["dirsync:1:0:0"]) + + msg = self.find_under_current_ou(res) + # This form ensures this is a case insensitive comparison + self.assertTrue(msg.get("samAccountName")) + if insist_on_empty_element: + self.assertTrue(msg.get(self.conf_attr) is not None) + self.assertEqual(len(msg.get(self.conf_attr)), 0) + else: + self.assertTrue(msg.get(self.conf_attr) is None + or len(msg.get(self.conf_attr)) == 0) + + def test_dirsync_with_GET_CHANGES(self): + self._test_dirsync_with_GET_CHANGES() + + def test_dirsync_with_GET_CHANGES_insist_on_empty_element(self): + self._test_dirsync_with_GET_CHANGES(insist_on_empty_element=True) + + def test_dirsync_with_GET_CHANGES_attr(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + try: + res = ldb_conn.search(self.base_dn, + attrs=[self.conf_attr, "samAccountName"], + expression=f"(samAccountName={self.conf_user})", + controls=["dirsync:1:0:0"]) + self.fail("ldb.search() should have failed with LDAP_INSUFFICIENT_ACCESS_RIGHTS") + except ldb.LdbError as e: + (errno, errstr) = e.args + self.assertEqual(errno, ldb.ERR_INSUFFICIENT_ACCESS_RIGHTS) + +class ConfidentialFilteredDirsyncTests(SpecialDirsyncTests): + + def setUp(self): + self.flag_under_test = SEARCH_FLAG_RODC_ATTRIBUTE|SEARCH_FLAG_CONFIDENTIAL + super().setUp() + + def test_attr(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + res = ldb_conn.search(self.base_dn, + attrs=["unicodePwd", "supplementalCredentials", "samAccountName"], + expression=f"(samAccountName={self.conf_user})") + + msg = res[0] + self.assertTrue(msg.get("samAccountName")) + self.assertTrue(msg.get(self.conf_attr) is None) + + def _test_dirsync_OBJECT_SECURITY(self, ldb_conn, insist_on_empty_element=False): + res = ldb_conn.search(self.base_dn, + attrs=[self.conf_attr, "samAccountName"], + expression=f"(samAccountName={self.conf_user})", + controls=["dirsync:1:1:0"]) + + msg = self.find_under_current_ou(res) + self.assertTrue("samAccountName" in msg) + # This form ensures this is a case insensitive comparison + self.assertTrue(msg.get("samAccountName")) + if insist_on_empty_element: + self.assertTrue(msg.get(self.conf_attr) is not None) + self.assertEqual(len(msg.get(self.conf_attr)), 0) + else: + self.assertTrue(msg.get(self.conf_attr) is None + or len(msg.get(self.conf_attr)) == 0) + + def test_dirsync_OBJECT_SECURITY(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn) + + def test_dirsync_OBJECT_SECURITY_insist_on_empty_element(self): + ldb_conn = self.get_ldb_connection(self.simple_user, self.simple_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn, insist_on_empty_element=True) + + def test_dirsync_OBJECT_SECURITY_with_GET_CHANGES(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn) + + def test_dirsync_OBJECT_SECURITY_with_GET_CHANGES_insist_on_empty_element(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + self._test_dirsync_OBJECT_SECURITY(ldb_conn, insist_on_empty_element=True) + + def _test_dirsync_with_GET_CHANGES(self, insist_on_empty_element=False): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + res = ldb_conn.search(self.base_dn, + expression=f"(samAccountName={self.conf_user})", + controls=["dirsync:1:0:0"]) + + msg = self.find_under_current_ou(res) + # This form ensures this is a case insensitive comparison + self.assertTrue(msg.get("samAccountName")) + if insist_on_empty_element: + self.assertTrue(msg.get(self.conf_attr) is not None) + self.assertEqual(len(msg.get(self.conf_attr)), 0) + else: + self.assertTrue(msg.get(self.conf_attr) is None + or len(msg.get(self.conf_attr)) == 0) + + def test_dirsync_with_GET_CHANGES(self): + self._test_dirsync_with_GET_CHANGES() + + def test_dirsync_with_GET_CHANGES_insist_on_empty_element(self): + self._test_dirsync_with_GET_CHANGES(insist_on_empty_element=True) + + def test_dirsync_with_GET_CHANGES_attr(self): + ldb_conn = self.get_ldb_connection(self.dirsync_user, self.dirsync_pass) + try: + res = ldb_conn.search(self.base_dn, + attrs=[self.conf_attr, "samAccountName"], + expression=f"(samAccountName={self.conf_user})", + controls=["dirsync:1:0:0"]) + self.fail("ldb.search() should have failed with LDAP_INSUFFICIENT_ACCESS_RIGHTS") + except ldb.LdbError as e: + (errno, errstr) = e.args + self.assertEqual(errno, ldb.ERR_INSUFFICIENT_ACCESS_RIGHTS) + if not getattr(opts, "listtests", False): lp = sambaopts.get_loadparm() -- 2.25.1 From e233376f19ab985df4f9e02bc5da533a8a8b5e9c Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 8 Aug 2023 17:58:27 +1200 Subject: [PATCH 9/9] 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 --- selftest/knownfail.d/dirsync | 3 +-- source4/dsdb/samdb/ldb_modules/dirsync.c | 22 +++++++++++----------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/selftest/knownfail.d/dirsync b/selftest/knownfail.d/dirsync index db098549a08..fcf4d469d6e 100644 --- a/selftest/knownfail.d/dirsync +++ b/selftest/knownfail.d/dirsync @@ -1,12 +1,11 @@ ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_OBJECT_SECURITY_insist_on_empty_element\(.*\) ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_OBJ_SEC_insist_on_empty_element\(.*\) -^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_with_GET_CHANGES\(.*\) ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_with_GET_CHANGES_OBJ_SEC_insist_on_empty_element\(.*\) ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_unicodePwd_with_GET_CHANGES_insist_on_empty_element\(.*\) ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_with_GET_CHANGES_OBJECT_SECURITY_insist_on_empty_element\(.*\) +^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialDirsyncTests.test_dirsync_with_GET_CHANGES\(.*\) ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_OBJECT_SECURITY_insist_on_empty_element\(.*\) ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_OBJECT_SECURITY_with_GET_CHANGES_insist_on_empty_element\(.*\) -^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_with_GET_CHANGES\(.*\) ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_with_GET_CHANGES_attr\(.*\) ^samba4.ldap.dirsync.python\(.*\).__main__.ConfidentialFilteredDirsyncTests.test_dirsync_with_GET_CHANGES_insist_on_empty_element\(.*\) ^samba4.ldap.dirsync.python\(.*\).__main__.FilteredDirsyncTests.test_dirsync_with_GET_CHANGES\(.*\) diff --git a/source4/dsdb/samdb/ldb_modules/dirsync.c b/source4/dsdb/samdb/ldb_modules/dirsync.c index b3c463741c8..fbb75790095 100644 --- a/source4/dsdb/samdb/ldb_modules/dirsync.c +++ b/source4/dsdb/samdb/ldb_modules/dirsync.c @@ -56,7 +56,6 @@ struct dirsync_context { bool linkIncrVal; bool localonly; bool partial; - bool assystem; int functional_level; const struct GUID *our_invocation_id; const struct dsdb_schema *schema; @@ -872,10 +871,6 @@ static int dirsync_search_callback(struct ldb_request *req, struct ldb_reply *ar DSDB_SEARCH_SHOW_DELETED | DSDB_SEARCH_SHOW_EXTENDED_DN; - if (dsc->assystem) { - flags = flags | DSDB_FLAG_AS_SYSTEM; - } - ret = dsdb_module_search_tree(dsc->module, dsc, &res, dn, LDB_SCOPE_BASE, req->op.search.tree, @@ -1102,16 +1097,21 @@ static int dirsync_ldb_search(struct ldb_module *module, struct ldb_request *req return LDB_ERR_OPERATIONS_ERROR; } objectclass = dsdb_get_structural_oc_from_msg(schema, acl_res->msgs[0]); + + /* + * While we never use the answer to this for access + * control (after CVE-2023-4154), we return a + * different error message depending on if the user + * was granted GUID_DRS_GET_CHANGES to provide a closer + * emulation and keep some tests passing. + * + * (Samba's ACL logic is not well suited to redacting + * only the secret and RODC filtered attributes). + */ 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; } -- 2.25.1