diff --git a/changelog/8414.bugfix.rst b/changelog/8414.bugfix.rst new file mode 100644 index 00000000000..73c3068a839 --- /dev/null +++ b/changelog/8414.bugfix.rst @@ -0,0 +1,10 @@ +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. + +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/pathlib.py b/src/_pytest/pathlib.py index 8875a28f84b..7d9269a1855 100644 --- a/src/_pytest/pathlib.py +++ b/src/_pytest/pathlib.py @@ -64,13 +64,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. @@ -214,7 +207,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 @@ -222,7 +215,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: @@ -354,13 +347,13 @@ 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 6833eb02149..31259d1bdcd 100644 --- a/src/_pytest/pytester.py +++ b/src/_pytest/pytester.py @@ -1426,7 +1426,7 @@ def runpytest_subprocess(self, *args, timeout: Optional[float] = None) -> RunRes :rtype: RunResult """ __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: @@ -1445,7 +1445,7 @@ def spawn_pytest( 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 08c445e2bf8..3fe17583788 100644 --- a/src/_pytest/tmpdir.py +++ b/src/_pytest/tmpdir.py @@ -8,10 +8,10 @@ import attr import py -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.config import Config from _pytest.deprecated import check_ispytest @@ -90,20 +90,22 @@ def mktemp(self, basename: str, numbered: bool = True) -> Path: 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 if self._given_basetemp is not None: basetemp = self._given_basetemp - ensure_reset_dir(basetemp) + if basetemp.exists(): + rm_rf(basetemp) + basetemp.mkdir(mode=0o700) basetemp = basetemp.resolve() else: from_env = os.environ.get("PYTEST_DEBUG_TEMPROOT") @@ -112,14 +114,37 @@ def getbasetemp(self) -> Path: # use a sub-directory in the temproot to speed-up # make_numbered_dir() call rootdir = temproot.joinpath(f"pytest-of-{user}") - rootdir.mkdir(exist_ok=True) + 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, 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 = t = basetemp - self._trace("new basetemp", t) - return t + self._basetemp = basetemp + self._trace("new basetemp", basetemp) + return basetemp @final diff --git a/testing/test_tmpdir.py b/testing/test_tmpdir.py index d123287aa38..8b3b65010d4 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -445,3 +445,44 @@ def test(tmp_path): # running a second time and ensure we don't crash result = pytester.runpytest("--basetemp=tmp") assert result.ret == 0 + + +@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, +) -> 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 + + +@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, +) -> 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