Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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 thread
mldangelo-oai marked this conversation as resolved.
Outdated
except (OSError, zipfile.BadZipFile):
embedded_members = None
if embedded_members is not None and not embedded_members:
return False
Comment thread
mldangelo-oai marked this conversation as resolved.
Outdated

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",
Comment thread
mldangelo-oai marked this conversation as resolved.
)
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")
Comment thread
mldangelo-oai marked this conversation as resolved.
Comment thread
mldangelo-oai marked this conversation as resolved.

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