diff --git a/comfy_cli/utils.py b/comfy_cli/utils.py index a39f13148..561b08674 100644 --- a/comfy_cli/utils.py +++ b/comfy_cli/utils.py @@ -169,31 +169,47 @@ def extract_tarball( shutil.rmtree(extractPath, ignore_errors=True) shutil.rmtree(outPath, ignore_errors=True) + # Both extraction paths below use the stdlib "data" filter so a member with an + # absolute path or a `../` traversal is rejected instead of being written + # wherever it points (CVE-2007-4559). + # + # This used to be skipped because of https://github.com/python/cpython/issues/107845, + # where data_filter resolved symlink targets against the destination root rather + # than against the directory holding the link, and so falsely raised + # 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 + # window. On those two, the worst case is a loud error rather than a silent escape. if not show_progress: with tarfile.open(inPath) as tar: - tar.extractall(filter=None) + tar.extractall(filter="data") shutil.move(extractPath, outPath) return fileSize = inPath.stat().st_size - _size = 0 - with _tarball_progress("extracting tarball...", fileSize) as (barProg, barTask, pathProg, pathTask): - def _filter(tinfo: tarfile.TarInfo, _path: PathLike): - nonlocal _size - pathProg.update(pathTask, description=tinfo.path) - barProg.advance(barTask, _size) - _size = tinfo.size - - # TODO: ideally we'd use data_filter here, but it's busted: https://github.com/python/cpython/issues/107845 - # return tarfile.data_filter(tinfo, _path) - return tinfo + def _reporting_members(tar: tarfile.TarFile): + """Yield every member, driving the progress bars as we go. + + Progress reporting used to ride on the ``filter`` argument, which + meant the extraction filter had to be a custom callable that + returned members unmodified — silently disabling the CVE-2007-4559 + checks. The two concerns are separable: ``members`` drives the UI + and ``filter`` stays the stdlib ``"data"`` filter. + """ + size = 0 + for tinfo in tar: + pathProg.update(pathTask, description=tinfo.path) + barProg.advance(barTask, size) + size = tinfo.size + yield tinfo + barProg.advance(barTask, size) with tarfile.open(inPath) as tar: - tar.extractall(filter=_filter) - barProg.advance(barTask, _size) + tar.extractall(members=_reporting_members(tar), filter="data") pathProg.update(pathTask, description="") shutil.move(extractPath, outPath) diff --git a/tests/comfy_cli/test_utils.py b/tests/comfy_cli/test_utils.py index 784b0df4b..55f20b7b4 100644 --- a/tests/comfy_cli/test_utils.py +++ b/tests/comfy_cli/test_utils.py @@ -1,4 +1,5 @@ import io +import tarfile from unittest.mock import MagicMock, patch import pytest @@ -82,3 +83,79 @@ def test_create_and_extract(self, tmp_path, monkeypatch): assert (dest / "hello.txt").read_text() == "hello world" assert (dest / "sub" / "nested.txt").read_text() == "nested content" + + +def _write_member(tar: tarfile.TarFile, name: str, data: bytes) -> None: + tinfo = tarfile.TarInfo(name) + tinfo.size = len(data) + tar.addfile(tinfo, io.BytesIO(data)) + + +def _write_symlink(tar: tarfile.TarFile, name: str, target: str) -> None: + tinfo = tarfile.TarInfo(name) + tinfo.type = tarfile.SYMTYPE + tinfo.linkname = target + tar.addfile(tinfo) + + +class TestExtractTarballFiltering: + """Regression tests for CVE-2007-4559 (see issue #725). + + ``extract_tarball`` extracts into the current working directory, so a member + named ``../evil.txt`` lands one level above it unless the stdlib ``data`` + filter rejects the archive. + """ + + @staticmethod + def _traversal_tarball(workdir): + tarball = workdir / "payload.tgz" + with tarfile.open(tarball, "w:gz") as tar: + _write_member(tar, "payload/keep.txt", b"benign") + _write_member(tar, "../evil.txt", b"pwned") + return tarball + + @pytest.mark.parametrize("show_progress", [False, True]) + def test_rejects_path_traversal_member(self, tmp_path, monkeypatch, show_progress): + """Both extraction paths must refuse to write outside the destination.""" + workdir = tmp_path / "work" + workdir.mkdir() + monkeypatch.chdir(workdir) + + tarball = self._traversal_tarball(workdir) + escaped = tmp_path / "evil.txt" + + rejection = None + with patch("comfy_cli.utils.Live"): + try: + extract_tarball(tarball, workdir / "out", show_progress=show_progress) + except tarfile.FilterError as exc: + rejection = exc + + assert not escaped.exists(), f"traversal member escaped the extraction directory: {escaped}" + assert rejection is not None, "the traversal member was extracted instead of being rejected" + + @pytest.mark.parametrize("show_progress", [False, True]) + def test_allows_internal_symlinks(self, tmp_path, monkeypatch, show_progress): + """Control: the filter must not reject the layout real payloads use. + + python-build-standalone tarballs (the only thing ``StandalonePython`` + extracts) are full of relative symlinks such as ``bin/python3 -> + python3.12``. Those stay inside the destination and must survive. + """ + workdir = tmp_path / "work" + workdir.mkdir() + monkeypatch.chdir(workdir) + + 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") + _write_symlink(tar, "python/bin/python", "python3") + + dest = workdir / "out" + with patch("comfy_cli.utils.Live"): + extract_tarball(tarball, dest, show_progress=show_progress) + + assert (dest / "bin" / "python3.12").read_bytes() == b"#!/bin/sh\n" + assert (dest / "bin" / "python3").is_symlink() + assert (dest / "bin" / "python").is_symlink()