Skip to content

fix: bound executorch zip scanning - #1546

Merged
mldangelo-oai merged 3 commits into
mainfrom
mdangelo/codex/fix-executorch-zip-budgets
Jun 9, 2026
Merged

mldangelo-oai merged 3 commits into
mainfrom
mdangelo/codex/fix-executorch-zip-budgets

Conversation

@mldangelo-oai

@mldangelo-oai mldangelo-oai commented Jun 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Bound ExecuTorch ZIP central-directory processing before ZipFile allocation.
  • Parse a bounded private snapshot and reject archives that mutate during the scan.
  • Fail closed on entry-count, central-directory-size, aggregate-size, pickle-member-size, compression-ratio, and ambiguous legacy/ZIP64 overlay limits.
  • Preserve eligible malicious pickle detections across aggregate limits, unreadable members, oversized siblings, and suspiciously compressed siblings.
  • Keep exact-limit benign archives clean and make the two touched Hugging Face deadline regressions state-driven.

False-positive / false-negative audit

  • Reject forged-low or zero counts, truncated directories, malformed ZIP64 metadata, repeated digital-signature records, and hidden prior archives outside the EOCD tail.
  • Structurally validate legacy and ZIP64 preambles while preserving incidental EOCD/locator near-matches.
  • Re-read descriptor size, snapshot the selected inode, and report in-place mutation instead of accepting a stale success.
  • Continue past unsupported or unreadable pickle members so later malicious payloads remain visible.
  • Preserve malicious findings in either archive order while enforcing the cumulative pickle scan budget.
  • Preserve exact entry-count, central-directory, aggregate, member-size, and compression-ratio boundaries.

Validation

  • Reviewed head: 462bd2e52a2a0a3dbaf376f6f7aa55264755fa7f
  • Exact published tree: 361fc08b923e417a5d699a94099d4afe5e218934
  • Complete ExecuTorch scanner module plus both touched Hugging Face deadline tests: 81 passed
  • Scoped Ruff check/format, mypy, and git diff --check: clean
  • All inline review threads are addressed and resolved.
  • Full-suite validation is intentionally deferred to CI.

@github-actions

github-actions Bot commented Jun 8, 2026 •

Copy link
Copy Markdown
Contributor

Workflow run and artifacts

Performance Benchmarks

Compared 12 shared benchmarks with a regression threshold of 15%.
Status: 0 regressions, 0 improved, 12 stable, 0 new, 0 missing.
Aggregate shared-benchmark median: 1.352s -> 1.339s (-1.0%).

Workload Benchmark Target Size Files Baseline Current Change Status
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_base64] nested_base64 98 B 1 514.9us 527.2us +2.4% stable
mixed-model-repository tests/benchmarks/test_scan_benchmarks.py::test_scan_release_candidate_repository release-candidate 547.3 KiB 32 476.08ms 468.62ms -1.6% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_hex] nested_hex 130 B 1 526.8us 518.8us -1.5% stable
warm-cache-rescan tests/benchmarks/test_scan_benchmarks.py::test_scan_warm_cached_repository_rescan release-candidate 547.3 KiB 32 98.27ms 96.86ms -1.4% stable
suspicious-pickle-intake tests/benchmarks/test_scan_benchmarks.py::test_scan_suspicious_pickle_intake suspicious-intake 183.8 KiB 4 120.17ms 118.54ms -1.4% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_raw] nested_raw 78 B 1 512.3us 506.1us -1.2% stable
duplicate-heavy-registry tests/benchmarks/test_scan_benchmarks.py::test_scan_duplicate_registry_snapshot registry-snapshot 915.2 KiB 13 372.77ms 368.77ms -1.1% stable
clean-training-checkpoint tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_clean_training_checkpoint safe_large 278.2 KiB 1 106.15ms 106.93ms +0.7% stable
padded-multi-stream-upload tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_padded_multi_stream_upload multi_stream_padded 4.1 KiB 1 576.7us 578.9us +0.4% stable
direct-malicious-upload tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_direct_malicious_upload malicious_reduce 52 B 1 494.1us 492.3us -0.4% stable
single-checkpoint-preflight tests/benchmarks/test_scan_benchmarks.py::test_scan_single_checkpoint_before_load single_checkpoint.pkl 183.0 KiB 1 66.81ms 66.92ms +0.2% stable
chunked-upload-stream tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_chunked_upload_stream chunked_stream 278.2 KiB 1 109.31ms 109.36ms +0.1% stable

@mldangelo-oai
mldangelo-oai force-pushed the mdangelo/codex/fix-executorch-zip-budgets branch from ec7ab1e to e612738 Compare June 8, 2026 19:14
@mldangelo-oai
mldangelo-oai marked this pull request as ready for review June 8, 2026 19:15

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cd7b51ebc6

ℹ️ 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".

Comment thread tests/utils/sources/test_huggingface.py

Copy link
Copy Markdown
Contributor Author

@codex address that feedback

Please update test_huggingface_prefix_rechecks_deadline_between_chunks so the mocked clock advances across the request setup and two chunks, using side_effect=[100.0, 100.0, 102.0] on modelaudit.utils.sources.huggingface.time.monotonic. This exact focused test passes locally with that change. Preserve the later streaming-deadline stabilization already on the branch.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 16b481f1ba

ℹ️ 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".

Comment thread modelaudit/scanners/executorch_scanner.py Outdated

Copy link
Copy Markdown
Contributor Author

@codex address that feedback

For the new central-directory P2, pass max_executorch_zip_central_directory_size into _read_zip_entry_count and return (entry_count, central_directory_size, True) before _count_central_directory_entries whenever either the entry count or declared directory size already exceeds its configured budget. Strengthen test_executorch_zip_central_directory_size_fails_before_archive_open by monkeypatching _count_central_directory_entries to fail if called, and update the preflight-unavailable test double for the added argument. Focused local validation: 42 scanner tests plus Ruff, format, mypy, and git diff --check all pass.

@chatgpt-codex-connector

Copy link
Copy Markdown

Summary

  • Updated test_huggingface_prefix_rechecks_deadline_between_chunks so the patched Hugging Face monotonic clock advances across request setup and two streamed chunks with side_effect=[100.0, 100.0, 102.0]. tests/utils/sources/test_huggingface.pyL1409-L1424
  • Preserved the existing streaming-deadline stabilization test already present on the branch. tests/utils/sources/test_huggingface.pyL2083-L2115
  • Committed the change as 573221e (test: advance Hugging Face prefix deadline clock) and created a PR with title test: advance Hugging Face prefix deadline clock.

Testing

  • ✅ PROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest tests/utils/sources/test_huggingface.py::TestModelDownload::test_huggingface_prefix_rechecks_deadline_between_chunks -q
  • ✅ PROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest tests/utils/sources/test_huggingface.py::TestModelDownloadStreaming::test_download_model_streaming_deadline_stops_content_probes -q
  • ✅ uv 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 mypy modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/
  • ✅ git diff --check
  • ⚠️ PROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest -n auto -m "not slow and not integration" --maxfail=1 — started and progressed to 73%, but showed one failure marker and then remained unresponsive for several minutes before pytest emitted failure details, so I terminated it.

View task →

@chatgpt-codex-connector

Copy link
Copy Markdown

Summary

Testing

  • ✅ PROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest tests/scanners/test_executorch_scanner.py -q --maxfail=1
  • ✅ uv run ruff check modelaudit/scanners/executorch_scanner.py tests/scanners/test_executorch_scanner.py
  • ✅ uv run ruff format --check modelaudit/scanners/executorch_scanner.py tests/scanners/test_executorch_scanner.py
  • ✅ uv run mypy modelaudit/scanners/executorch_scanner.py tests/scanners/test_executorch_scanner.py
  • ✅ git diff --check

View task →

Copy link
Copy Markdown
Contributor Author

Published the complete review follow-up at f560c029.

The P1 mocked-clock regression now advances to the deadline breach, and the P2 central-directory budget is checked before raw directory parsing. The adversarial pass also closed trailing-EOCD overlays, repeated digital-signature loops, split small-member compression bombs, zero-like config bypasses, preflight TOCTOU, and silent preflight read failures while adding benign exact-boundary coverage.

Focused QA: 56 ExecuTorch scanner tests and 2 Hugging Face deadline tests passed; scoped Ruff, format, mypy, and diff checks are clean. Remote tree matches 9c2a85402e28af0cee32ec64072b4d9e9d34bbf5.

Copy link
Copy Markdown
Contributor Author

Published the final critical-review fixes at ee3462ec on current main (f3ad8528).

The review uncovered and closed four additional gaps: stale path size before descriptor open, contradictory ZIP64 metadata that could hide entries, concatenated archives outside the bounded EOCD tail, and a call-count-dependent Hugging Face deadline test. The preamble check was then narrowed to distinguish real embedded ZIPs from incidental PK marker bytes.

Focused QA: 64 ExecuTorch tests and 2 Hugging Face deadline tests passed; scoped Ruff, format, mypy, and diff checks are clean. Exact published tree: 6c4a7f66056262e4919de179d09c69716d0be2ed. No full local suite was run.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ee3462ecc5

ℹ️ 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".

Comment thread modelaudit/scanners/executorch_scanner.py Outdated
@mldangelo-oai
mldangelo-oai force-pushed the mdangelo/codex/fix-executorch-zip-budgets branch from ee3462e to 0081e23 Compare June 9, 2026 06:02

Copy link
Copy Markdown
Contributor Author

Addressed the remaining false-negative review at 0081e23aa6cf9db67c526c03bb6d4e80dcea24d8, synchronized with current main after #1545.

Pickle ZIP budgets now skip only oversized/high-ratio offenders, mark the scan explicitly inconclusive, and continue scanning eligible members so an earlier malicious pickle finding is preserved. Blocked-member warnings are summarized by category to avoid output amplification at the archive entry cap. The #1545 raw-payload/launcher-prefix behavior was preserved through the merge conflict resolution.

Focused validation: ExecuTorch scanner module 70 passed; two associated Hugging Face deadline regressions 2 passed; scoped Ruff, format, mypy, and diff checks clean.

Copy link
Copy Markdown
Contributor Author

Verified the P2 follow-up on the true live head 0081e23aa6cf9db67c526c03bb6d4e80dcea24d8. Over-budget members are filtered before scanning, while safe siblings remain in archive order and still produce findings; regressions cover both oversized and high-ratio companions. Associated validation is 72 passed for the ExecuTorch scanner module plus the two Hugging Face deadline tests, with scoped Ruff/format/mypy/diff checks clean. Exact tree: 15469d948887b9eeacb2309102e96b8fb7476212. All review threads are resolved; fresh CI is still running, so this PR remains in the return queue.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0081e23aa6

ℹ️ 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".

Comment thread modelaudit/scanners/executorch_scanner.py Outdated
Preserve eligible pickle detections across aggregate and member failures, reject hidden legacy and ZIP64 archives, parse a private stable snapshot, and make the touched deadline tests state-driven.

Copy link
Copy Markdown
Contributor Author

Critical review complete on 462bd2e5.

Beyond the aggregate-order P2, the final pass fixed four reproduced false-negative classes: prior legacy ZIPs outside the EOCD tail, prior ZIP64 archives with forged counts, in-place source mutation after preflight, and unreadable early pickle members suppressing later malicious payloads. Benign EOCD/locator near-matches and exact budget boundaries remain covered.

Remote tree 361fc08b923e417a5d699a94099d4afe5e218934 exactly matches the tested local tree. Associated QA: 81 passed; scoped Ruff format/check, mypy, and diff checks are clean. Full-suite coverage remains with CI.

@mldangelo-oai
mldangelo-oai merged commit ff9e1ae into main Jun 9, 2026
25 of 27 checks passed
@mldangelo-oai
mldangelo-oai deleted the mdangelo/codex/fix-executorch-zip-budgets branch June 9, 2026 06:41
@github-actions github-actions Bot mentioned this pull request Jun 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant