mirror of
https://github.com/certbot/certbot.git
synced 2026-01-26 07:41:33 +03:00
[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 <ebportnoy@gmail.com>
* Update certbot/util.py
Co-Authored-By: ohemorange <ebportnoy@gmail.com>
* Update certbot/lock.py
Co-Authored-By: ohemorange <ebportnoy@gmail.com>
* Update certbot/compat/os.py
Co-Authored-By: ohemorange <ebportnoy@gmail.com>
* Update certbot/lock.py
Co-Authored-By: ohemorange <ebportnoy@gmail.com>
* Update certbot/compat/os.py
Co-Authored-By: ohemorange <ebportnoy@gmail.com>
* Simplify and make more clear comment on os.open.
* Secure implementation preventing race conditions
* Revert "Secure implementation preventing race conditions"
This reverts commit dbb8549219.
* 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 <ebportnoy@gmail.com>
* Update certbot/compat/os.py
Co-Authored-By: ohemorange <ebportnoy@gmail.com>
* Update certbot/compat/os.py
Co-Authored-By: ohemorange <ebportnoy@gmail.com>
* Update certbot/compat/filesystem.py
Co-Authored-By: ohemorange <ebportnoy@gmail.com>
* Add some comments
* Fix pylint
* Improve docstring
* Added test cases
* Improve docstring
* Update certbot/lock.py
Co-Authored-By: ohemorange <ebportnoy@gmail.com>
* Update certbot/lock.py
Co-Authored-By: ohemorange <ebportnoy@gmail.com>
* Fix lint
* Adapt coverage
* Adapt coverage
This commit is contained in:
Adrien Ferrand
committed by
ohemorange
ohemorange
parent
448d159223
commit
88876b9901
@@ -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:
|
||||
|
||||
@@ -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]
|
||||
|
||||
@@ -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):
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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"""
|
||||
|
||||
|
||||
@@ -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))
|
||||
|
||||
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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")
|
||||
|
||||
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
|
||||
@@ -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},
|
||||
|
||||
Reference in New Issue
Block a user