From 2411dd7e5f20c605322f3cb64f10d2f181e4e796 Mon Sep 17 00:00:00 2001 From: Damian Shaw Date: Fri, 14 Aug 2026 09:49:19 +0000 Subject: [PATCH 2/2] Fix Link.filename double URL decode - path traversal (CVE-2026-13346) Upstream PRs: https://github.com/pypa/pip/pull/14110 - Add PathComponent newtype to enforce single-component filenames - Remove double urllib.parse.unquote() call in Link.filename - Add join_within_directory() preventing path escape at download sites - Update download.py and prepare.py call sites - Backport test coverage from upstream commits 1, 3, 4 of PR #14110 Co-Authored-By: Lumir Balhar --- src/pip/_internal/models/link.py | 59 ++++++++++--- src/pip/_internal/network/download.py | 18 ++-- src/pip/_internal/operations/prepare.py | 6 +- tests/unit/test_link.py | 112 +++++++++++++++++++++++- 4 files changed, 175 insertions(+), 20 deletions(-) diff --git a/src/pip/_internal/models/link.py b/src/pip/_internal/models/link.py index 38423d1..6a855e2 100644 --- a/src/pip/_internal/models/link.py +++ b/src/pip/_internal/models/link.py @@ -14,6 +14,7 @@ from typing import ( List, Mapping, NamedTuple, + NewType, Optional, Tuple, Union, @@ -36,6 +37,47 @@ if TYPE_CHECKING: logger = logging.getLogger(__name__) +# A single path component: percent-decoded once and reduced to a basename, so it +# contains no path separator and is not a ``.`` or ``..`` reference. The empty +# string means "no component". +PathComponent = NewType("PathComponent", str) + + +def _to_path_component(name: str) -> PathComponent: + """Reduce ``name`` to a single path component, or ``""`` if it has none. + + ``os.path.basename`` drops any directory part, drive letter, or separator; + a ``.``, ``..``, or empty result is not a component and becomes ``""``. + """ + name = os.path.basename(name) + if name in ("", os.curdir, os.pardir): + return PathComponent("") + + return PathComponent(name) + + +def as_path_component(name: str) -> PathComponent: + """Like ``_to_path_component`` but reject the empty result. + + Use where a file is about to be written, so a missing name is an error + rather than a silent fallback to the directory itself. + """ + component = _to_path_component(name) + if not component: + raise ValueError(f"Unexpected file name derived from URL: {name!r}") + + return component + + +def join_within_directory(directory: str, component: PathComponent) -> str: + """Join a single path ``component`` onto ``directory``. + + ``component`` is a :data:`PathComponent`, so by type it has no separator and + is not a ``.`` or ``..`` reference; the result can never escape ``directory``. + """ + return os.path.join(directory, component) + + # Order matters, earlier hashes have a precedence over later hashes for what # we will pick to use. _SUPPORTED_HASHES = ("sha512", "sha384", "sha256", "sha224", "sha1", "md5") @@ -413,18 +455,13 @@ class Link: return redact_auth_from_url(self.url) @property - def filename(self) -> str: - path = self.path.rstrip("/") - name = posixpath.basename(path) - if not name: - # Make sure we don't leak auth information if the netloc - # includes a username and password. - netloc, user_pass = split_auth_from_netloc(self.netloc) - return netloc + def filename(self) -> PathComponent: + name = _to_path_component(posixpath.basename(self.path.rstrip("/"))) + if name: + return name - name = urllib.parse.unquote(name) - assert name, f"URL {self._url!r} produced no filename" - return name + # No component in the path; fall back to the netloc, dropping any auth. + return _to_path_component(split_auth_from_netloc(self.netloc)[0]) @property def file_path(self) -> str: diff --git a/src/pip/_internal/network/download.py b/src/pip/_internal/network/download.py index 15ef58b..f1c1592 100644 --- a/src/pip/_internal/network/download.py +++ b/src/pip/_internal/network/download.py @@ -13,7 +13,12 @@ from pip._vendor.urllib3.exceptions import ReadTimeoutError from pip._internal.cli.progress_bars import get_download_progress_renderer from pip._internal.exceptions import IncompleteDownloadError, NetworkConnectionError from pip._internal.models.index import PyPI -from pip._internal.models.link import Link +from pip._internal.models.link import ( + Link, + PathComponent, + as_path_component, + join_within_directory, +) from pip._internal.network.cache import is_from_cache from pip._internal.network.session import PipSession from pip._internal.network.utils import HEADERS, raise_for_status, response_chunks @@ -110,11 +115,14 @@ def parse_content_disposition(content_disposition: str, default_filename: str) - return filename or default_filename -def _get_http_response_filename(resp: Response, link: Link) -> str: +def _get_http_response_filename(resp: Response, link: Link) -> PathComponent: """Get an ideal filename from the given HTTP response, falling back to the link filename if not provided. + + The result is validated as a single path component, so it can be joined onto + a download directory without escaping it. """ - filename = link.filename # fallback + filename: str = link.filename # fallback # Have a look at the Content-Disposition header for a better guess content_disposition = resp.headers.get("content-disposition") if content_disposition: @@ -128,7 +136,7 @@ def _get_http_response_filename(resp: Response, link: Link) -> str: ext = os.path.splitext(resp.url)[1] if ext: filename += ext - return filename + return as_path_component(filename) def _http_get_download( @@ -179,7 +187,7 @@ class Downloader: content_type = resp.headers.get("Content-Type", "") filename = _get_http_response_filename(resp, link) - filepath = os.path.join(location, filename) + filepath = join_within_directory(location, filename) with open(filepath, "wb") as content_file: bytes_received = self._process_response( diff --git a/src/pip/_internal/operations/prepare.py b/src/pip/_internal/operations/prepare.py index 531070a..c8fef4d 100644 --- a/src/pip/_internal/operations/prepare.py +++ b/src/pip/_internal/operations/prepare.py @@ -26,7 +26,7 @@ from pip._internal.exceptions import ( from pip._internal.index.package_finder import PackageFinder from pip._internal.metadata import BaseDistribution, get_metadata_distribution from pip._internal.models.direct_url import ArchiveInfo -from pip._internal.models.link import Link +from pip._internal.models.link import Link, join_within_directory from pip._internal.models.wheel import Wheel from pip._internal.network.download import BatchDownloader, Downloader from pip._internal.network.lazy_wheel import ( @@ -195,7 +195,7 @@ def _check_download_dir( """Check download_dir for previously downloaded file with correct hash If a correct file is found return its path else None """ - download_path = os.path.join(download_dir, link.filename) + download_path = join_within_directory(download_dir, link.filename) if not os.path.exists(download_path): return None @@ -673,7 +673,7 @@ class RequirementPreparer: # No distribution was downloaded for this requirement. return - download_location = os.path.join(self.download_dir, link.filename) + download_location = join_within_directory(self.download_dir, link.filename) if not os.path.exists(download_location): shutil.copy(req.local_file_path, download_location) download_path = display_path(download_location) diff --git a/tests/unit/test_link.py b/tests/unit/test_link.py index a379d87..dc9cdbf 100644 --- a/tests/unit/test_link.py +++ b/tests/unit/test_link.py @@ -1,8 +1,15 @@ +import os +import posixpath from typing import Optional import pytest -from pip._internal.models.link import Link, links_equivalent +from pip._internal.models.link import ( + Link, + as_path_component, + join_within_directory, + links_equivalent, +) from pip._internal.utils.hashes import Hashes @@ -28,6 +35,13 @@ class TestLink: ("https://example.com/path/page.html", "page.html"), # Test a quoted character. ("https://example.com/path/page%231.html", "page#1.html"), + # A doubly-encoded separator must stay encoded: the path is decoded + # exactly once, so the file name keeps its literal "%2F" instead of + # collapsing into a "/". + ( + "https://example.com/a%252Fb.whl", + "a%2Fb.whl", + ), ( "http://yo/myproject-1.0%2Bfoobar.0-py2.py3-none-any.whl", "myproject-1.0+foobar.0-py2.py3-none-any.whl", @@ -48,6 +62,52 @@ class TestLink: link = Link(url) assert link.filename == expected + @pytest.mark.parametrize( + "url", + [ + "https://example.com/a%252Fb.whl", + "https://example.com/%252e%252e%252fb.whl", + ], + ) + def test_filename_decoded_once_stays_single_component(self, url: str) -> None: + # The path is decoded exactly once, so an encoded separator stays + # encoded and the file name remains a single path component rather + # than collapsing into a "/"-separated path. + filename = Link(url).filename + assert not posixpath.isabs(filename) + assert posixpath.basename(filename) == filename + + @pytest.mark.parametrize( + "url", + [ + "https://example.com/..", + "https://example.com/.", + "https://example.com/foo/%2e%2e", + ], + ) + def test_filename_parent_reference_falls_back_to_netloc(self, url: str) -> None: + # A path that is only a "." or ".." reference has no usable file name, + # so filename falls back to the netloc rather than handing back a + # traversal component that could escape a download directory. + assert Link(url).filename == "example.com" + + @pytest.mark.parametrize( + "url", + [ + # A path-less URL whose authority looks like a traversal: the netloc + # fallback must still reduce to a single path component. + "http://..\\..\\..\\evil.whl", + "http://../", + "http://..", + ], + ) + def test_filename_is_always_a_path_component(self, url: str) -> None: + # filename must never carry a separator or parent reference, so joining + # it onto a directory can never escape that directory. + name = Link(url).filename + assert os.path.basename(name) == name + assert name not in (os.curdir, os.pardir) + def test_splitext(self) -> None: assert ("wheel", ".whl") == Link("http://yo/wheel.whl").splitext() @@ -240,3 +300,53 @@ def test_links_equivalent(url1: str, url2: str) -> None: ) def test_links_equivalent_false(url1: str, url2: str) -> None: assert not links_equivalent(Link(url1), Link(url2)) + + +@pytest.mark.parametrize( + "name", + [ + "wheel.whl", + "myproject-1.0+foobar.0-py2.py3-none-any.whl", + # A literal "%2F" is a normal file name, not a separator. + "a%2Fb.whl", + ], +) +def test_as_path_component_keeps_plain_name(name: str) -> None: + assert as_path_component(name) == name + + +@pytest.mark.parametrize( + "name", + [ + os.path.join(os.sep, "abs", "pkg.whl"), + os.path.join("..", "pkg.whl"), + os.path.join("nested", "pkg.whl"), + ], +) +def test_as_path_component_reduces_to_basename(name: str) -> None: + # A name carrying directory components is reduced to its basename, so the + # result always stays inside the directory it is later joined onto. + assert as_path_component(name) == os.path.basename(name) + + +@pytest.mark.parametrize("name", ["", ".", "..", "/", os.path.join("sub", "..")]) +def test_as_path_component_rejects_empty_or_parent_reference(name: str) -> None: + with pytest.raises(ValueError): + as_path_component(name) + + +@pytest.mark.parametrize( + "name", + [ + "pkg.whl", + # A literal "%2F" is a normal file name, not a separator. + "a%2Fb.whl", + ], +) +def test_join_within_directory_stays_inside(name: str) -> None: + # The component is joined onto the directory as its final element, so the + # result stays inside the directory. + directory = os.path.join("base", "downloads") + joined = join_within_directory(directory, as_path_component(name)) + assert joined == os.path.join(directory, name) + assert os.path.basename(joined) == name -- 2.55.0