fix(ggml): detect embedded ZIP polyglot payloads - #1780
Conversation
mldangelo-oai
added
codex
security
Performance BenchmarksCompared
|
There was a problem hiding this comment.
Pull request overview
Closes a security bypass in the GGUF/GGML scanner by ensuring legacy GGML variants also trigger the existing ZIP-polyglot preflight and embedded-archive inspection path, with regressions to prevent recurrence.
Changes:
- Extend GGUF ZIP polyglot inspection helper to support GGML context labeling and invoke it from GGML scans.
- Add regression tests covering embedded ZIP polyglots across all legacy GGML magic variants plus an invalid-ZIP near match.
- Document the security fix in the Unreleased changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
modelaudit/scanners/gguf_scanner.py |
Parameterizes ZIP polyglot detection labeling and enables it for GGML scans. |
tests/scanners/test_gguf_scanner.py |
Adds malicious polyglot regressions for GGML variants and a benign invalid-ZIP near-match test. |
CHANGELOG.md |
Notes the GGML ZIP/pickle inspection security hardening under Unreleased. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8ee962c4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """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") |
There was a problem hiding this comment.
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 👍 / 👎.
| """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") |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
modelaudit/scanners/gguf_scanner.py:1053
_scan_ggml()always reports ZIP polyglot findings as "GGML …" even for other GGML-variant magics (e.g., GGMF/GGJT/GGLA/GGSA). Sinceresult.metadata["magic"]is already set from the real header, the check name/message can become misleading for users triaging alerts on those variant files.
result.metadata["format"] = "ggml"
result.metadata["magic"] = magic.decode("ascii", "ignore")
self._scan_zip_polyglot(result, format_name="GGML")
The main merge joined the new Security section directly onto Bug Fixes, which fails the documentation formatting check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
modelaudit/scanners/gguf_scanner.py:767
_scan_zip_polyglotcallszipfile.is_zipfile(...)and then delegates tomerge_executable_zip_container_findings(...), which (permodelaudit/scanners/archive_dispatch.py:538-540) callszipfile.is_zipfile(...)again and performs additional imports before returning. With this PR, the double ZIP probe now runs for GGML as well, which is extra overhead and also increases exposure to pathological ZIP central directories being parsed twice.
Consider refactoring so the ZIP validity/preflight is performed once (ideally via the existing bounded ZIP preflight helpers in zip_scanner.py), and then pass that result into merge_executable_zip_container_findings (or restructure it to accept an already-confirmed ZIP) to avoid redundant parsing and heavy imports on non-ZIP files.
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
zipfile.is_zipfile only proves an end-of-central-directory signature is present: it returns True for any file whose trailing 64 KiB contains PK\x05\x06 followed by 18 bytes. Model tensor data hits that by chance, so a benign file was reported as CRITICAL S908 with an archive carrying zero members. This was already reachable on main for GGUF; extending the preflight to the five legacy GGML magics would have widened it to raw tensor blobs. The existing benign near-match test cannot catch it because it appends PK\x03\x04, a local-file-header signature that end-of-central-directory scanning never inspects. A directory that fails to OPEN is deliberately not treated as benign: it falls through to the preflight so corrupted or truncated archives still fail closed as incomplete. Guarding on open-failure too silently skipped that path and broke the corrupted-directory near-match regressions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/scanners/test_gguf_scanner.py:2973
- The parameterized GGML magic list includes
b"GGSN", but the supported legacy GGML variant magic appears to beb"GGSA"(and this file’s other parametrization usesGGSA). Using an unknown magic here can make the test exercise the wrong code path (or fail early) for the 5th variant.
@pytest.mark.parametrize("magic", [b"GGML", b"GGMF", b"GGJT", b"GGLA", b"GGSN"])
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64e076a723
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if embedded_members is not None and not embedded_members: | ||
| return False |
There was a problem hiding this comment.
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 👍 / 👎.
| with zipfile.ZipFile(self.current_file_path) as embedded_archive: | ||
| embedded_members: list[str] | None = embedded_archive.namelist() |
There was a problem hiding this comment.
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 👍 / 👎.
Summary
Validation
The broader scanner run also reaches one unrelated compressed-wrapper cache baseline failure, and full local macOS mypy reports existing out-of-scope platform/type errors; current-main GitHub Python CI and Type Check are green.