Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Security

- Inspect hidden ZIP archives and malicious pickle payloads in legacy GGML model variants.
- Stop reporting a ZIP polyglot for GGUF/GGML files whose tensor data merely contains an end-of-central-directory signature.

### Bug Fixes

- Avoid network false positives for bounded README examples that download sample images over HTTPS from Hugging Face.
Expand Down
26 changes: 21 additions & 5 deletions modelaudit/scanners/gguf_scanner.py
Original file line number Diff line number Diff line change
Expand Up @@ -760,11 +760,26 @@ def _scan_gguf(self, f: BinaryIO, file_size: int, result: ScanResult) -> None:
)
result.bytes_scanned = max(result.bytes_scanned, f.tell())

def _scan_zip_polyglot(self, result: ScanResult) -> bool:
"""Inspect ZIP members even when GGUF metadata or tensor parsing fails."""
def _scan_zip_polyglot(self, result: ScanResult, *, format_name: str = "GGUF") -> bool:
"""Inspect ZIP members even when GGUF/GGML header parsing fails."""
if not zipfile.is_zipfile(self.current_file_path):
return False

# `is_zipfile` only proves an end-of-central-directory signature is present: it returns True
# for any file whose trailing bytes happen to contain b"PK\x05\x06" followed by 18 bytes.
# Model tensor data hits that by chance, so a cleanly-readable archive carrying no members
# is not a polyglot - a hidden payload always has at least one entry.
#
# A directory that fails to open is NOT treated as benign here: it falls through to the
# preflight below so a corrupted or truncated archive still fails closed as incomplete.
try:
with zipfile.ZipFile(self.current_file_path) as embedded_archive:
embedded_members: list[str] | None = embedded_archive.namelist()
Comment on lines +776 to +777

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Run bounded preflight before materializing ZIP entries

For a GGUF/GGML polyglot whose central directory exceeds max_zip_central_directory_size or max_zip_entries, constructing ZipFile and calling namelist() parses and materializes the attacker-controlled directory before ZipScanner can enforce either configured bound. A sufficiently large directory can therefore consume excessive memory and CPU in the scanner despite the archive preflight limits; determine emptiness through the bounded preflight rather than opening the archive first.

Useful? React with 👍 / 👎.

except (OSError, zipfile.BadZipFile):
embedded_members = None
if embedded_members is not None and not embedded_members:
return False
Comment on lines +780 to +781

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Do not trust an empty final EOCD as an empty archive

When a GGML polyglot contains a valid nonempty ZIP and an attacker appends a second empty EOCD record, Python's ZipFile.namelist() selects the final record and returns [], so this early return produces a successful scan with no S908 or nested-pickle finding. The bounded ZIP preflight deliberately examines earlier EOCD candidates and rejects this exact construction as ambiguous, but the return prevents it from running; continue into preflight so such hidden content at least fails closed.

AGENTS.md reference: AGENTS.md:L115-L115

Useful? React with 👍 / 👎.


from .archive_dispatch import (
_ZIP_CONTAINER_PREFLIGHT_REJECTED_PATHS_PRIVATE_METADATA_KEY,
merge_executable_zip_container_findings,
Expand All @@ -776,7 +791,7 @@ def _scan_zip_polyglot(self, result: ScanResult) -> bool:
self.current_file_path,
result,
archive_config,
context="GGUF trailing ZIP polyglot",
context=f"{format_name} trailing ZIP polyglot",
)
rejected_paths = result._private_metadata.get(
_ZIP_CONTAINER_PREFLIGHT_REJECTED_PATHS_PRIVATE_METADATA_KEY,
Expand All @@ -789,9 +804,9 @@ def _scan_zip_polyglot(self, result: ScanResult) -> bool:
return False

result.add_check(
name="GGUF ZIP Polyglot Detection",
name=f"{format_name} ZIP Polyglot Detection",
passed=False,
message="GGUF file is also a valid ZIP archive and may contain hidden archive content",
message=f"{format_name} file is also a valid ZIP archive and may contain hidden archive content",
severity=IssueSeverity.CRITICAL,
location=self.current_file_path,
details={"embedded_format": "zip"},
Expand Down Expand Up @@ -1050,6 +1065,7 @@ def _scan_ggml(
"""Basic GGML file validation with security checks."""
result.metadata["format"] = "ggml"
result.metadata["magic"] = magic.decode("ascii", "ignore")
self._scan_zip_polyglot(result, format_name="GGML")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve nested ZIP byte accounting

When a compressed ZIP member expands beyond the outer file size, this call merges the member scan's bytes_scanned, but _scan_ggml later replaces it with file_size. In a directory containing several GGML polyglots whose individual uncompressed totals are below max_total_size, aggregate accounting can therefore remain below --max-size and continue scanning past the safety budget; retain at least the merged count, as _scan_gguf does, and cover this budget outcome.

AGENTS.md reference: AGENTS.md:L137-L137

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restore outer GGML identity after ZIP merging

When a ZIP member is itself a GGUF/GGML model, merge_executable_zip_container_findings() merges that child's metadata into result; because the outer format and magic are assigned before this call, the child can replace them. For example, a GGMF polyglot containing nested.gguf is reported with format == "gguf", while a nested GGML variant can replace the outer magic. Move the outer identity assignments after the merge or restore them so reports describe the scanned outer artifact.

Useful? React with 👍 / 👎.


if file_size < 32:
result.add_check(
Expand Down
83 changes: 83 additions & 0 deletions tests/scanners/test_gguf_scanner.py
Original file line number Diff line number Diff line change
Expand Up @@ -2534,6 +2534,51 @@ def test_ggml_variant_scanner_basic(tmp_path):
assert result.metadata.get("magic") == "GGMF"


@pytest.mark.parametrize(
("magic", "suffix"),
[
(b"GGML", ".ggml"),
(b"GGMF", ".ggmf"),
(b"GGJT", ".ggjt"),
(b"GGLA", ".ggla"),
(b"GGSA", ".ggsa"),
],
ids=["ggml", "ggmf", "ggjt", "ggla", "ggsa"],
)
def test_ggml_scanner_inspects_embedded_zip_polyglot_members(
tmp_path: Path,
magic: bytes,
suffix: str,
) -> None:
path = tmp_path / f"polyglot{suffix}"
_write_ggml_variant_file(path, magic)
pickle_path = create_malicious_pickle(tmp_path / "payload.pkl")
_append_gguf_zip(path, {"payload.pkl": pickle_path.read_bytes(), "../escaped.txt": b"escape"})

direct = GgufScanner().scan(str(path))
aggregate = scan_model_directory_or_file(str(path), cache_enabled=False)

for result in (direct, aggregate):
assert any(issue.rule_code == "S908" and issue.severity == IssueSeverity.CRITICAL for issue in result.issues)
assert any(issue.rule_code == "S201" and "system" in issue.message.lower() for issue in result.issues)
assert any(issue.rule_code == "S405" and "escaped.txt" in issue.message for issue in result.issues)
assert any(check.name == "GGML ZIP Polyglot Detection" for check in direct.checks)
assert determine_exit_code(aggregate) == 1


def test_ggml_scanner_does_not_misclassify_invalid_zip_near_match(tmp_path: Path) -> None:
path = tmp_path / "zip-near-match.ggml"
_write_ggml_file(path)
with path.open("ab") as handle:
handle.write(b"PK\x03\x04not-a-valid-archive")

result = GgufScanner().scan(str(path))

assert result.success is True
assert not any(issue.rule_code == "S908" for issue in result.issues)
assert not any(issue.severity == IssueSeverity.CRITICAL for issue in result.issues)


def test_ggml_scanner_suspicious_version(tmp_path):
"""Test that GGML scanner handles unusual versions gracefully."""
path = tmp_path / "unusual_version.ggml"
Expand Down Expand Up @@ -2923,3 +2968,41 @@ def test_gguf_scanner_last_tensor_size(tmp_path):
# Should not have size mismatch warnings
size_warnings = [i for i in result.issues if "size mismatch" in i.message.lower()]
assert len(size_warnings) == 0


@pytest.mark.parametrize("magic", [b"GGML", b"GGMF", b"GGJT", b"GGLA", b"GGSN"])
def test_ggml_scanner_ignores_stray_end_of_central_directory_bytes(tmp_path: Path, magic: bytes) -> None:
"""A bare EOCD signature in tensor data is not a polyglot.

``zipfile.is_zipfile`` returns True for any file whose trailing bytes contain ``PK\\x05\\x06``
followed by 18 bytes, which model tensor data hits by chance. Reporting CRITICAL S908 for an
archive carrying no members at all is a false positive; a hidden payload always has an entry.
The existing near-match regression cannot catch this because it appends ``PK\\x03\\x04``, a
local-file-header signature that end-of-central-directory scanning never inspects.
"""
path = tmp_path / "stray-eocd.bin"
_write_ggml_variant_file(path, magic)
with path.open("ab") as handle:
handle.write(b"\x00" * 64 + b"PK\x05\x06" + b"\x00" * 18 + b"\x00" * 32)

assert zipfile.is_zipfile(str(path))

result = GgufScanner().scan(str(path))

assert not any("Polyglot" in check.name for check in result.checks)
assert not any(issue.rule_code == "S908" for issue in result.issues)


def test_gguf_scanner_ignores_stray_end_of_central_directory_bytes(tmp_path: Path) -> None:
"""The same false positive existed for GGUF before the member check."""
path = tmp_path / "stray-eocd.gguf"
_write_minimal_gguf(path)
with path.open("ab") as handle:
handle.write(b"\x00" * 64 + b"PK\x05\x06" + b"\x00" * 18 + b"\x00" * 32)

assert zipfile.is_zipfile(str(path))

result = GgufScanner().scan(str(path))

assert not any("Polyglot" in check.name for check in result.checks)
assert not any(issue.rule_code == "S908" for issue in result.issues)
Loading