Skip to content

fix: detect dynamic picklescan protocol hooks#1375

Open
mldangelo-oai wants to merge 1 commit into
mainfrom
mdangelo/codex/fix-picklescan-dynamic-dunder-rce
Open

fix: detect dynamic picklescan protocol hooks#1375
mldangelo-oai wants to merge 1 commit into
mainfrom
mdangelo/codex/fix-picklescan-dynamic-dunder-rce

Conversation

@mldangelo-oai
Copy link
Copy Markdown
Contributor

Summary

Fixes a Rust picklescanner bypass where a pickle could build a dunder protocol hook name dynamically, install it into a class namespace via builtins.type, and execute a callable protocol method during deserialization while the scanner returned clean.

The concrete repro built __del__ with str.join("__", "del", "__"), assigned pathlib.Path.touch to that dynamic namespace key, instantiated the generated Path subclass, and dropped it. CPython touched the marker file during unpickle; ModelAudit previously reported verdict=clean status=complete findings=0 notices=0.

Root Cause

Tracked dicts only preserved entries whose keys could be resolved as literal strings. If a namespace key was constructed dynamically, the scanner discarded the value entirely. builtins.type then fell through as a plain constructed object, and the suspicious string literal scanner never saw a complete __del__ token because the method name was split across fragments.

Fix

  • Preserve a bounded set of values assigned under non-literal dict keys.
  • Model simple literal str.join results so fragmented dunder names can become tracked string values.
  • Emit a warning-level DYNAMIC_TYPE_CALLABLE_ATTRIBUTE finding when builtins.type constructs a class namespace containing callable protocol hooks or callable values hidden behind dynamic keys.
  • Add an adversarial regression proving the dynamic __del__ payload now scans as suspicious and still touches the marker under CPython.
  • Add a picklescan changelog entry.

Validation

  • env UV_CACHE_DIR=/tmp/modelaudit-uv-cache uv lock --check
  • env UV_CACHE_DIR=/tmp/modelaudit-uv-cache uv run --with ruff ruff check src tests
  • env UV_CACHE_DIR=/tmp/modelaudit-uv-cache uv run --with ruff ruff format --check src tests
  • env UV_CACHE_DIR=/tmp/modelaudit-uv-cache uv run --with mypy mypy src tests
  • cargo fmt --manifest-path Cargo.toml -- --check
  • cargo check --manifest-path Cargo.toml
  • cargo clippy --manifest-path Cargo.toml --all-targets -- -D warnings
  • cargo test --manifest-path Cargo.toml
  • env UV_CACHE_DIR=/tmp/modelaudit-uv-cache PROMPTFOO_DISABLE_TELEMETRY=1 uv run --python 3.13 --with pytest --with pytest-xdist pytest -n auto tests --tb=short (1309 passed, 85 skipped)
  • env UV_CACHE_DIR=/tmp/modelaudit-uv-cache uv run --no-project --with ruff ruff format modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/
  • env UV_CACHE_DIR=/tmp/modelaudit-uv-cache uv run --no-project --with ruff ruff check modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/

The canonical root uv run --python 3.13 ruff format ... command could not sync the root environment on this macOS host because tensorrt-cu13-libs==10.16.1.11 is a Linux/Windows placeholder package that could not resolve a compatible wheel. I ran root ruff through an isolated tool environment after that sync failure.

@github-actions
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: 656.30ms -> 646.13ms (-1.5%).

Workload Benchmark Target Size Files Baseline Current Change Status
chunked-upload-stream tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_chunked_upload_stream chunked_stream 278.2 KiB 1 18.30ms 20.28ms +10.9% stable
suspicious-pickle-intake tests/benchmarks/test_scan_benchmarks.py::test_scan_suspicious_pickle_intake suspicious-intake 183.8 KiB 4 99.21ms 89.98ms -9.3% stable
clean-training-checkpoint tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_clean_training_checkpoint safe_large 278.2 KiB 1 15.52ms 16.88ms +8.8% stable
direct-malicious-upload tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_direct_malicious_upload malicious_reduce 52 B 1 1.59ms 1.56ms -1.9% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_base64] nested_base64 98 B 1 482.7us 473.4us -1.9% stable
duplicate-heavy-registry tests/benchmarks/test_scan_benchmarks.py::test_scan_duplicate_registry_snapshot registry-snapshot 915.2 KiB 13 191.97ms 188.81ms -1.6% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_hex] nested_hex 130 B 1 477.4us 470.4us -1.5% stable
single-checkpoint-preflight tests/benchmarks/test_scan_benchmarks.py::test_scan_single_checkpoint_before_load single_checkpoint.pkl 183.0 KiB 1 36.56ms 36.09ms -1.3% stable
warm-cache-rescan tests/benchmarks/test_scan_benchmarks.py::test_scan_warm_cached_repository_rescan release-candidate 547.3 KiB 32 37.38ms 37.06ms -0.9% stable
padded-multi-stream-upload tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_padded_multi_stream_upload multi_stream_padded 4.1 KiB 1 1.64ms 1.63ms -0.8% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_raw] nested_raw 78 B 1 469.6us 471.5us +0.4% stable
mixed-model-repository tests/benchmarks/test_scan_benchmarks.py::test_scan_release_candidate_repository release-candidate 547.3 KiB 32 252.70ms 252.42ms -0.1% stable

Copy link
Copy Markdown

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

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: 9c636f954c

ℹ️ 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 on lines +1381 to +1382
if unknown_key_values.len() < MAX_TRACKED_DICT_UNKNOWN_KEY_VALUES {
unknown_key_values.push(value);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Fail closed when dynamic namespace keys exceed the cap

When a type(name, bases, namespace) payload pads the namespace with more than 16 unresolved keys before the real dynamic hook, this helper silently stops recording later values. For example, 16 benign unknown-key entries followed by an unresolved runtime string key that evaluates to __del__ with pathlib.Path.touch as the value will cause dynamic_type_callable_attributes() to miss the callable hook even though unpickling still installs and can execute the finalizer. Please preserve an overflow sentinel or otherwise mark the scan suspicious/incomplete instead of dropping these values silently.

Useful? React with 👍 / 👎.

Comment on lines +1677 to +1679
&& matches!(
(reference.module.as_str(), reference.name.as_str()),
("builtins" | "__builtin__" | "__builtins__", "type")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Handle type.new class construction

A pickle can call builtins.type.__new__(type, name, bases, namespace) instead of builtins.type(name, bases, namespace) and CPython constructs the same class; I verified a payload whose namespace key is built with str.join into __del__ and whose value is pathlib.Path.touch touches a marker while scan_bytes still returns COMPLETE/CLEAN because this predicate rejects type.__new__ before the namespace is inspected. Please treat this equivalent constructor shape, with the namespace in the fourth argument, as dynamic type creation too.

Useful? React with 👍 / 👎.

Comment on lines +3934 to +3935
if index > 0 {
result.push_str(&separator);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Count separators before materializing joins

If a pickle models str.join with a large separator and up to the 16 tracked tuple items, this loop appends the separator without checking it against MAX_TRACKED_STR_JOIN_RESULT_BYTES; only each item length is considered. A multi-MB separator literal can therefore be copied into the tracked result many times during scanning, well beyond the intended 4 KB cap, so please include separator bytes in the projected length before push_str.

Useful? React with 👍 / 👎.

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