[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 <bmw@users.noreply.github.com>

* Update certbot/compat/filesystem.py

Co-Authored-By: Brad Warren <bmw@users.noreply.github.com>

* 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 <bmw@users.noreply.github.com>

.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

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

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]

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',

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]

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',

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]

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',

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]

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',

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]

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',

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]

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',

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]

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',

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]

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',

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

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',

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]

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',

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]

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',

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

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',

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]

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',

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]

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',

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]

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',

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())] ==
+    return ([dacl1.GetAce(index) for index in range(dacl1.GetAceCount())] ==
-            [dacl2.GetAce(index) for index in range(0, dacl2.GetAceCount())])
+            [dacl2.GetAce(index) for index in range(dacl2.GetAceCount())])
 
 
 def _get_current_user():

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 []
 

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).')

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

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 filesystem.has_world_permissions(filename):
-    if permissions & stat.S_IRWXO:
         logger.warning('Unsafe permissions on credentials configuration file: %s', filename)
 
 

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()

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
+        self.assertTrue(filesystem.has_same_ownership(self.validation_path, self.path))
-        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)
 
     def test_perform_cleanup(self):
         self.auth.prepare()

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) &
+            mode = filesystem.compute_private_key_mode(old_privkey, BASE_PRIVKEY_MODE)
-                        misc.MASK_FOR_PRIVATE_KEY_PERMISSIONS)
-            mode = BASE_PRIVKEY_MODE | old_mode
             filesystem.copy_ownership_and_apply_mode(
                 old_privkey, target["privkey"], mode, copy_user=False, copy_group=True)
 

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):
+class OwnershipTest(test_util.TempDirTestCase):
-    """Tests about replacement of chown: copy_ownership_and_apply_mode"""
+    """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"""

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',
+        for method in ['chmod', 'chown', 'open', 'mkdir', 'makedirs', 'rename',
-                       'makedirs', 'rename', 'replace', 'access']:
+                       'replace', 'access', 'stat', 'fstat']:
             self.assertRaises(RuntimeError, getattr(os, method))
         # Checks for os.path module
         for method in ['realpath']:

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.

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_*