From c4990af941de117fb38aba06e26bad7097d7632e Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Wed, 1 Jun 2022 16:07:17 +1200 Subject: [PATCH 01/14] s4-acl: Add tests for validated dNSHostName write BUG: https://bugzilla.samba.org/show_bug.cgi?id=14833 Signed-off-by: Joseph Sutton --- selftest/knownfail.d/validated-dns-host-name | 15 + source4/dsdb/tests/python/acl.py | 757 +++++++++++++++++++ 2 files changed, 772 insertions(+) create mode 100644 selftest/knownfail.d/validated-dns-host-name diff --git a/selftest/knownfail.d/validated-dns-host-name b/selftest/knownfail.d/validated-dns-host-name new file mode 100644 index 00000000000..ee51f440969 --- /dev/null +++ b/selftest/knownfail.d/validated-dns-host-name @@ -0,0 +1,15 @@ +^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name\( +^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_account_no_dollar\( +^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_allowed_suffixes\( +^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_case\( +^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_dollar\( +^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_empty_string\( +^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_invalid\( +^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_no_suffix\( +^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_no_value\( +^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_spn\( +^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_spn_matching_account_name_new\( +^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_spn_matching_account_name_original\( +^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_wrong_prefix\( +^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_wrong_suffix\( +^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_spn_matching_dns_host_name_invalid\( diff --git a/source4/dsdb/tests/python/acl.py b/source4/dsdb/tests/python/acl.py index 70dca9b7678..9052c986a34 100755 --- a/source4/dsdb/tests/python/acl.py +++ b/source4/dsdb/tests/python/acl.py @@ -300,6 +300,7 @@ class AclModifyTests(AclTests): delete_force(self.ldb_admin, "CN=test_modify_group1,CN=Users," + self.base_dn) delete_force(self.ldb_admin, "CN=test_modify_group2,CN=Users," + self.base_dn) delete_force(self.ldb_admin, "CN=test_modify_group3,CN=Users," + self.base_dn) + delete_force(self.ldb_admin, "CN=test_mod_hostname,OU=test_modify_ou1," + self.base_dn) delete_force(self.ldb_admin, "OU=test_modify_ou1," + self.base_dn) delete_force(self.ldb_admin, self.get_user_dn(self.user_with_wp)) delete_force(self.ldb_admin, self.get_user_dn(self.user_with_sm)) @@ -651,6 +652,762 @@ Member: CN=test_modify_user2,CN=Users,""" + self.base_dn else: self.fail() + def test_modify_dns_host_name(self): + '''Test modifying dNSHostName with validated write''' + + ou_dn = f'OU=test_modify_ou1,{self.base_dn}' + + account_name = 'test_mod_hostname' + dn = f'CN={account_name},{ou_dn}' + + self.ldb_admin.create_ou(ou_dn) + + # Grant Validated Write. + mod = (f'(OA;CI;SW;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + + # Create the account. + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'computer', + 'sAMAccountName': f'{account_name}$', + }) + + host_name = f'{account_name}.{self.ldb_user.domain_dns_name()}' + + m = Message(Dn(self.ldb_user, dn)) + m['dNSHostName'] = MessageElement(host_name, + FLAG_MOD_REPLACE, + 'dNSHostName') + try: + self.ldb_user.modify(m) + except LdbError: + self.fail() + + def test_modify_dns_host_name_no_validated_write(self): + '''Test modifying dNSHostName without validated write''' + + ou_dn = f'OU=test_modify_ou1,{self.base_dn}' + + account_name = 'test_mod_hostname' + dn = f'CN={account_name},{ou_dn}' + + self.ldb_admin.create_ou(ou_dn) + + # Create the account. + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'computer', + 'sAMAccountName': f'{account_name}$', + }) + + host_name = f'{account_name}.{self.ldb_user.domain_dns_name()}' + + m = Message(Dn(self.ldb_user, dn)) + m['dNSHostName'] = MessageElement(host_name, + FLAG_MOD_REPLACE, + 'dNSHostName') + try: + self.ldb_user.modify(m) + except LdbError as err: + num, estr = err.args + self.assertEqual(ERR_INSUFFICIENT_ACCESS_RIGHTS, num) + else: + self.fail() + + def test_modify_dns_host_name_invalid(self): + '''Test modifying dNSHostName to an invalid value''' + + ou_dn = f'OU=test_modify_ou1,{self.base_dn}' + + account_name = 'test_mod_hostname' + dn = f'CN={account_name},{ou_dn}' + + self.ldb_admin.create_ou(ou_dn) + + # Grant Validated Write. + mod = (f'(OA;CI;SW;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + + # Create the account. + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'computer', + 'sAMAccountName': f'{account_name}$', + }) + + host_name = 'invalid' + + m = Message(Dn(self.ldb_user, dn)) + m['dNSHostName'] = MessageElement(host_name, + FLAG_MOD_REPLACE, + 'dNSHostName') + try: + self.ldb_user.modify(m) + except LdbError as err: + num, estr = err.args + self.assertEqual(ERR_CONSTRAINT_VIOLATION, num) + else: + self.fail() + + def test_modify_dns_host_name_invalid_wp(self): + '''Test modifying dNSHostName to an invalid value when we have WP''' + + ou_dn = f'OU=test_modify_ou1,{self.base_dn}' + + account_name = 'test_mod_hostname' + dn = f'CN={account_name},{ou_dn}' + + self.ldb_admin.create_ou(ou_dn) + + # Grant Write Property. + mod = (f'(OA;CI;WP;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + + # Create the account. + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'computer', + 'sAMAccountName': f'{account_name}$', + }) + + host_name = 'invalid' + + m = Message(Dn(self.ldb_user, dn)) + m['dNSHostName'] = MessageElement(host_name, + FLAG_MOD_REPLACE, + 'dNSHostName') + try: + self.ldb_user.modify(m) + except LdbError: + self.fail() + + def test_modify_dns_host_name_invalid_non_computer(self): + '''Test modifying dNSHostName to an invalid value on a non-computer''' + + ou_dn = f'OU=test_modify_ou1,{self.base_dn}' + + account_name = 'test_mod_hostname' + dn = f'CN={account_name},{ou_dn}' + + self.ldb_admin.create_ou(ou_dn) + + # Grant Validated Write. + mod = (f'(OA;CI;SW;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + + # Create the account. + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'user', + 'sAMAccountName': f'{account_name}', + }) + + host_name = 'invalid' + + m = Message(Dn(self.ldb_user, dn)) + m['dNSHostName'] = MessageElement(host_name, + FLAG_MOD_REPLACE, + 'dNSHostName') + try: + self.ldb_user.modify(m) + except LdbError as err: + num, estr = err.args + self.assertEqual(ERR_INSUFFICIENT_ACCESS_RIGHTS, num) + else: + self.fail() + + def test_modify_dns_host_name_no_value(self): + '''Test modifying dNSHostName with validated write with no value''' + + ou_dn = f'OU=test_modify_ou1,{self.base_dn}' + + account_name = 'test_mod_hostname' + dn = f'CN={account_name},{ou_dn}' + + self.ldb_admin.create_ou(ou_dn) + + # Grant Validated Write. + mod = (f'(OA;CI;SW;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + + # Create the account. + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'computer', + 'sAMAccountName': f'{account_name}$', + }) + + m = Message(Dn(self.ldb_user, dn)) + m['dNSHostName'] = MessageElement([], + FLAG_MOD_REPLACE, + 'dNSHostName') + try: + self.ldb_user.modify(m) + except LdbError as err: + num, estr = err.args + self.assertEqual(ERR_OPERATIONS_ERROR, num) + else: + # Windows accepts this. + pass + + def test_modify_dns_host_name_empty_string(self): + '''Test modifying dNSHostName with validated write of an empty string''' + + ou_dn = f'OU=test_modify_ou1,{self.base_dn}' + + account_name = 'test_mod_hostname' + dn = f'CN={account_name},{ou_dn}' + + self.ldb_admin.create_ou(ou_dn) + + # Grant Validated Write. + mod = (f'(OA;CI;SW;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + + # Create the account. + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'computer', + 'sAMAccountName': f'{account_name}$', + }) + + m = Message(Dn(self.ldb_user, dn)) + m['dNSHostName'] = MessageElement('\0', + FLAG_MOD_REPLACE, + 'dNSHostName') + try: + self.ldb_user.modify(m) + except LdbError as err: + num, estr = err.args + self.assertEqual(ERR_CONSTRAINT_VIOLATION, num) + else: + self.fail() + + def test_modify_dns_host_name_dollar(self): + '''Test modifying dNSHostName with validated write of a value including a dollar''' + + ou_dn = f'OU=test_modify_ou1,{self.base_dn}' + + account_name = 'test_mod_hostname' + dn = f'CN={account_name},{ou_dn}' + + self.ldb_admin.create_ou(ou_dn) + + # Grant Validated Write. + mod = (f'(OA;CI;SW;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + + # Create the account. + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'computer', + 'sAMAccountName': f'{account_name}$', + }) + + host_name = f'{account_name}$.{self.ldb_user.domain_dns_name()}' + + m = Message(Dn(self.ldb_user, dn)) + m['dNSHostName'] = MessageElement(host_name, + FLAG_MOD_REPLACE, + 'dNSHostName') + try: + self.ldb_user.modify(m) + except LdbError as err: + num, estr = err.args + self.assertEqual(ERR_CONSTRAINT_VIOLATION, num) + else: + self.fail() + + def test_modify_dns_host_name_account_no_dollar(self): + '''Test modifying dNSHostName with validated write with no dollar in sAMAccountName''' + + ou_dn = f'OU=test_modify_ou1,{self.base_dn}' + + account_name = 'test_mod_hostname' + dn = f'CN={account_name},{ou_dn}' + + self.ldb_admin.create_ou(ou_dn) + + # Grant Validated Write. + mod = (f'(OA;CI;SW;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + + # Create the account. + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'computer', + 'sAMAccountName': f'{account_name}', + }) + + host_name = f'{account_name}.{self.ldb_user.domain_dns_name()}' + + m = Message(Dn(self.ldb_user, dn)) + m['dNSHostName'] = MessageElement(host_name, + FLAG_MOD_REPLACE, + 'dNSHostName') + try: + self.ldb_user.modify(m) + except LdbError: + self.fail() + + def test_modify_dns_host_name_no_suffix(self): + '''Test modifying dNSHostName with validated write of a value missing the suffix''' + + ou_dn = f'OU=test_modify_ou1,{self.base_dn}' + + account_name = 'test_mod_hostname' + dn = f'CN={account_name},{ou_dn}' + + self.ldb_admin.create_ou(ou_dn) + + # Grant Validated Write. + mod = (f'(OA;CI;SW;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + + # Create the account. + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'computer', + 'sAMAccountName': f'{account_name}$', + }) + + host_name = f'{account_name}' + + m = Message(Dn(self.ldb_user, dn)) + m['dNSHostName'] = MessageElement(host_name, + FLAG_MOD_REPLACE, + 'dNSHostName') + try: + self.ldb_user.modify(m) + except LdbError as err: + num, estr = err.args + self.assertEqual(ERR_CONSTRAINT_VIOLATION, num) + else: + self.fail() + + def test_modify_dns_host_name_wrong_prefix(self): + '''Test modifying dNSHostName with validated write of a value with the wrong prefix''' + + ou_dn = f'OU=test_modify_ou1,{self.base_dn}' + + account_name = 'test_mod_hostname' + dn = f'CN={account_name},{ou_dn}' + + self.ldb_admin.create_ou(ou_dn) + + # Grant Validated Write. + mod = (f'(OA;CI;SW;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + + # Create the account. + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'computer', + 'sAMAccountName': f'{account_name}$', + }) + + host_name = f'invalid.{self.ldb_user.domain_dns_name()}' + + m = Message(Dn(self.ldb_user, dn)) + m['dNSHostName'] = MessageElement(host_name, + FLAG_MOD_REPLACE, + 'dNSHostName') + try: + self.ldb_user.modify(m) + except LdbError as err: + num, estr = err.args + self.assertEqual(ERR_CONSTRAINT_VIOLATION, num) + else: + self.fail() + + def test_modify_dns_host_name_wrong_suffix(self): + '''Test modifying dNSHostName with validated write of a value with the wrong suffix''' + + ou_dn = f'OU=test_modify_ou1,{self.base_dn}' + + account_name = 'test_mod_hostname' + dn = f'CN={account_name},{ou_dn}' + + self.ldb_admin.create_ou(ou_dn) + + # Grant Validated Write. + mod = (f'(OA;CI;SW;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + + # Create the account. + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'computer', + 'sAMAccountName': f'{account_name}$', + }) + + host_name = f'{account_name}.invalid.example.com' + + m = Message(Dn(self.ldb_user, dn)) + m['dNSHostName'] = MessageElement(host_name, + FLAG_MOD_REPLACE, + 'dNSHostName') + try: + self.ldb_user.modify(m) + except LdbError as err: + num, estr = err.args + self.assertEqual(ERR_CONSTRAINT_VIOLATION, num) + else: + self.fail() + + def test_modify_dns_host_name_case(self): + '''Test modifying dNSHostName with validated write of a value with irregular case''' + + ou_dn = f'OU=test_modify_ou1,{self.base_dn}' + + account_name = 'test_mod_hostname' + dn = f'CN={account_name},{ou_dn}' + + self.ldb_admin.create_ou(ou_dn) + + # Grant Validated Write. + mod = (f'(OA;CI;SW;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + + # Create the account. + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'computer', + 'sAMAccountName': f'{account_name}$', + }) + + host_name = f'{account_name}.{self.ldb_user.domain_dns_name()}' + host_name = host_name.capitalize() + + m = Message(Dn(self.ldb_user, dn)) + m['dNSHostName'] = MessageElement(host_name, + FLAG_MOD_REPLACE, + 'dNSHostName') + try: + self.ldb_user.modify(m) + except LdbError: + self.fail() + + def test_modify_dns_host_name_allowed_suffixes(self): + '''Test modifying dNSHostName with validated write and an allowed suffix''' + + allowed_suffix = 'suffix.that.is.allowed' + + # Add the allowed suffix. + + res = self.ldb_admin.search(self.base_dn, + scope=SCOPE_BASE, + attrs=['msDS-AllowedDNSSuffixes']) + self.assertEqual(1, len(res)) + old_allowed_suffixes = res[0].get('msDS-AllowedDNSSuffixes') + + def modify_allowed_suffixes(suffixes): + if suffixes is None: + suffixes = [] + flag = FLAG_MOD_DELETE + else: + flag = FLAG_MOD_REPLACE + + m = Message(Dn(self.ldb_admin, self.base_dn)) + m['msDS-AllowedDNSSuffixes'] = MessageElement( + suffixes, + flag, + 'msDS-AllowedDNSSuffixes') + self.ldb_admin.modify(m) + + self.addCleanup(modify_allowed_suffixes, old_allowed_suffixes) + + if old_allowed_suffixes is None: + allowed_suffixes = [] + else: + allowed_suffixes = list(old_allowed_suffixes) + + if (allowed_suffix not in allowed_suffixes and + allowed_suffix.encode('utf-8') not in allowed_suffixes): + allowed_suffixes.append(allowed_suffix) + + modify_allowed_suffixes(allowed_suffixes) + + # Create the account and run the test. + + ou_dn = f'OU=test_modify_ou1,{self.base_dn}' + + account_name = 'test_mod_hostname' + dn = f'CN={account_name},{ou_dn}' + + self.ldb_admin.create_ou(ou_dn) + + # Grant Validated Write. + mod = (f'(OA;CI;SW;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + + # Create the account. + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'computer', + 'sAMAccountName': f'{account_name}$', + }) + + host_name = f'{account_name}.{allowed_suffix}' + + m = Message(Dn(self.ldb_user, dn)) + m['dNSHostName'] = MessageElement(host_name, + FLAG_MOD_REPLACE, + 'dNSHostName') + try: + self.ldb_user.modify(m) + except LdbError: + self.fail() + + def test_modify_dns_host_name_spn(self): + '''Test modifying dNSHostName and SPN with validated write''' + + ou_dn = f'OU=test_modify_ou1,{self.base_dn}' + + account_name = 'test_mod_hostname' + dn = f'CN={account_name},{ou_dn}' + + self.ldb_admin.create_ou(ou_dn) + + # Grant Validated Write. + mod = (f'(OA;CI;SW;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + mod = (f'(OA;CI;SW;{security.GUID_DRS_VALIDATE_SPN};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + + # Create the account. + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'computer', + 'sAMAccountName': f'{account_name}$', + }) + + host_name = f'{account_name}.{self.ldb_user.domain_dns_name()}' + spn = f'host/{host_name}' + + m = Message(Dn(self.ldb_user, dn)) + m['0'] = MessageElement(host_name, + FLAG_MOD_REPLACE, + 'dNSHostName') + m['1'] = MessageElement(spn, + FLAG_MOD_ADD, + 'servicePrincipalName') + try: + self.ldb_user.modify(m) + except LdbError: + self.fail() + + def test_modify_spn_matching_dns_host_name_invalid(self): + '''Test modifying SPN with validated write, matching a valid dNSHostName ''' + + ou_dn = f'OU=test_modify_ou1,{self.base_dn}' + + account_name = 'test_mod_hostname' + dn = f'CN={account_name},{ou_dn}' + + self.ldb_admin.create_ou(ou_dn) + + # Grant Write Property. + mod = (f'(OA;CI;WP;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + # Grant Validated Write. + mod = (f'(OA;CI;SW;{security.GUID_DRS_VALIDATE_SPN};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + + # Create the account. + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'computer', + 'sAMAccountName': f'{account_name}$', + }) + + invalid_host_name = 'invalid' + + host_name = f'{account_name}.{self.ldb_user.domain_dns_name()}' + spn = f'host/{host_name}' + + m = Message(Dn(self.ldb_user, dn)) + m['0'] = MessageElement(invalid_host_name, + FLAG_MOD_REPLACE, + 'dNSHostName') + m['1'] = MessageElement(spn, + FLAG_MOD_ADD, + 'servicePrincipalName') + m['2'] = MessageElement(host_name, + FLAG_MOD_REPLACE, + 'dNSHostName') + try: + self.ldb_user.modify(m) + except LdbError: + self.fail() + + def test_modify_spn_matching_dns_host_name_original(self): + '''Test modifying SPN with validated write, matching the original dNSHostName ''' + + ou_dn = f'OU=test_modify_ou1,{self.base_dn}' + + account_name = 'test_mod_hostname' + dn = f'CN={account_name},{ou_dn}' + + self.ldb_admin.create_ou(ou_dn) + + # Grant Validated Write. + mod = (f'(OA;CI;SW;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + mod = (f'(OA;CI;SW;{security.GUID_DRS_VALIDATE_SPN};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + + original_host_name = 'invalid_host_name' + original_spn = 'host/{original_host_name}' + + # Create the account. + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'computer', + 'sAMAccountName': f'{account_name}$', + 'dNSHostName': original_host_name, + }) + + host_name = f'{account_name}.{self.ldb_user.domain_dns_name()}' + + m = Message(Dn(self.ldb_user, dn)) + m['0'] = MessageElement(original_spn, + FLAG_MOD_ADD, + 'servicePrincipalName') + m['1'] = MessageElement(host_name, + FLAG_MOD_REPLACE, + 'dNSHostName') + try: + self.ldb_user.modify(m) + except LdbError as err: + num, estr = err.args + self.assertEqual(ERR_CONSTRAINT_VIOLATION, num) + else: + self.fail() + + def test_modify_dns_host_name_spn_matching_account_name_original(self): + '''Test modifying dNSHostName and SPN with validated write, matching the original sAMAccountName''' + + ou_dn = f'OU=test_modify_ou1,{self.base_dn}' + + account_name = 'test_mod_hostname' + dn = f'CN={account_name},{ou_dn}' + + self.ldb_admin.create_ou(ou_dn) + + sam_account_name = '3e0abfd0-126a-11d0-a060-00aa006c33ed' + + # Grant Write Property. + mod = (f'(OA;CI;WP;{sam_account_name};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + # Grant Validated Write. + mod = (f'(OA;CI;SW;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + mod = (f'(OA;CI;SW;{security.GUID_DRS_VALIDATE_SPN};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + + # Create the account. + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'computer', + 'sAMAccountName': f'{account_name}$', + }) + + new_account_name = 'test_mod_hostname2' + host_name = f'{account_name}.{self.ldb_user.domain_dns_name()}' + spn = f'host/{host_name}' + + m = Message(Dn(self.ldb_user, dn)) + m['0'] = MessageElement(host_name, + FLAG_MOD_REPLACE, + 'dNSHostName') + m['1'] = MessageElement(spn, + FLAG_MOD_ADD, + 'servicePrincipalName') + m['2'] = MessageElement(f'{new_account_name}$', + FLAG_MOD_REPLACE, + 'sAMAccountName') + try: + self.ldb_user.modify(m) + except LdbError as err: + num, estr = err.args + self.assertEqual(ERR_CONSTRAINT_VIOLATION, num) + else: + self.fail() + + def test_modify_dns_host_name_spn_matching_account_name_new(self): + '''Test modifying dNSHostName and SPN with validated write, matching the new sAMAccountName''' + + ou_dn = f'OU=test_modify_ou1,{self.base_dn}' + + account_name = 'test_mod_hostname' + dn = f'CN={account_name},{ou_dn}' + + self.ldb_admin.create_ou(ou_dn) + + sam_account_name = '3e0abfd0-126a-11d0-a060-00aa006c33ed' + + # Grant Write Property. + mod = (f'(OA;CI;WP;{sam_account_name};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + # Grant Validated Write. + mod = (f'(OA;CI;SW;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + mod = (f'(OA;CI;SW;{security.GUID_DRS_VALIDATE_SPN};;' + f'{self.user_sid})') + self.sd_utils.dacl_add_ace(ou_dn, mod) + + # Create the account. + self.ldb_admin.add({ + 'dn': dn, + 'objectClass': 'computer', + 'sAMAccountName': f'{account_name}$', + }) + + new_account_name = 'test_mod_hostname2' + new_host_name = f'{new_account_name}.{self.ldb_user.domain_dns_name()}' + new_spn = f'host/{new_host_name}' + + m = Message(Dn(self.ldb_user, dn)) + m['0'] = MessageElement(new_spn, + FLAG_MOD_ADD, + 'servicePrincipalName') + m['1'] = MessageElement(new_host_name, + FLAG_MOD_REPLACE, + 'dNSHostName') + m['2'] = MessageElement(f'{new_account_name}$', + FLAG_MOD_REPLACE, + 'sAMAccountName') + try: + self.ldb_user.modify(m) + except LdbError: + self.fail() + # enable these when we have search implemented -- 2.35.0 From e30a3f75f9f6aeabe6b49546780282dff0887955 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 7 Jun 2022 17:35:35 +1200 Subject: [PATCH 02/14] tests/py_credentials: Add tests for setting dNSHostName with LogonGetDomainInfo() Test that the value is properly validated, and that it can be set regardless of rights on the account. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14833 Signed-off-by: Joseph Sutton --- python/samba/tests/py_credentials.py | 236 +++++++++++++++++++- selftest/knownfail.d/netlogon-dns-host-name | 2 + 2 files changed, 236 insertions(+), 2 deletions(-) create mode 100644 selftest/knownfail.d/netlogon-dns-host-name diff --git a/python/samba/tests/py_credentials.py b/python/samba/tests/py_credentials.py index ecb8271b595..f106eb70c55 100644 --- a/python/samba/tests/py_credentials.py +++ b/python/samba/tests/py_credentials.py @@ -18,6 +18,8 @@ from samba.tests import TestCase, delete_force import os +import ldb + import samba from samba.auth import system_session from samba.credentials import ( @@ -25,7 +27,7 @@ from samba.credentials import ( CLI_CRED_NTLMv2_AUTH, CLI_CRED_NTLM_AUTH, DONT_USE_KERBEROS) -from samba.dcerpc import netlogon, ntlmssp, srvsvc +from samba.dcerpc import lsa, netlogon, ntlmssp, security, srvsvc from samba.dcerpc.netlogon import ( netr_Authenticator, netr_WorkstationInformation, @@ -36,10 +38,11 @@ from samba.dsdb import ( UF_WORKSTATION_TRUST_ACCOUNT, UF_PASSWD_NOTREQD, UF_NORMAL_ACCOUNT) -from samba.ndr import ndr_pack +from samba.ndr import ndr_pack, ndr_unpack from samba.samdb import SamDB from samba import NTSTATUSError, ntstatus from samba.common import get_string +from samba.sd_utils import SDUtils import ctypes @@ -105,6 +108,235 @@ class PyCredentialsTests(TestCase): (authenticator, subsequent) = self.get_authenticator(c) self.do_NetrLogonGetDomainInfo(c, authenticator, subsequent) + # Test using LogonGetDomainInfo to update dNSHostName to an allowed value. + def test_set_dns_hostname_valid(self): + c = self.get_netlogon_connection() + authenticator, subsequent = self.get_authenticator(c) + + domain_hostname = self.ldb.domain_dns_name() + + new_dns_hostname = f'{self.machine_name}.{domain_hostname}' + new_dns_hostname = new_dns_hostname.encode('utf-8') + + query = netr_WorkstationInformation() + query.os_name = lsa.String('some OS') + query.dns_hostname = new_dns_hostname + + c.netr_LogonGetDomainInfo( + server_name=self.server, + computer_name=self.user_creds.get_workstation(), + credential=authenticator, + return_authenticator=subsequent, + level=1, + query=query) + + # Check the result. + + res = self.ldb.search(self.machine_dn, + scope=ldb.SCOPE_BASE, + attrs=['dNSHostName']) + self.assertEqual(1, len(res)) + + got_dns_hostname = res[0].get('dNSHostName', idx=0) + self.assertEqual(new_dns_hostname, got_dns_hostname) + + # Test using LogonGetDomainInfo to update dNSHostName to an allowed value, + # when we are denied the right to do so. + def test_set_dns_hostname_valid_denied(self): + c = self.get_netlogon_connection() + authenticator, subsequent = self.get_authenticator(c) + + res = self.ldb.search(self.machine_dn, + scope=ldb.SCOPE_BASE, + attrs=['objectSid']) + self.assertEqual(1, len(res)) + + machine_sid = ndr_unpack(security.dom_sid, + res[0].get('objectSid', idx=0)) + + sd_utils = SDUtils(self.ldb) + + # Deny Validated Write and Write Property. + mod = (f'(OD;;SWWP;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{machine_sid})') + sd_utils.dacl_add_ace(self.machine_dn, mod) + + domain_hostname = self.ldb.domain_dns_name() + + new_dns_hostname = f'{self.machine_name}.{domain_hostname}' + new_dns_hostname = new_dns_hostname.encode('utf-8') + + query = netr_WorkstationInformation() + query.os_name = lsa.String('some OS') + query.dns_hostname = new_dns_hostname + + c.netr_LogonGetDomainInfo( + server_name=self.server, + computer_name=self.user_creds.get_workstation(), + credential=authenticator, + return_authenticator=subsequent, + level=1, + query=query) + + # Check the result. + + res = self.ldb.search(self.machine_dn, + scope=ldb.SCOPE_BASE, + attrs=['dNSHostName']) + self.assertEqual(1, len(res)) + + got_dns_hostname = res[0].get('dNSHostName', idx=0) + self.assertEqual(new_dns_hostname, got_dns_hostname) + + # Ensure we can't use LogonGetDomainInfo to update dNSHostName to an + # invalid value, even with rights. + def test_set_dns_hostname_invalid(self): + c = self.get_netlogon_connection() + authenticator, subsequent = self.get_authenticator(c) + + res = self.ldb.search(self.machine_dn, + scope=ldb.SCOPE_BASE, + attrs=['objectSid']) + self.assertEqual(1, len(res)) + + machine_sid = ndr_unpack(security.dom_sid, + res[0].get('objectSid', idx=0)) + + sd_utils = SDUtils(self.ldb) + + # Grant Validated Write and Write Property. + mod = (f'(OA;;SWWP;{security.GUID_DRS_DNS_HOST_NAME};;' + f'{machine_sid})') + sd_utils.dacl_add_ace(self.machine_dn, mod) + + new_dns_hostname = b'invalid' + + query = netr_WorkstationInformation() + query.os_name = lsa.String('some OS') + query.dns_hostname = new_dns_hostname + + c.netr_LogonGetDomainInfo( + server_name=self.server, + computer_name=self.user_creds.get_workstation(), + credential=authenticator, + return_authenticator=subsequent, + level=1, + query=query) + + # Check the result. + + res = self.ldb.search(self.machine_dn, + scope=ldb.SCOPE_BASE, + attrs=['dNSHostName']) + self.assertEqual(1, len(res)) + + got_dns_hostname = res[0].get('dNSHostName', idx=0) + self.assertIsNone(got_dns_hostname) + + # Show we can't use LogonGetDomainInfo to set the dNSHostName to just the + # machine name. + def test_set_dns_hostname_to_machine_name(self): + c = self.get_netlogon_connection() + authenticator, subsequent = self.get_authenticator(c) + + new_dns_hostname = self.machine_name.encode('utf-8') + + query = netr_WorkstationInformation() + query.os_name = lsa.String('some OS') + query.dns_hostname = new_dns_hostname + + c.netr_LogonGetDomainInfo( + server_name=self.server, + computer_name=self.user_creds.get_workstation(), + credential=authenticator, + return_authenticator=subsequent, + level=1, + query=query) + + # Check the result. + + res = self.ldb.search(self.machine_dn, + scope=ldb.SCOPE_BASE, + attrs=['dNSHostName']) + self.assertEqual(1, len(res)) + + got_dns_hostname = res[0].get('dNSHostName', idx=0) + self.assertIsNone(got_dns_hostname) + + # Show we can't use LogonGetDomainInfo to set dNSHostName with an invalid + # suffix. + def test_set_dns_hostname_invalid_suffix(self): + c = self.get_netlogon_connection() + authenticator, subsequent = self.get_authenticator(c) + + domain_hostname = self.ldb.domain_dns_name() + + new_dns_hostname = f'{self.machine_name}.foo.{domain_hostname}' + new_dns_hostname = new_dns_hostname.encode('utf-8') + + query = netr_WorkstationInformation() + query.os_name = lsa.String('some OS') + query.dns_hostname = new_dns_hostname + + c.netr_LogonGetDomainInfo( + server_name=self.server, + computer_name=self.user_creds.get_workstation(), + credential=authenticator, + return_authenticator=subsequent, + level=1, + query=query) + + # Check the result. + + res = self.ldb.search(self.machine_dn, + scope=ldb.SCOPE_BASE, + attrs=['dNSHostName']) + self.assertEqual(1, len(res)) + + got_dns_hostname = res[0].get('dNSHostName', idx=0) + self.assertIsNone(got_dns_hostname) + + # Test that setting the HANDLES_SPN_UPDATE flag inhibits the dNSHostName + # update, but other attributes are still updated. + def test_set_dns_hostname_with_flag(self): + c = self.get_netlogon_connection() + authenticator, subsequent = self.get_authenticator(c) + + domain_hostname = self.ldb.domain_dns_name() + + new_dns_hostname = f'{self.machine_name}.{domain_hostname}' + new_dns_hostname = new_dns_hostname.encode('utf-8') + + operating_system = 'some OS' + + query = netr_WorkstationInformation() + query.os_name = lsa.String(operating_system) + + query.dns_hostname = new_dns_hostname + query.workstation_flags = netlogon.NETR_WS_FLAG_HANDLES_SPN_UPDATE + + c.netr_LogonGetDomainInfo( + server_name=self.server, + computer_name=self.user_creds.get_workstation(), + credential=authenticator, + return_authenticator=subsequent, + level=1, + query=query) + + # Check the result. + + res = self.ldb.search(self.machine_dn, + scope=ldb.SCOPE_BASE, + attrs=['dNSHostName', + 'operatingSystem']) + self.assertEqual(1, len(res)) + + got_dns_hostname = res[0].get('dNSHostName', idx=0) + self.assertIsNone(got_dns_hostname) + + got_os = res[0].get('operatingSystem', idx=0) + self.assertEqual(operating_system.encode('utf-8'), got_os) + def test_SamLogonEx(self): c = self.get_netlogon_connection() diff --git a/selftest/knownfail.d/netlogon-dns-host-name b/selftest/knownfail.d/netlogon-dns-host-name new file mode 100644 index 00000000000..2d0a0ec570a --- /dev/null +++ b/selftest/knownfail.d/netlogon-dns-host-name @@ -0,0 +1,2 @@ +^samba.tests.py_credentials.samba.tests.py_credentials.PyCredentialsTests.test_set_dns_hostname_invalid_suffix\( +^samba.tests.py_credentials.samba.tests.py_credentials.PyCredentialsTests.test_set_dns_hostname_with_flag\( -- 2.35.0 From e40b582f962c738b944a1289be9a030e746598d3 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Wed, 1 Jun 2022 16:08:42 +1200 Subject: [PATCH 03/14] dsdb: Implement validated dNSHostName write BUG: https://bugzilla.samba.org/show_bug.cgi?id=14833 Signed-off-by: Joseph Sutton --- selftest/knownfail.d/validated-dns-host-name | 12 - source4/dsdb/samdb/ldb_modules/acl.c | 235 +++++++++++++++++++ 2 files changed, 235 insertions(+), 12 deletions(-) diff --git a/selftest/knownfail.d/validated-dns-host-name b/selftest/knownfail.d/validated-dns-host-name index ee51f440969..4b6165833e2 100644 --- a/selftest/knownfail.d/validated-dns-host-name +++ b/selftest/knownfail.d/validated-dns-host-name @@ -1,15 +1,3 @@ -^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name\( -^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_account_no_dollar\( -^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_allowed_suffixes\( -^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_case\( -^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_dollar\( -^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_empty_string\( -^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_invalid\( -^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_no_suffix\( -^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_no_value\( ^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_spn\( ^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_spn_matching_account_name_new\( -^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_spn_matching_account_name_original\( -^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_wrong_prefix\( -^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_wrong_suffix\( ^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_spn_matching_dns_host_name_invalid\( diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c index 289c885cdb1..d94dc10659d 100644 --- a/source4/dsdb/samdb/ldb_modules/acl.c +++ b/source4/dsdb/samdb/ldb_modules/acl.c @@ -801,6 +801,229 @@ static int acl_check_spn(TALLOC_CTX *mem_ctx, return LDB_SUCCESS; } +static int acl_check_dns_host_name(TALLOC_CTX *mem_ctx, + struct ldb_module *module, + struct ldb_request *req, + const struct ldb_message_element *el, + struct security_descriptor *sd, + struct dom_sid *sid, + const struct dsdb_attribute *attr, + const struct dsdb_class *objectclass) +{ + int ret; + unsigned i; + TALLOC_CTX *tmp_ctx = NULL; + struct ldb_context *ldb = ldb_module_get_ctx(module); + const struct ldb_message_element *oc_el = NULL; + const struct ldb_message_element *allowed_suffixes = NULL; + struct ldb_result *nc_res = NULL; + struct ldb_dn *nc_root = NULL; + const char *nc_dns_name = NULL; + const char *dns_host_name_value = NULL; + const char *dns_host_name = NULL; + const char *samAccountName = NULL; + size_t account_name_len; + const struct ldb_message *search_res = NULL; + + static const char *nc_attrs[] = { + "msDS-AllowedDNSSuffixes", + NULL + }; + + if (el->num_values == 0) { + return LDB_SUCCESS; + } + dns_host_name_value = (const char *)el->values[0].data; + + tmp_ctx = talloc_new(mem_ctx); + if (tmp_ctx == NULL) { + return ldb_oom(ldb); + } + + /* if we have wp, we can do whatever we like */ + ret = acl_check_access_on_attribute(module, + tmp_ctx, + sd, + sid, + SEC_ADS_WRITE_PROP, + attr, objectclass); + if (ret == LDB_SUCCESS) { + talloc_free(tmp_ctx); + return LDB_SUCCESS; + } + + ret = acl_check_extended_right(tmp_ctx, + module, + req, + objectclass, + sd, + acl_user_token(module), + GUID_DRS_DNS_HOST_NAME, + SEC_ADS_SELF_WRITE, + sid); + + if (ret != LDB_SUCCESS) { + dsdb_acl_debug(sd, acl_user_token(module), + req->op.mod.message->dn, + true, + 10); + talloc_free(tmp_ctx); + return ret; + } + + /* + * If we have "validated write dnshostname", allow delete of + * any existing value (this keeps constrained delete to the + * same rules as unconstrained) + */ + if (req->operation == LDB_MODIFY) { + struct ldb_result *acl_res = NULL; + + static const char *acl_attrs[] = { + "sAMAccountName", + "objectClass", + NULL + }; + + /* + * If not add or replace (eg delete), + * return success + */ + if ((el->flags + & (LDB_FLAG_MOD_ADD|LDB_FLAG_MOD_REPLACE)) == 0) + { + talloc_free(tmp_ctx); + return LDB_SUCCESS; + } + + ret = dsdb_module_search_dn(module, tmp_ctx, + &acl_res, req->op.mod.message->dn, + acl_attrs, + DSDB_FLAG_NEXT_MODULE | + DSDB_FLAG_AS_SYSTEM | + DSDB_SEARCH_SHOW_RECYCLED, + req); + if (ret != LDB_SUCCESS) { + talloc_free(tmp_ctx); + return ret; + } + + search_res = acl_res->msgs[0]; + } else if (req->operation == LDB_ADD) { + search_res = req->op.add.message; + } else { + talloc_free(tmp_ctx); + return LDB_ERR_OPERATIONS_ERROR; + } + + /* Check if the account has objectclass 'computer' or 'server'. */ + oc_el = samdb_find_attribute(ldb, + search_res, + "objectclass", + "computer"); + if (oc_el == NULL) { + oc_el = samdb_find_attribute(ldb, + search_res, + "objectclass", + "server"); + + if (oc_el == NULL) { + /* Not a computer or server, so no need to validate. */ + talloc_free(tmp_ctx); + return LDB_SUCCESS; + } + } + + samAccountName = ldb_msg_find_attr_as_string(search_res, "sAMAccountName", NULL); + if (samAccountName == NULL) { + talloc_free(tmp_ctx); + return ldb_operr(ldb); + } + account_name_len = strlen(samAccountName); + if (account_name_len && samAccountName[account_name_len - 1] == '$') { + /* Account for the '$' character. */ + --account_name_len; + } + + dns_host_name = dns_host_name_value; + + /* Check that sAMAccountName matches the new dNSHostName. */ + if (strncasecmp(dns_host_name, samAccountName, + account_name_len) != 0) + { + goto fail; + } + + dns_host_name += account_name_len; + + /* Check the '.' character */ + if (*dns_host_name++ != '.') { + goto fail; + } + + /* Now we check the suffix. */ + + ret = dsdb_find_nc_root(ldb, + tmp_ctx, + search_res->dn, + &nc_root); + if (ret != LDB_SUCCESS) { + talloc_free(tmp_ctx); + return ret; + } + + nc_dns_name = samdb_dn_to_dns_domain(tmp_ctx, nc_root); + if (nc_dns_name == NULL) { + talloc_free(tmp_ctx); + return ldb_operr(ldb); + } + + if (strcasecmp(dns_host_name, nc_dns_name) == 0) { + /* It matches -- success. */ + talloc_free(tmp_ctx); + return LDB_SUCCESS; + } + + /* We didn't get a match, so now try msDS-AllowedDNSSuffixes. */ + + ret = dsdb_module_search_dn(module, tmp_ctx, + &nc_res, nc_root, + nc_attrs, + DSDB_FLAG_NEXT_MODULE | + DSDB_FLAG_AS_SYSTEM | + DSDB_SEARCH_SHOW_RECYCLED, + req); + if (ret != LDB_SUCCESS) { + talloc_free(tmp_ctx); + return ret; + } + + allowed_suffixes = ldb_msg_find_element(nc_res->msgs[0], + "msDS-AllowedDNSSuffixes"); + if (allowed_suffixes == NULL) { + goto fail; + } + + for (i = 0; i < allowed_suffixes->num_values; ++i) { + const char *suffix = ldb_val_as_string(&allowed_suffixes->values[i], + NULL); + + if (suffix != NULL && strcasecmp(dns_host_name, suffix) == 0) { + /* It matches -- success. */ + talloc_free(tmp_ctx); + return LDB_SUCCESS; + } + } + +fail: + talloc_free(tmp_ctx); + ldb_debug_set(ldb, LDB_DEBUG_WARNING, + "acl: hostname validation failed for " + "hostname[%s] account[%s]\n", + dns_host_name_value, samAccountName); + return LDB_ERR_CONSTRAINT_VIOLATION; +} + static int acl_add(struct ldb_module *module, struct ldb_request *req) { int ret; @@ -1535,6 +1758,18 @@ static int acl_modify(struct ldb_module *module, struct ldb_request *req) if (ret != LDB_SUCCESS) { goto fail; } + } else if (ldb_attr_cmp("dnsHostName", el->name) == 0) { + ret = acl_check_dns_host_name(tmp_ctx, + module, + req, + el, + sd, + sid, + attr, + objectclass); + if (ret != LDB_SUCCESS) { + goto fail; + } } else if (is_undelete != NULL && (ldb_attr_cmp("isDeleted", el->name) == 0)) { /* * in case of undelete op permissions on -- 2.35.0 From fa6762ea61f5e7c046a68f62c8fdecdc73130d2b Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 7 Jun 2022 17:36:43 +1200 Subject: [PATCH 04/14] dsdb/common: Add VALIDATED_DNS_HOSTNAME_SPN_WRITE control Passing this control will grant the right to set validated values for dNSHostName and servicePrincipalName, and non-validated values for other attributes. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14833 Signed-off-by: Joseph Sutton --- source4/dsdb/common/util.c | 7 +++++++ source4/dsdb/samdb/ldb_modules/util.h | 1 + source4/dsdb/samdb/samdb.h | 6 ++++++ 3 files changed, 14 insertions(+) diff --git a/source4/dsdb/common/util.c b/source4/dsdb/common/util.c index bd59de5cb32..41b1f13a442 100644 --- a/source4/dsdb/common/util.c +++ b/source4/dsdb/common/util.c @@ -4481,6 +4481,13 @@ int dsdb_request_add_controls(struct ldb_request *req, uint32_t dsdb_flags) } } + if (dsdb_flags & DSDB_FLAG_VALIDATED_DNS_HOSTNAME_SPN_WRITE) { + ret = ldb_request_add_control(req, DSDB_CONTROL_VALIDATED_DNS_HOSTNAME_SPN_WRITE_OID, true, NULL); + if (ret != LDB_SUCCESS) { + return ret; + } + } + return LDB_SUCCESS; } diff --git a/source4/dsdb/samdb/ldb_modules/util.h b/source4/dsdb/samdb/ldb_modules/util.h index 5ecf0eee0d2..f3ac3bf5f5a 100644 --- a/source4/dsdb/samdb/ldb_modules/util.h +++ b/source4/dsdb/samdb/ldb_modules/util.h @@ -39,3 +39,4 @@ struct netlogon_samlogon_response; #define DSDB_FLAG_TOP_MODULE 0x00800000 #define DSDB_FLAG_TRUSTED 0x01000000 #define DSDB_FLAG_REPLICATED_UPDATE 0x02000000 +#define DSDB_FLAG_VALIDATED_DNS_HOSTNAME_SPN_WRITE 0x04000000 diff --git a/source4/dsdb/samdb/samdb.h b/source4/dsdb/samdb/samdb.h index 286c97f2ea5..6e3baa54495 100644 --- a/source4/dsdb/samdb/samdb.h +++ b/source4/dsdb/samdb/samdb.h @@ -226,6 +226,12 @@ struct dsdb_control_transaction_identifier { struct GUID transaction_guid; }; +/* + * passed when we want to allow validated writes to dNSHostName and + * servicePrincipalName. + */ +#define DSDB_CONTROL_VALIDATED_DNS_HOSTNAME_SPN_WRITE_OID "1.3.6.1.4.1.7165.4.3.35" + #define DSDB_EXTENDED_REPLICATED_OBJECTS_OID "1.3.6.1.4.1.7165.4.4.1" struct dsdb_extended_replicated_object { struct ldb_message *msg; -- 2.35.0 From 504620f3a26a6a84411aa2f343fadb643474c737 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 7 Jun 2022 17:39:07 +1200 Subject: [PATCH 05/14] dsdb/modules/acl: Handle VALIDATED_DNS_HOSTNAME_SPN_WRITE control When this control is specified, we'll assume we have Validated Write on dNSHostName and servicePrincipalName, and Write Property on other attributes. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14833 Signed-off-by: Joseph Sutton --- source4/dsdb/samdb/ldb_modules/acl.c | 131 +++++++++++++++------------ 1 file changed, 74 insertions(+), 57 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c index d94dc10659d..a97e1057d65 100644 --- a/source4/dsdb/samdb/ldb_modules/acl.c +++ b/source4/dsdb/samdb/ldb_modules/acl.c @@ -667,7 +667,8 @@ static int acl_check_spn(TALLOC_CTX *mem_ctx, struct security_descriptor *sd, struct dom_sid *sid, const struct dsdb_attribute *attr, - const struct dsdb_class *objectclass) + const struct dsdb_class *objectclass, + bool have_validated_write) { int ret; unsigned int i; @@ -694,34 +695,36 @@ static int acl_check_spn(TALLOC_CTX *mem_ctx, NULL }; - /* if we have wp, we can do whatever we like */ - if (acl_check_access_on_attribute(module, - tmp_ctx, - sd, - sid, - SEC_ADS_WRITE_PROP, - attr, objectclass) == LDB_SUCCESS) { - talloc_free(tmp_ctx); - return LDB_SUCCESS; - } + if (!have_validated_write) { + /* if we have wp, we can do whatever we like */ + if (acl_check_access_on_attribute(module, + tmp_ctx, + sd, + sid, + SEC_ADS_WRITE_PROP, + attr, objectclass) == LDB_SUCCESS) { + talloc_free(tmp_ctx); + return LDB_SUCCESS; + } - ret = acl_check_extended_right(tmp_ctx, - module, - req, - objectclass, - sd, - acl_user_token(module), - GUID_DRS_VALIDATE_SPN, - SEC_ADS_SELF_WRITE, - sid); + ret = acl_check_extended_right(tmp_ctx, + module, + req, + objectclass, + sd, + acl_user_token(module), + GUID_DRS_VALIDATE_SPN, + SEC_ADS_SELF_WRITE, + sid); - if (ret != LDB_SUCCESS) { - dsdb_acl_debug(sd, acl_user_token(module), - req->op.mod.message->dn, - true, - 10); - talloc_free(tmp_ctx); - return ret; + if (ret != LDB_SUCCESS) { + dsdb_acl_debug(sd, acl_user_token(module), + req->op.mod.message->dn, + true, + 10); + talloc_free(tmp_ctx); + return ret; + } } /* @@ -808,7 +811,8 @@ static int acl_check_dns_host_name(TALLOC_CTX *mem_ctx, struct security_descriptor *sd, struct dom_sid *sid, const struct dsdb_attribute *attr, - const struct dsdb_class *objectclass) + const struct dsdb_class *objectclass, + bool have_validated_write) { int ret; unsigned i; @@ -840,35 +844,37 @@ static int acl_check_dns_host_name(TALLOC_CTX *mem_ctx, return ldb_oom(ldb); } - /* if we have wp, we can do whatever we like */ - ret = acl_check_access_on_attribute(module, - tmp_ctx, - sd, - sid, - SEC_ADS_WRITE_PROP, - attr, objectclass); - if (ret == LDB_SUCCESS) { - talloc_free(tmp_ctx); - return LDB_SUCCESS; - } + if (!have_validated_write) { + /* if we have wp, we can do whatever we like */ + ret = acl_check_access_on_attribute(module, + tmp_ctx, + sd, + sid, + SEC_ADS_WRITE_PROP, + attr, objectclass); + if (ret == LDB_SUCCESS) { + talloc_free(tmp_ctx); + return LDB_SUCCESS; + } - ret = acl_check_extended_right(tmp_ctx, - module, - req, - objectclass, - sd, - acl_user_token(module), - GUID_DRS_DNS_HOST_NAME, - SEC_ADS_SELF_WRITE, - sid); + ret = acl_check_extended_right(tmp_ctx, + module, + req, + objectclass, + sd, + acl_user_token(module), + GUID_DRS_DNS_HOST_NAME, + SEC_ADS_SELF_WRITE, + sid); - if (ret != LDB_SUCCESS) { - dsdb_acl_debug(sd, acl_user_token(module), - req->op.mod.message->dn, - true, - 10); - talloc_free(tmp_ctx); - return ret; + if (ret != LDB_SUCCESS) { + dsdb_acl_debug(sd, acl_user_token(module), + req->op.mod.message->dn, + true, + 10); + talloc_free(tmp_ctx); + return ret; + } } /* @@ -1572,6 +1578,7 @@ static int acl_modify(struct ldb_module *module, struct ldb_request *req) struct dom_sid *sid = NULL; struct ldb_control *as_system; struct ldb_control *is_undelete; + struct ldb_control *validated_write = NULL; bool userPassword; bool password_rights_checked = false; TALLOC_CTX *tmp_ctx; @@ -1598,6 +1605,11 @@ static int acl_modify(struct ldb_module *module, struct ldb_request *req) is_undelete = ldb_request_get_control(req, DSDB_CONTROL_RESTORE_TOMBSTONE_OID); + validated_write = ldb_request_get_control(req, DSDB_CONTROL_VALIDATED_DNS_HOSTNAME_SPN_WRITE_OID); + if (validated_write != NULL) { + validated_write->critical = 0; + } + /* Don't print this debug statement if elements[0].name is going to be NULL */ if (msg->num_elements > 0) { DEBUG(10, ("ldb:acl_modify: %s\n", msg->elements[0].name)); @@ -1754,7 +1766,8 @@ static int acl_modify(struct ldb_module *module, struct ldb_request *req) sd, sid, attr, - objectclass); + objectclass, + validated_write != NULL); if (ret != LDB_SUCCESS) { goto fail; } @@ -1766,7 +1779,8 @@ static int acl_modify(struct ldb_module *module, struct ldb_request *req) sd, sid, attr, - objectclass); + objectclass, + validated_write != NULL); if (ret != LDB_SUCCESS) { goto fail; } @@ -1778,6 +1792,9 @@ static int acl_modify(struct ldb_module *module, struct ldb_request *req) * tombstone_reanimate module */ continue; + } else if (validated_write != NULL) { + /* Allow the update. */ + continue; } else { ret = acl_check_access_on_attribute(module, tmp_ctx, -- 2.35.0 From 72fc12e55a2ff8c807edeaf95976d6fc46617882 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Thu, 2 Jun 2022 17:11:08 +1200 Subject: [PATCH 06/14] s4:rpc_server/netlogon: Remove dNSHostName prefix check This check is not exhaustive (it does not check the suffix of the dNSHostName), and should be covered by a validated write check in acl_modify(). BUG: https://bugzilla.samba.org/show_bug.cgi?id=14833 Signed-off-by: Joseph Sutton --- selftest/knownfail.d/netlogon-dns-host-name | 1 + source4/rpc_server/netlogon/dcerpc_netlogon.c | 21 ++----------------- 2 files changed, 3 insertions(+), 19 deletions(-) diff --git a/selftest/knownfail.d/netlogon-dns-host-name b/selftest/knownfail.d/netlogon-dns-host-name index 2d0a0ec570a..50bb15f4b28 100644 --- a/selftest/knownfail.d/netlogon-dns-host-name +++ b/selftest/knownfail.d/netlogon-dns-host-name @@ -1,2 +1,3 @@ +^samba.tests.py_credentials.samba.tests.py_credentials.PyCredentialsTests.test_set_dns_hostname_invalid\( ^samba.tests.py_credentials.samba.tests.py_credentials.PyCredentialsTests.test_set_dns_hostname_invalid_suffix\( ^samba.tests.py_credentials.samba.tests.py_credentials.PyCredentialsTests.test_set_dns_hostname_with_flag\( diff --git a/source4/rpc_server/netlogon/dcerpc_netlogon.c b/source4/rpc_server/netlogon/dcerpc_netlogon.c index 062a92597ce..6db5d854cdb 100644 --- a/source4/rpc_server/netlogon/dcerpc_netlogon.c +++ b/source4/rpc_server/netlogon/dcerpc_netlogon.c @@ -2413,7 +2413,7 @@ static NTSTATUS dcesrv_netr_LogonGetDomainInfo(struct dcesrv_call_state *dce_cal }; const char * const attrs2[] = { "sAMAccountName", "dNSHostName", "msDS-SupportedEncryptionTypes", NULL }; - const char *sam_account_name, *old_dns_hostname, *prefix1, *prefix2; + const char *sam_account_name, *old_dns_hostname; struct ldb_context *sam_ctx; const struct GUID *our_domain_guid = NULL; struct lsa_TrustDomainInfoInfoEx *our_tdo = NULL; @@ -2483,24 +2483,7 @@ static NTSTATUS dcesrv_netr_LogonGetDomainInfo(struct dcesrv_call_state *dce_cal return NT_STATUS_INTERNAL_DB_CORRUPTION; } - /* - * Checks that the sam account name without a possible "$" - * matches as prefix with the DNS hostname in the workstation - * info structure. - */ - prefix1 = talloc_strndup(mem_ctx, sam_account_name, - strcspn(sam_account_name, "$")); - NT_STATUS_HAVE_NO_MEMORY(prefix1); - if (r->in.query->workstation_info->dns_hostname != NULL) { - prefix2 = talloc_strndup(mem_ctx, - r->in.query->workstation_info->dns_hostname, - strcspn(r->in.query->workstation_info->dns_hostname, ".")); - NT_STATUS_HAVE_NO_MEMORY(prefix2); - - if (strcasecmp(prefix1, prefix2) != 0) { - update_dns_hostname = false; - } - } else { + if (r->in.query->workstation_info->dns_hostname == NULL) { update_dns_hostname = false; } -- 2.35.0 From f614631880f9a37c709d33ff9d765a1f6b740543 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 7 Jun 2022 17:25:28 +1200 Subject: [PATCH 07/14] s4:rpc_server/netlogon: Always observe NETR_WS_FLAG_HANDLES_SPN_UPDATE flag Even when there is no old DNS hostname present. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14833 Signed-off-by: Joseph Sutton --- selftest/knownfail.d/netlogon-dns-host-name | 1 - source4/rpc_server/netlogon/dcerpc_netlogon.c | 7 ++----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/selftest/knownfail.d/netlogon-dns-host-name b/selftest/knownfail.d/netlogon-dns-host-name index 50bb15f4b28..360e45e0692 100644 --- a/selftest/knownfail.d/netlogon-dns-host-name +++ b/selftest/knownfail.d/netlogon-dns-host-name @@ -1,3 +1,2 @@ ^samba.tests.py_credentials.samba.tests.py_credentials.PyCredentialsTests.test_set_dns_hostname_invalid\( ^samba.tests.py_credentials.samba.tests.py_credentials.PyCredentialsTests.test_set_dns_hostname_invalid_suffix\( -^samba.tests.py_credentials.samba.tests.py_credentials.PyCredentialsTests.test_set_dns_hostname_with_flag\( diff --git a/source4/rpc_server/netlogon/dcerpc_netlogon.c b/source4/rpc_server/netlogon/dcerpc_netlogon.c index 6db5d854cdb..6c6aab6e0e6 100644 --- a/source4/rpc_server/netlogon/dcerpc_netlogon.c +++ b/source4/rpc_server/netlogon/dcerpc_netlogon.c @@ -2495,13 +2495,10 @@ static NTSTATUS dcesrv_netr_LogonGetDomainInfo(struct dcesrv_call_state *dce_cal /* * Updates the DNS hostname when the client wishes that the * server should handle this for him - * ("NETR_WS_FLAG_HANDLES_SPN_UPDATE" not set). And this is - * obviously only checked when we do already have a - * "dNSHostName". + * ("NETR_WS_FLAG_HANDLES_SPN_UPDATE" not set). * See MS-NRPC section 3.5.4.3.9 */ - if ((old_dns_hostname != NULL) && - (r->in.query->workstation_info->workstation_flags + if ((r->in.query->workstation_info->workstation_flags & NETR_WS_FLAG_HANDLES_SPN_UPDATE) != 0) { update_dns_hostname = false; } -- 2.35.0 From a09e2fe7a7613a6fae6666d1efc7ed65f03aef2f Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 7 Jun 2022 17:29:02 +1200 Subject: [PATCH 08/14] s4:rpc_server/netlogon: Connect to samdb as a user, rather than as system This allows us to perform validation on a client-specified dNSHostName value, to ensure that it matches the sAMAccountName. We might not have any rights to modify the account, so pass the control VALIDATED_DNS_HOSTNAME_SPN_WRITE which allows us to perform a validated write to dNSHostName and servicePrincipalName (and unvalidated writes to other attributes, such as operatingSystem). BUG: https://bugzilla.samba.org/show_bug.cgi?id=14833 Signed-off-by: Joseph Sutton --- selftest/knownfail.d/netlogon-dns-host-name | 4 ++-- source4/rpc_server/netlogon/dcerpc_netlogon.c | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/selftest/knownfail.d/netlogon-dns-host-name b/selftest/knownfail.d/netlogon-dns-host-name index 360e45e0692..3eca0cd3f75 100644 --- a/selftest/knownfail.d/netlogon-dns-host-name +++ b/selftest/knownfail.d/netlogon-dns-host-name @@ -1,2 +1,2 @@ -^samba.tests.py_credentials.samba.tests.py_credentials.PyCredentialsTests.test_set_dns_hostname_invalid\( -^samba.tests.py_credentials.samba.tests.py_credentials.PyCredentialsTests.test_set_dns_hostname_invalid_suffix\( +^samba.tests.py_credentials.samba.tests.py_credentials.PyCredentialsTests.test_set_dns_hostname_valid\( +^samba.tests.py_credentials.samba.tests.py_credentials.PyCredentialsTests.test_set_dns_hostname_valid_denied\( diff --git a/source4/rpc_server/netlogon/dcerpc_netlogon.c b/source4/rpc_server/netlogon/dcerpc_netlogon.c index 6c6aab6e0e6..189b6d94424 100644 --- a/source4/rpc_server/netlogon/dcerpc_netlogon.c +++ b/source4/rpc_server/netlogon/dcerpc_netlogon.c @@ -2450,7 +2450,8 @@ static NTSTATUS dcesrv_netr_LogonGetDomainInfo(struct dcesrv_call_state *dce_cal } NT_STATUS_NOT_OK_RETURN(status); - sam_ctx = dcesrv_samdb_connect_as_system(mem_ctx, dce_call); + /* We want to avoid connecting as system. */ + sam_ctx = dcesrv_samdb_connect_as_user(mem_ctx, dce_call); if (sam_ctx == NULL) { return NT_STATUS_INVALID_SYSTEM_SERVICE; } @@ -2607,7 +2608,7 @@ static NTSTATUS dcesrv_netr_LogonGetDomainInfo(struct dcesrv_call_state *dce_cal } } - if (dsdb_replace(sam_ctx, new_msg, 0) != LDB_SUCCESS) { + if (dsdb_replace(sam_ctx, new_msg, DSDB_FLAG_VALIDATED_DNS_HOSTNAME_SPN_WRITE) != LDB_SUCCESS) { DEBUG(3,("Impossible to update samdb: %s\n", ldb_errstring(sam_ctx))); } -- 2.35.0 From b2f9bc9afd25e016626d167520579d264b65b035 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 7 Jun 2022 17:36:18 +1200 Subject: [PATCH 09/14] ldb: Add ldb_val_as_string() This converts an ldb_val to a string, using the same logic as ldb_msg_find_attr_as_string(). BUG: https://bugzilla.samba.org/show_bug.cgi?id=14833 Signed-off-by: Joseph Sutton --- lib/ldb/common/ldb_msg.c | 15 +++++++++++---- lib/ldb/include/ldb.h | 3 +++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/lib/ldb/common/ldb_msg.c b/lib/ldb/common/ldb_msg.c index 57dfc5a04c2..b988e774079 100644 --- a/lib/ldb/common/ldb_msg.c +++ b/lib/ldb/common/ldb_msg.c @@ -790,11 +790,9 @@ int ldb_msg_find_attr_as_bool(const struct ldb_message *msg, return default_value; } -const char *ldb_msg_find_attr_as_string(const struct ldb_message *msg, - const char *attr_name, - const char *default_value) +const char *ldb_val_as_string(const struct ldb_val *v, + const char *default_value) { - const struct ldb_val *v = ldb_msg_find_ldb_val(msg, attr_name); if (!v || !v->data) { return default_value; } @@ -804,6 +802,15 @@ const char *ldb_msg_find_attr_as_string(const struct ldb_message *msg, return (const char *)v->data; } +const char *ldb_msg_find_attr_as_string(const struct ldb_message *msg, + const char *attr_name, + const char *default_value) +{ + const struct ldb_val *v = ldb_msg_find_ldb_val(msg, attr_name); + + return ldb_val_as_string(v, default_value); +} + struct ldb_dn *ldb_msg_find_attr_as_dn(struct ldb_context *ldb, TALLOC_CTX *mem_ctx, const struct ldb_message *msg, diff --git a/lib/ldb/include/ldb.h b/lib/ldb/include/ldb.h index bc44157eaf4..a7822151546 100644 --- a/lib/ldb/include/ldb.h +++ b/lib/ldb/include/ldb.h @@ -2046,6 +2046,9 @@ struct ldb_dn *ldb_msg_find_attr_as_dn(struct ldb_context *ldb, const struct ldb_message *msg, const char *attr_name); +const char *ldb_val_as_string(const struct ldb_val *v, + const char *default_value); + void ldb_msg_sort_elements(struct ldb_message *msg); struct ldb_message *ldb_msg_copy_shallow(TALLOC_CTX *mem_ctx, -- 2.35.0 From 6f08a94acfbb76fb76b0c27b6d6453215cb34503 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 7 Jun 2022 17:36:56 +1200 Subject: [PATCH 10/14] s4/dsdb/util: Add dsdb_msg_get_single_value() This function simulates an add or modify operation for an ldb message to determine the final value of a particular single-valued attribute. This is useful when validating attributes that should stay in sync with other attributes, such as servicePrincipalName and dNSHostName. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14833 Signed-off-by: Joseph Sutton --- source4/dsdb/samdb/ldb_modules/util.c | 91 +++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/source4/dsdb/samdb/ldb_modules/util.c b/source4/dsdb/samdb/ldb_modules/util.c index 405febf0b3d..ec03b523204 100644 --- a/source4/dsdb/samdb/ldb_modules/util.c +++ b/source4/dsdb/samdb/ldb_modules/util.c @@ -1564,6 +1564,97 @@ int dsdb_get_expected_new_values(TALLOC_CTX *mem_ctx, return LDB_SUCCESS; } +/* + * Get the value of a single-valued attribute after processing a + * message. 'operation' is either LDB_ADD or LDB_MODIFY. 'val' will only live as + * long as 'msg' and 'original_val' do, and must not be freed. + */ +int dsdb_msg_get_single_value(const struct ldb_message *msg, + const char *attr_name, + const struct ldb_val *original_val, + const struct ldb_val **val, + enum ldb_request_type operation) +{ + unsigned idx; + + *val = NULL; + + if (operation == LDB_ADD) { + const struct ldb_message_element *el = NULL; + + /* + * The ldb_msg_normalize() call in ldb_request() ensures that + * there is at most one message element for each + * attribute. Thus, we don't need a loop to deal with an + * LDB_ADD. + */ + el = ldb_msg_find_element(msg, attr_name); + if (el == NULL) { + *val = original_val; + return LDB_SUCCESS; + } + if (el->num_values != 1) { + return LDB_ERR_CONSTRAINT_VIOLATION; + } + + *val = &el->values[0]; + return LDB_SUCCESS; + } + + SMB_ASSERT(operation == LDB_MODIFY); + + *val = original_val; + + for (idx = 0; idx < msg->num_elements; ++idx) { + const struct ldb_message_element *el = &msg->elements[idx]; + + if (ldb_attr_cmp(el->name, attr_name) != 0) { + continue; + } + + switch (el->flags & LDB_FLAG_MOD_MASK) { + case LDB_FLAG_MOD_ADD: + if (el->num_values != 1) { + return LDB_ERR_CONSTRAINT_VIOLATION; + } + if (*val != NULL) { + return LDB_ERR_CONSTRAINT_VIOLATION; + } + + *val = &el->values[0]; + + break; + + case LDB_FLAG_MOD_REPLACE: + if (el->num_values > 1) { + return LDB_ERR_CONSTRAINT_VIOLATION; + } + + *val = el->num_values ? &el->values[0] : NULL; + + break; + + case LDB_FLAG_MOD_DELETE: + if (el->num_values > 1) { + return LDB_ERR_CONSTRAINT_VIOLATION; + } + + /* + * If a value was specified for the delete, we don't + * bother checking it matches the value we currently + * have. Any mismatch will be caught later (e.g. in + * ldb_kv_modify_internal). + */ + + *val = NULL; + + break; + } + } + + return LDB_SUCCESS; +} + /* * This function determines the (last) structural or 88 object class of a passed * "objectClass" attribute - per MS-ADTS 3.1.1.1.4 this is the last value. -- 2.35.0 From 130067b5c3c52ff9d148599d1c12f8df3909028c Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 7 Jun 2022 17:37:34 +1200 Subject: [PATCH 11/14] dsdb/modules/acl: Account for sAMAccountName without $ If we have an account without a trailing $, we should ensure the servicePrincipalName matches the entire sAMAccountName. We should not allow a match against the sAMAccountName prefix of length strlen(samAccountName) - 1, as that could conflict with a different account. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14833 Signed-off-by: Joseph Sutton --- source4/dsdb/samdb/ldb_modules/acl.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c index a97e1057d65..cbf0687c73f 100644 --- a/source4/dsdb/samdb/ldb_modules/acl.c +++ b/source4/dsdb/samdb/ldb_modules/acl.c @@ -543,6 +543,7 @@ static int acl_validate_spn_value(TALLOC_CTX *mem_ctx, char *instanceName; char *serviceType; char *serviceName; + size_t account_name_len; const char *forest_name = samdb_forest_name(ldb, mem_ctx); const char *base_domain = samdb_default_domain_name(ldb, mem_ctx); struct loadparm_context *lp_ctx = talloc_get_type(ldb_get_opaque(ldb, "loadparm"), @@ -616,11 +617,18 @@ static int acl_validate_spn_value(TALLOC_CTX *mem_ctx, } } } + + account_name_len = strlen(samAccountName); + if (account_name_len && samAccountName[account_name_len - 1] == '$') { + /* Account for the '$' character. */ + --account_name_len; + } + /* instanceName can be samAccountName without $ or dnsHostName * or "ntds_guid._msdcs.forest_domain for DC objects */ - if (strlen(instanceName) == (strlen(samAccountName) - 1) + if (strlen(instanceName) == account_name_len && strncasecmp(instanceName, samAccountName, - strlen(samAccountName) - 1) == 0) { + account_name_len) == 0) { goto success; } if ((dnsHostName != NULL) && -- 2.35.0 From fd4102214f5358950c398028235a003fe25358d6 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Tue, 7 Jun 2022 17:38:55 +1200 Subject: [PATCH 12/14] dsdb/modules/acl: Allow simultaneous sAMAccountName, dNSHostName, and servicePrincipalName change If the message changes the sAMAccountName, we'll check dNSHostName and servicePrincipalName values against the new value of sAMAccountName, rather than the account's current value. Similarly, if the message changes the dNSHostName, we'll check servicePrincipalName values against the new dNSHostName. This allows setting more than one of these attributes simultaneously with validated write rights. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14833 Signed-off-by: Joseph Sutton --- selftest/knownfail.d/netlogon-dns-host-name | 2 - selftest/knownfail.d/validated-dns-host-name | 3 - source4/dsdb/samdb/ldb_modules/acl.c | 74 +++++++++++++++++--- 3 files changed, 66 insertions(+), 13 deletions(-) delete mode 100644 selftest/knownfail.d/netlogon-dns-host-name delete mode 100644 selftest/knownfail.d/validated-dns-host-name diff --git a/selftest/knownfail.d/netlogon-dns-host-name b/selftest/knownfail.d/netlogon-dns-host-name deleted file mode 100644 index 3eca0cd3f75..00000000000 --- a/selftest/knownfail.d/netlogon-dns-host-name +++ /dev/null @@ -1,2 +0,0 @@ -^samba.tests.py_credentials.samba.tests.py_credentials.PyCredentialsTests.test_set_dns_hostname_valid\( -^samba.tests.py_credentials.samba.tests.py_credentials.PyCredentialsTests.test_set_dns_hostname_valid_denied\( diff --git a/selftest/knownfail.d/validated-dns-host-name b/selftest/knownfail.d/validated-dns-host-name deleted file mode 100644 index 4b6165833e2..00000000000 --- a/selftest/knownfail.d/validated-dns-host-name +++ /dev/null @@ -1,3 +0,0 @@ -^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_spn\( -^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_dns_host_name_spn_matching_account_name_new\( -^samba4.ldap.acl.python.*__main__.AclModifyTests.test_modify_spn_matching_dns_host_name_invalid\( diff --git a/source4/dsdb/samdb/ldb_modules/acl.c b/source4/dsdb/samdb/ldb_modules/acl.c index cbf0687c73f..150a2165785 100644 --- a/source4/dsdb/samdb/ldb_modules/acl.c +++ b/source4/dsdb/samdb/ldb_modules/acl.c @@ -686,9 +686,11 @@ static int acl_check_spn(TALLOC_CTX *mem_ctx, struct ldb_result *netbios_res; struct ldb_dn *partitions_dn = samdb_partitions_dn(ldb, tmp_ctx); uint32_t userAccountControl; - const char *samAccountName; - const char *dnsHostName; + const char *samAccountName = NULL; + const char *dnsHostName = NULL; const char *netbios_name; + const struct ldb_val *dns_host_name_val = NULL; + const struct ldb_val *sam_account_name_val = NULL; struct GUID ntds; char *ntds_guid = NULL; @@ -764,9 +766,44 @@ static int acl_check_spn(TALLOC_CTX *mem_ctx, return ret; } + dns_host_name_val = ldb_msg_find_ldb_val(acl_res->msgs[0], "dNSHostName"); + + ret = dsdb_msg_get_single_value(req->op.mod.message, + "dNSHostName", + dns_host_name_val, + &dns_host_name_val, + req->operation); + if (ret != LDB_SUCCESS) { + talloc_free(tmp_ctx); + return ret; + } + + if (dns_host_name_val != NULL) { + dnsHostName = ldb_val_as_string(dns_host_name_val, NULL); + } + userAccountControl = ldb_msg_find_attr_as_uint(acl_res->msgs[0], "userAccountControl", 0); - dnsHostName = ldb_msg_find_attr_as_string(acl_res->msgs[0], "dnsHostName", NULL); - samAccountName = ldb_msg_find_attr_as_string(acl_res->msgs[0], "samAccountName", NULL); + + sam_account_name_val = ldb_msg_find_ldb_val(acl_res->msgs[0], "sAMAccountName"); + + ret = dsdb_msg_get_single_value(req->op.mod.message, + "sAMAccountName", + sam_account_name_val, + &sam_account_name_val, + req->operation); + if (ret != LDB_SUCCESS) { + talloc_free(tmp_ctx); + return ret; + } + + if (sam_account_name_val != NULL) { + samAccountName = ldb_val_as_string(sam_account_name_val, NULL); + } + + if (samAccountName == NULL) { + talloc_free(tmp_ctx); + return LDB_ERR_CONSTRAINT_VIOLATION; + } ret = dsdb_module_search(module, tmp_ctx, &netbios_res, partitions_dn, @@ -835,7 +872,9 @@ static int acl_check_dns_host_name(TALLOC_CTX *mem_ctx, const char *dns_host_name = NULL; const char *samAccountName = NULL; size_t account_name_len; + const struct ldb_message *msg = NULL; const struct ldb_message *search_res = NULL; + const struct ldb_val *sam_account_name_val = NULL; static const char *nc_attrs[] = { "msDS-AllowedDNSSuffixes", @@ -899,6 +938,8 @@ static int acl_check_dns_host_name(TALLOC_CTX *mem_ctx, NULL }; + msg = req->op.mod.message; + /* * If not add or replace (eg delete), * return success @@ -911,7 +952,7 @@ static int acl_check_dns_host_name(TALLOC_CTX *mem_ctx, } ret = dsdb_module_search_dn(module, tmp_ctx, - &acl_res, req->op.mod.message->dn, + &acl_res, msg->dn, acl_attrs, DSDB_FLAG_NEXT_MODULE | DSDB_FLAG_AS_SYSTEM | @@ -924,7 +965,8 @@ static int acl_check_dns_host_name(TALLOC_CTX *mem_ctx, search_res = acl_res->msgs[0]; } else if (req->operation == LDB_ADD) { - search_res = req->op.add.message; + msg = req->op.add.message; + search_res = msg; } else { talloc_free(tmp_ctx); return LDB_ERR_OPERATIONS_ERROR; @@ -948,11 +990,27 @@ static int acl_check_dns_host_name(TALLOC_CTX *mem_ctx, } } - samAccountName = ldb_msg_find_attr_as_string(search_res, "sAMAccountName", NULL); + sam_account_name_val = ldb_msg_find_ldb_val(search_res, "sAMAccountName"); + + ret = dsdb_msg_get_single_value(msg, + "sAMAccountName", + sam_account_name_val, + &sam_account_name_val, + req->operation); + if (ret != LDB_SUCCESS) { + talloc_free(tmp_ctx); + return ret; + } + + if (sam_account_name_val != NULL) { + samAccountName = ldb_val_as_string(sam_account_name_val, NULL); + } + if (samAccountName == NULL) { talloc_free(tmp_ctx); - return ldb_operr(ldb); + return LDB_ERR_CONSTRAINT_VIOLATION; } + account_name_len = strlen(samAccountName); if (account_name_len && samAccountName[account_name_len - 1] == '$') { /* Account for the '$' character. */ -- 2.35.0 From 45fb72bdd37b98c47b50efcf1f494792680dce0f Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Thu, 9 Jun 2022 19:32:30 +1200 Subject: [PATCH 13/14] s4:rpc_server/common: Add dcesrv_samdb_connect_session_info() This function allows us to connect to samdb as a particular user by passing in that user's session info. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14833 Signed-off-by: Joseph Sutton --- source4/rpc_server/common/common.h | 1 + source4/rpc_server/common/server_info.c | 30 ++++++++++++------------- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/source4/rpc_server/common/common.h b/source4/rpc_server/common/common.h index 7d2f8c569e4..b57ddf20db5 100644 --- a/source4/rpc_server/common/common.h +++ b/source4/rpc_server/common/common.h @@ -30,6 +30,7 @@ struct dcesrv_context; struct dcesrv_call_state; struct ndr_interface_table; struct ncacn_packet; +struct auth_session_info; struct dcerpc_server_info { const char *domain_name; diff --git a/source4/rpc_server/common/server_info.c b/source4/rpc_server/common/server_info.c index a2af37653ef..ceddeff8a5f 100644 --- a/source4/rpc_server/common/server_info.c +++ b/source4/rpc_server/common/server_info.c @@ -190,13 +190,12 @@ bool dcesrv_common_validate_share_name(TALLOC_CTX *mem_ctx, const char *share_na return true; } -static struct ldb_context *dcesrv_samdb_connect_common( +struct ldb_context *dcesrv_samdb_connect_session_info( TALLOC_CTX *mem_ctx, struct dcesrv_call_state *dce_call, - bool as_system) + struct auth_session_info *session_info) { struct ldb_context *samdb = NULL; - struct auth_session_info *system_session_info = NULL; const struct auth_session_info *call_session_info = dcesrv_call_session_info(dce_call); struct auth_session_info *user_session_info = NULL; @@ -204,13 +203,6 @@ static struct ldb_context *dcesrv_samdb_connect_common( struct auth_session_info *audit_session_info = NULL; struct tsocket_address *remote_address = NULL; - if (as_system) { - system_session_info = system_session(dce_call->conn->dce_ctx->lp_ctx); - if (system_session_info == NULL) { - return NULL; - } - } - user_session_info = copy_session_info(mem_ctx, call_session_info); if (user_session_info == NULL) { return NULL; @@ -224,8 +216,8 @@ static struct ldb_context *dcesrv_samdb_connect_common( } } - if (system_session_info != NULL) { - ldb_session_info = system_session_info; + if (session_info != NULL) { + ldb_session_info = session_info; audit_session_info = user_session_info; } else { ldb_session_info = user_session_info; @@ -288,8 +280,15 @@ struct ldb_context *dcesrv_samdb_connect_as_system( TALLOC_CTX *mem_ctx, struct dcesrv_call_state *dce_call) { - return dcesrv_samdb_connect_common(mem_ctx, dce_call, - true /* as_system */); + struct auth_session_info *system_session_info = NULL; + + system_session_info = system_session(dce_call->conn->dce_ctx->lp_ctx); + if (system_session_info == NULL) { + return NULL; + } + + return dcesrv_samdb_connect_session_info(mem_ctx, dce_call, + system_session_info); } /* @@ -301,6 +300,5 @@ struct ldb_context *dcesrv_samdb_connect_as_user( TALLOC_CTX *mem_ctx, struct dcesrv_call_state *dce_call) { - return dcesrv_samdb_connect_common(mem_ctx, dce_call, - false /* not as_system */); + return dcesrv_samdb_connect_session_info(mem_ctx, dce_call, NULL); } -- 2.35.0 From a12d2c9df76d36aa69b130c0c05aeca1b71eb974 Mon Sep 17 00:00:00 2001 From: Joseph Sutton Date: Thu, 9 Jun 2022 19:46:07 +1200 Subject: [PATCH 14/14] s4:rpc_server/netlogon: Reconnect to samdb as workstation account This ensures that the database update can be attributed to the workstation account, rather than to the anonymous SID, in the audit logs. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14833 Signed-off-by: Joseph Sutton --- source4/rpc_server/netlogon/dcerpc_netlogon.c | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/source4/rpc_server/netlogon/dcerpc_netlogon.c b/source4/rpc_server/netlogon/dcerpc_netlogon.c index 189b6d94424..d6a3da6fecc 100644 --- a/source4/rpc_server/netlogon/dcerpc_netlogon.c +++ b/source4/rpc_server/netlogon/dcerpc_netlogon.c @@ -2422,6 +2422,7 @@ static NTSTATUS dcesrv_netr_LogonGetDomainInfo(struct dcesrv_call_state *dce_cal struct ldb_dn *workstation_dn; struct netr_DomainInformation *domain_info; struct netr_LsaPolicyInformation *lsa_policy_info; + struct auth_session_info *workstation_session_info = NULL; uint32_t default_supported_enc_types = 0xFFFFFFFF; bool update_dns_hostname = true; int ret, i; @@ -2468,6 +2469,32 @@ static NTSTATUS dcesrv_netr_LogonGetDomainInfo(struct dcesrv_call_state *dce_cal dom_sid_string(mem_ctx, creds->sid)); NT_STATUS_HAVE_NO_MEMORY(workstation_dn); + /* Get the workstation's session info from the database. */ + status = authsam_get_session_info_principal(mem_ctx, + dce_call->conn->dce_ctx->lp_ctx, + sam_ctx, + NULL, /* principal */ + workstation_dn, + 0, /* session_info_flags */ + &workstation_session_info); + if (!NT_STATUS_IS_OK(status)) { + return status; + } + + /* + * Reconnect to samdb as the workstation, now that we have its + * session info. We do this so the database update can be + * attributed to the workstation account in the audit logs -- + * otherwise it might be incorrectly attributed to + * SID_NT_ANONYMOUS. + */ + sam_ctx = dcesrv_samdb_connect_session_info(mem_ctx, + dce_call, + workstation_session_info); + if (sam_ctx == NULL) { + return NT_STATUS_INVALID_SYSTEM_SERVICE; + } + /* Lookup for attributes in workstation object */ ret = gendb_search_dn(sam_ctx, mem_ctx, workstation_dn, &res1, attrs2); -- 2.35.0