From 74292a10f52c152c321d06d5ca2048d0abee1610 Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Thu, 11 Jul 2019 01:26:30 +0200 Subject: [PATCH] [Windows] Security model for files permissions - STEP 3e (#7182) This PR implements the filesystem.copy_ownership_and_apply_mode method from #6497. This method is used in two places in Certbot, replacing os.chown, to copy the owner and group owner from a file to another one, and apply to the latter the given POSIX mode. * Implement copy_ownership_and_apply_mode * Update certbot/compat/os.py Co-Authored-By: Brad Warren * Remove default values * Rewrite a comment. * Relaunch CI * Pass as keyword arguments * Update certbot/compat/filesystem.py Co-Authored-By: Brad Warren * Update certbot/compat/filesystem.py Co-Authored-By: Brad Warren * Update certbot/compat/filesystem.py Co-Authored-By: Brad Warren * Make the private key permissions transfer platform specific * Update certbot/compat/filesystem.py Co-Authored-By: Brad Warren * Rename variable * Fix comment0 * Add unit test for copy_ownership_and_apply_mode * Adapt coverage * Execute unconditionally chmod with copy_ownership_and_apply_mode. Improve doc. --- .codecov.yml | 4 +-- certbot/compat/filesystem.py | 45 +++++++++++++++++++++++ certbot/compat/misc.py | 13 +++++++ certbot/compat/os.py | 10 ++++++ certbot/plugins/webroot.py | 12 +++---- certbot/plugins/webroot_test.py | 8 ++--- certbot/storage.py | 13 +++---- certbot/tests/compat/filesystem_test.py | 48 +++++++++++++++++++++++++ certbot/tests/compat/os_test.py | 2 +- certbot/tests/storage_test.py | 9 +++-- 10 files changed, 139 insertions(+), 25 deletions(-) diff --git a/.codecov.yml b/.codecov.yml index 4c0cf93f1..86813de6a 100644 --- a/.codecov.yml +++ b/.codecov.yml @@ -6,13 +6,13 @@ coverage: flags: linux # Fixed target instead of auto set by #7173, can # be removed when flags in Codecov are added back. - target: 97.7 + target: 97.6 threshold: 0.1 base: auto windows: flags: windows # Fixed target instead of auto set by #7173, can # be removed when flags in Codecov are added back. - target: 96.9 + target: 97.0 threshold: 0.1 base: auto diff --git a/certbot/compat/filesystem.py b/certbot/compat/filesystem.py index 657f2376c..d0d1c262d 100644 --- a/certbot/compat/filesystem.py +++ b/certbot/compat/filesystem.py @@ -44,6 +44,39 @@ def chmod(file_path, mode): _apply_win_mode(file_path, mode) +# One could ask why there is no copy_ownership() function, or even a reimplementation +# of os.chown() that would modify the ownership of file without touching the mode itself. +# This is because on Windows, it would require recalculating the existing DACL against +# the new owner, since the DACL is composed of ACEs that targets a specific user, not dynamically +# the current owner of a file. This action would be necessary to keep consistency between +# the POSIX mode applied to the file and the current owner of this file. +# Since copying and editing arbitrary DACL is very difficult, and since we actually know +# the mode to apply at the time the owner of a file should change, it is easier to just +# change the owner, then reapply the known mode, as copy_ownership_and_apply_mode() does. +def copy_ownership_and_apply_mode(src, dst, mode, copy_user, copy_group): + # type: (str, str, int, bool, bool) -> None + """ + Copy ownership (user and optionally group on Linux) from the source to the + destination, then apply given mode in compatible way for Linux and Windows. + This replaces the os.chown command. + :param str src: Path of the source file + :param str dst: Path of the destination file + :param int mode: Permission mode to apply on the destination file + :param bool copy_user: Copy user if `True` + :param bool copy_group: Copy group if `True` on Linux (has no effect on Windows) + """ + if POSIX_MODE: + stats = os.stat(src) + user_id = stats.st_uid if copy_user else -1 + group_id = stats.st_gid if copy_group else -1 + os.chown(dst, user_id, group_id) + elif copy_user: + # There is no group handling in Windows + _copy_win_ownership(src, dst) + + chmod(dst, mode) + + def open(file_path, flags, mode=0o777): # pylint: disable=redefined-builtin # type: (str, int, int) -> int """ @@ -251,6 +284,18 @@ def _analyze_mode(mode): } +def _copy_win_ownership(src, dst): + security_src = win32security.GetFileSecurity(src, win32security.OWNER_SECURITY_INFORMATION) + user_src = security_src.GetSecurityDescriptorOwner() + + security_dst = win32security.GetFileSecurity(dst, win32security.OWNER_SECURITY_INFORMATION) + # Second parameter indicates, if `False`, that the owner of the file is not provided by some + # default mechanism, but is explicitly set instead. This is obviously what we are doing here. + security_dst.SetSecurityDescriptorOwner(user_src, False) + + win32security.SetFileSecurity(dst, win32security.OWNER_SECURITY_INFORMATION, security_dst) + + def _generate_windows_flags(rights_desc): # Some notes about how each POSIX right is interpreted. # diff --git a/certbot/compat/misc.py b/certbot/compat/misc.py index c61672364..c840c333c 100644 --- a/certbot/compat/misc.py +++ b/certbot/compat/misc.py @@ -17,6 +17,19 @@ except ImportError: # pragma: no cover from certbot import errors from certbot.compat import os + +# MASK_FOR_PRIVATE_KEY_PERMISSIONS defines what are the permissions flags to keep +# when transferring the permissions from an old private key to a new one. +if POSIX_MODE: + # On Linux, we keep read/write/execute permissions + # for group and read permissions for everybody. + MASK_FOR_PRIVATE_KEY_PERMISSIONS = stat.S_IRGRP | stat.S_IWGRP | stat.S_IXGRP | stat.S_IROTH +else: + # On Windows, the mode returned by os.stat is not reliable, + # so we do not keep any permission from the previous private key. + MASK_FOR_PRIVATE_KEY_PERMISSIONS = 0 + + def raise_for_non_administrative_windows_rights(): # type: () -> None """ diff --git a/certbot/compat/os.py b/certbot/compat/os.py index 0c5582ca1..6d3c7bb02 100644 --- a/certbot/compat/os.py +++ b/certbot/compat/os.py @@ -17,6 +17,7 @@ from os import * # type: ignore # pylint: disable=wildcard-import,unused-wildc # and so not in `from os import *`. import os as std_os # pylint: disable=os-module-forbidden import sys as std_sys + ourselves = std_sys.modules[__name__] for attribute in dir(std_os): # Check if the attribute does not already exist in our module. It could be internal attributes @@ -51,6 +52,15 @@ def chmod(*unused_args, **unused_kwargs): 'Use certbot.compat.filesystem.chmod() instead.') +# Because uid is not a concept on Windows, chown is useless. In fact, it is not even available +# on Python for Windows. So to be consistent on both platforms for Certbot, this method is +# always forbidden. +def chown(*unused_args, **unused_kwargs): + """Method os.chown() is forbidden""" + raise RuntimeError('Usage of os.chown() is forbidden.' + 'Use certbot.compat.filesystem.copy_ownership_and_apply_mode() instead.') + + # The os.open function on Windows has the same effect as a call to os.chown concerning the file # modes: these modes lack the correct control over the permissions given to the file. Instead, # filesystem.open invokes the Windows native API `CreateFile` to ensure that permissions are diff --git a/certbot/plugins/webroot.py b/certbot/plugins/webroot.py index 2a42bd5ed..f767bdf54 100644 --- a/certbot/plugins/webroot.py +++ b/certbot/plugins/webroot.py @@ -169,19 +169,19 @@ to serve all files under specified web root ({0}).""" # run as non-root (GH #1795) old_umask = os.umask(0o022) try: - stat_path = os.stat(path) # We ignore the last prefix in the next iteration, # as it does not correspond to a folder path ('/' or 'C:') for prefix in sorted(util.get_prefixes(self.full_roots[name])[:-1], key=len): try: - # This is coupled with the "umask" call above because os.mkdir's - # "mode" parameter may not always work under Linux: + # Set owner as parent directory if possible, apply mode for Linux/Windows. + # For Linux, this is coupled with the "umask" call above because + # os.mkdir's "mode" parameter may not always work: # https://docs.python.org/3/library/os.html#os.mkdir - filesystem.mkdir(prefix, 0o0755) + filesystem.mkdir(prefix, 0o755) self._created_dirs.append(prefix) - # Set owner as parent directory if possible try: - os.chown(prefix, stat_path.st_uid, stat_path.st_gid) + filesystem.copy_ownership_and_apply_mode( + path, prefix, 0o755, copy_user=True, copy_group=True) except (OSError, AttributeError) as exception: logger.info("Unable to change owner and uid of webroot directory") logger.debug("Error was: %s", exception) diff --git a/certbot/plugins/webroot_test.py b/certbot/plugins/webroot_test.py index 5039aae9e..ba9950fae 100644 --- a/certbot/plugins/webroot_test.py +++ b/certbot/plugins/webroot_test.py @@ -142,13 +142,11 @@ class AuthenticatorTest(unittest.TestCase): self.assertRaises(errors.PluginError, self.auth.perform, []) filesystem.chmod(self.path, 0o700) - @test_util.skip_on_windows('On Windows, there is no chown.') - @mock.patch("certbot.plugins.webroot.os.chown") - def test_failed_chown(self, mock_chown): - mock_chown.side_effect = OSError(errno.EACCES, "msg") + @mock.patch("certbot.plugins.webroot.filesystem.copy_ownership_and_apply_mode") + def test_failed_chown(self, mock_ownership): + mock_ownership.side_effect = OSError(errno.EACCES, "msg") self.auth.perform([self.achall]) # exception caught and logged - @test_util.patch_get_utility() def test_perform_new_webroot_not_in_map(self, mock_get_utility): new_webroot = tempfile.mkdtemp() diff --git a/certbot/storage.py b/certbot/storage.py index 3ab37afe5..518ba9464 100644 --- a/certbot/storage.py +++ b/certbot/storage.py @@ -18,6 +18,7 @@ from certbot import crypto_util from certbot import error_handler from certbot import errors from certbot import util +from certbot.compat import misc from certbot.compat import os from certbot.compat import filesystem from certbot.plugins import common as plugins_common @@ -1104,13 +1105,13 @@ class RenewableCert(object): with util.safe_open(target["privkey"], "wb", chmod=BASE_PRIVKEY_MODE) as f: logger.debug("Writing new private key to %s.", target["privkey"]) f.write(new_privkey) - # Preserve gid and (mode & 074) from previous privkey in this lineage. - old_mode = stat.S_IMODE(os.stat(old_privkey).st_mode) & \ - (stat.S_IRGRP | stat.S_IWGRP | stat.S_IXGRP | \ - stat.S_IROTH) + # Preserve gid and (mode & MASK_FOR_PRIVATE_KEY_PERMISSIONS) + # from previous privkey in this lineage. + old_mode = (stat.S_IMODE(os.stat(old_privkey).st_mode) & + misc.MASK_FOR_PRIVATE_KEY_PERMISSIONS) mode = BASE_PRIVKEY_MODE | old_mode - os.chown(target["privkey"], -1, os.stat(old_privkey).st_gid) - filesystem.chmod(target["privkey"], mode) + filesystem.copy_ownership_and_apply_mode( + old_privkey, target["privkey"], mode, copy_user=False, copy_group=True) # Save everything else with open(target["cert"], "wb") as f: diff --git a/certbot/tests/compat/filesystem_test.py b/certbot/tests/compat/filesystem_test.py index b0edb31ba..27c48c19a 100644 --- a/certbot/tests/compat/filesystem_test.py +++ b/certbot/tests/compat/filesystem_test.py @@ -2,6 +2,8 @@ import errno import unittest +import mock + try: # pylint: disable=import-error import win32api @@ -270,6 +272,52 @@ class WindowsMkdirTests(test_util.TempDirTestCase): self.assertEqual(original_mkdir, std_os.mkdir) +class CopyOwnershipTest(test_util.TempDirTestCase): + """Tests about replacement of chown: copy_ownership_and_apply_mode""" + def setUp(self): + super(CopyOwnershipTest, self).setUp() + self.probe_path = _create_probe(self.tempdir) + + @unittest.skipIf(POSIX_MODE, reason='Test specific to Windows security') + def test_windows(self): + system = win32security.ConvertStringSidToSid(SYSTEM_SID) + security = win32security.SECURITY_ATTRIBUTES().SECURITY_DESCRIPTOR + security.SetSecurityDescriptorOwner(system, False) + + with mock.patch('win32security.GetFileSecurity') as mock_get: + with mock.patch('win32security.SetFileSecurity') as mock_set: + mock_get.return_value = security + filesystem.copy_ownership_and_apply_mode( + 'dummy', self.probe_path, 0o700, copy_user=True, copy_group=False) + + self.assertEqual(mock_set.call_count, 2) + + first_call = mock_set.call_args_list[0] + security = first_call[0][2] + self.assertEqual(system, security.GetSecurityDescriptorOwner()) + + second_call = mock_set.call_args_list[1] + security = second_call[0][2] + dacl = security.GetSecurityDescriptorDacl() + everybody = win32security.ConvertStringSidToSid(EVERYBODY_SID) + self.assertTrue(dacl.GetAceCount()) + self.assertFalse([dacl.GetAce(index) for index in range(0, dacl.GetAceCount()) + if dacl.GetAce(index)[2] == everybody]) + + @unittest.skipUnless(POSIX_MODE, reason='Test specific to Linux security') + def test_linux(self): + with mock.patch('os.chown') as mock_chown: + with mock.patch('os.chmod') as mock_chmod: + with mock.patch('os.stat') as mock_stat: + mock_stat.return_value.st_uid = 50 + mock_stat.return_value.st_gid = 51 + filesystem.copy_ownership_and_apply_mode( + 'dummy', self.probe_path, 0o700, copy_user=True, copy_group=True) + + mock_chown.assert_called_once_with(self.probe_path, 50, 51) + mock_chmod.assert_called_once_with(self.probe_path, 0o700) + + class OsReplaceTest(test_util.TempDirTestCase): """Test to ensure consistent behavior of rename method""" diff --git a/certbot/tests/compat/os_test.py b/certbot/tests/compat/os_test.py index a2b40a7f8..9e25548f0 100644 --- a/certbot/tests/compat/os_test.py +++ b/certbot/tests/compat/os_test.py @@ -7,7 +7,7 @@ from certbot.compat import os class OsTest(unittest.TestCase): """Unit tests for os module.""" def test_forbidden_methods(self): - for method in ['chmod', 'open', 'mkdir', 'makedirs', 'rename', 'replace']: + for method in ['chmod', 'chown', 'open', 'mkdir', 'makedirs', 'rename', 'replace']: self.assertRaises(RuntimeError, getattr(os, method)) diff --git a/certbot/tests/storage_test.py b/certbot/tests/storage_test.py index 017807a6d..fceef804a 100644 --- a/certbot/tests/storage_test.py +++ b/certbot/tests/storage_test.py @@ -588,10 +588,9 @@ class RenewableCertTests(BaseRenewableCertTest): self.assertTrue(misc.compare_file_modes( os.stat(self.test_rc.version("privkey", 4)).st_mode, 0o600)) - @test_util.broken_on_windows @mock.patch("certbot.storage.relevant_values") - @mock.patch("certbot.storage.os.chown") - def test_save_successor_maintains_gid(self, mock_chown, mock_rv): + @mock.patch("certbot.storage.filesystem.copy_ownership_and_apply_mode") + def test_save_successor_maintains_gid(self, mock_ownership, mock_rv): # Mock relevant_values() to claim that all values are relevant here # (to avoid instantiating parser) mock_rv.side_effect = lambda x: x @@ -599,9 +598,9 @@ class RenewableCertTests(BaseRenewableCertTest): self._write_out_kind(kind, 1) self.test_rc.update_all_links_to(1) self.test_rc.save_successor(1, b"newcert", None, b"new chain", self.config) - self.assertFalse(mock_chown.called) + self.assertFalse(mock_ownership.called) self.test_rc.save_successor(2, b"newcert", b"new_privkey", b"new chain", self.config) - self.assertTrue(mock_chown.called) + self.assertTrue(mock_ownership.called) @mock.patch("certbot.storage.relevant_values") def test_new_lineage(self, mock_rv):