fix(pytorch): trust referenced storage members#1654
Conversation
|
@codex review |
Performance BenchmarksCompared Top regressions:
|
|
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". |
|
New pinned HF QA blocks auto-merge. On main Auto-merge is disabled pending exact-head selected-file QA. @codex reproduce this single referenced member without downloading unrelated weights; prove the current branch suppresses the storage false positive while still detecting hidden/unreferenced malicious pickle members, malformed storage references, concatenated pickle streams, and disguised executable payloads. Post the pinned hash/results and request exact-head review. |
![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: c4ee93a01e
ℹ️ 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".
Independent adversarial review: promptfoo/modelaudit PR #1654FindingsP1 — Storage trust accepts provenance that PyTorch itself rejects, suppressing malicious membersReferences:
The trust predicate is materially weaker than PyTorch's loader semantics. Two exact-head adversarial cases demonstrate false negatives:
This is not the same as requiring every loader-valid tensor blob to be scanned as a pickle. Canonical, valid storage bytes are inert raw storage and may legitimately look like pickle or ELF data. The bug is that this implementation treats malformed descriptors and noncanonical member aliases as loader-valid provenance. Required fix: make trust match the loader rather than string shape. Use exact canonical ZIP member names for provenance (no slash/backslash aliasing), validate the complete persistent-ID shape and scalar fields, and restrict storage globals to the known PyTorch storage classes/modules. Any malformed descriptor or noncanonical member must remain on the generic probe path. Add mirrored main/standalone regressions for P2 — Valid
|
|
Pinned PiD QA on exact head
Before, on main
After, on exact PR head
Review follow-up controls added/kept:
Local validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d833e2d1c
ℹ️ 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".
Independent adversarial rereview: promptfoo/modelaudit PR #1654Snapshot: 2026-06-11T05:50:49Z FindingsP1 — Referenced storage members with a first pickle stream larger than 4 KiB bypass both scannersReferences:
Exact-head reproduction used a canonical PyTorch ZIP whose referenced
This is introduced by the PR. An unreferenced copy of the same 8.3 KiB payload remains detected by both scanners, proving the regression is the new trusted-member branch rather than the underlying scanner. The prior provenance blocker is only partially addressed. The parser still normalizes noncanonical member paths at Required fix: do not require a referenced member's first pickle stream to complete inside 4 KiB. Route structurally valid pickle prefixes to the existing bounded full scanner, or stream the probe with the same opcode/resource limits and explicit fail-closed notices. Make provenance match PyTorch loader semantics: exact canonical member names, exact persistent-ID shape/scalar validation, and a known storage-class/module allowlist. Add mirrored main/standalone tests for a first frame/body beyond 4 KiB, P2 — The new storage-reference pre-pass bypasses opcode, tracked-state, and timeout boundsReferences:
Measured on exact head with valid protocol-4 EMPTY_TUPLE streams:
An actual 1,048,579-byte archive scan configured with a 0.01-second main timeout took 0.73 seconds before returning Required fix: stream P2 — Valid
|
| Prior blocker | Exact-head result |
|---|---|
| Unsound PyTorch storage provenance | Unresolved. Small malicious referenced pickles are now probed, but suffix-based/normalized provenance still feeds loader-invalid archives into the weaker 4 KiB path. |
Valid UntypedStorage false positives |
Unresolved, but confirmed pre-existing rather than introduced by this PR. |
| Opcode/resource bounds | Unresolved. The new duplicated pre-pass still uses list(genops) and unbounded stack/memo state. |
| Wrong pinned regression | Fixed. The committed slow test now targets the exact PiD revision, file, and siglip2_decoder/data/356; it passed. |
Pinned checkpoint QA
Source:
- Repository/revision:
nvidia/PiD@b4887b3c8fc65277a9b7a084292bf9fc0778c5ac - File:
checkpoints/scale_rae/decoder/siglip2_sop14_i224_web73M_ganw3_decXL.pt - Bounded slice: central directory/local headers plus
data.pkl,version,byteorder, anddata/356.
Independent member evidence:
siglip2_decoder/data.pkl: 88,413 bytes; SHA-2568ee22aac231310f794cd58537e4d6d19f08ab32a99a573d18e0f0d2fcfa57bd8.siglip2_decoder/data/356: 5,308,416 bytes; SHA-25623c4f4d8d8546f94994eebaf5a4dbe5b7eb963e69000f71ba491b673591d0f20.- Rebuilt four-member slice: 5,397,355 bytes. Its ZIP hash is intentionally not stable because
writestrtimestamps vary; member hashes are stable.
Exact-head results:
- Main:
pickle_files=['siglip2_decoder/data.pkl']; no issue/check fordata/356. - Standalone:
pickle_files=['siglip2_decoder/data.pkl']; nodata/356finding. - Both remained inconclusive for the intentionally partial slice because 455 referenced storage members were omitted; main also hit the bounded JIT snippet limit.
- Committed pinned integration test: 1 passed.
Other adversarial controls
- Canonical short malicious referenced pickles: detected by both scanners.
- Canonical concatenated stream with an early benign pickle and later large malicious stream: detected by both scanners.
- Unreferenced large malicious storage-lookalike: detected by both scanners.
- Truncated
data.pkl: malicious storage member detected; scan marked inconclusive. - Missing referenced key: malicious existing member detected; missing-member outcome recorded.
torch.FakeStorageand backslash aliases with a large malicious member: missed on head, detected on base; PyTorch rejected both archives.- Leading-slash alias: PyTorch rejected it; standalone head omitted the large malicious member that base detected.
- Executable sidecar provenance suppression was reproduced on head and base and is therefore pre-existing, not attributed to this diff.
Focused validation
packages/modelaudit-picklescan/tests/test_api.py -k 'storage_blob or storage_lookalike or hidden_pytorch_zip_pickle_member': 10 passed.tests/scanners/test_pytorch_zip_scanner.py -k 'pytorch_zip_discovery': 14 passed.- Exact pinned PiD test: 1 passed.
- Ruff format check: 419 files already formatted.
- Ruff check: all checks passed.
- Focused mypy on the four changed Python source/test files: success.
git diff --check 8d6c4864...HEAD: clean.- Full local mypy on macOS reported five
os.listxattr/setxattr/getxattrtyping errors inmodelaudit/cli.pyandtests/test_cli.py; base reports the same five errors (and a mypy internal error), so these are pre-existing/platform-specific. Exact-head CI Type Check is green. - Isolated head worktree remained clean and detached.
Security scan artifacts:
- Markdown:
/tmp/codex-security-scans/modelaudit/9d833e2d1c90c24039a9a1857c83bf9f959fdc6c_20260611T053000Z/report.md - HTML:
/tmp/codex-security-scans/modelaudit/9d833e2d1c90c24039a9a1857c83bf9f959fdc6c_20260611T053000Z/report.html
Exact target, CI, and review state
- PR:
promptfoo/modelaudit#1654, open, non-draft. - Exact reviewed head:
9d833e2d1c90c24039a9a1857c83bf9f959fdc6c. - Exact base:
8d6c4864fe2ea833ceaef1b9803d225afb1e8d69(main). - Merge base equals the live base; branch is not behind.
- GitHub:
MERGEABLE,mergeStateStatus=BLOCKED,reviewDecision=REVIEW_REQUIRED. - CI: 28 checks passed, 4 intentionally skipped, 0 pending, 0 failed. Python CI, Docker, Windows, Python 3.10/3.12/3.13, standalone 3.10-3.13, Type Check, Ruff, CodeQL, package build, dependency audit, and benchmarks are green.
- Performance workflow: 0 regressions across 12 shared benchmarks; aggregate median +1.0%.
Review threads:
- 3 total, all technically unresolved in GitHub.
- 1 current/non-outdated exact-head P1 at
modelaudit/scanners/pytorch_zip_scanner.py:1676: large referenced storage pickles are omitted by the 4 KiB path. Independently validated here. - 2 outdated threads on
cf16a845: complete skipping of referenced members and the wrong Whisper pinned target. The latest head partially supersedes the first and fixes the second.
No GitHub comments, reviews, labels, approvals, or other state were posted or changed.
Merge disposition
DO NOT MERGE / request changes.
The exact head is CI-green and the PiD false positive is fixed, but the PR introduces a deterministic security-detection regression for larger referenced pickle streams and an unbounded pre-scan parser. It also leaves the known valid UntypedStorage case in the false-positive family the PR is intended to resolve.
Before merge:
- Remove the 4 KiB complete-stream false-negative path and add mirrored large-pickle/provenance regressions.
- Bound the storage-reference parser's opcode and state growth in both implementations.
- Add loadability-backed
torch.storage.UntypedStoragecoverage. - Re-run the exact PiD slice, adversarial controls, focused suites, and exact-head CI.
- Resolve the current exact-head P1 thread and request a new independent review on the replacement head.
|
Exact-head follow-up for the PiD storage-member blocker is pushed at Pinned bounded QA:
Follow-up fixes in this head:
Malicious controls covered:
Validation:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8804e039ee
ℹ️ 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".
|
Follow-up for the live P1 review thread is pushed at Change:
Added regressions:
Final exact-head validation after this commit:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 853404d4d1
ℹ️ 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".
|
Follow-up for the latest P1 review is pushed at Change:
Pinned PiD selected-member QA on exact head
Local validation on this head:
Malicious controls covered by deterministic regression include protocol-0 storage pickle, normal binary pickle, large framed binary pickle, the new large length-prefixed binary prefix, unreferenced storage-lookalike members, noncanonical storage aliases, malformed storage references, and opcode-budget incomplete trust parsing. @codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ 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". |
|
Final follow-up for the Windows CI failure is pushed at Change:
Pinned PiD selected-member QA on exact head
Additional local validation on this head:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c4853d197
ℹ️ 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".
|
Follow-up for the latest Codex review threads is pushed at exact head Addressed review threads:
Pinned PiD selected-member QA on exact head
Malicious/control coverage preserved or strengthened:
Local validation on this exact head:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25ade41af1
ℹ️ 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".
|
Validated-storage sidecar follow-up is pushed at exact head Addressed latest Codex review:
Pinned PiD selected-member QA on exact head
Local validation on this exact worktree:
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ 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". |
|
Windows alias CI follow-up is pushed at exact head Change:
Pinned PiD selected-member QA on exact head
Local validation on this exact worktree:
@codex review |
|
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 P1: loadable protocol-less framed pickle bypasses both scanners The trusted-storage probe returns false when a protocol-less pickle contains P2: the new trust pass ignores configured timeout and aggregate work budgets An archive with 64 prefixes, each containing P3: three changed persistent-ID tests fail in the standard macOS all-CI environment Installed Torch call-graph enrichment promotes the expected Validation: focused main storage suite |
…t26-pytorch-storage-member-discovery-20260610
Fail closed for referenced storage members that contain framed pickle structure beyond the trusted-storage probe, while preserving raw tensor false-positive suppression. Add root and standalone regressions for delayed dangerous opcodes and aggregate trust-parser budgets.
There was a problem hiding this comment.
💡 Codex Review
Downgrade trusted protocol-0 storage IDs
When data.pkl is saved with pickle_protocol=0, the new trust parser accepts the PERSID storage reference and trusts the referenced storage blob, but this predicate only downgrades BINPERSID reports. The subsequent pickle scan therefore still leaves a PERSISTENT_ID warning for a validated PyTorch storage lookup, so benign legacy PyTorch ZIP archives saved with protocol 0 continue to surface as suspicious even though their storage member was trusted.
ℹ️ 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".
…t26-pytorch-storage-member-discovery-20260610
Separate raw tensor storage trust from persistent-ID downgrade trust so mixed persistent-ID payloads remain suspicious. Cover root and standalone protocol-0 storage-only, arbitrary, noncanonical, and storage-first mixed cases.
…t26-pytorch-storage-member-discovery-20260610
Annotate canonical legacy PyTorch storage BINPERSID findings from the validated object-stream parser so trusted legacy containers downgrade correctly while bare .bin storage PIDs remain suspicious with structured metadata.
…t26-pytorch-storage-member-discovery-20260610 # Conflicts: # modelaudit/scanners/pytorch_zip_scanner.py # tests/scanners/test_pytorch_zip_scanner.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 617827abcd
ℹ️ 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".
| trusted_storage_blob_members = self._storage_blob_members_from_data_pkl_members( | ||
| trusted_pytorch_storage_data_pkl_members |
There was a problem hiding this comment.
With pickle scanning disabled, this broad trust set skips executable probing for data/<n> even when data.pkl also has an arbitrary PERSID. That can hide ELF bytes in data/0 and leave the PyTorch-only scan clean; gate this skip on the fully validated storage set. guidance
Useful? React with 👍 / 👎.
…t26-pytorch-storage-member-discovery-20260610
…t26-pytorch-storage-member-discovery-20260610 # Conflicts: # packages/modelaudit-picklescan/rust/src/stack.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0dd6a452d
ℹ️ 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".
…t26-pytorch-storage-member-discovery-20260610 # Conflicts: # modelaudit/scanners/pickle_scanner.py # tests/scanners/test_pytorch_zip_scanner.py
…t26-pytorch-storage-member-discovery-20260610
…t26-pytorch-storage-member-discovery-20260610
…t26-pytorch-storage-member-discovery-20260610
…t26-pytorch-storage-member-discovery-20260610
1 participant
Summary
Fixes a PyTorch ZIP false positive where raw tensor storage members under
data/<key>were discovered as hidden pickle streams even when the same-prefixdata.pklreferenced them through canonical PyTorch storage persistent IDs.The scanner now derives validated storage members from bounded
data.pklparsing before generic embedded-pickle discovery. Referenced tensor storage bytes are skipped as pickle streams; unreferenced extensionless members, mismatched/missing keys, noncanonical persistent IDs, truncateddata.pkl, and ambiguous layouts remain scanned or explicitly inconclusive.This preserves hidden-pickle detection for attacker-controlled sidecars while avoiding opaque extension/parse-incomplete findings on legitimate tensor bytes such as pinned Whisper
pytorch_model/data/379.Validation
uv run ruff format --check 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 mypy modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/uv run pytest -n auto -m "not slow and not integration" --maxfail=1Additional checks:
PROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest packages/modelaudit-picklescan/tests/test_api.py -k "storage_blob or storage_lookalike or hidden_pytorch_zip_pickle_member" --tb=short— 8 passedPROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest tests/scanners/test_pytorch_zip_scanner.py -k "pytorch_zip_discovery" --tb=short— 12 passedPROMPTFOO_DISABLE_TELEMETRY=1 HF_HOME=/home/dev-user/code/modelaudit-fp-fixes/20260611T005000Z-hf-fp-followup/task-26-pytorch-storage-member-discovery/artifacts/hf-cache uv run pytest tests/scanners/test_pytorch_zip_scanner.py::test_pytorch_zip_pinned_whisper_storage_member_qa --tb=short -s— 1 passedPROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest packages/modelaudit-picklescan/tests/test_api.py --tb=short— 639 passedPROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest tests/scanners/test_pytorch_zip_scanner.py -m "not slow and not integration" --tb=short— 839 passed, 1 deselectedcargo fmt --manifest-path Cargo.toml -- --checkcargo check --manifest-path Cargo.tomlcargo clippy --manifest-path Cargo.toml --all-targets -- -D warningscargo test --manifest-path Cargo.toml— 164 passeduv lock --checkfrompackages/modelaudit-picklescan/git diff --checkPinned real-model QA:
openai/whisper-large-v3at revision06f233fe06e710322aca913c1bc4249a0d71fce1using exact remote bytes forpytorch_model/data.pklandpytorch_model/data/379. Before the fix, standalone picklescan reportedpickle_files=('pytorch_model/data.pkl', 'pytorch_model/data/379')plusEXTENSION_REF/parse_incompleteevidence forpytorch_model/data/379.pickle_files=('pytorch_model/data.pkl',)and nopytorch_model/data/379findings or notices.pytorch_model/data.pkl,pytorch_model/version,pytorch_model/byteorder, andpytorch_model/data/379via HTTP ranges. It intentionally reports an inconclusive missing-storage outcome for the sliced artifact while provingdata/379is no longer scanned as a pickle stream.Post-rebase broad suite on
origin/main8d6c4864:PROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest -n auto -m "not slow and not integration" --maxfail=1— 17,376 passed, 1,292 skipped, 39 warningsChecklist
AGENTS.md.