fix(picklescan): close encoded protocol0 probe gaps#1594
Conversation
Detect base64 protocol 0 payloads that begin with long scalar operands, and continue bounded probing after lenient prefixes unless the entire literal decodes to exactly one pickle.
Performance BenchmarksCompared
|
Sync current main and preserve bounded long-scalar coverage across wrapper alignments and lenient base64 separators.
|
Critical review follow-up published at I reproduced and fixed three clean-verdict false-negative classes in the original branch:
The revised scanner performs bounded four-alignment discovery, requires an actual parseable pickle before promoting a candidate, and uses the existing incomplete/fail-closed signal when a confirmed candidate is beyond bounded coverage. Benign long-scalar near-matches remain clean. Scoped QA is clean: 150 Rust tests, 48 focused Python tests, a 36-case malicious/benign adversarial matrix, Cargo fmt/clippy, Ruff, mypy, and a post-main-sync rerun. The remote tree was fetched back and matches the locally verified tree exactly. No inline review threads are open at publication time. Fresh CI is now running; moving on to the next randomly selected PR rather than waiting here. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51772bc840
ℹ️ 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".
Pull request was converted to draft
mldangelo-oai
left a comment
There was a problem hiding this comment.
Critical review found and fixed a false negative in the new long protocol-0 base64 probe: ignored separators could inflate the raw literal past the 1 MiB probe input budget while leaving the compact encoded payload small, causing a 1.5 MiB malicious os.system pickle to return complete/clean. The probe now budgets retained compact base64 characters after the bounded start region, and complete long protocol-0 lenient literals use the same bounded single-pickle coverage proof to avoid a false truncation notice.
Validation on the exact pushed tree:
- 51/51 associated Python tests passed
- 37/37 nested Rust unit tests passed
- focused encoded-probe fail-closed state test passed
- 32-case adversarial separator matrix passed across
I,S, andV, including benign unterminated near-matches up to the 8 MiB literal limit - Ruff format/check, full mypy (466 files), Cargo fmt/check/Clippy, and package lock check passed
- 8 MiB benign timing remained approximately 0.04s dense / 0.17s separated locally
All existing review threads remain resolved.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f365c0a75d
ℹ️ 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".
Pull request was converted to draft
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 354ab74599
ℹ️ 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".
| let Some(value) = base64_value(*byte) else { | ||
| continue; |
There was a problem hiding this comment.
Stop base64 scanning at padding
Skipping = lets padded unterminated protocol-0 base64 scalars consume later base64; if that yields \n, benign input becomes limit-exceeded/malicious though normal decoding stops at padding. Stop at padding per guidance.
Useful? React with 👍 / 👎.
Summary
F,I,L,P,S,V,g, andpg/pcandidates without charging synthetic framing tomax_nested_pickle_bytes[Unreleased]changelog entryFollow-up to merged #1583 and its post-merge review findings:
Critical review fixes
I/S/Vto every protocol 0 line opcode that can hide later dangerous opcodesPROTOaccounting so exact nested-byte limits remain complete and reported sizes never include the two internal framing bytesAll three existing review threads are resolved.
Validation
Scoped to the changed picklescan surfaces, per review policy:
cargo test --manifest-path packages/modelaudit-picklescan/Cargo.toml(154 passed)test_protocol0_line_operands.pyplustest_nested_budget_limits.py(425 passed)cargo check, strict Clippy, and Cargo fmt (clean)git diff --check(clean)Published revision
354ab74599c5b74acf787e9b91d454aeccab277f69ade858021436fee8a1381f67555ed4326a8e9bmainat493436e795be760b8dcec031d4d60e9a044091f1