fix(gguf): calibrate metadata string findings - #1651
mldangelo-oai merged 35 commits into
Conversation
|
@codex review |
Performance BenchmarksCompared
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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: 513911dd22
ℹ️ 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".
|
@codex address that feedback. Fix all five unresolved threads on the exact current head: eliminate both CodeQL regex backtracking paths with bounded/linear parsing, preserve curl URLs when output/options precede them, support keyword-argument network API calls, and detect common command prefixes such as sudo before destructive commands. Add adversarial performance tests plus malicious and benign end-to-end GGUF metadata regressions, push a new head, and report focused/broad test evidence. |
|
@codex review latest |
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Summary
Testing
|
|
@codex review latest after addressing the metadata evidence feedback |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c98239554e
ℹ️ 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".
|
@codex review latest after addressing split rm options, command-span fetch evidence, and bounded URL scanning |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6effa9184f
ℹ️ 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".
|
Additional pinned GGUF metadata QA: |
|
Additional pinned GGUF metadata QA: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6effa9184f
ℹ️ 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".
|
@codex include this additional BF16 calibration fixture: |
|
Summary
Testing
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60847c32c3
ℹ️ 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".
|
Pinned real-model QA from Hugging Face rank 250 on exact main
That is 33 informational metadata false positives. Please add the exact pinned model as an end-to-end regression and ensure calibration does not hide genuinely executable or attacker-controlled metadata. Full audit: |
|
Additional exact-main GGUF metadata QA from Hugging Face rank 253:
Please include the exact pinned file in current-head QA and preserve detection for genuinely dangerous metadata. Full audit: |
Security Review: promptfoo/modelaudit PR #1651Scope
Scan Summary
Artifacts:
Threat ModelThe repository's authoritative
Findings
Confidence Scale
[1] Curl options with separate values stop remote-fetch detection
Summary
ValidationMethod: generated GGUF fixture through
The same parser class is vulnerable to other omitted value-taking options. The reproduction is not dependent on optional packages or host state. DataflowGGUF string metadata -> ReachabilityAny party able to supply a GGUF can choose this metadata. The production scanner parses it without authentication. The immediate impact is loss of a security finding; the repository does not establish that ModelAudit executes the command or that a downstream loader necessarily does. SeverityMedium. The bypass is trivial and directly affects a production security control, but direct code execution is not proven in this repository and S902 is informational. Additional evidence that a supported downstream consumer executes or trusts this metadata would raise severity; proof that all consumers ignore it would lower impact. RemediationUse a bounded shell tokenizer and an authoritative option-arity table, or otherwise continue scanning safely past recognized value operands without stopping at the first ordinary token. Add malicious tests for [2] Absolute-path shell launchers bypass shell-command evidence
SummaryThe ValidationMethod: generated GGUF fixture through the real scanner, exact current/base comparison, and regex trace.
DataflowGGUF string metadata -> ReachabilityA malicious model author controls the metadata string and can use ordinary absolute executable paths. The finding affects report integrity at the model-file trust boundary. No execution by ModelAudit itself is claimed. SeverityMedium. The production scanner deterministically loses an explicit command-execution signal, but downstream execution and major compromise are not proven. Evidence of a loader executing this field would raise severity; an exact downstream non-execution invariant would lower it. RemediationNormalize the candidate executable to its basename before command classification, or extend the bounded parser already used for fetch commands to shell launchers. Cover Unix paths, Windows paths, [3] Network API detection misses URLs assigned before the call
SummaryThe network API helper searches only the immediate call argument window for a literal URL. A single metadata value containing ValidationMethod: generated GGUF fixture through the real scanner, exact current/base comparison, and bounded dataflow trace.
The scanner intentionally avoids full language interpretation; the report does not require general Python execution analysis, only preservation of this concrete same-value case. DataflowGGUF string metadata -> ReachabilityA model author can place the code-like string in ordinary metadata. The production scanner reports the file without this signal. The attack boundary is the untrusted artifact to scanner result; no runtime execution sink is asserted. SeverityMedium. The omission is easy to trigger and violates a security-control invariant, but the direct consequence is a missing informational finding rather than proven compromise. A verified loader consuming executable metadata would raise severity; a documented restriction that such fields are never used would lower it. RemediationAdd bounded same-statement literal-assignment correlation for supported API calls, or use a bounded AST/token parser for Python-like snippets. Keep strict byte/step budgets and add positive tests for variable assignment plus negative tests for documentation prose and unrelated URLs. [4] Remote-fetch detection excludes valid curl destinations outside three schemes
Summary
ValidationMethod: generated GGUF fixtures through the real scanner, exact current/base comparison, and local curl 8.7.1 protocol/manual verification.
This issue was not present in the fetched prior review threads and is an independent new finding. DataflowGGUF string metadata -> ReachabilityA malicious model author can use syntax accepted by common curl builds. The exact supported protocol set varies by build, but scheme guessing and the tested GOPHER support are concrete on the validation host. Immediate impact remains scanner-report integrity. SeverityMedium. This is a reliable bypass of the new security evidence gate, but the repository does not prove an execution sink or major downstream impact. A supported consumer that runs these commands would raise severity; restricting the detection contract to a documented smaller protocol set would lower scope but would contradict the PR's active-fetch framing. RemediationClassify destinations using command semantics rather than a three-scheme tuple. At minimum, handle scheme-less host/path operands and curl-supported remote schemes under a bounded allowlist, while treating local schemes such as Reviewed Surfaces
PR Merge-Gate State
Thread Reconciliation
Validation Evidence
Changed-Code vs Pre-existing Behavior
Merge DispositionRequest changes / do not merge at The intended false-positive calibration is demonstrated and CI is green, but four deterministic changed-code false negatives remain in the same security evidence gate. Merge after all four forms are covered with bounded malicious-positive and benign-negative regressions, then rerun pinned GGUF QA and exact-head CI. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06212e55db
ℹ️ 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".
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Exact-head independent review of P2: curl short-option forms bypass remote-fetch detection The GGUF scanner misses P2: legal spaced attribute calls are false positives
P3: rank-262-shaped tokenizer output remains unbounded A synthetic GGUF with 262,144 tokenizer tokens, merges, and scores produced a 13.3 MB JSON result with no metadata-array truncation flags. This is residual resource/output risk rather than a merge blocker by itself. Validation on this exact head: full GGUF scanner tests |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Closeout on head
Validation:
Review state: |
Closeout Acknowledged
Checks
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f102d0d68f
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0d2318d6d
ℹ️ 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".
2 participants
Summary
Calibrates GGUF metadata string findings so benign punctuation does not produce S902 by itself.
Root cause:
GgufScannertreated any metadata value containing/,\,;,&&,|, or backticks as suspicious. That flagged ordinary repository URLs and normal Gemma chat-template syntax even when the dedicated Jinja analysis found no unsafe template behavior.Security tradeoff: GGUF metadata values now require concrete evidence before adding S902:
..path segmentsChat-template metadata is delegated to the existing Jinja scanner, so unsafe templates still produce Jinja findings while normal template syntax stays clean. Malformed metadata, duplicate keys, metadata/tensor parser budgets, oversized chat templates, and GGUF bounded-parser failures are unchanged.
Validation
uv sync --extra all-ciuv run ruff format modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/uv run ruff check --fix modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/uv run ruff check modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/uv run ruff format --check modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/uv run mypy modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/PROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest tests/scanners/test_gguf_scanner.py tests/scanners/test_jinja2_template_scanner.py -q480 passed, 1 skipped(ggufoptional package unavailable)git diff --checkPROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest -n auto -m "not slow and not integration" --maxfail=118549 passed, 793 skipped, 40 warningsPinned Real-Model QA
Used 128 MiB real HF prefixes from exact pinned
/resolve/<sha>/...ggufURLs, written into sparse local files with the advertised model sizes. This exercises ModelAudit's local end-to-end GGUF scan path without materializing multi-GB tensor payloads.OBLITERATUS/Gemma-4-12B-OBLITERATED @
f81b0cbd28a3650138635823bc101adb56a0bc4aGemma-4-12B-OBLITERATED-Q4_K_M.gguf7381382208PROMPTFOO_DISABLE_TELEMETRY=1 uv run modelaudit scan ../qa/pinned-gguf-prefixes/obliteratus-gemma-4-12b-obliterated-q4_k_m.sparse.gguf --format json --output ../qa/final-obliteratus.json --no-cache --max-size 8GB2S902 metadata-value issues (general.base_model.0.repo_url,tokenizer.chat_template)success: true,issue_count: 0unsloth/gemma-4-12B-it-qat-GGUF @
7102bdea62863acff919c945405ef29973113d66gemma-4-12B-it-qat-UD-Q4_K_XL.gguf6716355328PROMPTFOO_DISABLE_TELEMETRY=1 uv run modelaudit scan ../qa/pinned-gguf-prefixes/unsloth-gemma-4-12b-it-qat-ud-q4_k_xl.sparse.gguf --format json --output ../qa/final-unsloth.json --no-cache --max-size 8GB1S902 metadata-value issue (tokenizer.chat_template)success: true,issue_count: 0