crypt/certbot
1
0
Fork 0
mirror of https://github.com/certbot/certbot.git synced 2025-09-10 22:11:53 +03:00
Code Activity

Implement umask for Windows (#7967)

Browse Source 
This PR gets its root from an observation I did on current version of Certbot (1.3.0): the `renewal-hooks` directory in Certbot configuration directory is created on Windows with write permissions to everybody.

I thought it was a critical bug since this directory contains hooks that are executed by Certbot, and you certainly do not want this folder to be open to any malicious hook that could be inserted by everyone, then executed with administrator privileges by Certbot.

Turns out for this specific problem that the bug is not critical for the hooks, because the scripts are expected to be in subdirectories of `renewal-hooks` (namely `pre`, `post` and `deploy`), and these subdirectories have proper permissions because we set them explicitly when Certbot is starting.

Still, there is a divergence here between Linux and Windows: on Linux all Certbot directories without explicit permissions have at maximum `0o755` permissions by default, while on Windows it is a `0o777` equivalent. It is not an immediate security risk, but it is definitly error-prone, not expected, and so a potential breach in the future if we forget about it.

Root cause is that umask is not existing in Windows. Indeed under Linux the umask defines the default permissions when you create a file or a directory. Python takes that into account, with an API for `os.open` and `os.mkdir` that expose a `mode` parameter with default value of `0o777`. In practice it is never `0o777` (either you the the `mode` explictly or left the default one) because the effective mode is masked by the current umask value in the system: on Linux it is `0o022`, so files/directories have a maximum mode of `0o755` if you did not set the umask explicitly, and it is what it is observed for Certbot.

However on Windows, the `mode` value passed (and got from default) to the `open` and `mkdir` of `certbot.compat.filesystem` module is taken verbatim, since umask does not exit, and then is used to calculate the DACL of the newly created file/directory. So if the mode is not set explicitly, we end up with files and directories with `0o777` permissions.

This PR fixes this problem by implementing a umask behavior in the `certbot.compat.filesystem` module, that will be applied to any file or directory created by Certbot since we forbid to use the `os` module directly.

The implementation is quite straight-forward. For Linux the behavior is not changed. On Windows a `mask` parameter is added to the function that calculates the DACL, to be invoked appropriately when file or directory are created. The actual value of the mask is taken from an internal class of the `filesystem` module: its default value is `0o755` to match default umasks on Linux, and can be changed with the new method `umask` that have the same behavior than the original `os.umask`. Of course `os.umask` becomes a forbidden function and `filesystem.umask` must be used instead.

Existing code that is impacted have been updated, and new unit tests are created for this new function.

* Implement umask for Windows

* Set umask at the beginning of tests

* Fix lint, update local oldest requirements

* Update certbot-apache/setup.py

Co-authored-by: Brad Warren <bmw@users.noreply.github.com>

* Improve tests

* Adapt filesystem.makedirs for Windows

* Fix

* Update certbot-apache/setup.py

Co-authored-by: Brad Warren <bmw@users.noreply.github.com>

* Changelog entries

* Fix lint

* Update certbot/CHANGELOG.md

Co-authored-by: Brad Warren <bmw@users.noreply.github.com>

Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
This commit is contained in:
Adrien Ferrand
2020-06-10 02:08:22 +02:00
committed by GitHub
parent e31834a6cd
commit 50fa04ba0c
8 changed files with 142 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
certbot-apache/certbot_apache/_internal/http_01.py
View File

@@ -169,7 +169,7 @@ class ApacheHttp01(common.ChallengePerformer):
def _set_up_challenges(self):
if not os.path.isdir(self.challenge_dir):
old_umask = os.umask(0o022)
old_umask = filesystem.umask(0o022)
try:
filesystem.makedirs(self.challenge_dir, 0o755)
except OSError as exception:
@@ -177,7 +177,7 @@ class ApacheHttp01(common.ChallengePerformer):
raise errors.PluginError(
"Couldn't create root for http-01 challenge")
finally:
os.umask(old_umask)
filesystem.umask(old_umask)
responses = []
for achall in self.achalls:

2
certbot-apache/local-oldest-requirements.txt
View File

@@ -1,3 +1,3 @@
# Remember to update setup.py to match the package versions below.
acme[dev]==0.29.0
certbot[dev]==1.1.0
-e certbot[dev]

2
certbot-apache/setup.py
View File

@@ -12,7 +12,7 @@ version = '1.6.0.dev0'
# acme/certbot version.
install_requires = [
'acme>=0.29.0',
'certbot>=1.1.0',
'certbot>=1.6.0.dev0',
'python-augeas',
'setuptools',
'zope.component',

5
certbot/CHANGELOG.md
View File

@@ -7,10 +7,15 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
### Added
* Add minimal code to run Nginx plugin on NetBSD.
* Function `certbot.compat.filesystem.umask` is a drop-in replacement for `os.umask`
implementing umask for both UNIX and Windows systems.
### Changed
* Allow session tickets to be disabled in Apache when mod_ssl is statically linked.
* Certbot behaves similarly on Windows to on UNIX systems regarding umask, and
the umask `022` is applied by default: all files/directories are not writable by anyone
other than the user running Certbot and the system/admin users.
* Read acmev1 Let's Encrypt server URL from renewal config as acmev2 URL to prepare
for impending acmev1 deprecation.

8
certbot/certbot/_internal/plugins/webroot.py
View File

@@ -164,7 +164,7 @@ to serve all files under specified web root ({0})."""
# Change the permissions to be writable (GH #1389)
# Umask is used instead of chmod to ensure the client can also
# run as non-root (GH #1795)
old_umask = os.umask(0o022)
old_umask = filesystem.umask(0o022)
try:
# We ignore the last prefix in the next iteration,
# as it does not correspond to a folder path ('/' or 'C:')
@@ -191,7 +191,7 @@ to serve all files under specified web root ({0})."""
"Couldn't create root for {0} http-01 "
"challenge responses: {1}".format(name, exception))
finally:
os.umask(old_umask)
filesystem.umask(old_umask)
def _get_validation_path(self, root_path, achall): # pylint: no-self-use
return os.path.join(root_path, achall.chall.encode("token"))
@@ -204,13 +204,13 @@ to serve all files under specified web root ({0})."""
logger.debug("Attempting to save validation to %s", validation_path)
# Change permissions to be world-readable, owner-writable (GH #1795)
old_umask = os.umask(0o022)
old_umask = filesystem.umask(0o022)
try:
with safe_open(validation_path, mode="wb", chmod=0o644) as validation_file:
validation_file.write(validation.encode())
finally:
os.umask(old_umask)
filesystem.umask(old_umask)
self.performed[root_path].add(achall)
return response

83
certbot/certbot/compat/filesystem.py
View File

@@ -21,6 +21,20 @@ else:
POSIX_MODE = False
# Windows umask implementation, since Windows does not have a concept of umask by default.
# We choose 022 as initial value since it is the default one on most Linux distributions, and
# it is a decent choice to not have write permissions for group owner and everybody by default.
# We use a class here to avoid needing to define a global variable, and the potential mistakes
# that could happen with this kind of pattern.
class _WindowsUmask:
"""Store the current umask to apply on Windows"""
def __init__(self):
self.mask = 0o022
_WINDOWS_UMASK = _WindowsUmask()
def chmod(file_path, mode):
# type: (str, int) -> None
"""
@@ -43,6 +57,24 @@ def chmod(file_path, mode):
_apply_win_mode(file_path, mode)
def umask(mask):
# type: (int) -> int
"""
Set the current numeric umask and return the previous umask. On Linux, the built-in umask
method is used. On Windows, our Certbot-side implementation is used.
:param int mask: The user file-creation mode mask to apply.
:rtype: int
:return: The previous umask value.
"""
if POSIX_MODE:
return os.umask(mask)
previous_umask = _WINDOWS_UMASK.mask
_WINDOWS_UMASK.mask = mask
return previous_umask
# One could ask why there is no copy_ownership() function, or even a reimplementation
# of os.chown() that would modify the ownership of file without touching the mode itself.
# This is because on Windows, it would require recalculating the existing DACL against
@@ -194,7 +226,7 @@ def open(file_path, flags, mode=0o777): # pylint: disable=redefined-builtin
attributes = win32security.SECURITY_ATTRIBUTES()
security = attributes.SECURITY_DESCRIPTOR
user = _get_current_user()
dacl = _generate_dacl(user, mode)
dacl = _generate_dacl(user, mode, _WINDOWS_UMASK.mask)
# We set second 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-setsecuritydescriptorowner # pylint: disable=line-too-long
@@ -244,28 +276,27 @@ def makedirs(file_path, mode=0o777):
:param int mode: POSIX mode to apply on leaf directory when created, Python defaults
will be applied if ``None``
"""
if POSIX_MODE:
# Since Python 3.7, os.makedirs does not set the given mode to the intermediate directories
# that could be created in the process. To keep things safe and consistent on all
# Python versions, we set the umask accordingly to have all directories (intermediate and
# leaf) created with the given mode.
current_umask = os.umask(0)
current_umask = umask(0)
try:
# Since Python 3.7, os.makedirs does not set the given mode to the intermediate
# directories that could be created in the process. To keep things safe and consistent
# on all Python versions, we set the umask accordingly to have all directories
# (intermediate and leaf) created with the given mode.
umask(current_umask | 0o777 ^ mode)
if POSIX_MODE:
return os.makedirs(file_path, mode)
orig_mkdir_fn = os.mkdir
try:
os.umask(current_umask | 0o777 ^ mode)
# As we know that os.mkdir is called internally by os.makedirs, we will swap the
# function in os module for the time of makedirs execution on Windows.
os.mkdir = mkdir # type: ignore
return os.makedirs(file_path, mode)
finally:
os.umask(current_umask)
# TODO: Windows does not support umask. A specific PR (#7967) is handling this, and will need
# to add appropriate umask call for the Windows part of the logic below.
orig_mkdir_fn = os.mkdir
try:
# As we know that os.mkdir is called internally by os.makedirs, we will swap the function in
# os module for the time of makedirs execution on Windows.
os.mkdir = mkdir # type: ignore
return os.makedirs(file_path, mode)
os.mkdir = orig_mkdir_fn
finally:
os.mkdir = orig_mkdir_fn
umask(current_umask)
def mkdir(file_path, mode=0o777):
@@ -284,7 +315,7 @@ def mkdir(file_path, mode=0o777):
attributes = win32security.SECURITY_ATTRIBUTES()
security = attributes.SECURITY_DESCRIPTOR
user = _get_current_user()
dacl = _generate_dacl(user, mode)
dacl = _generate_dacl(user, mode, _WINDOWS_UMASK.mask)
security.SetSecurityDescriptorOwner(user, False)
security.SetSecurityDescriptorDacl(1, dacl, 0)
@@ -322,6 +353,10 @@ def realpath(file_path):
"""
Find the real path for the given path. This method resolves symlinks, including
recursive symlinks, and is protected against symlinks that creates an infinite loop.
:param str file_path: The path to resolve
:returns: The real path for the given path
:rtype: str
"""
original_path = file_path
@@ -372,7 +407,7 @@ def is_executable(path):
def has_world_permissions(path):
# type: (str) -> bool
"""
Check if everybody/world has any right (read/write/execute) on a file given its path
Check if everybody/world has any right (read/write/execute) on a file given its path.
:param str path: path to test
:return: True if everybody/world has any right to the file
@@ -394,7 +429,7 @@ def has_world_permissions(path):
def compute_private_key_mode(old_key, base_mode):
# type: (str, int) -> int
"""
Calculate the POSIX mode to apply to a private key given the previous private key
Calculate the POSIX mode to apply to a private key given the previous private key.
:param str old_key: path to the previous private key
:param int base_mode: the minimum modes to apply to a private key
@@ -520,7 +555,9 @@ def _apply_win_mode(file_path, mode):
win32security.SetFileSecurity(file_path, win32security.DACL_SECURITY_INFORMATION, security)
def _generate_dacl(user_sid, mode):
def _generate_dacl(user_sid, mode, mask=None):
if mask:
mode = mode & (0o777 - mask)
analysis = _analyze_mode(mode)
# Get standard accounts from "well-known" sid

10
certbot/certbot/compat/os.py
View File

@@ -56,6 +56,16 @@ def chmod(*unused_args, **unused_kwargs):
'Use certbot.compat.filesystem.chmod() instead.')
# Since there is no mode on Windows, there is no umask either, and so this method is a noop for
# this platform. In order to have a consistent behavior between Linux and Windows on Certbot files
# and directories, the filesystem umask method must be used instead, since it implements umask for
# Windows.
def umask(*unused_args, **unused_kwargs):
"""Method os.chmod() is forbidden"""
raise RuntimeError('Usage of os.umask() is forbidden. '
'Use certbot.compat.filesystem.umask() instead.')
# Because uid is not a concept on Windows, chown is useless. In fact, it is not even available
# on Python for Windows. So to be consistent on both platforms for Certbot, this method is
# always forbidden.

70
certbot/tests/compat/filesystem_test.py
View File

@@ -151,6 +151,57 @@ class WindowsChmodTests(TempDirTestCase):
self.assertEqual(security_dacl.GetSecurityDescriptorDacl().GetAceCount(), 2)
class UmaskTest(TempDirTestCase):
def test_umask_on_dir(self):
previous_umask = filesystem.umask(0o022)
try:
dir1 = os.path.join(self.tempdir, 'probe1')
filesystem.mkdir(dir1)
self.assertTrue(filesystem.check_mode(dir1, 0o755))
filesystem.umask(0o077)
dir2 = os.path.join(self.tempdir, 'dir2')
filesystem.mkdir(dir2)
self.assertTrue(filesystem.check_mode(dir2, 0o700))
dir3 = os.path.join(self.tempdir, 'dir3')
filesystem.mkdir(dir3, mode=0o777)
self.assertTrue(filesystem.check_mode(dir3, 0o700))
finally:
filesystem.umask(previous_umask)
def test_umask_on_file(self):
previous_umask = filesystem.umask(0o022)
try:
file1 = os.path.join(self.tempdir, 'probe1')
UmaskTest._create_file(file1)
self.assertTrue(filesystem.check_mode(file1, 0o755))
filesystem.umask(0o077)
file2 = os.path.join(self.tempdir, 'probe2')
UmaskTest._create_file(file2)
self.assertTrue(filesystem.check_mode(file2, 0o700))
file3 = os.path.join(self.tempdir, 'probe3')
UmaskTest._create_file(file3)
self.assertTrue(filesystem.check_mode(file3, 0o700))
finally:
filesystem.umask(previous_umask)
@staticmethod
def _create_file(path, mode=0o777):
file_desc = None
try:
file_desc = filesystem.open(path, flags=os.O_CREAT, mode=mode)
finally:
if file_desc:
os.close(file_desc)
class ComputePrivateKeyModeTest(TempDirTestCase):
def setUp(self):
super(ComputePrivateKeyModeTest, self).setUp()
@@ -281,24 +332,21 @@ class WindowsMkdirTests(test_util.TempDirTestCase):
self.assertEqual(original_mkdir, std_os.mkdir)
# TODO: This test can be used both by Linux and Windows once on #7967
@unittest.skipUnless(POSIX_MODE, reason='Needs umask to succeed, and Windows does not have it')
class LinuxMkdirTests(test_util.TempDirTestCase):
"""Unit tests for Linux mkdir + makedirs functions in filesystem module"""
class MakedirsTests(test_util.TempDirTestCase):
"""Unit tests for makedirs function in filesystem module"""
def test_makedirs_correct_permissions(self):
path = os.path.join(self.tempdir, 'dir')
subpath = os.path.join(path, 'subpath')
previous_umask = os.umask(0o022)
previous_umask = filesystem.umask(0o022)
try:
filesystem.makedirs(subpath, 0o700)
import os as std_os # pylint: disable=os-module-forbidden
assert stat.S_IMODE(std_os.stat(path).st_mode) == 0o700
assert stat.S_IMODE(std_os.stat(subpath).st_mode) == 0o700
assert filesystem.check_mode(path, 0o700)
assert filesystem.check_mode(subpath, 0o700)
finally:
os.umask(previous_umask)
filesystem.umask(previous_umask)
class CopyOwnershipAndModeTest(test_util.TempDirTestCase):
@@ -513,8 +561,8 @@ class IsExecutableTest(test_util.TempDirTestCase):
from certbot.compat.filesystem import _generate_dacl
def _execute_mock(user_sid, mode):
dacl = _generate_dacl(user_sid, mode)
def _execute_mock(user_sid, mode, mask=None):
dacl = _generate_dacl(user_sid, mode, mask)
for _ in range(1, dacl.GetAceCount()):
dacl.DeleteAce(1) # DeleteAce dynamically updates the internal index mapping.
return dacl