Skip to content

fix: classify pytorch zip symlink targets - #1573

Merged
mldangelo-oai merged 3 commits into
mainfrom
mdangelo/codex/fix-pytorch-zip-link-target-classification
Jun 9, 2026
Merged

mldangelo-oai merged 3 commits into
mainfrom
mdangelo/codex/fix-pytorch-zip-link-target-classification

Conversation

@mldangelo-oai

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

Copy link
Copy Markdown
Contributor

Summary

  • classify PyTorch ZIP symlink targets as safe, external, critical-system, invalid, or unreadable
  • resolve relative targets against the link entry while enforcing the archive extraction root
  • preserve raw Unix path bytes with surrogateescape so non-UTF-8 targets still receive traversal classification
  • bound both decompressed target bytes and compressed input
  • normalize dot segments before critical-system attribution
  • integrate current main at 21956abfe97ec8312bb4f6312b5da99ec0fa640e

False-positive and false-negative fixes

  • accept in-root parent targets and the exact configured byte limit
  • reject POSIX, Windows, UNC, reserved DOS-device, empty, NUL-containing, oversized, and compression-abusive targets
  • preserve benign device-name near matches and non-UTF-8 relative targets
  • keep pickle/JIT/content analysis active when symlink mode bits are attached to a PyTorch member
  • trust Unix mode bits only for Unix/Darwin ZIP creator systems, avoiding DOS/FAT metadata false positives
  • verify local-header or data-descriptor payload bounds against central-directory metadata
  • prevent forged central CRC, compressed size, and uncompressed size from hiding an escaping suffix behind a safe prefix
  • redact attacker-controlled ZIP read errors and bound stored evidence

Focused validation

  • integrated associated-module baseline: 806 passed, 5 warnings
  • final symlink/security slice: 35 passed, 773 deselected
  • valid data-descriptor and forced-ZIP64 symlink probes passed
  • touched-file Ruff format/check clean
  • touched-file mypy clean
  • git diff --check clean
  • full repository suite intentionally left to CI per review workflow

Published commit: 2b2fb4f548d79d9a6f5380dd284f76dea0c79592
Published tree: 7df9a6a009130673453fe0dfb8e1fce99af3cc84

@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.419s -> 1.428s (+0.6%).

Workload Benchmark Target Size Files Baseline Current Change Status
warm-cache-rescan tests/benchmarks/test_scan_benchmarks.py::test_scan_warm_cached_repository_rescan release-candidate 547.3 KiB 32 100.12ms 93.13ms -7.0% stable
padded-multi-stream-upload tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_padded_multi_stream_upload multi_stream_padded 4.1 KiB 1 535.6us 560.6us +4.7% stable
mixed-model-repository tests/benchmarks/test_scan_benchmarks.py::test_scan_release_candidate_repository release-candidate 547.3 KiB 32 473.88ms 482.75ms +1.9% stable
suspicious-pickle-intake tests/benchmarks/test_scan_benchmarks.py::test_scan_suspicious_pickle_intake suspicious-intake 183.8 KiB 4 143.25ms 145.79ms +1.8% stable
single-checkpoint-preflight tests/benchmarks/test_scan_benchmarks.py::test_scan_single_checkpoint_before_load single_checkpoint.pkl 183.0 KiB 1 72.57ms 73.66ms +1.5% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_base64] nested_base64 98 B 1 474.7us 479.0us +0.9% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_raw] nested_raw 78 B 1 468.9us 473.0us +0.9% stable
duplicate-heavy-registry tests/benchmarks/test_scan_benchmarks.py::test_scan_duplicate_registry_snapshot registry-snapshot 915.2 KiB 13 402.08ms 405.49ms +0.8% stable
direct-malicious-upload tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_direct_malicious_upload malicious_reduce 52 B 1 435.4us 437.5us +0.5% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_hex] nested_hex 130 B 1 491.1us 489.7us -0.3% stable
chunked-upload-stream tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_chunked_upload_stream chunked_stream 278.2 KiB 1 113.45ms 113.75ms +0.3% stable
clean-training-checkpoint tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_clean_training_checkpoint safe_large 278.2 KiB 1 111.12ms 111.08ms -0.0% stable

Copy link
Copy Markdown
Contributor Author

Critical review complete and fixes published in 943995b8.

The final patch closes both directions of target-classification error: relative parent paths that remain within the archive are accepted, while traversal, UNC/Windows paths, invalid encodings, empty/NUL targets, and oversized targets fail closed. Absolute dot segments are normalized before critical-system attribution, and finding evidence is bounded/redacted.

Focused validation: 19 symlink/timeout tests passed; Ruff, formatting, mypy, and git diff --check are clean. No review threads were open. Full-suite validation is left to CI.

@mldangelo-oai
mldangelo-oai marked this pull request as ready for review June 8, 2026 21:07

@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: 943995b8b5

ℹ️ 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/pytorch_zip_scanner.py Outdated
Comment thread modelaudit/scanners/pytorch_zip_scanner.py Outdated

mldangelo-oai commented Jun 8, 2026 •

Copy link
Copy Markdown
Contributor Author

@codex Publication handoff for all four unresolved symlink-target threads: apply local fix commit d73ed981 to mdangelo/codex/fix-pytorch-zip-link-target-classification. The verified current-main merge head is 5291ed9f (merged origin/main at 532ae596).

The fix adds bounded-prefix reads with completeness tracking, surrogateescape classification for raw POSIX path bytes, safe non-UTF-8 near-match coverage, critical non-UTF-8 traversal coverage, and CRITICAL S406 bounded-evidence handling for oversized targets.

Current-main validation:

  • tests/scanners/test_pytorch_zip_scanner.py -k 'symlink or entry_validation_checks_timeout_between_members': 20 passed, 773 deselected
  • Ruff check/format and mypy on the two touched files: clean
  • git diff --check: clean

Direct push still fails with Invalid username or token; keep the threads unresolved until this commit is published.

@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: 943995b8b5

ℹ️ 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/pytorch_zip_scanner.py Outdated
Comment thread modelaudit/scanners/pytorch_zip_scanner.py Outdated

Copy link
Copy Markdown
Contributor Author

Local review repair is complete on current main in commit bcf6efcb (after merge commit 25b26bee), but the Git HTTPS credential on this runner rejected the push, so this commit is not yet on the published PR head.

The validated repair:

  • reads at most MAX_SYMLINK_TARGET_BYTES + 1 bytes and reports whether the target was complete;
  • decodes POSIX symlink payloads with surrogateescape, preserving traversal semantics for non-UTF-8 bytes;
  • keeps benign safe-\xff-target names clean while classifying ../../outside-\xff as CRITICAL S406 external traversal;
  • classifies oversized targets as CRITICAL S406 invalid targets with bounded/redacted evidence instead of INFO S902 inconclusive;
  • updates the timeout helper typing and focused regressions.

Validation: focused PyTorch ZIP symlink slice 20 passed, 773 deselected; Ruff check clean; Ruff format clean across 409 files; mypy clean across 464 files. No full pytest suite was run, per campaign instructions.

@codex address that feedback by applying the equivalent changes to mdangelo/codex/fix-pytorch-zip-link-target-classification. Keep the four review threads unresolved until the published head contains the fix.

@chatgpt-codex-connector

Copy link
Copy Markdown

Summary

Testing

  • ✅ PROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest -q tests/scanners/test_pytorch_zip_scanner.py -k 'symlink or entry_validation_checks_timeout_between_members' --maxfail=1
  • ✅ uv run ruff check --fix 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/
  • ✅ git diff --check

Full pytest was not run, matching the PR campaign guidance to use the focused PyTorch ZIP symlink slice.

View task →

@mldangelo-oai
mldangelo-oai force-pushed the mdangelo/codex/fix-pytorch-zip-link-target-classification branch from 943995b to e5f60cd Compare June 9, 2026 06:57
@mldangelo-oai
mldangelo-oai force-pushed the mdangelo/codex/fix-pytorch-zip-link-target-classification branch from e5f60cd to 032c95d Compare June 9, 2026 06:58

Copy link
Copy Markdown
Contributor Author

Published the reviewed follow-up as e5f60cde6605b60837d766451bad389d1b8108fc, based on current main.

The duplicate review findings are addressed:

  • non-UTF-8 path bytes use surrogateescape, with safe and traversal regressions
  • symlink payloads are read with a MAX + 1 bounded prefix and completeness tracking
  • oversized targets produce CRITICAL S406 invalid-target findings with bounded evidence instead of INFO/S902
  • the exact-size boundary remains accepted

Focused QA: 21 passed, 773 deselected; touched-file Ruff, mypy, and diff checks are clean.

Copy link
Copy Markdown
Contributor Author

Published the reviewed fix at 032c95df5a48ce15c6e2de04e6cd7376bb26d3a5 on current main.

Review findings addressed:

  • non-UTF-8 raw POSIX path bytes use surrogateescape, preserving traversal classification without falsely rejecting benign relative targets
  • symlink targets are read with a bounded MAX + 1 prefix and completeness tracking
  • oversized targets now produce CRITICAL S406 invalid-target findings with bounded/redacted evidence instead of S902
  • added the exact 64 KiB boundary regression to prevent an off-by-one false positive

Focused QA: 21 passed, 773 deselected; scoped Ruff format/check, mypy, and git diff --check are clean. The full suite is left to CI per the audit workflow.

Copy link
Copy Markdown
Contributor Author

Published the latest-main critical-review fixes in f5ceda9a84b8556bb0ab67074587e11be75a333d (exact tree 254ec90821bab56a919f67b9baaa451c3c9dd21c). The review closed four additional gaps: malicious data.pkl content can no longer hide behind symlink mode bits, symlink reads cap compressed input as well as output, malformed-header errors are redacted, and Windows DOS devices are rejected with benign near-match coverage. Critical-system paths now receive S408. Full associated module: 806 passed; focused adversarial slice: 33 passed; Ruff, format, mypy, and diff checks are clean.

@mldangelo-oai
mldangelo-oai marked this pull request as draft June 9, 2026 10:25

Copy link
Copy Markdown
Contributor Author

Published the final critical-review hardening in 2b2fb4f548d79d9a6f5380dd284f76dea0c79592 (exact tested tree 7df9a6a009130673453fe0dfb8e1fce99af3cc84).

Two additional gaps are closed:

  • forged central CRC, compressed size, and uncompressed size can no longer truncate an escaping symlink target to a passing safe prefix; local-header/data-descriptor payload bounds must agree
  • DOS/FAT creator entries no longer become false symlinks from Unix-looking high attribute bits

Final focused QA: 35 passed, 773 deselected; valid data-descriptor and forced-ZIP64 probes passed; touched-file Ruff, mypy, and diff checks are clean. All review threads are resolved.

@mldangelo-oai
mldangelo-oai marked this pull request as ready for review June 9, 2026 15:51
@mldangelo-oai
mldangelo-oai merged commit c7e1699 into main Jun 9, 2026
23 of 27 checks passed
@mldangelo-oai
mldangelo-oai deleted the mdangelo/codex/fix-pytorch-zip-link-target-classification branch June 9, 2026 15:51
@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