diff --git a/CHANGELOG.md b/CHANGELOG.md index a35827d21..b1cd72a71 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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. diff --git a/modelaudit/scanners/gguf_scanner.py b/modelaudit/scanners/gguf_scanner.py index b278926de..4ab4a33a6 100644 --- a/modelaudit/scanners/gguf_scanner.py +++ b/modelaudit/scanners/gguf_scanner.py @@ -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() + except (OSError, zipfile.BadZipFile): + embedded_members = None + if embedded_members is not None and not embedded_members: + return False + from .archive_dispatch import ( _ZIP_CONTAINER_PREFLIGHT_REJECTED_PATHS_PRIVATE_METADATA_KEY, merge_executable_zip_container_findings, @@ -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, @@ -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"}, @@ -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") if file_size < 32: result.add_check( diff --git a/tests/scanners/test_gguf_scanner.py b/tests/scanners/test_gguf_scanner.py index 6a9b7947c..5602f48f2 100644 --- a/tests/scanners/test_gguf_scanner.py +++ b/tests/scanners/test_gguf_scanner.py @@ -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" @@ -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)