fix(utils): apply the stdlib data filter on both tarball extraction paths - #734
fix(utils): apply the stdlib data filter on both tarball extraction paths#734christian-byrne wants to merge 1 commit into
Conversation
…aths `extract_tarball()` bypassed Python's extraction filter on both of its paths: `tar.extractall(filter=None)` on the non-progress path, and a custom `_filter` that returned members unmodified on the progress path. A member named `../evil` or `/etc/evil` was therefore written wherever it pointed (CVE-2007-4559). This is reachable from caller-supplied input via `StandalonePython.FromTarball(fpath)`. The filter was disabled deliberately, citing python/cpython#107845. That bug made `data_filter` resolve symlink targets against the destination root instead of against the directory containing the link, so it falsely raised `LinkOutsideDestinationError` on valid archives. It was a false-rejection bug, never an escape, and it was fixed in 3.10.13 / 3.11.5 / 3.12.0rc2 on 2023-08-24. The only affected releases inside our `requires-python = ">=3.10"` range are 3.10.12 and 3.11.4 — and `extractall(filter=...)` does not exist at all before 3.10.12, so that is the entire window. Progress reporting and the safety decision were conflated in the same callback. They are separable: `members` now drives the progress bars and `filter` stays the literal `"data"` filter, which also clears ruff's S202 across `comfy_cli`.
dosubot
Bot
added
size:M
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 42 minutes Limit details: You’ve used all 3 included reviews currently available. Your 76 included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Comment |
|
Verified this branch does what it says, and found one thing it does not cover — details and repro in #725 (comment). What this branch fixes, confirmed. Ran all three filter forms against a tarball with a So both paths really were open and both are really closed. The commit message's read of cpython#107845 matches mine — false-rejection, never an escape, fixed in 3.10.13 / 3.11.5 / 3.12.0rc2. What it does not cover. old_name = info.name.split("/")[0]
extractPath = inPath.with_name(old_name) # with_name("..") is legal
shutil.rmtree(extractPath, ignore_errors=True)A first member named The filter can't help here — the delete is header-derived and happens first. Since this PR closes #725, merging as-is would retire the ticket with the delete primitive still live. Two options: extend this branch to also scope the extraction ( Also worth knowing while this is open, from the same review: the download host is not pinned ( |
|
Re-verified the above independently against old_name = info.name.split("/")[0]
extractPath = inPath.with_name(old_name)
shutil.rmtree(extractPath, ignore_errors=True) # <-- filter never gets a voteOne detail worth adding, because the obvious way to check this gives the wrong answer. On 3.12.3:
Not asking for scope creep on this PR — just flagging that merging it as-is would auto-close #725 |
|
Following up on the earlier comment about the The two tests this PR adds both build their tarball with _write_member(tar, "payload/keep.txt", b"benign")
_write_member(tar, "../evil.txt", b"pwned")So Making the malicious member first is the whole difference. Measured with the same harness on three variants:
Failure message on both unfixed refs, for both The control passing on every row is what rules out a broken harness. The guard that turns it green, at if old_name in ("", ".", "..") or "/" in old_name or "\\" in old_name:
raise ValueError(f"refusing to extract tarball with unsafe top-level member name: {old_name!r}")Drop-in test, written against this PR's own file so it can be appended to @pytest.mark.parametrize("show_progress", [False, True])
def test_first_member_name_cannot_select_an_rmtree_target(self, tmp_path, monkeypatch, show_progress):
"""The first tar header must not be able to pick what gets deleted.
extractPath is derived from tar.next().name and rmtree'd BEFORE extraction,
and Path.with_name("..") is legal, so a first member named "../evil"
resolves the delete target to the parent of the download directory.
"""
workspace = tmp_path / "workspace"
downloads = workspace / "downloads"
downloads.mkdir(parents=True)
(workspace / "comfy.settings.json").write_text('{"real": "settings"}')
tarball = downloads / "python.tgz"
with tarfile.open(tarball, "w:gz") as tar:
_write_member(tar, "../evil", b"x") # FIRST — this is the point
_write_member(tar, "payload/keep.txt", b"x")
monkeypatch.chdir(downloads)
with patch("comfy_cli.utils.Live"):
try:
extract_tarball(tarball, downloads / "out", show_progress=show_progress)
except Exception:
pass # the delete already happened; how extraction ends is not the point
assert (workspace / "comfy.settings.json").exists(), "workspace settings were deleted by a tar header"Filing note: this came out of a closed-bug regression audit whose lens was "closed bugs whose regression test tests the wrong half". This PR is the cleanest live example of that shape, which is why it got a test rather than another issue. No objection to the PR's actual change — |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 8 finding(s).
| Severity | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟠 High | 1 |
| 🟡 Medium | 3 |
| 🟢 Low | 3 |
Panel: 8/8 reviewers contributed findings.
| @@ -169,31 +169,47 @@ def extract_tarball( | |||
| shutil.rmtree(extractPath, ignore_errors=True) | |||
There was a problem hiding this comment.
🔴 Critical — extractPath is derived from the archive's own first member (old_name = info.name.split("/")[0]) and handed to shutil.rmtree(..., ignore_errors=True) before any filter runs, so filter="data" cannot protect it: a tarball whose first member is ../evil yields old_name == "..", and Path.with_name("..") is accepted, so this recursively deletes the parent of the tarball's directory with the errors swallowed (for comfy standalone --rehydrate, which passes python.tgz, that is the parent of the user's cwd). An absolute first member such as /etc/passwd instead yields old_name == "" and crashes in with_name. Validate old_name — reject "", ".", "..", and any name containing a separator — before using it as an rmtree target.
Raised by 5 of 8 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial, kimi-k3-max adversarial, claude-opus-5-thinking-max edge-case, gpt-5.6-sol-max edge-case).
| if not show_progress: | ||
| with tarfile.open(inPath) as tar: | ||
| tar.extractall(filter=None) | ||
| tar.extractall(filter="data") |
There was a problem hiding this comment.
🟠 High — Neither extractall call passes path=, so members are unpacked into the process CWD and the data filter anchors its containment check there instead of at the intended destination: a member like payload/../evil.txt (or a plain .bashrc) passes the filter, is written beside rather than inside the payload tree, and survives the later shutil.move(extractPath, outPath). The same implicit assumption that CWD equals inPath.parent means any caller passing an absolute inPath from another directory hits FileNotFoundError in that move. Extract into an explicit staging directory via path= so the filter's boundary is the destination you actually intend.
Raised by 4 of 8 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial, claude-opus-5-thinking-max edge-case, gpt-5.6-sol-max edge-case).
| with tarfile.open(inPath) as tar: | ||
| tar.extractall(filter=_filter) | ||
| barProg.advance(barTask, _size) | ||
| tar.extractall(members=_reporting_members(tar), filter="data") |
There was a problem hiding this comment.
🟡 Medium — filter="data" introduces a mid-extraction failure mode the old no-op callable never had: with the default errorlevel=1, a rejected member N aborts extractall after members 1..N-1 are already on disk, and the shutil.rmtree(outPath) at the top of the function has already deleted the previous install. Nothing cleans up, so a hostile (or falsely rejected) archive leaves a half-extracted tree behind and no outPath. Wrap both extraction calls in try/except, remove the partial tree, then re-raise.
Raised by 5 of 8 reviewers (claude-opus-5-thinking-max adversarial, kimi-k3-max adversarial, claude-opus-5-thinking-max edge-case, gpt-5.6-sol-max edge-case, kimi-k3-max edge-case).
| # LinkOutsideDestinationError on valid archives. That was a false-rejection bug, | ||
| # never an escape, and it was fixed in 3.10.13 / 3.11.5 / 3.12.0rc2 (2023-08-24). | ||
| # The only affected releases in our range are 3.10.12 and 3.11.4 — and since | ||
| # `extractall(filter=...)` does not exist before 3.10.12 at all, that is the whole |
There was a problem hiding this comment.
🟡 Medium — The version reasoning falls short in both directions: extractall(filter=...) not existing before 3.10.12/3.11.4 is not a reason to dismiss that range but a hard TypeError on every extraction, and pyproject.toml still allows >=3.10; meanwhile 3.10.12 is the system Python on Ubuntu 22.04 LTS, so the #107845 false rejection would break the primary comfy standalone path there, not an exotic one. Raising requires-python to >=3.10.13 / >=3.11.5 (or gating on sys.version_info) closes both instead of documenting them. Worth noting in the comment as well that the data filter itself had genuine escape fixes in 3.10.18/3.11.13/3.12.11/3.13.4 (CVE-2025-4517).
Raised by 6 of 8 reviewers (gpt-5.6-sol-max adversarial, kimi-k3-max adversarial, gpt-5.6-sol-max edge-case, gemini-3.1-pro edge-case, claude-opus-5-thinking-max edge-case, kimi-k3-max edge-case).
| def _traversal_tarball(workdir): | ||
| tarball = workdir / "payload.tgz" | ||
| with tarfile.open(tarball, "w:gz") as tar: | ||
| _write_member(tar, "payload/keep.txt", b"benign") |
There was a problem hiding this comment.
🟡 Medium — This traversal test only passes because the benign member is written first: old_name becomes "payload", so the pre-extraction rmtree is harmless and the data filter catches ../evil.txt later. Member order is attacker-controlled, and putting the traversal member first makes old_name == ".." and wipes the grandparent directory before the filter ever runs. Add a case with the traversal member as the first entry so the test covers the ordering an attacker would choose.
Raised by 2 of 8 reviewers (claude-opus-5-thinking-max edge-case, claude-opus-5-thinking-max adversarial).
| """ | ||
| size = 0 | ||
| for tinfo in tar: | ||
| pathProg.update(pathTask, description=tinfo.path) |
There was a problem hiding this comment.
🟢 Low — tinfo.path is archive-controlled and is passed unescaped as a description rendered by progress.TextColumn("{task.description}"), which parses console markup by default. A perfectly legal member name like foo[/bold].txt makes Text.from_markup raise MarkupError during the Live refresh, and [link=...] lets the archive emit OSC-8 escapes into the user's terminal — neither is blocked by the data filter, so a crafted archive breaks the progress path while succeeding with show_progress=False. Wrap the name in rich.markup.escape() or build the column with markup=False.
Raised by 3 of 8 reviewers (claude-opus-5-thinking-max adversarial, kimi-k3-max adversarial, claude-opus-5-thinking-max edge-case).
| tarball = workdir / "python.tgz" | ||
| with tarfile.open(tarball, "w:gz") as tar: | ||
| _write_member(tar, "python/bin/python3.12", b"#!/bin/sh\n") | ||
| _write_symlink(tar, "python/bin/python3", "python3.12") |
There was a problem hiding this comment.
🟢 Low — This control test is meant to back the comment's claim that the 3.10.12/3.11.4 data_filter bug is tolerable, but same-directory targets (python3 -> python3.12) never triggered cpython#107845 — joining such a linkname onto the destination root still lands inside it. The false LinkOutsideDestinationError only fires for targets that ascend, e.g. python/bin/x -> ../lib/y, so this test stays green on the affected interpreters regardless. Add an ascending relative symlink member so it actually exercises the window the new comment describes.
Raised by 2 of 8 reviewers (claude-opus-5-thinking-max adversarial, kimi-k3-max edge-case).
| size = 0 | ||
| for tinfo in tar: | ||
| pathProg.update(pathTask, description=tinfo.path) | ||
| barProg.advance(barTask, size) |
There was a problem hiding this comment.
🟢 Low — The bar's total is the compressed archive size (fileSize = inPath.stat().st_size) while each advance uses the member's uncompressed tinfo.size, so the bar overruns its total on any compressible tarball. The mismatch predates this change, but since the reporting loop is being rewritten here it is the natural place to make the units agree (e.g. base the total on the members' uncompressed sizes).
Raised by 1 of 8 reviewers (gemini-3.1-pro edge-case).
bigcat88
left a comment
bigcat88
left a comment
There was a problem hiding this comment.
Verified this end-to-end:
- Reproduced the vulnerability class: the new
test_rejects_path_traversal_memberfails against the unfixedextract_tarball(progress path) and passes with the fix. - Ran a real-world extraction test with an actual python-build-standalone tarball (cpython-3.12.3 aarch64-apple-darwin
install_only), both with and without progress: extraction succeeds, internal symlinks are preserved, and the extracted interpreter runs. So thefilter="data"concerns from cpython#107845 don't bite on real PBS payloads. ruff check, targeted tests (test_utils.py,test_standalone.py), and full CI are green.
Nice touch driving progress via the members= generator instead of abusing the filter callback — that keeps reporting and security concerns separate.
| if not show_progress: | ||
| with tarfile.open(inPath) as tar: | ||
| tar.extractall(filter=None) | ||
| tar.extractall(filter="data") |
There was a problem hiding this comment.
Not this PR's fault (pre-existing in both paths), but worth a follow-up: extractall is called without path=, so extraction lands in the CWD, while extractPath is computed next to the tarball (inPath.with_name(...)). When CWD != the tarball's parent directory, the subsequent shutil.move fails with FileNotFoundError — I hit this in practice while testing. Current callers happen to satisfy the assumption, but passing path=inPath.parent to extractall would make the function self-consistent. Happy to see that as a separate PR.
3 participants
Closes #725
The problem
extract_tarball()bypassed Python's tarball extraction filter on both of its paths:tar.extractall(filter=None)_filterthat updated the progress bar and thenreturn tinfounmodifiedSo a member named
../evil.txtor/etc/evilwas written wherever it pointed — the CVE-2007-4559 class. It is reachable from caller-supplied input:StandalonePython.FromTarball(fpath)takes an arbitrary path, andFromDistro()downloads from a configurableasset_url_prefix.The crux: is cpython#107845 still a problem?
The filter was disabled deliberately, not by accident. The code carried:
# TODO: ideally we'd use data_filter here, but it's busted: https://github.com/python/cpython/issues/107845That TODO is now stale. What the bug actually was:
data_filterresolved a symlink's target against the destination root rather than against the directory containing the link.TarInfo.linknameis relative to the link's own directory for symlinks but relative to the archive root for hardlinks, and PEP 706's original implementation used the hardlink rule for both. Result:LinkOutsideDestinationErrorraised on perfectly valid archives.Two things follow, and both matter here:
realpath(dest/linkname); the correct one isrealpath(dest/dirname(name)/linkname).dirname(name)is always insidedest(the member name itself is checked earlier), so the buggy computation starts strictly shallower and can only reject more than the correct one — never accept a link that actually escapes. Upstream carries no security label and no CVE; the NEWS entry says it "will no longer reject some valid tarballs".The affected set was exactly the releases that introduced the filter: 3.8.17, 3.9.17, 3.10.12, 3.11.4, plus 3.12.0b1–rc1.
requires-python = ">=3.10", and CI runs 3.10 (pytest/build) and 3.12 (mac/windows/GPU). Intersecting that with the affected set leaves exactly 3.10.12 and 3.11.4 — two patch releases superseded three years ago. And sinceextractall(filter=...)does not exist at all before 3.10.12, the existing code already could not run below that line, so that two-release window is the entire residual exposure. On those two the worst case is a loudLinkOutsideDestinationError, not a silent escape — it fails closed.So: use
data_filterdirectly, no version gate. Thepypa/build-style version gate is the wrong trade here, because its fallback branch reverts to unfiltered extraction on precisely those versions — reintroducing the hole this PR closes, to dodge a bug that only ever produced a loud error.The fix
The progress bar and the safety decision were conflated in one callback. They are separable concerns:
tar.extractall(filter="data")tar.extractall(members=_reporting_members(tar), filter="data"), where the generator drives the two progress bars while yielding membersmembersdrives the UI;filterstays the literal"data"filter. Using the string literal rather thanfilter=tarfile.data_filteralso clears ruff'sS202—ruff check --select S202 comfy_clinow passes clean across the package, which the callable form would not have.Tests
TestExtractTarballFilteringintests/comfy_cli/test_utils.py, parametrised over bothshow_progressvalues so neither path can regress. The malicious tarball is built in-test; no binary fixture.test_rejects_path_traversal_member— a tarball carrying../evil.txtmust not write outside the extraction directory, and must be rejected.test_allows_internal_symlinks— the control. python-build-standalone tarballs (the only thingStandalonePythonextracts) are full of relative symlinks likebin/python3 -> python3.12. Those stay inside the destination and must still extract. This is the case cpython#107845 would have broken.Verified the regression test genuinely fails against the unfixed code, on both paths:
With the fix,
tests/comfy_cli/test_utils.pyis 8 passed, andtests/comfy_cli/test_standalone.py(the caller) is 11 passed / 3 skipped.Notes for the reviewer
comfy_cli/utils.py, but only to makerequestsa lazy import insidedownload_url. It does not touchextract_tarball, so these should not conflict.filter="data"is not an absolute guarantee on an unpatched interpreter — there is a separate, later cluster of filter-bypass bugs (cpython#135034, CVE-2025-4517 / 4330 / 4138 / 4435, fixed June 2025). That is an argument for keeping interpreters current, not against this change: filtered extraction is strictly better than thefilter=Noneit replaces.