diff --git a/AUTHORS b/AUTHORS index 6885ec6e793..8329ddf9288 100644 --- a/AUTHORS +++ b/AUTHORS @@ -264,6 +264,7 @@ Kojo Idrissa Kostis Anagnostopoulos Kristoffer Nordström Kyle Altendorf +Laura Kaminskiy Lawrence Mitchell Lee Kamentsky Leonardus Chen diff --git a/changelog/13669.bugfix.rst b/changelog/13669.bugfix.rst new file mode 100644 index 00000000000..f216b0b3ae2 --- /dev/null +++ b/changelog/13669.bugfix.rst @@ -0,0 +1,9 @@ +Fixed a symlink attack vulnerability ([CVE-2025-71176](https://github.com/pytest-dev/pytest/issues/13669)) in +the [tmp_path](https://github.com/pytest-dev/pytest/blob/295d9da900a0dbe8b4093d6a6bc977cd567aa4b0/src/_pytest/tmpdir.py#L258) +fixture's base directory handling. + +The ``pytest-of-`` directory under the system temp root is now opened +with [O_NOFOLLOW](https://man7.org/linux/man-pages/man2/open.2.html#:~:text=not%20have%20one.-,O_NOFOLLOW,-If%20the%20trailing) +and verified using +file-descriptor-based [fstat](https://linux.die.net/man/2/fstat)/[fchmod](https://linux.die.net/man/2/fchmod), +preventing symlink attacks and [TOCTOU](https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use) races. diff --git a/src/_pytest/tmpdir.py b/src/_pytest/tmpdir.py index 855ad273ecf..ad7b9bbea01 100644 --- a/src/_pytest/tmpdir.py +++ b/src/_pytest/tmpdir.py @@ -4,6 +4,7 @@ from __future__ import annotations from collections.abc import Generator +import contextlib import dataclasses import os from pathlib import Path @@ -37,6 +38,64 @@ RetentionType = Literal["all", "failed", "none"] +@contextlib.contextmanager +def _safe_open_dir(path: Path) -> Generator[int]: + """Open a directory without following symlinks and yield its file descriptor. + + Uses O_NOFOLLOW and O_DIRECTORY (when available) to prevent symlink + attacks (CVE-2025-71176). The fd-based operations (fstat, fchmod) + also eliminate TOCTOU races. + + Args: + path: Directory to open. + + Yields: + An open file descriptor for the directory. + + Raises: + OSError: If the path cannot be safely opened (e.g. it is a symlink). + """ + open_flags = os.O_RDONLY + for _flag in ("O_NOFOLLOW", "O_DIRECTORY"): + open_flags |= getattr(os, _flag, 0) + try: + dir_fd = os.open(str(path), open_flags) + except OSError as e: + raise OSError( + f"The temporary directory {path} could not be " + "safely opened (it may be a symlink). " + "Remove the symlink or directory and try again." + ) from e + try: + yield dir_fd + finally: + os.close(dir_fd) + + +def _cleanup_old_rootdirs( + temproot: Path, prefix: str, keep: int, current: Path +) -> None: + """Remove old randomly-named rootdirs, keeping the *keep* most recent. + + *current* is excluded so the running session's rootdir is never removed. + Errors are silently ignored (other sessions may hold locks, etc.). + """ + try: + candidates = sorted( + ( + p + for p in temproot.iterdir() + if p.is_dir() and p.name.startswith(prefix) and p != current + ), + key=lambda p: p.stat().st_mtime, + reverse=True, + ) + except OSError: + return + for old in candidates[keep:]: + rmtree(old, ignore_errors=True) + + @final @dataclasses.dataclass class TempPathFactory: @@ -157,29 +216,32 @@ def getbasetemp(self) -> Path: user = get_user() or "unknown" # use a sub-directory in the temproot to speed-up # make_numbered_dir() call - rootdir = temproot.joinpath(f"pytest-of-{user}") + # Use a randomly-named rootdir created via mkdtemp to avoid + # the entire class of predictable-name attacks (symlink races, + # DoS via pre-created files/dirs, etc.). See #13669. + rootdir_prefix = f"pytest-of-{user}-" try: - rootdir.mkdir(mode=0o700, exist_ok=True) + rootdir = Path(tempfile.mkdtemp(prefix=rootdir_prefix, dir=temproot)) except OSError: - # 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. + # getuser() likely returned illegal characters for the + # platform, fall back to a safe prefix. + rootdir_prefix = "pytest-of-unknown-" + rootdir = Path(tempfile.mkdtemp(prefix=rootdir_prefix, dir=temproot)) + # mkdtemp applies the umask; ensure 0o700 unconditionally. + os.chmod(rootdir, 0o700) + # Defense-in-depth: verify ownership and tighten permissions + # via fd-based ops to eliminate TOCTOU races (CVE-2025-71176). uid = get_user_id() if uid is not None: - rootdir_stat = rootdir.stat() - 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) + with _safe_open_dir(rootdir) as dir_fd: + rootdir_stat = os.fstat(dir_fd) + 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.fchmod(dir_fd, rootdir_stat.st_mode & ~0o077) keep = self._retention_count if self._retention_policy == "none": keep = 0 @@ -190,6 +252,8 @@ def getbasetemp(self) -> Path: lock_timeout=LOCK_TIMEOUT, mode=0o700, ) + # Clean up old rootdirs from previous sessions. + _cleanup_old_rootdirs(temproot, rootdir_prefix, keep, current=rootdir) assert basetemp is not None, basetemp self._basetemp = basetemp self._trace("new basetemp", basetemp) diff --git a/testing/test_tmpdir.py b/testing/test_tmpdir.py index 12891d81488..ddcda79bfb5 100644 --- a/testing/test_tmpdir.py +++ b/testing/test_tmpdir.py @@ -7,6 +7,8 @@ from pathlib import Path import stat import sys +import tempfile +from typing import Any from typing import cast import warnings @@ -21,11 +23,19 @@ from _pytest.pathlib import register_cleanup_lock_removal from _pytest.pathlib import rm_rf from _pytest.pytester import Pytester +from _pytest.tmpdir import _cleanup_old_rootdirs +from _pytest.tmpdir import _safe_open_dir from _pytest.tmpdir import get_user +from _pytest.tmpdir import pytest_sessionfinish from _pytest.tmpdir import TempPathFactory import pytest +skip_if_no_getuid = pytest.mark.skipif( + not hasattr(os, "getuid"), reason="checks unix permissions" +) + + def test_tmp_path_fixture(pytester: Pytester) -> None: p = pytester.copy_example("tmpdir/tmp_path_fixture.py") results = pytester.runpytest(p) @@ -592,30 +602,313 @@ def test_tmp_path_factory_create_directory_with_safe_permissions( # No world-readable permissions. assert (basetemp.stat().st_mode & 0o077) == 0 - # Parent too (pytest-of-foo). + # Parent rootdir too (pytest-of--XXXXXX). 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( +def test_tmp_path_factory_defense_in_depth_fchmod( 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). + """Defense-in-depth: even if os.chmod after mkdtemp somehow leaves + world-readable permissions, the fchmod inside _safe_open_dir corrects + them.""" monkeypatch.setenv("PYTEST_DEBUG_TEMPROOT", str(tmp_path)) + + # Sabotage os.chmod so it *widens* permissions instead of tightening; + # the fd-based fchmod should still correct them. + original_chmod = os.chmod + + def _widen_chmod(path: Any, mode: int, **kw: Any) -> None: + original_chmod(path, 0o755) + + monkeypatch.setattr(os, "chmod", _widen_chmod) + tmp_factory = TempPathFactory(None, 3, "all", 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 + # The fchmod defense should have corrected the permissions. + assert (basetemp.parent.stat().st_mode & 0o077) == 0 + + +@skip_if_no_getuid +def test_tmp_path_factory_rejects_symlink_rootdir( + tmp_path: Path, monkeypatch: MonkeyPatch +) -> None: + """CVE-2025-71176: defense-in-depth — if the mkdtemp-created rootdir is + somehow replaced by a symlink before _safe_open_dir runs, it must be + rejected.""" + monkeypatch.setenv("PYTEST_DEBUG_TEMPROOT", str(tmp_path)) + + attacker_dir = tmp_path / "attacker-controlled" + attacker_dir.mkdir(mode=0o700) + + original_mkdtemp = tempfile.mkdtemp + + def _mkdtemp_then_replace_with_symlink(**kwargs: Any) -> str: + """Create the dir normally, then swap it for a symlink.""" + path: str = original_mkdtemp(**kwargs) + os.rmdir(path) + os.symlink(str(attacker_dir), path) + return path + + monkeypatch.setattr(tempfile, "mkdtemp", _mkdtemp_then_replace_with_symlink) + + tmp_factory = TempPathFactory(None, 3, "all", lambda *args: None, _ispytest=True) + with pytest.raises(OSError, match="could not be safely opened"): + tmp_factory.getbasetemp() + + +@pytest.mark.skipif(not hasattr(os, "getuid"), reason="checks unix permissions") +def test_tmp_path_factory_rejects_wrong_owner( + tmp_path: Path, monkeypatch: MonkeyPatch +) -> None: + """CVE-2025-71176: verify that a rootdir owned by a different user is + rejected (covers the fstat uid mismatch branch).""" + monkeypatch.setenv("PYTEST_DEBUG_TEMPROOT", str(tmp_path)) + + # Make get_user_id() return a uid that won't match the directory owner. + monkeypatch.setattr("_pytest.tmpdir.get_user_id", lambda: os.getuid() + 1) + + tmp_factory = TempPathFactory(None, 3, "all", lambda *args: None, _ispytest=True) + with pytest.raises(OSError, match="not owned by the current user"): + tmp_factory.getbasetemp() + + +@pytest.mark.skipif(not hasattr(os, "getuid"), reason="checks unix permissions") +def test_tmp_path_factory_nofollow_flag_missing( + tmp_path: Path, monkeypatch: MonkeyPatch +) -> None: + """CVE-2025-71176: verify that the code still works when O_NOFOLLOW or + O_DIRECTORY flags are not available on the platform.""" + monkeypatch.setenv("PYTEST_DEBUG_TEMPROOT", str(tmp_path)) + monkeypatch.delattr(os, "O_NOFOLLOW", raising=False) + monkeypatch.delattr(os, "O_DIRECTORY", raising=False) tmp_factory = TempPathFactory(None, 3, "all", lambda *args: None, _ispytest=True) basetemp = tmp_factory.getbasetemp() - # After - fixed. + # Should still create the directory with safe permissions. + assert basetemp.is_dir() assert (basetemp.parent.stat().st_mode & 0o077) == 0 + + +def test_tmp_path_factory_from_config_rejects_negative_count( + tmp_path: Path, +) -> None: + """Verify that a negative tmp_path_retention_count raises ValueError.""" + + @dataclasses.dataclass + class BadCountConfig: + basetemp: str | Path = "" + + def getini(self, name): + if name == "tmp_path_retention_count": + return -1 + assert False + + config = cast(Config, BadCountConfig(tmp_path)) + with pytest.raises(ValueError, match="tmp_path_retention_count must be >= 0"): + TempPathFactory.from_config(config, _ispytest=True) + + +def test_tmp_path_factory_from_config_rejects_invalid_policy( + tmp_path: Path, +) -> None: + """Verify that an invalid tmp_path_retention_policy raises ValueError.""" + + @dataclasses.dataclass + class BadPolicyConfig: + basetemp: str | Path = "" + + def getini(self, name): + if name == "tmp_path_retention_count": + return 3 + elif name == "tmp_path_retention_policy": + return "invalid_policy" + assert False + + config = cast(Config, BadPolicyConfig(tmp_path)) + with pytest.raises(ValueError, match="tmp_path_retention_policy must be either"): + TempPathFactory.from_config(config, _ispytest=True) + + +def test_tmp_path_factory_none_policy_sets_keep_zero( + tmp_path: Path, monkeypatch: MonkeyPatch +) -> None: + """Verify that retention_policy='none' sets keep=0.""" + monkeypatch.setenv("PYTEST_DEBUG_TEMPROOT", str(tmp_path)) + tmp_factory = TempPathFactory(None, 3, "none", lambda *args: None, _ispytest=True) + basetemp = tmp_factory.getbasetemp() + assert basetemp.is_dir() + + +def test_pytest_sessionfinish_noop_when_no_basetemp( + pytester: Pytester, +) -> None: + """Verify that pytest_sessionfinish returns early when basetemp is None.""" + p = pytester.makepyfile( + """ + def test_no_tmp(): + pass + """ + ) + result = pytester.runpytest(p) + result.assert_outcomes(passed=1) + + +def test_pytest_sessionfinish_handles_missing_basetemp_dir() -> None: + """Cover the branch where basetemp is set but the directory no longer + exists when pytest_sessionfinish runs.""" + factory = TempPathFactory(None, 3, "failed", lambda *args: None, _ispytest=True) + # Point _basetemp at a path that does not exist on disk. + factory._basetemp = Path("/nonexistent/already-gone") + + class FakeSession: + class config: + _tmp_path_factory = factory + + # exitstatus=0 + policy="failed" + _given_basetemp=None enters the + # cleanup block; basetemp.is_dir() is False so rmtree is skipped. + pytest_sessionfinish(FakeSession, exitstatus=0) + + +# -- Tests for mkdtemp-based rootdir creation (DoS mitigation, #13669) -- + + +def test_getbasetemp_uses_mkdtemp_rootdir( + tmp_path: Path, monkeypatch: MonkeyPatch +) -> None: + """Verify that getbasetemp always creates a randomly-named rootdir + via mkdtemp, not a predictable pytest-of- directory.""" + monkeypatch.setenv("PYTEST_DEBUG_TEMPROOT", str(tmp_path)) + factory = TempPathFactory(None, 3, "all", lambda *args: None, _ispytest=True) + basetemp = factory.getbasetemp() + user = get_user() or "unknown" + rootdir = basetemp.parent + # The rootdir name should start with the user prefix but have a + # random suffix (from mkdtemp), NOT be exactly "pytest-of-". + assert rootdir.name.startswith(f"pytest-of-{user}-") + assert rootdir.name != f"pytest-of-{user}" + assert rootdir.is_dir() + + +def test_getbasetemp_immune_to_predictable_path_dos( + tmp_path: Path, monkeypatch: MonkeyPatch +) -> None: + """An attacker pre-creating files at /tmp/pytest-of- cannot DoS + pytest because we no longer use predictable paths (#13669).""" + temproot = tmp_path / "temproot" + temproot.mkdir() + monkeypatch.setenv("PYTEST_DEBUG_TEMPROOT", str(temproot)) + + user = get_user() or "unknown" + # Simulate the attack: touch /tmp/pytest-of-$user for every user. + (temproot / f"pytest-of-{user}").touch() + (temproot / "pytest-of-unknown").touch() + + factory = TempPathFactory(None, 3, "all", lambda *args: None, _ispytest=True) + basetemp = factory.getbasetemp() + assert basetemp.is_dir() + # The blocking files are simply ignored; mkdtemp picks a unique name. + assert basetemp.parent.name.startswith(f"pytest-of-{user}-") + + +# -- Unit tests for _cleanup_old_rootdirs -- + + +class TestCleanupOldRootdirs: + """Tests for cross-session cleanup of mkdtemp-created rootdirs.""" + + def test_removes_excess_rootdirs(self, tmp_path: Path) -> None: + prefix = "pytest-of-testuser-" + dirs = [] + for i in range(5): + d = tmp_path / f"{prefix}{i:08}" + d.mkdir() + dirs.append(d) + + current = dirs[-1] + _cleanup_old_rootdirs(tmp_path, prefix, keep=2, current=current) + + remaining = sorted(p for p in tmp_path.iterdir() if p.name.startswith(prefix)) + # current + 2 most recent old dirs = 3 total + assert len(remaining) == 3 + assert current in remaining + + def test_never_removes_current(self, tmp_path: Path) -> None: + prefix = "pytest-of-testuser-" + current = tmp_path / f"{prefix}only" + current.mkdir() + _cleanup_old_rootdirs(tmp_path, prefix, keep=0, current=current) + assert current.is_dir() + + def test_tolerates_missing_temproot(self) -> None: + _cleanup_old_rootdirs( + Path("/nonexistent"), "pytest-of-x-", keep=3, current=Path("/x") + ) + + def test_ignores_non_matching_entries(self, tmp_path: Path) -> None: + prefix = "pytest-of-testuser-" + current = tmp_path / f"{prefix}current" + current.mkdir() + unrelated = tmp_path / "some-other-dir" + unrelated.mkdir() + _cleanup_old_rootdirs(tmp_path, prefix, keep=0, current=current) + assert unrelated.is_dir() + + +# -- Direct unit tests for _safe_open_dir context manager -- + + +class TestSafeOpenDir: + """Unit tests for the _safe_open_dir context manager (CVE-2025-71176).""" + + @skip_if_no_getuid + def test_yields_valid_fd_for_real_directory(self, tmp_path: Path) -> None: + """Happy path: yields a valid file descriptor for a real directory.""" + with _safe_open_dir(tmp_path) as fd: + st = os.fstat(fd) + assert stat.S_ISDIR(st.st_mode) + + @skip_if_no_getuid + def test_fd_is_closed_after_context_exit(self, tmp_path: Path) -> None: + """The file descriptor must be closed when the context exits.""" + with _safe_open_dir(tmp_path) as fd: + pass + # After exiting, fstat on the closed fd should raise. + with pytest.raises(OSError): + os.fstat(fd) + + @skip_if_no_getuid + def test_rejects_symlink(self, tmp_path: Path) -> None: + """A symlink must be rejected with a clear error message.""" + real_dir = tmp_path / "real" + real_dir.mkdir() + link = tmp_path / "link" + link.symlink_to(real_dir) + + with pytest.raises(OSError, match="could not be safely opened"): + with _safe_open_dir(link): + pass # pragma: no cover + + @skip_if_no_getuid + def test_rejects_nonexistent_path(self, tmp_path: Path) -> None: + """A non-existent path must be rejected with a clear error message.""" + missing = tmp_path / "does-not-exist" + with pytest.raises(OSError, match="could not be safely opened"): + with _safe_open_dir(missing): + pass # pragma: no cover + + @skip_if_no_getuid + def test_fd_closed_on_exception_inside_context(self, tmp_path: Path) -> None: + """The fd must be closed even if the caller raises inside the with block.""" + fd_holder: list[int] = [] + with pytest.raises(RuntimeError, match="boom"): + with _safe_open_dir(tmp_path) as fd: + fd_holder.append(fd) + raise RuntimeError("boom") + + # fd should be closed despite the exception. + with pytest.raises(OSError): + os.fstat(fd_holder[0])