From 22675c6f91c02675f2999c0660037cc380acbf02 Mon Sep 17 00:00:00 2001 From: pyoor Date: Mon, 31 Aug 2026 10:03:16 -0400 Subject: [PATCH] fix: reject unsafe bugzilla attachment paths --- pyproject.toml | 1 + src/bugmon/bugmon.py | 16 +++++++++++---- src/bugmon/utils.py | 43 +++++++++++++++++++++++++++++++++++++- tests/test_bugmon.py | 49 ++++++++++++++++++++++++++++++++++++++++++++ tests/test_utils.py | 45 ++++++++++++++++++++++++++++++++++++++++ uv.lock | 13 +++++++++++- 6 files changed, 161 insertions(+), 6 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index b549c5b..31ce82a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -31,6 +31,7 @@ dependencies = [ "autobisect>=9.2,<10", "bugsy @ git+https://github.com/MozillaSecurity/Bugsy.git", "fuzzfetch>=16.1,<17", + "pathvalidate>=3,<4", "typing-extensions>=4.2,<5", ] urls.Homepage = "https://github.com/MozillaSecurity/bugmon" diff --git a/src/bugmon/bugmon.py b/src/bugmon/bugmon.py index 76c2652..50612ff 100644 --- a/src/bugmon/bugmon.py +++ b/src/bugmon/bugmon.py @@ -27,6 +27,7 @@ PernoscoCreds, get_pernosco_trace, is_pernosco_available, + sanitize_attachment_path, submit_pernosco, ) @@ -501,15 +502,22 @@ def fetch_attachments(self, unpack: Optional[bool] = True) -> None: try: with zipfile.ZipFile(io.BytesIO(data)) as z: for filename in z.namelist(): - if (self.test_dir / filename).exists(): - log.warning("Duplicate filename: %s", filename) - z.extract(filename, self.test_dir) + # Skip directory entries + if filename.endswith("/"): + continue + dest = sanitize_attachment_path(self.test_dir, filename) + if dest.exists(): + log.warning("Duplicate filename: %s", dest) + dest.parent.mkdir(parents=True, exist_ok=True) + dest.write_bytes(z.read(filename)) except zipfile.BadZipFile as e: log.warning("Failed to decompress attachment: %s", e) continue else: - Path(self.test_dir, attachment.file_name).write_bytes(data) + dest = sanitize_attachment_path(self.test_dir, attachment.file_name) + dest.parent.mkdir(parents=True, exist_ok=True) + dest.write_bytes(data) def needs_bisect(self) -> bool: """Helper function to determine eligibility for 'bisect'""" diff --git a/src/bugmon/utils.py b/src/bugmon/utils.py index 31a3060..10d427b 100644 --- a/src/bugmon/utils.py +++ b/src/bugmon/utils.py @@ -8,7 +8,7 @@ import tempfile import zipfile from contextlib import contextmanager -from pathlib import Path, PurePosixPath +from pathlib import Path, PurePosixPath, PureWindowsPath from urllib.parse import urlparse try: @@ -18,6 +18,7 @@ from typing_extensions import TypedDict import requests +from pathvalidate import sanitize_filename from requests.adapters import HTTPAdapter, Retry from requests.models import Response @@ -36,6 +37,46 @@ log = logging.getLogger(__name__) +def sanitize_attachment_path(base_dir: Path, file_name: str) -> Path: + """Build a sanitized destination path for an attachment within base_dir. + + Each path component is sanitized individually with "universal" rules so + that characters grizzly's TestCase loader rejects (e.g. ':') are stripped + everywhere - including the leading component, which sanitize_filepath would + otherwise preserve as a Windows drive letter. Subdirectory structure is + kept, while paths containing traversal ('..'), current-dir ('.'), or + absolute ('/') components raise ValueError. + + :param base_dir: Directory the attachment should be written to. + :param file_name: Original attachment or archive member name. + :return: Sanitized path within base_dir. + :raises ValueError: If file_name is empty, absolute, contains path traversal, + or has a component without a valid sanitized name. + """ + + displayed_name = repr(file_name) + if len(displayed_name) > 100: + displayed_name = f"{displayed_name[:97]}..." + + windows_path = PureWindowsPath(file_name) + raw_parts = [part for part in file_name.replace("\\", "/").split("/") if part] + if ( + windows_path.drive + or windows_path.root + or not raw_parts + or any(part in (".", "..") for part in raw_parts) + ): + raise ValueError(f"Unsafe attachment path: {displayed_name}") + + # Sanitization can normalize names like ".. " into "..", so the sanitized + # components must be re-checked before they are used. + parts = [sanitize_filename(part, platform="universal") for part in raw_parts] + if any(part in ("", ".", "..") for part in parts): + raise ValueError(f"Unsafe attachment path: {displayed_name}") + + return base_dir.joinpath(*parts) + + class PernoscoCreds(TypedDict): """Interface representing required pernosco creds""" diff --git a/tests/test_bugmon.py b/tests/test_bugmon.py index 7b3bef7..39de099 100644 --- a/tests/test_bugmon.py +++ b/tests/test_bugmon.py @@ -2,6 +2,10 @@ # This Source Code Form is subject to the terms of the Mozilla Public License, # v. 2.0. If a copy of the MPL was not distributed with this file, You can # obtain one at http://mozilla.org/MPL/2.0/. +import base64 +import io +import zipfile + import pytest from autobisect.bisect import BisectionResult from fuzzfetch import Platform @@ -11,6 +15,16 @@ from bugmon.exceptions import BugmonException +def _attachment(file_name, data, creation_time="2020-06-30T12:40:45Z"): + attachment = type("Attachment", (), {})() + attachment.file_name = file_name + attachment.data = base64.b64encode(data).decode() + attachment.creation_time = creation_time + attachment.content_type = "text/plain" + attachment.is_obsolete = False + return attachment + + def test_bugmon_need_info_on_bisect_fix(mocker, bugmon, build): """Test that the assignee is NI'd when the testcase no longer reproduces""" mocker.patch.object(bugmon, "detect_config", return_value=True) @@ -46,6 +60,41 @@ def test_bugmon_throws_without_pernosco_submit( BugMonitor(bugsy, bug, working_dir, pernosco_creds, False) +def test_bugmon_fetch_attachments_preserves_nested_paths(mocker, bugmon): + """Verify valid ZIP and regular attachments preserve nested paths.""" + archive_data = io.BytesIO() + with zipfile.ZipFile(archive_data, "w") as archive: + archive.writestr("nested/test.js", b"nested") + archive.writestr("nested/", b"") + + attachments = [ + _attachment("attachments.zip", archive_data.getvalue()), + _attachment("other/regular.js", b"regular", "2020-07-01T12:40:45Z"), + ] + mocker.patch("bugmon.bug.EnhancedBug.get_attachments", return_value=attachments) + + bugmon.fetch_attachments() + + assert (bugmon.test_dir / "nested/test.js").read_bytes() == b"nested" + assert (bugmon.test_dir / "other/regular.js").read_bytes() == b"regular" + + +def test_bugmon_fetch_attachments_rejects_unsafe_path(mocker, bugmon): + """Verify unsafe attachment paths raise instead of being normalized.""" + archive_data = io.BytesIO() + with zipfile.ZipFile(archive_data, "w") as archive: + archive.writestr("../escaped.js", b"escaped") + + attachments = [_attachment("attachments.zip", archive_data.getvalue())] + mocker.patch("bugmon.bug.EnhancedBug.get_attachments", return_value=attachments) + + with pytest.raises(ValueError, match="Unsafe attachment path"): + bugmon.fetch_attachments() + + assert not (bugmon.test_dir / "escaped.js").exists() + assert not (bugmon.test_dir.parent / "escaped.js").exists() + + def test_bugmon_no_need_pernosco_with_pernosco_failed(bugmon): """Test that bugmon does not attempt to record a pernosco session when the pernosco-failed status command is present""" diff --git a/tests/test_utils.py b/tests/test_utils.py index 2990930..fe1e73e 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -36,6 +36,51 @@ def test_get_pernosco_trace_none_match(tmp_path): assert utils.get_pernosco_trace(tmp_path) is None +@pytest.mark.parametrize( + "file_name, expected", + [ + ("test.js", "test.js"), + ("subdir/test.js", "subdir/test.js"), + ("foo:bar.js", "foobar.js"), + (r"subdir\\test.js", "subdir/test.js"), + ], +) +def test_sanitize_attachment_path(tmp_path, file_name, expected): + """Verify attachment paths are sanitized and remain relative.""" + assert utils.sanitize_attachment_path(tmp_path, file_name) == tmp_path / expected + + +@pytest.mark.parametrize( + "file_name", + [ + "", + ".", + "..", + "../..", + "../outside.js", + ".. /outside.js", + "..\x00/outside.js", + "nested/./test.js", + "nested/../test.js", + "/absolute/path.js", + "C:\\absolute\\path.js", + ], +) +def test_sanitize_attachment_path_rejects_unsafe_paths(tmp_path, file_name): + """Verify unsafe paths raise instead of being normalized.""" + with pytest.raises(ValueError, match="Unsafe attachment path"): + utils.sanitize_attachment_path(tmp_path, file_name) + + +def test_sanitize_attachment_path_limits_error_length(tmp_path): + """Verify unsafe filenames cannot produce excessively long errors.""" + with pytest.raises(ValueError) as exc_info: + utils.sanitize_attachment_path(tmp_path, f"../{'x' * 1000}.js") + + assert len(str(exc_info.value)) <= 125 + assert str(exc_info.value).endswith("...") + + def test_has_pernosco_creds_all(pernosco_creds): """Verify that has_pernosco_creds returns True when all creds present""" assert utils.has_pernosco_creds(pernosco_creds) is True diff --git a/uv.lock b/uv.lock index 83d41be..c3e8e19 100644 --- a/uv.lock +++ b/uv.lock @@ -139,12 +139,13 @@ wheels = [ [[package]] name = "bugmon" -version = "5.2.1" +version = "5.3.0" source = { editable = "." } dependencies = [ { name = "autobisect" }, { name = "bugsy" }, { name = "fuzzfetch" }, + { name = "pathvalidate" }, { name = "typing-extensions" }, ] @@ -175,6 +176,7 @@ requires-dist = [ { name = "autobisect", specifier = ">=9.2,<10" }, { name = "bugsy", git = "https://github.com/MozillaSecurity/Bugsy.git" }, { name = "fuzzfetch", specifier = ">=16.1,<17" }, + { name = "pathvalidate", specifier = ">=3,<4" }, { name = "typing-extensions", specifier = ">=4.2,<5" }, ] @@ -1339,6 +1341,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/ef/3c/2c197d226f9ea224a9ab8d197933f9da0ae0aac5b6e0f884e2b8d9c8e9f7/pathspec-1.0.4-py3-none-any.whl", hash = "sha256:fb6ae2fd4e7c921a165808a552060e722767cfa526f99ca5156ed2ce45a5c723", size = 55206, upload-time = "2026-01-27T03:59:45.137Z" }, ] +[[package]] +name = "pathvalidate" +version = "3.3.1" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/fa/2a/52a8da6fe965dea6192eb716b357558e103aea0a1e9a8352ad575a8406ca/pathvalidate-3.3.1.tar.gz", hash = "sha256:b18c07212bfead624345bb8e1d6141cdcf15a39736994ea0b94035ad2b1ba177", size = 63262, upload-time = "2025-06-15T09:07:20.736Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/9a/70/875f4a23bfc4731703a5835487d0d2fb999031bd415e7d17c0ae615c18b7/pathvalidate-3.3.1-py3-none-any.whl", hash = "sha256:5263baab691f8e1af96092fa5137ee17df5bdfbd6cff1fcac4d6ef4bc2e1735f", size = 24305, upload-time = "2025-06-15T09:07:19.117Z" }, +] + [[package]] name = "platformdirs" version = "4.9.4"