From 88876b990178006ee52a4ed2324d6bdf8a78cb9f Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Wed, 3 Jul 2019 01:21:24 +0200 Subject: [PATCH] [Windows] Security model for files permissions - STEP 3c (#6967) * Implement security.open * Clean lint * Rename security into filesystem * Update certbot/compat/filesystem.py Co-Authored-By: ohemorange * Update certbot/util.py Co-Authored-By: ohemorange * Update certbot/lock.py Co-Authored-By: ohemorange * Update certbot/compat/os.py Co-Authored-By: ohemorange * Update certbot/lock.py Co-Authored-By: ohemorange * Update certbot/compat/os.py Co-Authored-By: ohemorange * Simplify and make more clear comment on os.open. * Secure implementation preventing race conditions * Revert "Secure implementation preventing race conditions" This reverts commit dbb85492195122020ca0b4a685ddb4836fdc6d12. * Simplify the logic on Windows. * Implement os.open to prevent race conditions * Add unit tests * Handle os.O_CREAT and os.O_EXCL directly from the Windows APIs * Improve comments * Use CREATE_ALWAYS * Adapt coverage threshold to new Windows specific LOCs. * Update certbot/compat/os.py Co-Authored-By: ohemorange * Update certbot/compat/os.py Co-Authored-By: ohemorange * Update certbot/compat/os.py Co-Authored-By: ohemorange * Update certbot/compat/filesystem.py Co-Authored-By: ohemorange * Add some comments * Fix pylint * Improve docstring * Added test cases * Improve docstring * Update certbot/lock.py Co-Authored-By: ohemorange * Update certbot/lock.py Co-Authored-By: ohemorange * Fix lint * Adapt coverage * Adapt coverage --- .codecov.yml | 2 +- certbot/compat/filesystem.py | 88 ++++++++++++++++++++++++- certbot/compat/os.py | 13 +++- certbot/lock.py | 11 +++- certbot/plugins/storage.py | 9 ++- certbot/plugins/storage_test.py | 2 +- certbot/tests/compat/filesystem_test.py | 83 +++++++++++++++++++++-- certbot/tests/compat/os_test.py | 2 +- certbot/tests/hook_test.py | 2 +- certbot/tests/util_test.py | 2 +- certbot/util.py | 11 ++-- test | 1 + tox.cover.py | 2 +- 13 files changed, 202 insertions(+), 26 deletions(-) create mode 120000 test diff --git a/.codecov.yml b/.codecov.yml index af67ea27f..3a7be503d 100644 --- a/.codecov.yml +++ b/.codecov.yml @@ -6,7 +6,7 @@ coverage: flags: linux # Fixed target instead of auto set by #7173, can # be removed when flags in Codecov are added back. - target: 98.0 + target: 97.8 threshold: 0.1 base: auto windows: diff --git a/certbot/compat/filesystem.py b/certbot/compat/filesystem.py index 5dc01a622..8cdff17d3 100644 --- a/certbot/compat/filesystem.py +++ b/certbot/compat/filesystem.py @@ -1,12 +1,20 @@ """Compat module to handle files security on Windows and Linux""" from __future__ import absolute_import +import errno import os # pylint: disable=os-module-forbidden import stat try: - import ntsecuritycon # pylint: disable=import-error - import win32security # pylint: disable=import-error + # pylint: disable=import-error + import ntsecuritycon + import win32security + import win32con + import win32api + import win32file + import pywintypes + import winerror + # pylint: enable=import-error except ImportError: POSIX_MODE = True else: @@ -36,6 +44,68 @@ def chmod(file_path, mode): _apply_win_mode(file_path, mode) +def open(file_path, flags, mode=0o777): # pylint: disable=redefined-builtin + # type: (str, int, int) -> int + """ + Wrapper of original os.open function, that will ensure on Windows that given mode + is correctly applied. + :param str file_path: The file path to open + :param int flags: Flags to apply on file while opened + :param int mode: POSIX mode to apply on file when opened, + Python defaults will be applied if ``None`` + :returns: the file descriptor to the opened file + :rtype: int + :raise: OSError(errno.EEXIST) if the file already exists and os.O_CREAT & os.O_EXCL are set, + OSError(errno.EACCES) on Windows if the file already exists and is a directory, and + os.O_CREAT is set. + """ + if POSIX_MODE: + # On Linux, invoke os.open directly. + return os.open(file_path, flags, mode) + + # Windows: handle creation of the file atomically with proper permissions. + if flags & os.O_CREAT: + # If os.O_EXCL is set, we will use the "CREATE_NEW", that will raise an exception if + # file exists, matching the API contract of this bit flag. Otherwise, we use + # "CREATE_ALWAYS" that will always create the file whether it exists or not. + disposition = win32con.CREATE_NEW if flags & os.O_EXCL else win32con.CREATE_ALWAYS + + attributes = win32security.SECURITY_ATTRIBUTES() + security = attributes.SECURITY_DESCRIPTOR + user = _get_current_user() + dacl = _generate_dacl(user, mode) + # We set first parameter to 1 (`True`) to say that this security descriptor contains + # a DACL. Otherwise second and third parameters are ignored. + # We set third parameter to 0 (`False`) to say that this security descriptor is + # NOT constructed from a default mechanism, but is explicitly set by the user. + # See https://docs.microsoft.com/en-us/windows/desktop/api/securitybaseapi/nf-securitybaseapi-setsecuritydescriptordacl # pylint: disable=line-too-long + security.SetSecurityDescriptorDacl(1, dacl, 0) + + try: + handle = win32file.CreateFile(file_path, win32file.GENERIC_READ, + win32file.FILE_SHARE_READ & win32file.FILE_SHARE_WRITE, + attributes, disposition, 0, None) + handle.Close() + except pywintypes.error as err: + # Handle native windows errors into python errors to be consistent with the API + # of os.open in the situation of a file already existing or locked. + if err.winerror == winerror.ERROR_FILE_EXISTS: + raise OSError(errno.EEXIST, err.strerror) + if err.winerror == winerror.ERROR_SHARING_VIOLATION: + raise OSError(errno.EACCES, err.strerror) + raise err + + # At this point, the file that did not exist has been created with proper permissions, + # so os.O_CREAT and os.O_EXCL are not needed anymore. We remove them from the flags to + # avoid a FileExists exception before calling os.open. + return os.open(file_path, flags ^ os.O_CREAT ^ os.O_EXCL) + + # Windows: general case, we call os.open, let exceptions be thrown, then chmod if all is fine. + handle = os.open(file_path, flags) + chmod(file_path, mode) + return handle + + def replace(src, dst): # type: (str, str) -> None """ @@ -173,3 +243,17 @@ def _compare_dacls(dacl1, dacl2): """ return ([dacl1.GetAce(index) for index in range(0, dacl1.GetAceCount())] == [dacl2.GetAce(index) for index in range(0, dacl2.GetAceCount())]) + + +def _get_current_user(): + """ + Return the pySID corresponding to the current user. + """ + account_name = win32api.GetUserNameEx(win32api.NameSamCompatible) + # LookupAccountName() expects the system name as first parameter. By passing None to it, + # we instruct Windows to first search the matching account in the machine local accounts, + # then into the primary domain accounts, if the machine has joined a domain, then finally + # into the trusted domains accounts. This is the preferred lookup mechanism to use in Windows + # if there is no reason to use a specific lookup mechanism. + # See https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-lookupaccountnamea + return win32security.LookupAccountName(None, account_name)[0] diff --git a/certbot/compat/os.py b/certbot/compat/os.py index f704055f4..a7de464fa 100644 --- a/certbot/compat/os.py +++ b/certbot/compat/os.py @@ -45,12 +45,23 @@ del ourselves, std_os, std_sys # 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 + "System" user, as they can do everything anyway. -def chmod(*unused_args, **unused_kwargs): # pylint: disable=function-redefined +def chmod(*unused_args, **unused_kwargs): """Method os.chmod() is forbidden""" raise RuntimeError('Usage of os.chmod() is forbidden. ' 'Use certbot.compat.filesystem.chmod() 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 +# atomically set in case of file creation, or invokes filesystem.chmod to properly set the +# permissions for the other cases. +def open(*unused_args, **unused_kwargs): + """Method os.open() is forbidden""" + raise RuntimeError('Usage of os.open() is forbidden. ' + 'Use certbot.compat.filesystem.open() instead.') + + # 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): diff --git a/certbot/lock.py b/certbot/lock.py index fad8a5175..cdb0fbb3c 100644 --- a/certbot/lock.py +++ b/certbot/lock.py @@ -13,6 +13,7 @@ from acme.magic_typing import Optional # pylint: disable=unused-import, no-name from certbot import errors from certbot.compat import os +from certbot.compat import filesystem logger = logging.getLogger(__name__) @@ -131,7 +132,7 @@ class _UnixLockMechanism(_BaseLockMechanism): """Acquire the lock.""" while self._fd is None: # Open the file - fd = os.open(self._path, os.O_CREAT | os.O_WRONLY, 0o600) + fd = filesystem.open(self._path, os.O_CREAT | os.O_WRONLY, 0o600) try: self._try_lock(fd) if self._lock_success(fd): @@ -223,11 +224,15 @@ class _WindowsLockMechanism(_BaseLockMechanism): """Acquire the lock""" open_mode = os.O_RDWR | os.O_CREAT | os.O_TRUNC - fd = os.open(self._path, open_mode, 0o600) + fd = None try: + # Under Windows, filesystem.open will raise directly an EACCES error + # if the lock file is already locked. + fd = filesystem.open(self._path, open_mode, 0o600) msvcrt.locking(fd, msvcrt.LK_NBLCK, 1) except (IOError, OSError) as err: - os.close(fd) + if fd: + os.close(fd) # Anything except EACCES is unexpected. Raise directly the error in that case. if err.errno != errno.EACCES: raise diff --git a/certbot/plugins/storage.py b/certbot/plugins/storage.py index 51350802c..294dfa0e8 100644 --- a/certbot/plugins/storage.py +++ b/certbot/plugins/storage.py @@ -6,9 +6,11 @@ from acme.magic_typing import Any, Dict # pylint: disable=unused-import, no-nam from certbot import errors from certbot.compat import os +from certbot.compat import filesystem logger = logging.getLogger(__name__) + class PluginStorage(object): """Class implementing storage functionality for plugins""" @@ -84,9 +86,10 @@ class PluginStorage(object): logger.error(errmsg) raise errors.PluginStorageError(errmsg) try: - with os.fdopen(os.open(self._storagepath, - os.O_WRONLY | os.O_CREAT | os.O_TRUNC, - 0o600), 'w') as fh: + with os.fdopen(filesystem.open( + self._storagepath, + os.O_WRONLY | os.O_CREAT | os.O_TRUNC, + 0o600), 'w') as fh: fh.write(serialized) except IOError as e: errmsg = "Could not write PluginStorage data to file {0} : {1}".format( diff --git a/certbot/plugins/storage_test.py b/certbot/plugins/storage_test.py index 2fa2c0345..3251e6b29 100644 --- a/certbot/plugins/storage_test.py +++ b/certbot/plugins/storage_test.py @@ -72,7 +72,7 @@ class PluginStorageTest(test_util.ConfigTestCase): def test_save_errors_unable_to_write_file(self): mock_open = mock.mock_open() mock_open.side_effect = IOError - with mock.patch("certbot.compat.os.open", mock_open): + with mock.patch("certbot.compat.filesystem.open", mock_open): with mock.patch("certbot.plugins.storage.logger.error") as mock_log: self.plugin.storage._data = {"valid": "data"} # pylint: disable=protected-access self.plugin.storage._initialized = True # pylint: disable=protected-access diff --git a/certbot/tests/compat/filesystem_test.py b/certbot/tests/compat/filesystem_test.py index 3d1363e6a..fc2b3f80f 100644 --- a/certbot/tests/compat/filesystem_test.py +++ b/certbot/tests/compat/filesystem_test.py @@ -1,15 +1,19 @@ """Tests for certbot.compat.filesystem""" +import errno import unittest try: - import win32api # pylint: disable=import-error - import win32security # pylint: disable=import-error - import ntsecuritycon # pylint: disable=import-error + # pylint: disable=import-error + import win32api + import win32security + import ntsecuritycon + # pylint: enable=import-error POSIX_MODE = False except ImportError: POSIX_MODE = True import certbot.tests.util as test_util +from certbot import lock from certbot.compat import os from certbot.compat import filesystem from certbot.tests.util import TempDirTestCase @@ -20,7 +24,7 @@ SYSTEM_SID = 'S-1-5-18' ADMINS_SID = 'S-1-5-32-544' -@unittest.skipIf(POSIX_MODE, reason='Test specific to Windows security') +@unittest.skipIf(POSIX_MODE, reason='Tests specific to Windows security') class WindowsChmodTests(TempDirTestCase): """Unit tests for Windows chmod function in filesystem module""" def setUp(self): @@ -154,6 +158,77 @@ class WindowsChmodTests(TempDirTestCase): self.assertEqual(security_dacl.GetSecurityDescriptorDacl().GetAceCount(), 2) +@unittest.skipIf(POSIX_MODE, reason='Tests specific to Windows security') +class WindowsOpenTest(TempDirTestCase): + def test_new_file_correct_permissions(self): + path = os.path.join(self.tempdir, 'file') + + desc = filesystem.open(path, os.O_CREAT | os.O_EXCL | os.O_RDWR, 0o700) + os.close(desc) + + dacl = _get_security_dacl(path).GetSecurityDescriptorDacl() + everybody = win32security.ConvertStringSidToSid(EVERYBODY_SID) + + self.assertFalse([dacl.GetAce(index) for index in range(0, dacl.GetAceCount()) + if dacl.GetAce(index)[2] == everybody]) + + def test_existing_file_correct_permissions(self): + path = os.path.join(self.tempdir, 'file') + open(path, 'w').close() + + desc = filesystem.open(path, os.O_EXCL | os.O_RDWR, 0o700) + os.close(desc) + + dacl = _get_security_dacl(path).GetSecurityDescriptorDacl() + everybody = win32security.ConvertStringSidToSid(EVERYBODY_SID) + + self.assertFalse([dacl.GetAce(index) for index in range(0, dacl.GetAceCount()) + if dacl.GetAce(index)[2] == everybody]) + + def test_create_file_on_open(self): + # os.O_CREAT | os.O_EXCL + file not exists = OK + self._test_one_creation(1, file_exist=False, flags=(os.O_CREAT | os.O_EXCL)) + + # os.O_CREAT | os.O_EXCL + file exists = EEXIST OS exception + with self.assertRaises(OSError) as raised: + self._test_one_creation(2, file_exist=True, flags=(os.O_CREAT | os.O_EXCL)) + self.assertEqual(raised.exception.errno, errno.EEXIST) + + # os.O_CREAT + file not exists = OK + self._test_one_creation(3, file_exist=False, flags=os.O_CREAT) + + # os.O_CREAT + file exists = OK + self._test_one_creation(4, file_exist=True, flags=os.O_CREAT) + + # os.O_CREAT + file exists (locked) = EACCES OS exception + path = os.path.join(self.tempdir, '5') + open(path, 'w').close() + filelock = lock.LockFile(path) + try: + with self.assertRaises(OSError) as raised: + self._test_one_creation(5, file_exist=True, flags=os.O_CREAT) + self.assertEqual(raised.exception.errno, errno.EACCES) + finally: + filelock.release() + + # os.O_CREAT not set + file not exists = OS exception + with self.assertRaises(OSError): + self._test_one_creation(6, file_exist=False, flags=os.O_RDONLY) + + def _test_one_creation(self, num, file_exist, flags): + one_file = os.path.join(self.tempdir, str(num)) + if file_exist and not os.path.exists(one_file): + open(one_file, 'w').close() + + handler = None + try: + handler = filesystem.open(one_file, flags) + except BaseException as err: + if handler: + os.close(handler) + raise err + + 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 85a3fa9f4..41b99d06e 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', 'rename', 'replace']: + for method in ['chmod', 'open', 'rename', 'replace']: self.assertRaises(RuntimeError, getattr(os, method)) diff --git a/certbot/tests/hook_test.py b/certbot/tests/hook_test.py index ef40d674a..fd2320494 100644 --- a/certbot/tests/hook_test.py +++ b/certbot/tests/hook_test.py @@ -42,7 +42,7 @@ class ValidateHookTest(util.TempDirTestCase): def test_not_executable(self): file_path = os.path.join(self.tempdir, "foo") # create a non-executable file - os.close(os.open(file_path, os.O_CREAT | os.O_WRONLY, 0o666)) + os.close(filesystem.open(file_path, os.O_CREAT | os.O_WRONLY, 0o666)) # prevent unnecessary modifications to PATH with mock.patch("certbot.hooks.plug_util.path_surgery"): self.assertRaises(errors.HookCommandNotFound, diff --git a/certbot/tests/util_test.py b/certbot/tests/util_test.py index dccd96ab5..38bf370e5 100644 --- a/certbot/tests/util_test.py +++ b/certbot/tests/util_test.py @@ -288,7 +288,7 @@ class UniqueLineageNameTest(test_util.TempDirTestCase): f.close() def test_failure(self): - with mock.patch("certbot.util.os.open", side_effect=OSError(errno.EIO)): + with mock.patch("certbot.compat.filesystem.open", side_effect=OSError(errno.EIO)): self.assertRaises(OSError, self._call, "wow") diff --git a/certbot/util.py b/certbot/util.py index 66e5d2524..097edd05d 100644 --- a/certbot/util.py +++ b/certbot/util.py @@ -23,6 +23,7 @@ from certbot import errors from certbot import lock from certbot.compat import misc from certbot.compat import os +from certbot.compat import filesystem logger = logging.getLogger(__name__) @@ -206,24 +207,20 @@ def check_permissions(filepath, mode, uid=0): return misc.compare_file_modes(file_stat.st_mode, mode) and file_stat.st_uid == uid -def safe_open(path, mode="w", chmod=None, buffering=None): +def safe_open(path, mode="w", chmod=None): """Safely open a file. :param str path: Path to a file. :param str mode: Same os `mode` for `open`. - :param int chmod: Same as `mode` for `os.open`, uses Python defaults + :param int chmod: Same as `mode` for `filesystem.open`, uses Python defaults if ``None``. - :param int buffering: Same as `bufsize` for `os.fdopen`, uses Python - defaults if ``None``. """ open_args = () # type: Union[Tuple[()], Tuple[int]] if chmod is not None: open_args = (chmod,) fdopen_args = () # type: Union[Tuple[()], Tuple[int]] - if buffering is not None: - fdopen_args = (buffering,) - fd = os.open(path, os.O_CREAT | os.O_EXCL | os.O_RDWR, *open_args) + fd = filesystem.open(path, os.O_CREAT | os.O_EXCL | os.O_RDWR, *open_args) return os.fdopen(fd, mode, *fdopen_args) diff --git a/test b/test new file mode 120000 index 000000000..bf39a010e --- /dev/null +++ b/test @@ -0,0 +1 @@ +tox.ini \ No newline at end of file diff --git a/tox.cover.py b/tox.cover.py index 0a94cb73f..c313419ed 100755 --- a/tox.cover.py +++ b/tox.cover.py @@ -12,7 +12,7 @@ DEFAULT_PACKAGES = [ 'certbot_dns_sakuracloud', 'certbot_nginx', 'letshelp_certbot'] COVER_THRESHOLDS = { - 'certbot': {'linux': 97, 'windows': 96}, + 'certbot': {'linux': 96, 'windows': 96}, 'acme': {'linux': 100, 'windows': 99}, 'certbot_apache': {'linux': 100, 'windows': 100}, 'certbot_dns_cloudflare': {'linux': 98, 'windows': 98},