tmpdir: prevent symlink attacks and TOCTOU races (CVE-2025-71176)#14279
tmpdir: prevent symlink attacks and TOCTOU races (CVE-2025-71176)#14279laurac8r wants to merge 58 commits intopytest-dev:mainfrom
Conversation
Open the base temporary directory using `os.open` with `O_NOFOLLOW` and `O_DIRECTORY` flags to prevent symlink attacks. Use the resulting file descriptor for `fstat` and `fchmod` operations to eliminate Time-of-Check Time-of-Use (TOCTOU) races. Co-authored-by: Windsurf, Gemini
Co-authored-by: Windsurf, Gemini
Co-authored-by Windsurf
Co-authored-by Windsurf, Gemini
for more information, see https://pre-commit.ci
Co-authored-by: Windsurf
for more information, see https://pre-commit.ci
Co-authored-by: Windsurf
|
This certainly looks like an improvement, but it's still possible for any user to DoS the machine until a reboot with |
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: 🇺🇦 Sviatoslav Sydorenko (Святослав Сидоренко) <wk.cvs.github@sydorenko.org.ua>
Co-authored-by: Windsurf
# Conflicts: # changelog/13669.bugfix.rst
…ytest-dev#13669) Co-authored-by: Windsurf
for more information, see https://pre-commit.ci
Fixed! Previously, TempPathFactory created a fixed, predictable directory ( I've replaced Because |
- Remove blank line before tmppath_result_key assignment - Collapse multi-line Path(tempfile.mkdtemp(...)) calls to single lines - Remove unused `getpass` import and `original_chmod` variable in tests - Add explicit `str` type annotation to `path` variable in symlink test - Add blank line before test_getbasetemp_uses_mkdtemp_rootdir function - Collapse multi-line sorted() call in TestCleanupOldRootdirs to single line Co-authored-by: Windsurf
for more information, see https://pre-commit.ci
…missions instead of no-op Replace the `_noop_chmod` stub (which simply skipped our chmod call) with `_widen_chmod`, which actively sets permissions to 0o755. This makes the test more realistic: `fchmod` must now *correct* an adversarially-widened directory, not merely compensate for a missing tightening step. Co-authored-by: Windsurf
The new approach looks good to me FWIW. mkdtemp() guarantees that the directory will be 0700 and owned by the current user, so the pre-existing chmod/stat safeguards conveniently become redundant if this approach is adopted :) |
TY Michael, and let's leave it to another enthusiastic coder ;) and close the book on this CVE |
|
In light of another recent tmpdir exploit related to directory cleanup, I think we could bullet-proof the cleanup routine even more by ensuring that https://docs.python.org/3/library/shutil.html#shutil.rmtree.avoids_symlink_attacks is true before running It would be nice if we could confirm that the user/owner agree on the directories to be removed, and if we could remove them by file descriptor (to avoid TOCTOU again), but I don't see an obvious way to do that. |
for more information, see https://pre-commit.ci
…val handling Co-authored-by: Windsurf
Co-authored-by: Windsurf
…VE-2025-71176 Co-authored-by: Windsurf
| key=lambda p: p.lstat().st_mtime, | ||
| reverse=True, | ||
| ) | ||
| except OSError: |
There was a problem hiding this comment.
Why this try/except? Please add a comment for future reference.
src/_pytest/tmpdir.py
Outdated
| mode=0o700, | ||
| ) | ||
| # Clean up old rootdirs from previous sessions. | ||
| _cleanup_old_rootdirs(temproot, rootdir_prefix, keep, current=rootdir) |
There was a problem hiding this comment.
I noticed that before we used to keep 3 directories around, but on this branch we always end up with 4 directories, which is a bit confusing if configured to keep 3 directories around (the default).
There was a problem hiding this comment.
Another thing I noticed is now that we always generate a "root" temporary dir on every run:
pytest-of-Bruno-xe7su5xh, pytest-of-Bruno-vwyhe35x, etc, which is fine as it is the whole point of this PR.
However, each directory also has a pytest-0 and pytest-current (a symlink to pytest-0), which no longer makes sense in this scenario.
src/_pytest/tmpdir.py
Outdated
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def _safe_open_dir(path: Path) -> Generator[int]: |
There was a problem hiding this comment.
This function is only used in one place:
Lines 242 to 254 in bb1371b
I think we should instead have a more focused function: _verify_ownership_and_tighthen_permission(path, user_id), instead of splitting the logic that we ultimately want in two separate places.
| symlink_warnings = [x for x in w if "avoids_symlink_attacks" in str(x.message)] | ||
| assert len(symlink_warnings) == 0 |
There was a problem hiding this comment.
This lets us see the warning in case something goes wrong:
| symlink_warnings = [x for x in w if "avoids_symlink_attacks" in str(x.message)] | |
| assert len(symlink_warnings) == 0 | |
| symlink_warnings = [x for x in w if "avoids_symlink_attacks" in str(x.message)] | |
| assert symlink_warnings == [] |
|
|
||
|
|
||
| class TestSafeRmtree: | ||
| """Tests for safe_rmtree and the avoids_symlink_attacks guard.""" |
There was a problem hiding this comment.
| """Tests for safe_rmtree and the avoids_symlink_attacks guard.""" | |
| """Tests for safe_rmtree and the avoids_symlink_attacks guard (#13669).""" |
testing/test_tmpdir.py
Outdated
| assert (basetemp.parent.stat().st_mode & 0o077) == 0 | ||
|
|
||
|
|
||
| def test_tmp_path_factory_from_config_rejects_negative_count( |
There was a problem hiding this comment.
I think we might be going overboard with the number of tests here. Could you verify if it is possible to remove/merge some of them?
changelog/13669.bugfix.rst
Outdated
| @@ -0,0 +1,9 @@ | |||
| Fixed a symlink attack vulnerability ([CVE-2025-71176](https://github.com/pytest-dev/pytest/issues/13669)) in | |||
There was a problem hiding this comment.
This description is detailed, but we should begin it with the user facing change:
Temporary directories created by :fixture:`tmp_path` used to be created inside a `pytest-of-{user}` root directory within the system's temporary directory. Each session would create a new subdirectory with an incrementing counter -- `pytest-0`, `pytest-1`, etc. -- along with a `pytest-current` symlink pointing to the current one.
This left the system vulnerable to symlink attacks, so pytest has changed how these directories are created.
Now, each session creates a single `pytest-of-{user}-{random}` directory directly inside the system's temporary directory. This makes the directories unpredictable and safe from symlink attacks and `TOCTOU <https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use>`__ races.Also note that this entry should be written in ReStructuredText, not Markdown.
…issions and update tests for CVE-2025-71176 compliance
for more information, see https://pre-commit.ci
…issions for improved security and update usage Co-authored-by: Claude
for more information, see https://pre-commit.ci
Summary
Replace the predictable
pytest-of-<user>rootdir with a randomly-named rootdir created viatempfile.mkdtempto prevent the entire class of predictable-name attacks (symlink races, DoS via pre-created files/dirs). As defense-in-depth, open the rootdir withos.openusingO_NOFOLLOWandO_DIRECTORYflags and perform ownership/permission checks via fd-basedfstat/fchmodto eliminate TOCTOU races.Fixes CVE-2025-71176.
closes #13669
Changes
tempfile.mkdtemp(prefix=f"pytest-of-{user}-", dir=temproot)instead of a fixedpytest-of-{user}directory, making pre-creation attacks infeasible._safe_open_dircontext manager: Opens the rootdir withO_NOFOLLOW | O_DIRECTORY(where available), yields the fd, and guarantees cleanup. Symlinks are rejected at theos.openlevel.fstatandfchmodoperate on the open file descriptor, eliminating the TOCTOU window betweenstat/chmodand the actual directory._cleanup_old_rootdirs: Garbage-collects stale randomly-named rootdirs from previous sessions, respecting the retention count. The current session's rootdir is never removed.O_NOFOLLOW/O_DIRECTORYfallback, predictable-path DoS immunity,_cleanup_old_rootdirsbehavior,_safe_open_dirfd lifecycle, config validation, and session-finish edge cases.If this change fixes an issue, please:
closes #XYZWto the PR description and/or commits (whereXYZWis the issue number). See the github docs for more information.Important
Unsupervised agentic contributions are not accepted. See our AI/LLM-Assisted Contributions Policy.
Co-authored-bycommit trailers.Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:
Create a new changelog file in the
changelogdirectory, with a name like<ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.Write sentences in the past or present tense, examples:
Also make sure to end the sentence with a
..Add yourself to
AUTHORSin alphabetical order.