From 78122a530401d039a0e5aefe4d0e4b938da0c4d6 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Sat, 6 Mar 2021 16:40:42 +0200 Subject: [PATCH 1/4] pathlib: remove useless temporary variable --- src/_pytest/tmpdir.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/_pytest/tmpdir.py b/src/_pytest/tmpdir.py index 99b54e9bf..c22d4b65f 100644 --- a/src/_pytest/tmpdir.py +++ b/src/_pytest/tmpdir.py @@ -126,9 +126,9 @@ class TempPathFactory: prefix="pytest-", root=rootdir, keep=3, lock_timeout=LOCK_TIMEOUT ) assert basetemp is not None, basetemp - self._basetemp = t = basetemp - self._trace("new basetemp", t) - return t + self._basetemp = basetemp + self._trace("new basetemp", basetemp) + return basetemp @final From 0dd1e5b4f43bf3c38111410a330fce91b9304b50 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Sat, 6 Mar 2021 16:22:59 +0200 Subject: [PATCH 2/4] pathlib: inline ensure_reset_dir() This is only used in TempPathFactory.getbasetemp(). We'll be wanting further control/care there, so move it into there. --- src/_pytest/pathlib.py | 7 ------- src/_pytest/tmpdir.py | 6 ++++-- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index 63764b341..eaae09083 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -62,13 +62,6 @@ def get_lock_path(path: _AnyPurePath) -> _AnyPurePath: return path.joinpath(".lock") -def ensure_reset_dir(path: Path) -> None: - """Ensure the given path is an empty directory.""" - if path.exists(): - rm_rf(path) - path.mkdir() - - def on_rm_rf_error(func, path: str, exc, *, start_path: Path) -> bool: """Handle known read-only errors during rmtree. diff --git a/src/_pytest/tmpdir.py b/src/_pytest/tmpdir.py index c22d4b65f..309754fdd 100644 --- a/src/_pytest/tmpdir.py +++ b/src/_pytest/tmpdir.py @@ -7,10 +7,10 @@ from typing import Optional import attr -from .pathlib import ensure_reset_dir from .pathlib import LOCK_TIMEOUT from .pathlib import make_numbered_dir from .pathlib import make_numbered_dir_with_cleanup +from .pathlib import rm_rf from _pytest.compat import final from _pytest.compat import LEGACY_PATH from _pytest.compat import legacy_path @@ -107,7 +107,9 @@ class TempPathFactory: if self._given_basetemp is not None: basetemp = self._given_basetemp - ensure_reset_dir(basetemp) + if basetemp.exists(): + rm_rf(basetemp) + basetemp.mkdir() basetemp = basetemp.resolve() else: from_env = os.environ.get("PYTEST_DEBUG_TEMPROOT") From 1278f8b97e28b8d8058b57de241636d4225e0a52 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Sat, 6 Mar 2021 17:01:29 +0200 Subject: [PATCH 3/4] tmpdir: fix temporary directories created with world-readable permissions (Written for a Unix system, but might be applicable to Windows as well). pytest creates a root temporary directory under /tmp, named `pytest-of-`, and creates tmp_path's and other under it. /tmp is shared between all users of the system. This root temporary directory was created with 0o777&~umask permissions, which usually becomes 0o755, meaning any user in the system could list and read the files, which is undesirable. Use 0o700 permissions instead. Also for subdirectories, because the root dir is adjustable. --- changelog/8414.bugfix.rst | 5 +++++ src/_pytest/pathlib.py | 12 ++++++++---- src/_pytest/pytester.py | 4 ++-- src/_pytest/tmpdir.py | 18 +++++++++++------- testing/test_tmpdir.py | 16 ++++++++++++++++ 5 files changed, 42 insertions(+), 13 deletions(-) create mode 100644 changelog/8414.bugfix.rst diff --git a/changelog/8414.bugfix.rst b/changelog/8414.bugfix.rst new file mode 100644 index 000000000..a8b791ae2 --- /dev/null +++ b/changelog/8414.bugfix.rst @@ -0,0 +1,5 @@ +pytest used to create directories under ``/tmp`` with world-readable +permissions. This means that any user in the system was able to read +information written by tests in temporary directories (such as those created by +the ``tmp_path``/``tmpdir`` fixture). Now the directories are created with +private permissions. diff --git a/src/_pytest/pathlib.py b/src/_pytest/pathlib.py index eaae09083..e86811f12 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -205,7 +205,7 @@ def _force_symlink( pass -def make_numbered_dir(root: Path, prefix: str) -> Path: +def make_numbered_dir(root: Path, prefix: str, mode: int = 0o700) -> Path: """Create a directory with an increased number as suffix for the given prefix.""" for i in range(10): # try up to 10 times to create the folder @@ -213,7 +213,7 @@ def make_numbered_dir(root: Path, prefix: str) -> Path: new_number = max_existing + 1 new_path = root.joinpath(f"{prefix}{new_number}") try: - new_path.mkdir() + new_path.mkdir(mode=mode) except Exception: pass else: @@ -345,13 +345,17 @@ def cleanup_numbered_dir( def make_numbered_dir_with_cleanup( - root: Path, prefix: str, keep: int, lock_timeout: float + root: Path, + prefix: str, + keep: int, + lock_timeout: float, + mode: int, ) -> Path: """Create a numbered dir with a cleanup lock and remove old ones.""" e = None for i in range(10): try: - p = make_numbered_dir(root, prefix) + p = make_numbered_dir(root, prefix, mode) lock_path = create_cleanup_lock(p) register_cleanup_lock_removal(lock_path) except Exception as exc: diff --git a/src/_pytest/pytester.py b/src/_pytest/pytester.py index 968a53651..5a29c8eae 100644 --- a/src/_pytest/pytester.py +++ b/src/_pytest/pytester.py @@ -1456,7 +1456,7 @@ class Pytester: :py:class:`Pytester.TimeoutExpired`. """ __tracebackhide__ = True - p = make_numbered_dir(root=self.path, prefix="runpytest-") + p = make_numbered_dir(root=self.path, prefix="runpytest-", mode=0o700) args = ("--basetemp=%s" % p,) + args plugins = [x for x in self.plugins if isinstance(x, str)] if plugins: @@ -1475,7 +1475,7 @@ class Pytester: The pexpect child is returned. """ basetemp = self.path / "temp-pexpect" - basetemp.mkdir() + basetemp.mkdir(mode=0o700) invoke = " ".join(map(str, self._getpytestargs())) cmd = f"{invoke} --basetemp={basetemp} {string}" return self.spawn(cmd, expect_timeout=expect_timeout) diff --git a/src/_pytest/tmpdir.py b/src/_pytest/tmpdir.py index 309754fdd..275ec7334 100644 --- a/src/_pytest/tmpdir.py +++ b/src/_pytest/tmpdir.py @@ -94,14 +94,14 @@ class TempPathFactory: basename = self._ensure_relative_to_basetemp(basename) if not numbered: p = self.getbasetemp().joinpath(basename) - p.mkdir() + p.mkdir(mode=0o700) else: - p = make_numbered_dir(root=self.getbasetemp(), prefix=basename) + p = make_numbered_dir(root=self.getbasetemp(), prefix=basename, mode=0o700) self._trace("mktemp", p) return p def getbasetemp(self) -> Path: - """Return base temporary directory.""" + """Return the base temporary directory, creating it if needed.""" if self._basetemp is not None: return self._basetemp @@ -109,7 +109,7 @@ class TempPathFactory: basetemp = self._given_basetemp if basetemp.exists(): rm_rf(basetemp) - basetemp.mkdir() + basetemp.mkdir(mode=0o700) basetemp = basetemp.resolve() else: from_env = os.environ.get("PYTEST_DEBUG_TEMPROOT") @@ -119,13 +119,17 @@ class TempPathFactory: # make_numbered_dir() call rootdir = temproot.joinpath(f"pytest-of-{user}") try: - rootdir.mkdir(exist_ok=True) + rootdir.mkdir(mode=0o700, exist_ok=True) except OSError: # getuser() likely returned illegal characters for the platform, use unknown back off mechanism rootdir = temproot.joinpath("pytest-of-unknown") - rootdir.mkdir(exist_ok=True) + rootdir.mkdir(mode=0o700, exist_ok=True) basetemp = make_numbered_dir_with_cleanup( - prefix="pytest-", root=rootdir, keep=3, lock_timeout=LOCK_TIMEOUT + prefix="pytest-", + root=rootdir, + keep=3, + lock_timeout=LOCK_TIMEOUT, + mode=0o700, ) assert basetemp is not None, basetemp self._basetemp = basetemp diff --git a/testing/test_tmpdir.py b/testing/test_tmpdir.py index 63450eab2..2ae23ffca 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -454,3 +454,19 @@ def test_tmp_path_factory_handles_invalid_dir_characters( monkeypatch.setattr(tmp_path_factory, "_given_basetemp", None) p = tmp_path_factory.getbasetemp() assert "pytest-of-unknown" in str(p) + + +@pytest.mark.skipif(not hasattr(os, "getuid"), reason="checks unix permissions") +def test_tmp_path_factory_create_directory_with_safe_permissions( + tmp_path: Path, monkeypatch: MonkeyPatch +) -> None: + """Verify that pytest creates directories under /tmp with private permissions.""" + # Use the test's tmp_path as the system temproot (/tmp). + monkeypatch.setenv("PYTEST_DEBUG_TEMPROOT", str(tmp_path)) + tmp_factory = TempPathFactory(None, lambda *args: None, _ispytest=True) + basetemp = tmp_factory.getbasetemp() + + # No world-readable permissions. + assert (basetemp.stat().st_mode & 0o077) == 0 + # Parent too (pytest-of-foo). + assert (basetemp.parent.stat().st_mode & 0o077) == 0 From c49100cef8073c5de117199d17d632cfd8cb11c1 Mon Sep 17 00:00:00 2001 From: Ran Benita Date: Sat, 6 Mar 2021 23:17:46 +0200 Subject: [PATCH 4/4] tmpdir: prevent using a non-private root temp directory pytest uses a root temp directory named `/tmp/pytest-of-`. The name is predictable, and the directory might already exists from a previous run, so that's allowed. This makes it possible for my_user to pre-create `/tmp/pytest-of-another_user`, thus giving my_user control of another_user's tempdir. Prevent this scenario by adding a couple of safety checks. I believe they are sufficient. Testing the first check requires changing the owner, which requires root permissions, so can't be unit-tested easily, but I checked it manually. --- changelog/8414.bugfix.rst | 5 +++++ src/_pytest/tmpdir.py | 19 +++++++++++++++++++ testing/test_tmpdir.py | 25 +++++++++++++++++++++++++ 3 files changed, 49 insertions(+) diff --git a/changelog/8414.bugfix.rst b/changelog/8414.bugfix.rst index a8b791ae2..73c3068a8 100644 --- a/changelog/8414.bugfix.rst +++ b/changelog/8414.bugfix.rst @@ -3,3 +3,8 @@ permissions. This means that any user in the system was able to read information written by tests in temporary directories (such as those created by the ``tmp_path``/``tmpdir`` fixture). Now the directories are created with private permissions. + +pytest used silenty use a pre-existing ``/tmp/pytest-of-`` directory, +even if owned by another user. This means another user could pre-create such a +directory and gain control of another user's temporary directory. Now such a +condition results in an error. diff --git a/src/_pytest/tmpdir.py b/src/_pytest/tmpdir.py index 275ec7334..dcc37dc26 100644 --- a/src/_pytest/tmpdir.py +++ b/src/_pytest/tmpdir.py @@ -124,6 +124,25 @@ class TempPathFactory: # getuser() likely returned illegal characters for the platform, use unknown back off mechanism rootdir = temproot.joinpath("pytest-of-unknown") rootdir.mkdir(mode=0o700, exist_ok=True) + # Because we use exist_ok=True with a predictable name, make sure + # we are the owners, to prevent any funny business (on unix, where + # temproot is usually shared). + # Also, to keep things private, fixup any world-readable temp + # rootdir's permissions. Historically 0o755 was used, so we can't + # just error out on this, at least for a while. + if hasattr(os, "getuid"): + rootdir_stat = rootdir.stat() + uid = os.getuid() + # getuid shouldn't fail, but cpython defines such a case. + # Let's hope for the best. + if uid != -1: + if rootdir_stat.st_uid != uid: + raise OSError( + f"The temporary directory {rootdir} is not owned by the current user. " + "Fix this and try again." + ) + if (rootdir_stat.st_mode & 0o077) != 0: + os.chmod(rootdir, rootdir_stat.st_mode & ~0o077) basetemp = make_numbered_dir_with_cleanup( prefix="pytest-", root=rootdir, diff --git a/testing/test_tmpdir.py b/testing/test_tmpdir.py index 2ae23ffca..40e75663c 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -470,3 +470,28 @@ def test_tmp_path_factory_create_directory_with_safe_permissions( assert (basetemp.stat().st_mode & 0o077) == 0 # Parent too (pytest-of-foo). assert (basetemp.parent.stat().st_mode & 0o077) == 0 + + +@pytest.mark.skipif(not hasattr(os, "getuid"), reason="checks unix permissions") +def test_tmp_path_factory_fixes_up_world_readable_permissions( + tmp_path: Path, monkeypatch: MonkeyPatch +) -> None: + """Verify that if a /tmp/pytest-of-foo directory already exists with + world-readable permissions, it is fixed. + + pytest used to mkdir with such permissions, that's why we fix it up. + """ + # Use the test's tmp_path as the system temproot (/tmp). + monkeypatch.setenv("PYTEST_DEBUG_TEMPROOT", str(tmp_path)) + tmp_factory = TempPathFactory(None, lambda *args: None, _ispytest=True) + basetemp = tmp_factory.getbasetemp() + + # Before - simulate bad perms. + os.chmod(basetemp.parent, 0o777) + assert (basetemp.parent.stat().st_mode & 0o077) != 0 + + tmp_factory = TempPathFactory(None, lambda *args: None, _ispytest=True) + basetemp = tmp_factory.getbasetemp() + + # After - fixed. + assert (basetemp.parent.stat().st_mode & 0o077) == 0