From 43b618fe203c73aede38ccc0bc426606bdedc790 Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Fri, 24 May 2019 18:07:52 +0200 Subject: [PATCH] Add unit tests to filesystem --- certbot/compat/filesystem.py | 29 ++++- certbot/tests/compat/filesystem_test.py | 164 ++++++++++++++++++++---- 2 files changed, 167 insertions(+), 26 deletions(-) diff --git a/certbot/compat/filesystem.py b/certbot/compat/filesystem.py index 91c73f503..d8f799d84 100644 --- a/certbot/compat/filesystem.py +++ b/certbot/compat/filesystem.py @@ -126,8 +126,35 @@ def _generate_windows_flags(rights_desc): if rights_desc['write']: flag = flag | (ntsecuritycon.FILE_ALL_ACCESS ^ ntsecuritycon.FILE_GENERIC_READ - ^ ntsecuritycon.FILE_GENERIC_EXECUTE) + ^ ntsecuritycon.FILE_GENERIC_EXECUTE + # Despite bit `512` being present in ntsecuritycon.FILE_ALL_ACCESS, it is + # not effectively applied to the file or the directory. + # As _generate_windows_flags is also used to compare two dacls, we remove + # it right now to have flags that contain only the bits effectively applied + # by Windows. + ^ 512) if rights_desc['execute']: flag = flag | ntsecuritycon.FILE_GENERIC_EXECUTE return flag + + +def _compare_dacls(dacl1, dacl2): + aces1 = [dacl1.GetAce(index) for index in range(0, dacl1.GetAceCount())] + aces2 = [dacl2.GetAce(index) for index in range(0, dacl2.GetAceCount())] + + # Convert PySIDs into hashable objects + aces1_refined = [] + aces2_refined = [] + for ace in aces1: + if len(ace) == 3: + aces1_refined.append((ace[0], ace[1], str(ace[2]))) + else: + aces1_refined.append((ace[0], ace[1], ace[2], ace[3], str(ace[4]))) # type: ignore + for index, ace in enumerate(aces2): + if len(ace) == 3: + aces2_refined.append((ace[0], ace[1], str(ace[2]))) + else: + aces2_refined.append((ace[0], ace[1], ace[2], ace[3], str(ace[4]))) # type: ignore + + return set(aces1_refined) == set(aces2_refined) diff --git a/certbot/tests/compat/filesystem_test.py b/certbot/tests/compat/filesystem_test.py index f933ee1cf..b38db8a6d 100644 --- a/certbot/tests/compat/filesystem_test.py +++ b/certbot/tests/compat/filesystem_test.py @@ -6,48 +6,162 @@ from certbot.compat import filesystem from certbot.tests.util import TempDirTestCase -class FilesystemTest(TempDirTestCase): - """Unit tests for filesystem module.""" - @unittest.skipIf(os.name != 'nt', reason='Test specific to Windows security') +@unittest.skipIf(os.name != 'nt', reason='Test specific to Windows security') +class WindowsChmodTests(TempDirTestCase): + """Unit tests for Windows chmod function in filesystem module""" + def test_symlink_resolution(self): + probe_path = _create_probe(self.tempdir) # This is 0o744 by default + + link_path = os.path.join(self.tempdir, 'link') + os.symlink(probe_path, link_path) + + ref_dacl_probe = _get_security_dacl(probe_path).GetSecurityDescriptorDacl() + ref_dacl_link = _get_security_dacl(link_path).GetSecurityDescriptorDacl() + + # Removing the rights for `all`, we have at least one ACL less than in the case of 0o744. + filesystem.chmod(link_path, 0o700) + + # Assert the real file is impacted, not the link. + cur_dacl_probe = _get_security_dacl(probe_path).GetSecurityDescriptorDacl() + cur_dacl_link = _get_security_dacl(link_path).GetSecurityDescriptorDacl() + self.assertFalse(filesystem._compare_dacls(ref_dacl_probe, cur_dacl_probe)) # pylint: disable=protected-access + self.assertTrue(filesystem._compare_dacls(ref_dacl_link, cur_dacl_link)) # pylint: disable=protected-access + + def test_world_permission(self): + import win32security + probe_path = _create_probe(self.tempdir) + + all = win32security.ConvertStringSidToSid('S-1-1-0') + + filesystem.chmod(probe_path, 0o700) + dacl = _get_security_dacl(probe_path).GetSecurityDescriptorDacl() + + self.assertFalse([dacl.GetAce(index) for index in range(0, dacl.GetAceCount()) + if dacl.GetAce(index)[2] == all]) + + filesystem.chmod(probe_path, 0o704) + dacl = _get_security_dacl(probe_path).GetSecurityDescriptorDacl() + + self.assertTrue([dacl.GetAce(index) for index in range(0, dacl.GetAceCount()) + if dacl.GetAce(index)[2] == all]) + + def test_group_permissions_noop(self): + probe_path = _create_probe(self.tempdir) + + filesystem.chmod(probe_path, 0o700) + ref_dacl_probe = _get_security_dacl(probe_path).GetSecurityDescriptorDacl() + + filesystem.chmod(probe_path, 0o740) + cur_dacl_probe = _get_security_dacl(probe_path).GetSecurityDescriptorDacl() + + self.assertTrue(filesystem._compare_dacls(ref_dacl_probe, cur_dacl_probe)) # pylint: disable=protected-access + + def test_admin_permissions(self): + import win32security + probe_path = _create_probe(self.tempdir) + + system = win32security.ConvertStringSidToSid('S-1-5-18') + admins = win32security.ConvertStringSidToSid('S-1-5-32-544') + + filesystem.chmod(probe_path, 0o700) + dacl = _get_security_dacl(probe_path).GetSecurityDescriptorDacl() + + self.assertTrue([dacl.GetAce(index) for index in range(0, dacl.GetAceCount()) + if dacl.GetAce(index)[2] == system]) + self.assertTrue([dacl.GetAce(index) for index in range(0, dacl.GetAceCount()) + if dacl.GetAce(index)[2] == admins]) + + def test_read_flag(self): + import ntsecuritycon + self._test_flag(4, ntsecuritycon.FILE_GENERIC_READ) + + def test_execute_flag(self): + import ntsecuritycon + self._test_flag(1, ntsecuritycon.FILE_GENERIC_EXECUTE) + + def test_write_flag(self): + import ntsecuritycon + self._test_flag(2, (ntsecuritycon.FILE_ALL_ACCESS + ^ ntsecuritycon.FILE_GENERIC_READ + ^ ntsecuritycon.FILE_GENERIC_EXECUTE + ^ 512)) + + def test_full_flag(self): + import ntsecuritycon + self._test_flag(7, (ntsecuritycon.FILE_ALL_ACCESS + ^ 512)) + + def _test_flag(self, everyone_mode, windows_flag): + # Note that flag are tested again `everyone`, not `user`, because practically these unit + # tests are executed with admin privilege, so current user is effectively the admins group, + # and so will always have all rights. + import win32security + probe_path = _create_probe(self.tempdir) + + filesystem.chmod(probe_path, 0o700 + everyone_mode) + dacl = _get_security_dacl(probe_path).GetSecurityDescriptorDacl() + all = win32security.ConvertStringSidToSid('S-1-1-0') + + acls_user = [dacl.GetAce(index) for index in range(0, dacl.GetAceCount()) + if dacl.GetAce(index)[2] == all] + + self.assertEqual(len(acls_user), 1) + + acl_user = acls_user[0] + + self.assertEqual(acl_user[1], windows_flag) + def test_user_admin_dacl_consistency(self): import win32security # pylint: disable=import-error import win32api # pylint: disable=import-error - target = os.path.join(self.tempdir, 'target') - open(target, 'w').close() + probe_path = _create_probe(self.tempdir) # Set ownership of target to authenticated user authenticated_user, _, _ = win32security.LookupAccountName("", win32api.GetUserName()) - security_owner = win32security.GetFileSecurity( - target, win32security.OWNER_SECURITY_INFORMATION) - security_owner.SetSecurityDescriptorOwner(authenticated_user, False) - win32security.SetFileSecurity( - target, win32security.OWNER_SECURITY_INFORMATION, security_owner) + security_owner = _get_security_owner(probe_path) + _set_owner(probe_path, security_owner, authenticated_user) - filesystem.chmod(target, 0o700) + filesystem.chmod(probe_path, 0o700) - security_dacl = win32security.GetFileSecurity( - target, win32security.DACL_SECURITY_INFORMATION) - dacl = security_dacl.GetSecurityDescriptorDacl() + security_dacl = _get_security_dacl(probe_path) # We expect three ACE: one for admins, one for system, and one for the user - self.assertEqual(dacl.GetAceCount(), 3) + self.assertEqual(security_dacl.GetSecurityDescriptorDacl().GetAceCount(), 3) # Set ownership of target to Administrators user group admin_user = win32security.ConvertStringSidToSid('S-1-5-32-544') - security_owner = win32security.GetFileSecurity( - target, win32security.OWNER_SECURITY_INFORMATION) - security_owner.SetSecurityDescriptorOwner(admin_user, False) - win32security.SetFileSecurity( - target, win32security.OWNER_SECURITY_INFORMATION, security_owner) + security_owner = _get_security_owner(probe_path) + _set_owner(probe_path, security_owner, admin_user) - filesystem.chmod(target, 0o700) + filesystem.chmod(probe_path, 0o700) - security_dacl = win32security.GetFileSecurity( - target, win32security.DACL_SECURITY_INFORMATION) - dacl = security_dacl.GetSecurityDescriptorDacl() + security_dacl = _get_security_dacl(probe_path) # We expect only two ACE: one for admins, one for system, # since the user is also the admins group - self.assertEqual(dacl.GetAceCount(), 2) + self.assertEqual(security_dacl.GetSecurityDescriptorDacl().GetAceCount(), 2) + + +def _get_security_dacl(target): + import win32security + return win32security.GetFileSecurity( target, win32security.DACL_SECURITY_INFORMATION) + + +def _get_security_owner(target): + import win32security + return win32security.GetFileSecurity(target, win32security.OWNER_SECURITY_INFORMATION) + + +def _set_owner(target, security_owner, user): + import win32security + security_owner.SetSecurityDescriptorOwner(user, False) + win32security.SetFileSecurity( + target, win32security.OWNER_SECURITY_INFORMATION, security_owner) + + +def _create_probe(tempdir): + probe_path = os.path.join(tempdir, 'probe') + open(probe_path, 'w').close() + return probe_path if __name__ == "__main__":