From d75908c645e0d13827b60c4c94491e0c41859ad4 Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Wed, 12 Jun 2019 02:08:48 +0200 Subject: [PATCH] [Windows] Security model for files permissions - STEP 3b (#6965) * Modifications for misc * Add some types * Use os_rename * Move rename into filesystem * Use our os package * Rename filesystem.rename to filesystem.replace * Disable globally function redefined lint in os module --- certbot/compat/filesystem.py | 20 ++++++++ certbot/compat/misc.py | 63 ++++++------------------- certbot/compat/os.py | 17 +++++++ certbot/main.py | 2 +- certbot/reverter.py | 3 +- certbot/storage.py | 6 +-- certbot/tests/compat/compat_test.py | 21 --------- certbot/tests/compat/filesystem_test.py | 21 +++++++++ certbot/tests/reverter_test.py | 6 +-- 9 files changed, 82 insertions(+), 77 deletions(-) create mode 100644 certbot/compat/filesystem.py delete mode 100644 certbot/tests/compat/compat_test.py create mode 100644 certbot/tests/compat/filesystem_test.py diff --git a/certbot/compat/filesystem.py b/certbot/compat/filesystem.py new file mode 100644 index 000000000..8025c9ad0 --- /dev/null +++ b/certbot/compat/filesystem.py @@ -0,0 +1,20 @@ +"""Compat module to handle files security on Windows and Linux""" +from __future__ import absolute_import + +import os # pylint: disable=os-module-forbidden + + +def replace(src, dst): + # type: (str, str) -> None + """ + Rename a file to a destination path and handles situations where the destination exists. + :param str src: The current file path. + :param str dst: The new file path. + """ + if hasattr(os, 'replace'): + # Use replace if possible. On Windows, only Python >= 3.4 is supported + # so we can assume that os.replace() is always available for this platform. + getattr(os, 'replace')(src, dst) + else: + # Otherwise, use os.rename() that behaves like os.replace() on Linux. + os.rename(src, dst) diff --git a/certbot/compat/misc.py b/certbot/compat/misc.py index 4f0e22078..c61672364 100644 --- a/certbot/compat/misc.py +++ b/certbot/compat/misc.py @@ -2,42 +2,31 @@ This compat module handles various platform specific calls that do not fall into one particular category. """ -import ctypes -import errno +from __future__ import absolute_import + import select import stat import sys +try: + from win32com.shell import shell as shellwin32 # pylint: disable=import-error + POSIX_MODE = False +except ImportError: # pragma: no cover + POSIX_MODE = True + from certbot import errors from certbot.compat import os -UNPRIVILEGED_SUBCOMMANDS_ALLOWED = [ - 'certificates', 'enhance', 'revoke', 'delete', - 'register', 'unregister', 'config_changes', 'plugins'] - - -def raise_for_non_administrative_windows_rights(subcommand): +def raise_for_non_administrative_windows_rights(): + # type: () -> None """ On Windows, raise if current shell does not have the administrative rights. Do nothing on Linux. - :param str subcommand: The subcommand (like 'certonly') passed to the certbot client. - - :raises .errors.Error: If the provided subcommand must be run on a shell with - administrative rights, and current shell does not have these rights. - + :raises .errors.Error: If the current shell does not have administrative rights on Windows. """ - # Why not simply try ctypes.windll.shell32.IsUserAnAdmin() and catch AttributeError ? - # Because windll exists only on a Windows runtime, and static code analysis engines - # do not like at all non existent objects when run from Linux (even if we handle properly - # all the cases in the code). - # So we access windll only by reflection to trick these engines. - if hasattr(ctypes, 'windll') and subcommand not in UNPRIVILEGED_SUBCOMMANDS_ALLOWED: - windll = getattr(ctypes, 'windll') - if windll.shell32.IsUserAnAdmin() == 0: - raise errors.Error( - 'Error, "{0}" subcommand must be run on a shell with administrative rights.' - .format(subcommand)) + if not POSIX_MODE and shellwin32.IsUserAnAdmin() == 0: # pragma: no cover + raise errors.Error('Error, certbot must be run on a shell with administrative rights.') def os_geteuid(): @@ -56,31 +45,8 @@ def os_geteuid(): return 0 -def os_rename(src, dst): - """ - Rename a file to a destination path and handles situations where the destination exists. - - :param str src: The current file path. - :param str dst: The new file path. - """ - try: - os.rename(src, dst) - except OSError as err: - # Windows specific, renaming a file on an existing path is not possible. - # On Python 3, the best fallback with atomic capabilities we have is os.replace. - if err.errno != errno.EEXIST: - # Every other error is a legitimate exception. - raise - if not hasattr(os, 'replace'): # pragma: no cover - # We should never go on this line. Either we are on Linux and os.rename has succeeded, - # or we are on Windows, and only Python >= 3.4 is supported where os.replace is - # available. - raise RuntimeError('Error: tried to run os_rename on Python < 3.3. ' - 'Certbot supports only Python 3.4 >= on Windows.') - getattr(os, 'replace')(src, dst) - - def readline_with_timeout(timeout, prompt): + # type: (float, str) -> str """ Read user input to return the first line entered, or raise after specified timeout. @@ -132,6 +98,7 @@ LINUX_DEFAULT_FOLDERS = { def get_default_folder(folder_type): + # type: (str) -> str """ Return the relevant default folder for the current OS diff --git a/certbot/compat/os.py b/certbot/compat/os.py index 8a139a141..68f4e7cf6 100644 --- a/certbot/compat/os.py +++ b/certbot/compat/os.py @@ -3,6 +3,7 @@ This compat modules is a wrapper of the core os module that forbids usage of spe (e.g. chown, chmod, getuid) that would be harmful to the Windows file security model of Certbot. This module is intended to replace standard os module throughout certbot projects (except acme). """ +# pylint: disable=function-redefined from __future__ import absolute_import # First round of wrapping: we import statically all public attributes exposed by the os module @@ -29,3 +30,19 @@ std_sys.modules[__name__ + '.path'] = path # Clean all remaining importables that are not from the core os module. del ourselves, std_os, std_sys + + +# Because of the blocking strategy on file handlers on Windows, rename does not behave as expected +# with POSIX systems: an exception will be raised if dst already exists. +def rename(*unused_args, **unused_kwargs): + """Method os.rename() is forbidden""" + raise RuntimeError('Usage of os.rename() is forbidden. ' + 'Use certbot.compat.filesystem.replace() instead.') + + +# Behavior of os.replace is consistent between Windows and Linux. However, it is not supported on +# Python 2.x. So, as for os.rename, we forbid it in favor of filesystem.replace. +def replace(*unused_args, **unused_kwargs): + """Method os.replace() is forbidden""" + raise RuntimeError('Usage of os.replace() is forbidden. ' + 'Use certbot.compat.filesystem.replace() instead.') diff --git a/certbot/main.py b/certbot/main.py index 9ed44cceb..d5239fa11 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -1359,7 +1359,7 @@ def main(cli_args=None): # On windows, shell without administrative right cannot create symlinks required by certbot. # So we check the rights before continuing. - misc.raise_for_non_administrative_windows_rights(config.verb) + misc.raise_for_non_administrative_windows_rights() try: log.post_arg_parse_setup(config) diff --git a/certbot/reverter.py b/certbot/reverter.py index c7992e73c..f3088ed7e 100644 --- a/certbot/reverter.py +++ b/certbot/reverter.py @@ -16,6 +16,7 @@ from certbot import interfaces from certbot import util from certbot.compat import misc from certbot.compat import os +from certbot.compat import filesystem logger = logging.getLogger(__name__) @@ -583,7 +584,7 @@ class Reverter(object): timestamp = self._checkpoint_timestamp() final_dir = os.path.join(self.config.backup_dir, timestamp) try: - misc.os_rename(self.config.in_progress_dir, final_dir) + filesystem.replace(self.config.in_progress_dir, final_dir) return except OSError: logger.warning("Extreme, unexpected race condition, retrying (%s)", timestamp) diff --git a/certbot/storage.py b/certbot/storage.py index 01e012920..048b224d7 100644 --- a/certbot/storage.py +++ b/certbot/storage.py @@ -18,8 +18,8 @@ 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 from certbot.plugins import disco as plugins_disco @@ -162,7 +162,7 @@ def rename_renewal_config(prev_name, new_name, cli_config): raise errors.ConfigurationError("The new certificate name " "is already in use.") try: - os.rename(prev_filename, new_filename) + filesystem.replace(prev_filename, new_filename) except OSError: raise errors.ConfigurationError("Please specify a valid filename " "for the new certificate name.") @@ -191,7 +191,7 @@ def update_configuration(lineagename, archive_dir, target, cli_config): # Save only the config items that are relevant to renewal values = relevant_values(vars(cli_config.namespace)) write_renewal_config(config_filename, temp_filename, archive_dir, target, values) - misc.os_rename(temp_filename, config_filename) + filesystem.replace(temp_filename, config_filename) return configobj.ConfigObj(config_filename) diff --git a/certbot/tests/compat/compat_test.py b/certbot/tests/compat/compat_test.py deleted file mode 100644 index 832c557f6..000000000 --- a/certbot/tests/compat/compat_test.py +++ /dev/null @@ -1,21 +0,0 @@ -"""Tests for certbot.compat.""" -import certbot.tests.util as test_util -from certbot.compat import misc -from certbot.compat import os - - -class OsReplaceTest(test_util.TempDirTestCase): - """Test to ensure consistent behavior of os_rename method""" - - def test_os_rename_to_existing_file(self): - """Ensure that os_rename will effectively rename src into dst for all platforms.""" - src = os.path.join(self.tempdir, 'src') - dst = os.path.join(self.tempdir, 'dst') - open(src, 'w').close() - open(dst, 'w').close() - - # On Windows, a direct call to os.rename will fail because dst already exists. - misc.os_rename(src, dst) - - self.assertFalse(os.path.exists(src)) - self.assertTrue(os.path.exists(dst)) diff --git a/certbot/tests/compat/filesystem_test.py b/certbot/tests/compat/filesystem_test.py new file mode 100644 index 000000000..591ee87c3 --- /dev/null +++ b/certbot/tests/compat/filesystem_test.py @@ -0,0 +1,21 @@ +"""Tests for certbot.compat.filesystem""" +import certbot.tests.util as test_util +from certbot.compat import os +from certbot.compat import filesystem + + +class OsReplaceTest(test_util.TempDirTestCase): + """Test to ensure consistent behavior of rename method""" + + def test_os_replace_to_existing_file(self): + """Ensure that replace will effectively rename src into dst for all platforms.""" + src = os.path.join(self.tempdir, 'src') + dst = os.path.join(self.tempdir, 'dst') + open(src, 'w').close() + open(dst, 'w').close() + + # On Windows, a direct call to os.rename would fail because dst already exists. + filesystem.replace(src, dst) + + self.assertFalse(os.path.exists(src)) + self.assertTrue(os.path.exists(dst)) diff --git a/certbot/tests/reverter_test.py b/certbot/tests/reverter_test.py index f90708a69..6a805072b 100644 --- a/certbot/tests/reverter_test.py +++ b/certbot/tests/reverter_test.py @@ -347,11 +347,11 @@ class TestFullCheckpointsReverter(test_util.ConfigTestCase): self.assertRaises( errors.ReverterError, self.reverter.finalize_checkpoint, "Title") - @mock.patch("certbot.reverter.misc.os_rename") - def test_finalize_checkpoint_no_rename_directory(self, mock_rename): + @mock.patch("certbot.reverter.filesystem.replace") + def test_finalize_checkpoint_no_rename_directory(self, mock_replace): self.reverter.add_to_checkpoint(self.sets[0], "perm save") - mock_rename.side_effect = OSError + mock_replace.side_effect = OSError self.assertRaises( errors.ReverterError, self.reverter.finalize_checkpoint, "Title")