From ab76834100d75e5330f585b9619332e2e0c8a43e Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Fri, 6 Sep 2019 23:30:25 +0200 Subject: [Windows|Linux] Forbid os.stat and os.fstat (#7325) Fixes #7212 This PR forbid os.stat and os.fstat, and fix or provide alternatives to avoid its usage in certbot outside of certbot.compat.filesystem. * Reimplement private key mode propagation * Remove other os.stat * Remove last call of os.stat in certbot package * Forbid stat and fstat * Implement mode comparison checks * Add unit tests * Update certbot/compat/filesystem.py Co-Authored-By: Brad Warren * Update certbot/compat/filesystem.py Co-Authored-By: Brad Warren * Handle case where multiple ace concerns a given SID in has_min_permissions * Add a new test scenario * Add a simple test for has_same_ownership * Fix name function * Add a comment explaining an ACE structure * Move a test in its dedicated class * Improve a message error * Calculate has_min_permission result using effective permission rights to be more generic. * Change an exception message * Add comments, avoid to skip a test. * Update certbot/compat/filesystem.py Co-Authored-By: Brad Warren --- .codecov.yml | 4 +- .../certbot_apache/tests/http_01_test.py | 10 +- certbot-apache/local-oldest-requirements.txt | 2 +- certbot-apache/setup.py | 2 +- .../local-oldest-requirements.txt | 2 +- certbot-dns-cloudflare/setup.py | 2 +- certbot-dns-cloudxns/local-oldest-requirements.txt | 2 +- certbot-dns-cloudxns/setup.py | 2 +- .../local-oldest-requirements.txt | 2 +- certbot-dns-digitalocean/setup.py | 2 +- certbot-dns-dnsimple/local-oldest-requirements.txt | 2 +- certbot-dns-dnsimple/setup.py | 2 +- .../local-oldest-requirements.txt | 2 +- certbot-dns-dnsmadeeasy/setup.py | 2 +- certbot-dns-gehirn/local-oldest-requirements.txt | 2 +- certbot-dns-gehirn/setup.py | 2 +- certbot-dns-google/local-oldest-requirements.txt | 2 +- certbot-dns-google/setup.py | 2 +- certbot-dns-linode/local-oldest-requirements.txt | 2 +- certbot-dns-linode/setup.py | 2 +- certbot-dns-luadns/local-oldest-requirements.txt | 2 +- certbot-dns-luadns/setup.py | 2 +- certbot-dns-nsone/local-oldest-requirements.txt | 2 +- certbot-dns-nsone/setup.py | 2 +- certbot-dns-ovh/local-oldest-requirements.txt | 2 +- certbot-dns-ovh/setup.py | 2 +- certbot-dns-rfc2136/local-oldest-requirements.txt | 2 +- certbot-dns-rfc2136/setup.py | 2 +- certbot-dns-route53/local-oldest-requirements.txt | 2 +- certbot-dns-route53/setup.py | 2 +- .../local-oldest-requirements.txt | 2 +- certbot-dns-sakuracloud/setup.py | 2 +- certbot/compat/filesystem.py | 122 ++++++++++++++++++++- certbot/compat/misc.py | 13 --- certbot/compat/os.py | 18 +++ certbot/lock.py | 8 +- certbot/plugins/dns_common.py | 5 +- certbot/plugins/dns_common_test.py | 21 ++-- certbot/plugins/webroot_test.py | 15 +-- certbot/storage.py | 5 +- certbot/tests/compat/filesystem_test.py | 55 +++++++++- certbot/tests/compat/os_test.py | 4 +- certbot/tests/lock_test.py | 9 +- tox.ini | 2 +- 44 files changed, 258 insertions(+), 93 deletions(-) diff --git a/.codecov.yml b/.codecov.yml index 8a1503da8..55af1a36c 100644 --- a/.codecov.yml +++ b/.codecov.yml @@ -6,13 +6,13 @@ coverage: flags: linux # Fixed target instead of auto set by #7173, can # be removed when flags in Codecov are added back. - target: 97.5 + target: 97.4 threshold: 0.1 base: auto windows: flags: windows # Fixed target instead of auto set by #7173, can # be removed when flags in Codecov are added back. - target: 97.6 + target: 97.7 threshold: 0.1 base: auto diff --git a/certbot-apache/certbot_apache/tests/http_01_test.py b/certbot-apache/certbot_apache/tests/http_01_test.py index a6af68037..fade85b3a 100644 --- a/certbot-apache/certbot_apache/tests/http_01_test.py +++ b/certbot-apache/certbot_apache/tests/http_01_test.py @@ -7,6 +7,7 @@ from acme.magic_typing import List # pylint: disable=unused-import, no-name-in- from certbot import achallenges from certbot import errors +from certbot.compat import filesystem from certbot.compat import os from certbot.tests import acme_util @@ -180,7 +181,7 @@ class ApacheHttp01Test(util.ApacheTest): self.assertEqual(self.http.perform(), expected_response) self.assertTrue(os.path.isdir(self.http.challenge_dir)) - self._has_min_permissions(self.http.challenge_dir, 0o755) + self.assertTrue(filesystem.has_min_permissions(self.http.challenge_dir, 0o755)) self._test_challenge_conf() for achall in achalls: @@ -218,15 +219,10 @@ class ApacheHttp01Test(util.ApacheTest): name = os.path.join(self.http.challenge_dir, achall.chall.encode("token")) validation = achall.validation(self.account_key) - self._has_min_permissions(name, 0o644) + self.assertTrue(filesystem.has_min_permissions(name, 0o644)) with open(name, 'rb') as f: self.assertEqual(f.read(), validation.encode()) - def _has_min_permissions(self, path, min_mode): - """Tests the given file has at least the permissions in mode.""" - st_mode = os.stat(path).st_mode - self.assertEqual(st_mode, st_mode | min_mode) - if __name__ == "__main__": unittest.main() # pragma: no cover diff --git a/certbot-apache/local-oldest-requirements.txt b/certbot-apache/local-oldest-requirements.txt index aafd37702..da509406e 100644 --- a/certbot-apache/local-oldest-requirements.txt +++ b/certbot-apache/local-oldest-requirements.txt @@ -1,3 +1,3 @@ # Remember to update setup.py to match the package versions below. acme[dev]==0.29.0 -certbot[dev]==0.37.0 +-e .[dev] diff --git a/certbot-apache/setup.py b/certbot-apache/setup.py index 21d11ea72..1393165ed 100644 --- a/certbot-apache/setup.py +++ b/certbot-apache/setup.py @@ -10,7 +10,7 @@ version = '0.39.0.dev0' # acme/certbot version. install_requires = [ 'acme>=0.29.0', - 'certbot>=0.37.0', + 'certbot>=0.39.0.dev0', 'mock', 'python-augeas', 'setuptools', diff --git a/certbot-dns-cloudflare/local-oldest-requirements.txt b/certbot-dns-cloudflare/local-oldest-requirements.txt index 0bc9ee027..da509406e 100644 --- a/certbot-dns-cloudflare/local-oldest-requirements.txt +++ b/certbot-dns-cloudflare/local-oldest-requirements.txt @@ -1,3 +1,3 @@ # Remember to update setup.py to match the package versions below. acme[dev]==0.29.0 -certbot[dev]==0.34.0 +-e .[dev] diff --git a/certbot-dns-cloudflare/setup.py b/certbot-dns-cloudflare/setup.py index 0de6ac2fb..7676f595c 100644 --- a/certbot-dns-cloudflare/setup.py +++ b/certbot-dns-cloudflare/setup.py @@ -8,7 +8,7 @@ version = '0.39.0.dev0' # acme/certbot version. install_requires = [ 'acme>=0.29.0', - 'certbot>=0.34.0', + 'certbot>=0.39.0.dev0', 'cloudflare>=1.5.1', 'mock', 'setuptools', diff --git a/certbot-dns-cloudxns/local-oldest-requirements.txt b/certbot-dns-cloudxns/local-oldest-requirements.txt index c9999e87a..2b3ba9f32 100644 --- a/certbot-dns-cloudxns/local-oldest-requirements.txt +++ b/certbot-dns-cloudxns/local-oldest-requirements.txt @@ -1,3 +1,3 @@ # Remember to update setup.py to match the package versions below. acme[dev]==0.31.0 -certbot[dev]==0.34.0 +-e .[dev] diff --git a/certbot-dns-cloudxns/setup.py b/certbot-dns-cloudxns/setup.py index 37b77c8de..2b93056cb 100644 --- a/certbot-dns-cloudxns/setup.py +++ b/certbot-dns-cloudxns/setup.py @@ -8,7 +8,7 @@ version = '0.39.0.dev0' # acme/certbot version. install_requires = [ 'acme>=0.31.0', - 'certbot>=0.34.0', + 'certbot>=0.39.0.dev0', 'dns-lexicon>=2.2.1', # Support for >1 TXT record per name 'mock', 'setuptools', diff --git a/certbot-dns-digitalocean/local-oldest-requirements.txt b/certbot-dns-digitalocean/local-oldest-requirements.txt index 0bc9ee027..da509406e 100644 --- a/certbot-dns-digitalocean/local-oldest-requirements.txt +++ b/certbot-dns-digitalocean/local-oldest-requirements.txt @@ -1,3 +1,3 @@ # Remember to update setup.py to match the package versions below. acme[dev]==0.29.0 -certbot[dev]==0.34.0 +-e .[dev] diff --git a/certbot-dns-digitalocean/setup.py b/certbot-dns-digitalocean/setup.py index 3b88276a2..8d17e9d61 100644 --- a/certbot-dns-digitalocean/setup.py +++ b/certbot-dns-digitalocean/setup.py @@ -8,7 +8,7 @@ version = '0.39.0.dev0' # acme/certbot version. install_requires = [ 'acme>=0.29.0', - 'certbot>=0.34.0', + 'certbot>=0.39.0.dev0', 'mock', 'python-digitalocean>=1.11', 'setuptools', diff --git a/certbot-dns-dnsimple/local-oldest-requirements.txt b/certbot-dns-dnsimple/local-oldest-requirements.txt index c9999e87a..2b3ba9f32 100644 --- a/certbot-dns-dnsimple/local-oldest-requirements.txt +++ b/certbot-dns-dnsimple/local-oldest-requirements.txt @@ -1,3 +1,3 @@ # Remember to update setup.py to match the package versions below. acme[dev]==0.31.0 -certbot[dev]==0.34.0 +-e .[dev] diff --git a/certbot-dns-dnsimple/setup.py b/certbot-dns-dnsimple/setup.py index 860c4819e..1ca843189 100644 --- a/certbot-dns-dnsimple/setup.py +++ b/certbot-dns-dnsimple/setup.py @@ -9,7 +9,7 @@ version = '0.39.0.dev0' # acme/certbot version. install_requires = [ 'acme>=0.31.0', - 'certbot>=0.34.0', + 'certbot>=0.39.0.dev0', 'mock', 'setuptools', 'zope.interface', diff --git a/certbot-dns-dnsmadeeasy/local-oldest-requirements.txt b/certbot-dns-dnsmadeeasy/local-oldest-requirements.txt index c9999e87a..2b3ba9f32 100644 --- a/certbot-dns-dnsmadeeasy/local-oldest-requirements.txt +++ b/certbot-dns-dnsmadeeasy/local-oldest-requirements.txt @@ -1,3 +1,3 @@ # Remember to update setup.py to match the package versions below. acme[dev]==0.31.0 -certbot[dev]==0.34.0 +-e .[dev] diff --git a/certbot-dns-dnsmadeeasy/setup.py b/certbot-dns-dnsmadeeasy/setup.py index 2b110d042..d7fc4d795 100644 --- a/certbot-dns-dnsmadeeasy/setup.py +++ b/certbot-dns-dnsmadeeasy/setup.py @@ -8,7 +8,7 @@ version = '0.39.0.dev0' # acme/certbot version. install_requires = [ 'acme>=0.31.0', - 'certbot>=0.34.0', + 'certbot>=0.39.0.dev0', 'dns-lexicon>=2.2.1', # Support for >1 TXT record per name 'mock', 'setuptools', diff --git a/certbot-dns-gehirn/local-oldest-requirements.txt b/certbot-dns-gehirn/local-oldest-requirements.txt index c9999e87a..2b3ba9f32 100644 --- a/certbot-dns-gehirn/local-oldest-requirements.txt +++ b/certbot-dns-gehirn/local-oldest-requirements.txt @@ -1,3 +1,3 @@ # Remember to update setup.py to match the package versions below. acme[dev]==0.31.0 -certbot[dev]==0.34.0 +-e .[dev] diff --git a/certbot-dns-gehirn/setup.py b/certbot-dns-gehirn/setup.py index f9a818fdf..faf986187 100644 --- a/certbot-dns-gehirn/setup.py +++ b/certbot-dns-gehirn/setup.py @@ -7,7 +7,7 @@ version = '0.39.0.dev0' # Please update tox.ini when modifying dependency version requirements install_requires = [ 'acme>=0.31.0', - 'certbot>=0.34.0', + 'certbot>=0.39.0.dev0', 'dns-lexicon>=2.1.22', 'mock', 'setuptools', diff --git a/certbot-dns-google/local-oldest-requirements.txt b/certbot-dns-google/local-oldest-requirements.txt index 0bc9ee027..da509406e 100644 --- a/certbot-dns-google/local-oldest-requirements.txt +++ b/certbot-dns-google/local-oldest-requirements.txt @@ -1,3 +1,3 @@ # Remember to update setup.py to match the package versions below. acme[dev]==0.29.0 -certbot[dev]==0.34.0 +-e .[dev] diff --git a/certbot-dns-google/setup.py b/certbot-dns-google/setup.py index 83ec28253..c6fadad41 100644 --- a/certbot-dns-google/setup.py +++ b/certbot-dns-google/setup.py @@ -8,7 +8,7 @@ version = '0.39.0.dev0' # acme/certbot version. install_requires = [ 'acme>=0.29.0', - 'certbot>=0.34.0', + 'certbot>=0.39.0.dev0', # 1.5 is the first version that supports oauth2client>=2.0 'google-api-python-client>=1.5', 'mock', diff --git a/certbot-dns-linode/local-oldest-requirements.txt b/certbot-dns-linode/local-oldest-requirements.txt index ff1651cf7..d48a789bb 100644 --- a/certbot-dns-linode/local-oldest-requirements.txt +++ b/certbot-dns-linode/local-oldest-requirements.txt @@ -1,4 +1,4 @@ # Remember to update setup.py to match the package versions below. acme[dev]==0.31.0 -certbot[dev]==0.34.0 +-e .[dev] dns-lexicon==2.2.3 diff --git a/certbot-dns-linode/setup.py b/certbot-dns-linode/setup.py index c8d453e49..6a1421778 100644 --- a/certbot-dns-linode/setup.py +++ b/certbot-dns-linode/setup.py @@ -6,7 +6,7 @@ version = '0.39.0.dev0' # Please update tox.ini when modifying dependency version requirements install_requires = [ 'acme>=0.31.0', - 'certbot>=0.34.0', + 'certbot>=0.39.0.dev0', 'dns-lexicon>=2.2.3', 'mock', 'setuptools', diff --git a/certbot-dns-luadns/local-oldest-requirements.txt b/certbot-dns-luadns/local-oldest-requirements.txt index c9999e87a..2b3ba9f32 100644 --- a/certbot-dns-luadns/local-oldest-requirements.txt +++ b/certbot-dns-luadns/local-oldest-requirements.txt @@ -1,3 +1,3 @@ # Remember to update setup.py to match the package versions below. acme[dev]==0.31.0 -certbot[dev]==0.34.0 +-e .[dev] diff --git a/certbot-dns-luadns/setup.py b/certbot-dns-luadns/setup.py index 0bccca2d4..0b8ce9671 100644 --- a/certbot-dns-luadns/setup.py +++ b/certbot-dns-luadns/setup.py @@ -8,7 +8,7 @@ version = '0.39.0.dev0' # acme/certbot version. install_requires = [ 'acme>=0.31.0', - 'certbot>=0.34.0', + 'certbot>=0.39.0.dev0', 'dns-lexicon>=2.2.1', # Support for >1 TXT record per name 'mock', 'setuptools', diff --git a/certbot-dns-nsone/local-oldest-requirements.txt b/certbot-dns-nsone/local-oldest-requirements.txt index c9999e87a..2b3ba9f32 100644 --- a/certbot-dns-nsone/local-oldest-requirements.txt +++ b/certbot-dns-nsone/local-oldest-requirements.txt @@ -1,3 +1,3 @@ # Remember to update setup.py to match the package versions below. acme[dev]==0.31.0 -certbot[dev]==0.34.0 +-e .[dev] diff --git a/certbot-dns-nsone/setup.py b/certbot-dns-nsone/setup.py index cb4963c17..bb945a834 100644 --- a/certbot-dns-nsone/setup.py +++ b/certbot-dns-nsone/setup.py @@ -8,7 +8,7 @@ version = '0.39.0.dev0' # acme/certbot version. install_requires = [ 'acme>=0.31.0', - 'certbot>=0.34.0', + 'certbot>=0.39.0.dev0', 'dns-lexicon>=2.2.1', # Support for >1 TXT record per name 'mock', 'setuptools', diff --git a/certbot-dns-ovh/local-oldest-requirements.txt b/certbot-dns-ovh/local-oldest-requirements.txt index 5472399aa..ed5aa6c87 100644 --- a/certbot-dns-ovh/local-oldest-requirements.txt +++ b/certbot-dns-ovh/local-oldest-requirements.txt @@ -1,4 +1,4 @@ # Remember to update setup.py to match the package versions below. acme[dev]==0.31.0 -certbot[dev]==0.34.0 +-e .[dev] dns-lexicon==2.7.14 diff --git a/certbot-dns-ovh/setup.py b/certbot-dns-ovh/setup.py index c3f1ea636..a7fb6a5dc 100644 --- a/certbot-dns-ovh/setup.py +++ b/certbot-dns-ovh/setup.py @@ -8,7 +8,7 @@ version = '0.39.0.dev0' # acme/certbot version. install_requires = [ 'acme>=0.31.0', - 'certbot>=0.34.0', + 'certbot>=0.39.0.dev0', 'dns-lexicon>=2.7.14', # Correct proxy use on OVH provider 'mock', 'setuptools', diff --git a/certbot-dns-rfc2136/local-oldest-requirements.txt b/certbot-dns-rfc2136/local-oldest-requirements.txt index 0bc9ee027..da509406e 100644 --- a/certbot-dns-rfc2136/local-oldest-requirements.txt +++ b/certbot-dns-rfc2136/local-oldest-requirements.txt @@ -1,3 +1,3 @@ # Remember to update setup.py to match the package versions below. acme[dev]==0.29.0 -certbot[dev]==0.34.0 +-e .[dev] diff --git a/certbot-dns-rfc2136/setup.py b/certbot-dns-rfc2136/setup.py index 1e480b046..d25ebb2a8 100644 --- a/certbot-dns-rfc2136/setup.py +++ b/certbot-dns-rfc2136/setup.py @@ -8,7 +8,7 @@ version = '0.39.0.dev0' # acme/certbot version. install_requires = [ 'acme>=0.29.0', - 'certbot>=0.34.0', + 'certbot>=0.39.0.dev0', 'dnspython', 'mock', 'setuptools', diff --git a/certbot-dns-route53/local-oldest-requirements.txt b/certbot-dns-route53/local-oldest-requirements.txt index 0bc9ee027..da509406e 100644 --- a/certbot-dns-route53/local-oldest-requirements.txt +++ b/certbot-dns-route53/local-oldest-requirements.txt @@ -1,3 +1,3 @@ # Remember to update setup.py to match the package versions below. acme[dev]==0.29.0 -certbot[dev]==0.34.0 +-e .[dev] diff --git a/certbot-dns-route53/setup.py b/certbot-dns-route53/setup.py index 2f49e77f2..14af3d8c9 100644 --- a/certbot-dns-route53/setup.py +++ b/certbot-dns-route53/setup.py @@ -7,7 +7,7 @@ version = '0.39.0.dev0' # acme/certbot version. install_requires = [ 'acme>=0.29.0', - 'certbot>=0.34.0', + 'certbot>=0.39.0.dev0', 'boto3', 'mock', 'setuptools', diff --git a/certbot-dns-sakuracloud/local-oldest-requirements.txt b/certbot-dns-sakuracloud/local-oldest-requirements.txt index c9999e87a..2b3ba9f32 100644 --- a/certbot-dns-sakuracloud/local-oldest-requirements.txt +++ b/certbot-dns-sakuracloud/local-oldest-requirements.txt @@ -1,3 +1,3 @@ # Remember to update setup.py to match the package versions below. acme[dev]==0.31.0 -certbot[dev]==0.34.0 +-e .[dev] diff --git a/certbot-dns-sakuracloud/setup.py b/certbot-dns-sakuracloud/setup.py index a87fbb147..3fb1cb8ee 100644 --- a/certbot-dns-sakuracloud/setup.py +++ b/certbot-dns-sakuracloud/setup.py @@ -7,7 +7,7 @@ version = '0.39.0.dev0' # Please update tox.ini when modifying dependency version requirements install_requires = [ 'acme>=0.31.0', - 'certbot>=0.34.0', + 'certbot>=0.39.0.dev0', 'dns-lexicon>=2.1.23', 'mock', 'setuptools', diff --git a/certbot/compat/filesystem.py b/certbot/compat/filesystem.py index 0649f9bad..6bcc9a693 100644 --- a/certbot/compat/filesystem.py +++ b/certbot/compat/filesystem.py @@ -20,7 +20,7 @@ except ImportError: else: POSIX_MODE = False -from acme.magic_typing import List # pylint: disable=unused-import, no-name-in-module +from acme.magic_typing import List, Union, Tuple # pylint: disable=unused-import, no-name-in-module def chmod(file_path, mode): @@ -264,6 +264,7 @@ def replace(src, dst): def realpath(file_path): + # type: (str) -> str """ 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. @@ -300,10 +301,11 @@ def realpath(file_path): # requires to be run under a privileged shell, so the user will always benefit # from the highest (privileged one) set of permissions on a given file. def is_executable(path): + # type: (str) -> bool """ Is path an executable file? :param str path: path to test - :returns: True if path is an executable file + :return: True if path is an executable file :rtype: bool """ if POSIX_MODE: @@ -312,6 +314,118 @@ def is_executable(path): return _win_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 + :param str path: path to test + :return: True if everybody/world has any right to the file + :rtype: bool + """ + if POSIX_MODE: + return bool(stat.S_IMODE(os.stat(path).st_mode) & stat.S_IRWXO) + + security = win32security.GetFileSecurity(path, win32security.DACL_SECURITY_INFORMATION) + dacl = security.GetSecurityDescriptorDacl() + + return bool(dacl.GetEffectiveRightsFromAcl({ + 'TrusteeForm': win32security.TRUSTEE_IS_SID, + 'TrusteeType': win32security.TRUSTEE_IS_USER, + 'Identifier': win32security.ConvertStringSidToSid('S-1-1-0'), + })) + + +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 + :param str old_key: path to the previous private key + :param int base_mode: the minimum modes to apply to a private key + :return: the POSIX mode to apply + :rtype: int + """ + if POSIX_MODE: + # On Linux, we keep read/write/execute permissions + # for group and read permissions for everybody. + old_mode = (stat.S_IMODE(os.stat(old_key).st_mode) & + (stat.S_IRGRP | stat.S_IWGRP | stat.S_IXGRP | stat.S_IROTH)) + return base_mode | old_mode + + # On Windows, the mode returned by os.stat is not reliable, + # so we do not keep any permission from the previous private key. + return base_mode + + +def has_same_ownership(path1, path2): + # type: (str, str) -> bool + """ + Return True if the ownership of two files given their respective path is the same. + On Windows, ownership is checked against owner only, since files do not have a group owner. + :param str path1: path to the first file + :param str path2: path to the second file + :return: True if both files have the same ownership, False otherwise + :rtype: bool + + """ + if POSIX_MODE: + stats1 = os.stat(path1) + stats2 = os.stat(path2) + return (stats1.st_uid, stats1.st_gid) == (stats2.st_uid, stats2.st_gid) + + security1 = win32security.GetFileSecurity(path1, win32security.OWNER_SECURITY_INFORMATION) + user1 = security1.GetSecurityDescriptorOwner() + + security2 = win32security.GetFileSecurity(path2, win32security.OWNER_SECURITY_INFORMATION) + user2 = security2.GetSecurityDescriptorOwner() + + return user1 == user2 + + +def has_min_permissions(path, min_mode): + # type: (str, int) -> bool + """ + Check if a file given its path has at least the permissions defined by the given minimal mode. + On Windows, group permissions are ignored since files do not have a group owner. + :param str path: path to the file to check + :param int min_mode: the minimal permissions expected + :return: True if the file matches the minimal permissions expectations, False otherwise + :rtype: bool + """ + if POSIX_MODE: + st_mode = os.stat(path).st_mode + return st_mode == st_mode | min_mode + + # Resolve symlinks, to get a consistent result with os.stat on Linux, + # that follows symlinks by default. + path = realpath(path) + + # Get owner sid of the file + security = win32security.GetFileSecurity( + path, win32security.OWNER_SECURITY_INFORMATION | win32security.DACL_SECURITY_INFORMATION) + user = security.GetSecurityDescriptorOwner() + dacl = security.GetSecurityDescriptorDacl() + min_dacl = _generate_dacl(user, min_mode) + + for index in range(min_dacl.GetAceCount()): + min_ace = min_dacl.GetAce(index) + + # On a given ACE, index 0 is the ACE type, 1 is the permission mask, and 2 is the SID. + # See: http://timgolden.me.uk/pywin32-docs/PyACL__GetAce_meth.html + mask = min_ace[1] + user = min_ace[2] + + effective_mask = dacl.GetEffectiveRightsFromAcl({ + 'TrusteeForm': win32security.TRUSTEE_IS_SID, + 'TrusteeType': win32security.TRUSTEE_IS_USER, + 'Identifier': user, + }) + + if effective_mask != effective_mask | mask: + return False + + return True + + def _win_is_executable(path): if not os.path.isfile(path): return False @@ -472,8 +586,8 @@ def _compare_dacls(dacl1, dacl2): This method compare the two given DACLs to check if they are identical. Identical means here that they contains the same set of ACEs in the same order. """ - return ([dacl1.GetAce(index) for index in range(0, dacl1.GetAceCount())] == - [dacl2.GetAce(index) for index in range(0, dacl2.GetAceCount())]) + return ([dacl1.GetAce(index) for index in range(dacl1.GetAceCount())] == + [dacl2.GetAce(index) for index in range(dacl2.GetAceCount())]) def _get_current_user(): diff --git a/certbot/compat/misc.py b/certbot/compat/misc.py index 5151e7156..a8fbf2c96 100644 --- a/certbot/compat/misc.py +++ b/certbot/compat/misc.py @@ -5,7 +5,6 @@ particular category. from __future__ import absolute_import import select -import stat import sys try: @@ -18,18 +17,6 @@ from certbot import errors from certbot.compat import os -# MASK_FOR_PRIVATE_KEY_PERMISSIONS defines what are the permissions flags to keep -# when transferring the permissions from an old private key to a new one. -if POSIX_MODE: - # On Linux, we keep read/write/execute permissions - # for group and read permissions for everybody. - MASK_FOR_PRIVATE_KEY_PERMISSIONS = stat.S_IRGRP | stat.S_IWGRP | stat.S_IXGRP | stat.S_IROTH -else: - # On Windows, the mode returned by os.stat is not reliable, - # so we do not keep any permission from the previous private key. - MASK_FOR_PRIVATE_KEY_PERMISSIONS = 0 - - # For Linux: define OS specific standard binary directories STANDARD_BINARY_DIRS = ["/usr/sbin", "/usr/local/bin", "/usr/local/sbin"] if POSIX_MODE else [] diff --git a/certbot/compat/os.py b/certbot/compat/os.py index 2857ea408..e5438f365 100644 --- a/certbot/compat/os.py +++ b/certbot/compat/os.py @@ -116,3 +116,21 @@ def access(*unused_args, **unused_kwargs): raise RuntimeError('Usage of os.access() is forbidden. ' 'Use certbot.compat.filesystem.check_mode() or ' 'certbot.compat.filesystem.is_executable() instead.') + + +# On Windows os.stat call result is inconsistent, with a lot of flags that are not set or +# meaningless. We need to use specialized functions from the certbot.compat.filesystem module. +def stat(*unused_args, **unused_kwargs): + """Method os.stat() is forbidden""" + raise RuntimeError('Usage of os.stat() is forbidden. ' + 'Use certbot.compat.filesystem functions instead ' + '(eg. has_min_permissions, has_same_ownership).') + + +# Method os.fstat has the same problem than os.stat, since it is the same function, +# but accepting a file descriptor instead of a path. +def fstat(*unused_args, **unused_kwargs): + """Method os.stat() is forbidden""" + raise RuntimeError('Usage of os.fstat() is forbidden. ' + 'Use certbot.compat.filesystem functions instead ' + '(eg. has_min_permissions, has_same_ownership).') diff --git a/certbot/lock.py b/certbot/lock.py index cdb0fbb3c..eda2a72a1 100644 --- a/certbot/lock.py +++ b/certbot/lock.py @@ -167,14 +167,18 @@ class _UnixLockMechanism(_BaseLockMechanism): :returns: True if the lock was successfully acquired :rtype: bool """ + # Normally os module should not be imported in certbot codebase except in certbot.compat + # for the sake of compatibility over Windows and Linux. + # We make an exception here, since _lock_success is private and called only on Linux. + from os import stat, fstat # pylint: disable=os-module-forbidden try: - stat1 = os.stat(self._path) + stat1 = stat(self._path) except OSError as err: if err.errno == errno.ENOENT: return False raise - stat2 = os.fstat(fd) + stat2 = fstat(fd) # If our locked file descriptor and the file on disk refer to # the same device and inode, they're the same file. return stat1.st_dev == stat2.st_dev and stat1.st_ino == stat2.st_ino diff --git a/certbot/plugins/dns_common.py b/certbot/plugins/dns_common.py index e7fbd3889..931778b07 100644 --- a/certbot/plugins/dns_common.py +++ b/certbot/plugins/dns_common.py @@ -2,7 +2,6 @@ import abc import logging -import stat from time import sleep import configobj @@ -12,6 +11,7 @@ from acme import challenges from certbot import errors from certbot import interfaces +from certbot.compat import filesystem from certbot.compat import os from certbot.display import ops from certbot.display import util as display_util @@ -312,8 +312,7 @@ def validate_file_permissions(filename): validate_file(filename) - permissions = stat.S_IMODE(os.stat(filename).st_mode) - if permissions & stat.S_IRWXO: + if filesystem.has_world_permissions(filename): logger.warning('Unsafe permissions on credentials configuration file: %s', filename) diff --git a/certbot/plugins/dns_common_test.py b/certbot/plugins/dns_common_test.py index 6741ff8e5..eba3c89d6 100644 --- a/certbot/plugins/dns_common_test.py +++ b/certbot/plugins/dns_common_test.py @@ -7,14 +7,15 @@ import unittest import mock from certbot import errors +from certbot import util from certbot.compat import os from certbot.display import util as display_util from certbot.plugins import dns_common from certbot.plugins import dns_test_common -from certbot.tests import util +from certbot.tests import util as test_util -class DNSAuthenticatorTest(util.TempDirTestCase, dns_test_common.BaseAuthenticatorTest): +class DNSAuthenticatorTest(test_util.TempDirTestCase, dns_test_common.BaseAuthenticatorTest): # pylint: disable=protected-access class _FakeDNSAuthenticator(dns_common.DNSAuthenticator): @@ -50,7 +51,7 @@ class DNSAuthenticatorTest(util.TempDirTestCase, dns_test_common.BaseAuthenticat self.auth._cleanup.assert_called_once_with(dns_test_common.DOMAIN, mock.ANY, mock.ANY) - @util.patch_get_utility() + @test_util.patch_get_utility() def test_prompt(self, mock_get_utility): mock_display = mock_get_utility() mock_display.input.side_effect = ((display_util.OK, "",), @@ -59,14 +60,14 @@ class DNSAuthenticatorTest(util.TempDirTestCase, dns_test_common.BaseAuthenticat self.auth._configure("other_key", "") self.assertEqual(self.auth.config.fake_other_key, "value") - @util.patch_get_utility() + @test_util.patch_get_utility() def test_prompt_canceled(self, mock_get_utility): mock_display = mock_get_utility() mock_display.input.side_effect = ((display_util.CANCEL, "c",),) self.assertRaises(errors.PluginError, self.auth._configure, "other_key", "") - @util.patch_get_utility() + @test_util.patch_get_utility() def test_prompt_file(self, mock_get_utility): path = os.path.join(self.tempdir, 'file.ini') open(path, "wb").close() @@ -80,7 +81,7 @@ class DNSAuthenticatorTest(util.TempDirTestCase, dns_test_common.BaseAuthenticat self.auth._configure_file("file_path", "") self.assertEqual(self.auth.config.fake_file_path, path) - @util.patch_get_utility() + @test_util.patch_get_utility() def test_prompt_file_canceled(self, mock_get_utility): mock_display = mock_get_utility() mock_display.directory_select.side_effect = ((display_util.CANCEL, "c",),) @@ -96,7 +97,7 @@ class DNSAuthenticatorTest(util.TempDirTestCase, dns_test_common.BaseAuthenticat self.assertEqual(credentials.conf("test"), "value") - @util.patch_get_utility() + @test_util.patch_get_utility() def test_prompt_credentials(self, mock_get_utility): bad_path = os.path.join(self.tempdir, 'bad-file.ini') dns_test_common.write({"fake_other": "other_value"}, bad_path) @@ -116,7 +117,7 @@ class DNSAuthenticatorTest(util.TempDirTestCase, dns_test_common.BaseAuthenticat self.assertEqual(credentials.conf("test"), "value") -class CredentialsConfigurationTest(util.TempDirTestCase): +class CredentialsConfigurationTest(test_util.TempDirTestCase): class _MockLoggingHandler(logging.Handler): messages = None @@ -150,14 +151,14 @@ class CredentialsConfigurationTest(util.TempDirTestCase): dns_common.logger.addHandler(log) path = os.path.join(self.tempdir, 'too-permissive-file.ini') - open(path, "wb").close() + util.safe_open(path, "wb", 0o744).close() dns_common.CredentialsConfiguration(path) self.assertEqual(1, len([_ for _ in log.messages['warning'] if _.startswith("Unsafe")])) -class CredentialsConfigurationRequireTest(util.TempDirTestCase): +class CredentialsConfigurationRequireTest(test_util.TempDirTestCase): def setUp(self): super(CredentialsConfigurationRequireTest, self).setUp() diff --git a/certbot/plugins/webroot_test.py b/certbot/plugins/webroot_test.py index a0b701cac..d1aea5817 100644 --- a/certbot/plugins/webroot_test.py +++ b/certbot/plugins/webroot_test.py @@ -34,7 +34,13 @@ class AuthenticatorTest(unittest.TestCase): def setUp(self): from certbot.plugins.webroot import Authenticator - self.path = tempfile.mkdtemp() + # On Linux directories created by tempfile.mkdtemp inherit their permissions from their + # parent directory. So the actual permissions are inconsistent over various tests env. + # To circumvent this, a dedicated sub-workspace is created under the workspace, using + # filesystem.mkdir to get consistent permissions. + self.workspace = tempfile.mkdtemp() + self.path = os.path.join(self.workspace, 'webroot') + filesystem.mkdir(self.path) self.partial_root_challenge_path = os.path.join( self.path, ".well-known") self.root_challenge_path = os.path.join( @@ -170,17 +176,12 @@ class AuthenticatorTest(unittest.TestCase): self.assertTrue(filesystem.check_mode(self.validation_path, 0o644)) # Check permissions of the directories - for dirpath, dirnames, _ in os.walk(self.path): for directory in dirnames: full_path = os.path.join(dirpath, directory) self.assertTrue(filesystem.check_mode(full_path, 0o755)) - parent_gid = os.stat(self.path).st_gid - parent_uid = os.stat(self.path).st_uid - - self.assertEqual(os.stat(self.validation_path).st_gid, parent_gid) - self.assertEqual(os.stat(self.validation_path).st_uid, parent_uid) + self.assertTrue(filesystem.has_same_ownership(self.validation_path, self.path)) def test_perform_cleanup(self): self.auth.prepare() diff --git a/certbot/storage.py b/certbot/storage.py index 518ba9464..9bf85fa0f 100644 --- a/certbot/storage.py +++ b/certbot/storage.py @@ -18,7 +18,6 @@ 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 @@ -1107,9 +1106,7 @@ class RenewableCert(object): f.write(new_privkey) # Preserve gid and (mode & MASK_FOR_PRIVATE_KEY_PERMISSIONS) # from previous privkey in this lineage. - old_mode = (stat.S_IMODE(os.stat(old_privkey).st_mode) & - misc.MASK_FOR_PRIVATE_KEY_PERMISSIONS) - mode = BASE_PRIVKEY_MODE | old_mode + mode = filesystem.compute_private_key_mode(old_privkey, BASE_PRIVKEY_MODE) filesystem.copy_ownership_and_apply_mode( old_privkey, target["privkey"], mode, copy_user=False, copy_group=True) diff --git a/certbot/tests/compat/filesystem_test.py b/certbot/tests/compat/filesystem_test.py index c808a5238..ccb93efa8 100644 --- a/certbot/tests/compat/filesystem_test.py +++ b/certbot/tests/compat/filesystem_test.py @@ -150,6 +150,24 @@ class WindowsChmodTests(TempDirTestCase): self.assertEqual(security_dacl.GetSecurityDescriptorDacl().GetAceCount(), 2) +class ComputePrivateKeyModeTest(TempDirTestCase): + def setUp(self): + super(ComputePrivateKeyModeTest, self).setUp() + self.probe_path = _create_probe(self.tempdir) + + def test_compute_private_key_mode(self): + filesystem.chmod(self.probe_path, 0o777) + new_mode = filesystem.compute_private_key_mode(self.probe_path, 0o600) + + if POSIX_MODE: + # On Linux RWX permissions for group and R permission for world + # are persisted from the existing moe + self.assertEqual(new_mode, 0o674) + else: + # On Windows no permission is persisted + self.assertEqual(new_mode, 0o600) + + @unittest.skipIf(POSIX_MODE, reason='Tests specific to Windows security') class WindowsOpenTest(TempDirTestCase): def test_new_file_correct_permissions(self): @@ -262,14 +280,14 @@ class WindowsMkdirTests(test_util.TempDirTestCase): self.assertEqual(original_mkdir, std_os.mkdir) -class CopyOwnershipTest(test_util.TempDirTestCase): - """Tests about replacement of chown: copy_ownership_and_apply_mode""" +class OwnershipTest(test_util.TempDirTestCase): + """Tests about copy_ownership_and_apply_mode and has_same_ownership""" def setUp(self): - super(CopyOwnershipTest, self).setUp() + super(OwnershipTest, self).setUp() self.probe_path = _create_probe(self.tempdir) @unittest.skipIf(POSIX_MODE, reason='Test specific to Windows security') - def test_windows(self): + def test_copy_ownership_windows(self): system = win32security.ConvertStringSidToSid(SYSTEM_SID) security = win32security.SECURITY_ATTRIBUTES().SECURITY_DESCRIPTOR security.SetSecurityDescriptorOwner(system, False) @@ -295,7 +313,7 @@ class CopyOwnershipTest(test_util.TempDirTestCase): if dacl.GetAce(index)[2] == everybody]) @unittest.skipUnless(POSIX_MODE, reason='Test specific to Linux security') - def test_linux(self): + def test_copy_ownership_linux(self): with mock.patch('os.chown') as mock_chown: with mock.patch('os.chmod') as mock_chmod: with mock.patch('os.stat') as mock_stat: @@ -307,8 +325,18 @@ class CopyOwnershipTest(test_util.TempDirTestCase): mock_chown.assert_called_once_with(self.probe_path, 50, 51) mock_chmod.assert_called_once_with(self.probe_path, 0o700) + def test_has_same_ownership(self): + path1 = os.path.join(self.tempdir, 'test1') + path2 = os.path.join(self.tempdir, 'test2') + + util.safe_open(path1, 'w').close() + util.safe_open(path2, 'w').close() + + self.assertTrue(filesystem.has_same_ownership(path1, path2)) + class CheckPermissionsTest(test_util.TempDirTestCase): + """Tests relative to functions that check modes.""" def setUp(self): super(CheckPermissionsTest, self).setUp() self.probe_path = _create_probe(self.tempdir) @@ -353,6 +381,23 @@ class CheckPermissionsTest(test_util.TempDirTestCase): mock_owner.return_value = False self.assertFalse(filesystem.check_permissions(self.probe_path, 0o744)) + def test_check_min_permissions(self): + filesystem.chmod(self.probe_path, 0o744) + self.assertTrue(filesystem.has_min_permissions(self.probe_path, 0o744)) + + filesystem.chmod(self.probe_path, 0o700) + self.assertFalse(filesystem.has_min_permissions(self.probe_path, 0o744)) + + filesystem.chmod(self.probe_path, 0o741) + self.assertFalse(filesystem.has_min_permissions(self.probe_path, 0o744)) + + def test_is_world_reachable(self): + filesystem.chmod(self.probe_path, 0o744) + self.assertTrue(filesystem.has_world_permissions(self.probe_path)) + + filesystem.chmod(self.probe_path, 0o700) + self.assertFalse(filesystem.has_world_permissions(self.probe_path)) + 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 e4928e4fb..2fe23f700 100644 --- a/certbot/tests/compat/os_test.py +++ b/certbot/tests/compat/os_test.py @@ -8,8 +8,8 @@ class OsTest(unittest.TestCase): """Unit tests for os module.""" def test_forbidden_methods(self): # Checks for os module - for method in ['chmod', 'chown', 'open', 'mkdir', - 'makedirs', 'rename', 'replace', 'access']: + for method in ['chmod', 'chown', 'open', 'mkdir', 'makedirs', 'rename', + 'replace', 'access', 'stat', 'fstat']: self.assertRaises(RuntimeError, getattr(os, method)) # Checks for os.path module for method in ['realpath']: diff --git a/certbot/tests/lock_test.py b/certbot/tests/lock_test.py index fb3a8fedf..cd1f9ea86 100644 --- a/certbot/tests/lock_test.py +++ b/certbot/tests/lock_test.py @@ -82,7 +82,10 @@ class LockFileTest(test_util.TempDirTestCase): 'Race conditions on lock are specific to the non-blocking file access approach on Linux.') def test_race(self): should_delete = [True, False] - stat = os.stat + # Normally os module should not be imported in certbot codebase except in certbot.compat + # for the sake of compatibility over Windows and Linux. + # We make an exception here, since test_race is a test function called only on Linux. + from os import stat # pylint: disable=os-module-forbidden def delete_and_stat(path): """Wrap os.stat and maybe delete the file first.""" @@ -90,7 +93,7 @@ class LockFileTest(test_util.TempDirTestCase): os.remove(path) return stat(path) - with mock.patch('certbot.lock.os.stat') as mock_stat: + with mock.patch('certbot.lock.filesystem.os.stat') as mock_stat: mock_stat.side_effect = delete_and_stat self._call(self.lock_path) self.assertFalse(should_delete) @@ -117,7 +120,7 @@ class LockFileTest(test_util.TempDirTestCase): def test_unexpected_os_err(self): if POSIX_MODE: - mock_function = 'certbot.lock.os.stat' + mock_function = 'certbot.lock.filesystem.os.stat' else: mock_function = 'certbot.lock.msvcrt.locking' # The only expected errno are ENOENT and EACCES in lock module. diff --git a/tox.ini b/tox.ini index a4f4bd3e3..763f786fa 100644 --- a/tox.ini +++ b/tox.ini @@ -228,7 +228,7 @@ commands = --acme-server={env:ACME_SERVER:pebble} \ --cov=acme --cov=certbot --cov=certbot_nginx --cov-report= \ --cov-config=certbot-ci/certbot_integration_tests/.coveragerc - coverage report --include 'certbot/*' --show-missing --fail-under=66 + coverage report --include 'certbot/*' --show-missing --fail-under=65 coverage report --include 'certbot-nginx/*' --show-missing --fail-under=74 passenv = DOCKER_* -- cgit v1.2.3