From 21ef8e4332b41a7dd87ac83e548620cefe794d68 Mon Sep 17 00:00:00 2001 From: Will Greenberg Date: Mon, 14 Nov 2022 14:35:29 -0800 Subject: [PATCH] main: set more permissive umask when creating work_dir (#9448) * main: set more permissive umask when creating work_dir This'll guarantee our working dir has the appropriate permissions, even when a user has a strict umask * update changelog Co-authored-by: Brad Warren --- .../certbot_apache/_internal/http_01.py | 16 ++++++------ certbot/CHANGELOG.md | 3 +-- certbot/certbot/_internal/main.py | 5 +++- certbot/certbot/_internal/plugins/webroot.py | 11 ++------ certbot/certbot/compat/filesystem.py | 19 ++++++++++++++ certbot/tests/compat/filesystem_test.py | 25 +++++++++++++++++++ 6 files changed, 58 insertions(+), 21 deletions(-) diff --git a/certbot-apache/certbot_apache/_internal/http_01.py b/certbot-apache/certbot_apache/_internal/http_01.py index bceec2c32..e7ca87608 100644 --- a/certbot-apache/certbot_apache/_internal/http_01.py +++ b/certbot-apache/certbot_apache/_internal/http_01.py @@ -157,15 +157,13 @@ class ApacheHttp01(common.ChallengePerformer): def _set_up_challenges(self) -> List[KeyAuthorizationChallengeResponse]: if not os.path.isdir(self.challenge_dir): - old_umask = filesystem.umask(0o022) - try: - filesystem.makedirs(self.challenge_dir, 0o755) - except OSError as exception: - if exception.errno not in (errno.EEXIST, errno.EISDIR): - raise errors.PluginError( - "Couldn't create root for http-01 challenge") - finally: - filesystem.umask(old_umask) + with filesystem.temp_umask(0o022): + try: + filesystem.makedirs(self.challenge_dir, 0o755) + except OSError as exception: + if exception.errno not in (errno.EEXIST, errno.EISDIR): + raise errors.PluginError( + "Couldn't create root for http-01 challenge") responses = [] for achall in self.achalls: diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 04d337ac0..ab9d53e0b 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -31,10 +31,9 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). `certbot.tests.util.patch_display_util` as a temporary workaround. * Certbot's test API under `certbot.tests` now uses `unittest.mock` instead of the 3rd party `mock` library. - ### Fixed -* +* Fixes a bug where the certbot working directory has unusably restrictive permissions on systems with stricter default umasks. More details about these changes can be found on our GitHub repo. diff --git a/certbot/certbot/_internal/main.py b/certbot/certbot/_internal/main.py index d72fa4d06..0712f1962 100644 --- a/certbot/certbot/_internal/main.py +++ b/certbot/certbot/_internal/main.py @@ -1642,7 +1642,10 @@ def make_or_verify_needed_dirs(config: configuration.NamespaceConfig) -> None: """ util.set_up_core_dir(config.config_dir, constants.CONFIG_DIRS_MODE, config.strict_permissions) - util.set_up_core_dir(config.work_dir, constants.CONFIG_DIRS_MODE, config.strict_permissions) + + # Ensure the working directory has the expected mode, even under stricter umask settings + with filesystem.temp_umask(0o022): + util.set_up_core_dir(config.work_dir, constants.CONFIG_DIRS_MODE, config.strict_permissions) hook_dirs = (config.renewal_pre_hooks_dir, config.renewal_deploy_hooks_dir, diff --git a/certbot/certbot/_internal/plugins/webroot.py b/certbot/certbot/_internal/plugins/webroot.py index 4a84197b0..e98cb77c8 100644 --- a/certbot/certbot/_internal/plugins/webroot.py +++ b/certbot/certbot/_internal/plugins/webroot.py @@ -193,8 +193,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 = filesystem.umask(0o022) - try: + with filesystem.temp_umask(0o022): # We ignore the last prefix in the next iteration, # as it does not correspond to a folder path ('/' or 'C:') for prefix in sorted(util.get_prefixes(self.full_roots[name])[:-1], key=len): @@ -219,8 +218,6 @@ to serve all files under specified web root ({0}).""" raise errors.PluginError( "Couldn't create root for {0} http-01 " "challenge responses: {1}".format(name, exception)) - finally: - filesystem.umask(old_umask) # On Windows, generate a local web.config file that allows IIS to serve expose # challenge files despite the fact they do not have a file extension. @@ -246,13 +243,9 @@ 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 = filesystem.umask(0o022) - - try: + with filesystem.temp_umask(0o022): with safe_open(validation_path, mode="wb", chmod=0o644) as validation_file: validation_file.write(validation.encode()) - finally: - filesystem.umask(old_umask) self.performed[root_path].add(achall) return response diff --git a/certbot/certbot/compat/filesystem.py b/certbot/certbot/compat/filesystem.py index bd832f7a2..1bf89a733 100644 --- a/certbot/certbot/compat/filesystem.py +++ b/certbot/certbot/compat/filesystem.py @@ -1,6 +1,7 @@ """Compat module to handle files security on Windows and Linux""" from __future__ import absolute_import +from contextlib import contextmanager import errno import os # pylint: disable=os-module-forbidden import stat @@ -8,6 +9,7 @@ import sys from typing import Any from typing import Dict from typing import List +from typing import Generator from typing import Optional try: @@ -76,6 +78,23 @@ def umask(mask: int) -> int: return previous_umask +@contextmanager +def temp_umask(mask: int) -> Generator[None, None, None]: + """ + Apply a umask temporarily, meant to be used in a `with` block. Uses the Certbot + implementation of umask. + + :param int mask: The user file-creation mode mask to apply temporarily + """ + old_umask: Optional[int] = None + try: + old_umask = umask(mask) + yield None + finally: + if old_umask is not None: + umask(old_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 diff --git a/certbot/tests/compat/filesystem_test.py b/certbot/tests/compat/filesystem_test.py index a9a258ba2..e94068d4e 100644 --- a/certbot/tests/compat/filesystem_test.py +++ b/certbot/tests/compat/filesystem_test.py @@ -286,6 +286,31 @@ class WindowsOpenTest(TempDirTestCase): os.close(handler) +class TempUmaskTests(test_util.TempDirTestCase): + """Tests for using the TempUmask class in `with` statements""" + def _check_umask(self): + old_umask = filesystem.umask(0) + filesystem.umask(old_umask) + return old_umask + + def test_works_normally(self): + filesystem.umask(0o0022) + self.assertEqual(self._check_umask(), 0o0022) + with filesystem.temp_umask(0o0077): + self.assertEqual(self._check_umask(), 0o0077) + self.assertEqual(self._check_umask(), 0o0022) + + def test_resets_umask_after_exception(self): + filesystem.umask(0o0022) + self.assertEqual(self._check_umask(), 0o0022) + try: + with filesystem.temp_umask(0o0077): + self.assertEqual(self._check_umask(), 0o0077) + raise Exception() + except: + self.assertEqual(self._check_umask(), 0o0022) + + @unittest.skipIf(POSIX_MODE, reason='Test specific to Windows security') class WindowsMkdirTests(test_util.TempDirTestCase): """Unit tests for Windows mkdir + makedirs functions in filesystem module"""