Implement security.chmod

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
 

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"
 

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.')

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

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))

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)

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")

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:

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]))

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__':

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"

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)
 

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):

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',