diff --git a/certbot-apache/certbot_apache/http_01.py b/certbot-apache/certbot_apache/http_01.py index 1d1f99919..253c49924 100644 --- a/certbot-apache/certbot_apache/http_01.py +++ b/certbot-apache/certbot_apache/http_01.py @@ -5,6 +5,7 @@ from acme.magic_typing import List, Set # pylint: disable=unused-import, no-nam from certbot import errors from certbot.compat import os +from certbot.compat import security from certbot.plugins import common from certbot_apache.obj import VirtualHost # pylint: disable=unused-import @@ -169,7 +170,7 @@ class ApacheHttp01(common.TLSSNI01): def _set_up_challenges(self): if not os.path.isdir(self.challenge_dir): os.makedirs(self.challenge_dir) - os.chmod(self.challenge_dir, 0o755) + security.chmod(self.challenge_dir, 0o755) responses = [] for achall in self.achalls: @@ -185,7 +186,7 @@ class ApacheHttp01(common.TLSSNI01): self.configurator.reverter.register_file_creation(True, name) with open(name, 'wb') as f: f.write(validation.encode()) - os.chmod(name, 0o644) + security.chmod(name, 0o644) return response diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 5b2884eb2..f4826defa 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -16,6 +16,7 @@ from certbot import achallenges from certbot import crypto_util from certbot import errors from certbot.compat import os +from certbot.compat import security from certbot.tests import acme_util from certbot.tests import util as certbot_util @@ -1366,7 +1367,7 @@ class MultipleVhostsTest(util.ApacheTest): self.config.parser.modules.add("mod_ssl.c") self.config.parser.modules.add("socache_shmcb_module") tmp_path = os.path.realpath(tempfile.mkdtemp("vhostroot")) - os.chmod(tmp_path, 0o755) + security.chmod(tmp_path, 0o755) mock_p = "certbot_apache.configurator.ApacheConfigurator._get_ssl_vhost_path" mock_a = "certbot_apache.parser.ApacheParser.add_include" diff --git a/certbot/compat/os.py b/certbot/compat/os.py index 0112fbc73..55efa7f4b 100644 --- a/certbot/compat/os.py +++ b/certbot/compat/os.py @@ -29,3 +29,22 @@ std_sys.modules[__name__ + '.path'] = path # Clean all remaining importables that are not from the core os module. del ourselves, std_os, std_sys + + +# Chmod is the root of all evil for our security model on Windows. With the default implementation +# of os.chmod on Windows, almost all bits on mode will be ignored, and only a general RO or RW will +# be applied. The DACL, the inner mechanism to control file access on Windows, will stay on its +# default definition, giving effectively at least read permissions to any one, as the default +# permissions on root path will be inherit by the file (as NTFS state), and root path can be read +# by anyone. So the given mode will be translated into a secured and not inherited DACL that will +# be applied to this file using security.chmod, that will call internally the win32security +# module to construct and apply the DACL. Complete security model to translate a POSIX mode for +# something usable on Windows for Certbot can be found here: +# https://github.com/certbot/certbot/issues/6356 +# Basically, it states that appropriate permissions will be set for the owner, nothing for the +# group, appropriate permissions for the "Everyone" group, and all permissions to the +# "Administrators" group, as they can do everything anyway. +def chmod(*unused_args, **unused_kwargs): # pylint: disable=function-redefined + """Method os.chmod() is forbidden""" + raise RuntimeError('Usage of os.chmod() is forbidden. ' # pragma: no cover + 'Use certbot.compat.security.chmod() instead.') diff --git a/certbot/compat/security.py b/certbot/compat/security.py new file mode 100644 index 000000000..31838a990 --- /dev/null +++ b/certbot/compat/security.py @@ -0,0 +1,125 @@ +"""Compat module to handle files security on Windows and Linux""" +from __future__ import absolute_import + +import os # pylint: disable=os-module-forbidden +import stat + +try: + import ntsecuritycon # pylint: disable=import-error +except ImportError: # pragma: no cover + ntsecuritycon = None # type: ignore +try: + import win32security # pylint: disable=import-error +except ImportError: # pragma: no cover + win32security = None # type: ignore + + +def chmod(file_path, mode): + # type: (str, int) -> None + """ + Apply a POSIX mode on given file_path: + * for Linux, the POSIX mode will be directly applied using chmod, + * for Windows, the POSIX mode will be translated into a Windows DACL that make sense for + Certbot context, and applied to the file using kernel calls. + :param str file_path: Path of the file + :param int mode: POSIX mode to apply + """ + if not win32security: + os.chmod(file_path, mode) + else: + _apply_win_mode(file_path, mode) + + +def _apply_win_mode(file_path, mode): + # Resolve symbolic links + if os.path.islink(file_path): + link_path = file_path + file_path = os.readlink(file_path) + if not os.path.isabs(file_path): + file_path = os.path.join(os.path.dirname(link_path), file_path) + # Get owner sid of the file + security = win32security.GetFileSecurity(file_path, win32security.OWNER_SECURITY_INFORMATION) + user = security.GetSecurityDescriptorOwner() + + # New DACL, that will overwrite existing one (including inherited permissions) + dacl = _generate_dacl(user, mode) + + # Apply the new DACL + security.SetSecurityDescriptorDacl(1, dacl, 0) + win32security.SetFileSecurity(file_path, win32security.DACL_SECURITY_INFORMATION, security) + + +def _generate_dacl(user_sid, mode): + analysis = _analyze_mode(mode) + + # Get standard accounts sid + system = win32security.ConvertStringSidToSid('S-1-5-18') + admins = win32security.ConvertStringSidToSid('S-1-5-32-544') + everyone = win32security.ConvertStringSidToSid('S-1-1-0') + + # New dacl, without inherited permissions + dacl = win32security.ACL() + + # If user is already system or admins, any ACE defined here would be superseeded by + # the full control ACE that will be added after. + if str(user_sid) not in [str(system), str(admins)]: + # Handle user rights + user_flags = _generate_windows_flags(analysis['user']) + if user_flags: + dacl.AddAccessAllowedAce(win32security.ACL_REVISION, user_flags, user_sid) + + # Handle everybody rights + everybody_flags = _generate_windows_flags(analysis['all']) + if everybody_flags: + dacl.AddAccessAllowedAce(win32security.ACL_REVISION, everybody_flags, everyone) + + # Handle administrator rights + full_permissions = _generate_windows_flags({'read': True, 'write': True, 'execute': True}) + dacl.AddAccessAllowedAce(win32security.ACL_REVISION, full_permissions, system) + dacl.AddAccessAllowedAce(win32security.ACL_REVISION, full_permissions, admins) + + return dacl + + +def _analyze_mode(mode): + return { + 'user': { + 'read': mode & stat.S_IRUSR, + 'write': mode & stat.S_IWUSR, + 'execute': mode & stat.S_IXUSR, + }, + 'all': { + 'read': mode & stat.S_IROTH, + 'write': mode & stat.S_IWOTH, + 'execute': mode & stat.S_IXOTH, + }, + } + + +def _generate_windows_flags(rights_desc): + # Some notes about how each POSIX right is interpreted. + # + # For the rights read and execute, we have a pretty bijective relation between + # POSIX flags and their generic counterparts on Windows, so we use them directly + # (respectively ntsecuritycon.GENERIC_READ) and (respectively ntsecuritycon.GENERIC_EXECUTE). + # + # But ntsecuritycon.GENERIC_WRITE does not correspond to what one could expect from a write + # access on Linux: for Windows, GENERIC_WRITE does not include delete, move or + # rename. This is something that requires ntsecuritycon.GENERIC_ALL. + # So to reproduce the write right as POSIX, we will apply ntsecuritycon.GENERIC_ALL + # substracted of the rights corresponding to POSIX read and POSIX execute. + # + # Finally, having read + write + execute gives a ntsecuritycon.GENERIC_ALL, + # so a full control of the file. + flag = 0 + if rights_desc['read']: + flag = flag | ntsecuritycon.FILE_GENERIC_READ + if rights_desc['write']: + flag = flag | (ntsecuritycon.FILE_ALL_ACCESS + ^ ntsecuritycon.FILE_GENERIC_READ + ^ ntsecuritycon.FILE_GENERIC_EXECUTE + ^ 512) # This bit is never set with file/directory objects + if rights_desc['execute']: + flag = flag | ntsecuritycon.FILE_GENERIC_EXECUTE + + return flag diff --git a/certbot/plugins/common.py b/certbot/plugins/common.py index 3dd9534db..cb978539e 100644 --- a/certbot/plugins/common.py +++ b/certbot/plugins/common.py @@ -20,6 +20,7 @@ from certbot import interfaces from certbot import reverter from certbot import util from certbot.compat import os +from certbot.compat import security from certbot.plugins.storage import PluginStorage logger = logging.getLogger(__name__) @@ -482,9 +483,9 @@ def dir_setup(test_dir, pkg): # pragma: no cover config_dir = expanded_tempdir("config") work_dir = expanded_tempdir("work") - os.chmod(temp_dir, constants.CONFIG_DIRS_MODE) - os.chmod(config_dir, constants.CONFIG_DIRS_MODE) - os.chmod(work_dir, constants.CONFIG_DIRS_MODE) + security.chmod(temp_dir, constants.CONFIG_DIRS_MODE) + security.chmod(config_dir, constants.CONFIG_DIRS_MODE) + security.chmod(work_dir, constants.CONFIG_DIRS_MODE) test_configs = pkg_resources.resource_filename( pkg, os.path.join("testdata", test_dir)) diff --git a/certbot/plugins/dns_test_common.py b/certbot/plugins/dns_test_common.py index 7f57b9431..f22d5a1a4 100644 --- a/certbot/plugins/dns_test_common.py +++ b/certbot/plugins/dns_test_common.py @@ -9,6 +9,7 @@ from acme import challenges from certbot import achallenges from certbot.compat import os +from certbot.compat import security from certbot.tests import acme_util from certbot.tests import util as test_util @@ -60,4 +61,4 @@ def write(values, path): with open(path, "wb") as f: config.write(outfile=f) - os.chmod(path, 0o600) + security.chmod(path, 0o600) diff --git a/certbot/plugins/webroot_test.py b/certbot/plugins/webroot_test.py index 3a902b91f..fd0b8ce39 100644 --- a/certbot/plugins/webroot_test.py +++ b/certbot/plugins/webroot_test.py @@ -19,6 +19,7 @@ from certbot import achallenges from certbot import errors from certbot.compat import misc from certbot.compat import os +from certbot.compat import security from certbot.display import util as display_util from certbot.tests import acme_util from certbot.tests import util as test_util @@ -132,14 +133,14 @@ class AuthenticatorTest(unittest.TestCase): permission_canary = os.path.join(self.path, "rnd") with open(permission_canary, "w") as f: f.write("thingimy") - os.chmod(self.path, 0o000) + security.chmod(self.path, 0o000) try: open(permission_canary, "r") print("Warning, running tests as root skips permissions tests...") except IOError: # ok, permissions work, test away... self.assertRaises(errors.PluginError, self.auth.perform, []) - os.chmod(self.path, 0o700) + security.chmod(self.path, 0o700) @test_util.skip_on_windows('On Windows, there is no chown.') @mock.patch("certbot.plugins.webroot.os.chown") diff --git a/certbot/storage.py b/certbot/storage.py index 01e012920..0bc943251 100644 --- a/certbot/storage.py +++ b/certbot/storage.py @@ -20,6 +20,7 @@ from certbot import errors from certbot import util from certbot.compat import misc from certbot.compat import os +from certbot.compat import security from certbot.plugins import common as plugins_common from certbot.plugins import disco as plugins_disco @@ -143,7 +144,7 @@ def write_renewal_config(o_filename, n_filename, archive_dir, target, relevant_d # Copy permissions from the old version of the file, if it exists. if os.path.exists(o_filename): current_permissions = stat.S_IMODE(os.lstat(o_filename).st_mode) - os.chmod(n_filename, current_permissions) + security.chmod(n_filename, current_permissions) with open(n_filename, "wb") as f: config.write(outfile=f) @@ -1110,7 +1111,7 @@ class RenewableCert(object): stat.S_IROTH) mode = BASE_PRIVKEY_MODE | old_mode os.chown(target["privkey"], -1, os.stat(old_privkey).st_gid) - os.chmod(target["privkey"], mode) + security.chmod(target["privkey"], mode) # Save everything else with open(target["cert"], "wb") as f: diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index 711346dbb..14260dd8c 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -12,6 +12,7 @@ import certbot.tests.util as test_util from certbot import account from certbot import errors from certbot.compat import os +from certbot.compat import security from certbot import util KEY = test_util.load_vector("rsa512_key.pem") @@ -423,7 +424,7 @@ class ClientTest(ClientTestCommon): # pylint: disable=too-many-locals certs = ["cert_512.pem", "cert-san_512.pem"] tmp_path = tempfile.mkdtemp() - os.chmod(tmp_path, 0o755) # TODO: really?? + security.chmod(tmp_path, 0o755) # TODO: really?? cert_pem = test_util.load_vector(certs[0]) chain_pem = (test_util.load_vector(certs[0]) + test_util.load_vector(certs[1])) diff --git a/certbot/tests/hook_test.py b/certbot/tests/hook_test.py index 2a3742aa2..718e3c229 100644 --- a/certbot/tests/hook_test.py +++ b/certbot/tests/hook_test.py @@ -7,6 +7,7 @@ from acme.magic_typing import List # pylint: disable=unused-import, no-name-in- from certbot import errors from certbot.compat import os +from certbot.compat import security from certbot.tests import util @@ -488,7 +489,7 @@ def create_hook(file_path): """ open(file_path, "w").close() - os.chmod(file_path, os.stat(file_path).st_mode | stat.S_IXUSR) + security.chmod(file_path, os.stat(file_path).st_mode | stat.S_IXUSR) if __name__ == '__main__': diff --git a/certbot/tests/storage_test.py b/certbot/tests/storage_test.py index 13c09395d..3fa133c3e 100644 --- a/certbot/tests/storage_test.py +++ b/certbot/tests/storage_test.py @@ -15,6 +15,7 @@ import certbot.tests.util as test_util from certbot import errors from certbot.compat import misc from certbot.compat import os +from certbot.compat import security from certbot.storage import ALL_FOUR CERT = test_util.load_cert('cert_512.pem') @@ -132,7 +133,7 @@ class BaseRenewableCertTest(test_util.ConfigTestCase): with open(link, "wb") as f: f.write(kind.encode('ascii') if value is None else value) if kind == "privkey": - os.chmod(link, 0o600) + security.chmod(link, 0o600) def _write_out_ex_kinds(self): for kind in ALL_FOUR: @@ -572,7 +573,7 @@ class RenewableCertTests(BaseRenewableCertTest): self.test_rc.update_all_links_to(1) self.assertTrue(misc.compare_file_modes( os.stat(self.test_rc.version("privkey", 1)).st_mode, 0o600)) - os.chmod(self.test_rc.version("privkey", 1), 0o444) + security.chmod(self.test_rc.version("privkey", 1), 0o444) # If no new key, permissions should be the same (we didn't write any keys) self.test_rc.save_successor(1, b"newcert", None, b"new chain", self.config) self.assertTrue(misc.compare_file_modes( @@ -582,7 +583,7 @@ class RenewableCertTests(BaseRenewableCertTest): self.assertTrue(misc.compare_file_modes( os.stat(self.test_rc.version("privkey", 3)).st_mode, 0o644)) # If permissions reverted, next renewal will also revert permissions of new key - os.chmod(self.test_rc.version("privkey", 3), 0o400) + security.chmod(self.test_rc.version("privkey", 3), 0o400) self.test_rc.save_successor(3, b"newcert", b"new_privkey", b"new chain", self.config) self.assertTrue(misc.compare_file_modes( os.stat(self.test_rc.version("privkey", 4)).st_mode, 0o600)) @@ -776,7 +777,7 @@ class RenewableCertTests(BaseRenewableCertTest): with open(temp, "w") as f: f.write("[renewalparams]\nuseful = value # A useful value\n" "useless = value # Not needed\n") - os.chmod(temp, 0o640) + security.chmod(temp, 0o640) target = {} for x in ALL_FOUR: target[x] = "somewhere" diff --git a/certbot/tests/util.py b/certbot/tests/util.py index 49ff6731b..cd7956657 100644 --- a/certbot/tests/util.py +++ b/certbot/tests/util.py @@ -27,6 +27,7 @@ from certbot import lock from certbot import storage from certbot import util from certbot.compat import os +from certbot.compat import security from certbot.display import util as display_util @@ -340,7 +341,7 @@ class TempDirTestCase(unittest.TestCase): def handle_rw_files(_, path, __): """Handle read-only files, that will fail to be removed on Windows.""" - os.chmod(path, stat.S_IWRITE) + security.chmod(path, stat.S_IWRITE) os.remove(path) shutil.rmtree(self.tempdir, onerror=handle_rw_files) diff --git a/certbot/tests/util_test.py b/certbot/tests/util_test.py index 6e231b74d..f7b539d7f 100644 --- a/certbot/tests/util_test.py +++ b/certbot/tests/util_test.py @@ -11,6 +11,7 @@ import certbot.tests.util as test_util from certbot import errors from certbot.compat import misc from certbot.compat import os +from certbot.compat import security class RunScriptTest(unittest.TestCase): @@ -188,17 +189,17 @@ class CheckPermissionsTest(test_util.TempDirTestCase): return check_permissions(self.tempdir, mode, self.uid) def test_ok_mode(self): - os.chmod(self.tempdir, 0o600) + security.chmod(self.tempdir, 0o600) self.assertTrue(self._call(0o600)) def test_wrong_mode(self): - os.chmod(self.tempdir, 0o400) + security.chmod(self.tempdir, 0o400) try: self.assertFalse(self._call(0o600)) finally: # Without proper write permissions, Windows is unable to delete a folder, # even with admin permissions. Write access must be explicitly set first. - os.chmod(self.tempdir, 0o700) + security.chmod(self.tempdir, 0o700) class UniqueFileTest(test_util.TempDirTestCase): diff --git a/setup.py b/setup.py index 09ffe8cb5..668cd41d8 100644 --- a/setup.py +++ b/setup.py @@ -52,6 +52,16 @@ install_requires = [ 'zope.interface', ] +# Add pywin32 on Windows platform to handle low-level system calls. +# This dependency needs to be added dynamically to avoid being here +# when certbot-oldest tests are launched. +# Indeed, during these tests, a requirements constraints file will be +# passed to pip, pip will ignore platform_system directive and try +# to install pywin32 on Unix systems. +# This would fail and break certbot-oldest tests. +if os.environ.get('CERTBOT_OLDEST') != '1': + install_requires.append('pywin32;platform_system=="Windows"') + dev_extras = [ 'astroid==1.6.5', 'coverage',