Open
Conversation
Contributor
WalkthroughRemoved Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
82-147:⚠️ Potential issue | 🟡 MinorConsolidate duplicate
### Fixedsections.The
[Unreleased]section contains two separate### Fixedheadings (lines 82 and 122). According to Keep a Changelog format, there should be only one section per category. Merge all Fixed entries under a single### Fixedheading.♻️ Suggested consolidation
Remove the duplicate
### Fixedheading at line 122 and keep all Fixed entries under the first### Fixedsection at line 82.As per coding guidelines: "Add CHANGELOG entry under the existing
[Unreleased]section when adding user-visible changes (Keep a Changelog format)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 82 - 147, Under the [Unreleased] section there are two "### Fixed" headings; remove the duplicate "### Fixed" block (the second one) and merge its bullet entries into the first "### Fixed" list so all fixes appear under a single "### Fixed" heading; update only the duplicate heading and its bullets (leave the first "### Fixed" heading, the section title "[Unreleased]", and other sections intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/scanners/test_pickle_scanner.py`:
- Around line 698-707: The test test_safe_builtins_remain_allowlisted currently
only asserts no errors; strengthen it by also asserting that none of the
returned issues are from the specific check "Global Module Reference Check" (or
the canonical check name used by your scanner) so regressions in check routing
fail the test. After calling _scan_bytes(...) and asserting result.success,
iterate result.issues (or use a comprehension) and assert no issue.check equals
"Global Module Reference Check" (and/or assert that the set of issue.check
values does not contain that string), referencing the test name
test_safe_builtins_remain_allowlisted and the result/issue fields returned by
_scan_bytes/_craft_global_reduce_pickle.
---
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 82-147: Under the [Unreleased] section there are two "### Fixed"
headings; remove the duplicate "### Fixed" block (the second one) and merge its
bullet entries into the first "### Fixed" list so all fixes appear under a
single "### Fixed" heading; update only the duplicate heading and its bullets
(leave the first "### Fixed" heading, the section title "[Unreleased]", and
other sections intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 22e3687e-b7f5-40e5-9dd0-9dd59fea1fee
📒 Files selected for processing (3)
CHANGELOG.mdmodelaudit/scanners/pickle_scanner.pytests/scanners/test_pickle_scanner.py
mldangelo
force-pushed
the
feat/pickle-hasattr-allowlist
branch
from
92d97dc to
2fb3890
Compare
Member
Author
|
Rebased onto current Changes in this update:
Validation passed:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/scanners/test_pickle_scanner.py`:
- Around line 698-716: The test test_safe_builtins_remain_allowlisted currently
uses result.has_errors (which only reflects CRITICAL issues) and only checks the
"Global Module Reference Check"; change it to assert that there are no checks
with status == CheckStatus.FAILED that mention the specific
builtins.{safe_builtin} across all checks (e.g., filter result.checks for
entries whose message or name references f"builtins.{safe_builtin}" and assert
none have status FAILED), removing the reliance on result.has_errors; keep the
existing assertions that a passed Global Module Reference Check exists for
builtins.{safe_builtin} and that result.success is true.
- Around line 754-772: The test
test_builtins_hasattr_binput_binget_recall_is_critical currently leaves the
original GLOBAL on the pickle stack because the payload uses BINPUT without
popping, so a resolver that ignores BINGET can still pass; modify the payload
bytes used in this test so that after the BINPUT opcode the original callable is
removed (e.g., add a POP immediately after BINPUT) and optionally add a filler
test case (like test_reduce_pattern_detects_memoized_callable) to ensure the
detection only succeeds when memo lookup via BINGET actually occurs; update the
payload variable in the test to include the POP and any needed filler so the
test enforces true memo recall behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1843f3a7-a992-418d-a58f-217dbcfffef6
📒 Files selected for processing (3)
CHANGELOG.mdmodelaudit/scanners/pickle_scanner.pytests/scanners/test_pickle_scanner.py
Comment on lines
+754
to
+772
| def test_builtins_hasattr_binput_binget_recall_is_critical(self) -> None: | ||
| """Memoized callable recall via BINPUT/BINGET must keep builtins.hasattr dangerous.""" | ||
| payload = b"\x80\x02cbuiltins\nhasattr\nq\x01h\x01(tR." | ||
|
|
||
| result = self._scan_bytes(payload) | ||
|
|
||
| assert result.success | ||
| assert result.has_errors | ||
| failed_reduce_checks = [ | ||
| check | ||
| for check in result.checks | ||
| if check.name == "REDUCE Opcode Safety Check" | ||
| and check.status == CheckStatus.FAILED | ||
| and check.severity == IssueSeverity.CRITICAL | ||
| ] | ||
| assert any(check.details.get("associated_global") == "builtins.hasattr" for check in failed_reduce_checks), ( | ||
| f"Expected CRITICAL REDUCE finding for builtins.hasattr memo recall, " | ||
| f"got: {[check.details for check in failed_reduce_checks]}" | ||
| ) |
Contributor
There was a problem hiding this comment.
This payload does not actually force memo recall.
On Line 756, BINPUT memoizes without popping, so the original GLOBAL builtins.hasattr stays on the stack. A resolver that ignores BINGET and simply falls back to the earlier GLOBAL can still satisfy this test, which means the memo-recall bypass could regress unnoticed. POP the original callable after BINPUT (and ideally add filler, like test_reduce_pattern_detects_memoized_callable) so the test only passes when memo resolution really works.
Proposed fix
- payload = b"\x80\x02cbuiltins\nhasattr\nq\x01h\x01(tR."
+ # Memoize the callable, drop the original stack reference, then recall it.
+ payload = b"\x80\x02cbuiltins\nhasattr\nq\x010h\x01(tR."Based on learnings, “Preserve or strengthen security detections; test both benign and malicious samples when adding scanner/feature changes.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/scanners/test_pickle_scanner.py` around lines 754 - 772, The test
test_builtins_hasattr_binput_binget_recall_is_critical currently leaves the
original GLOBAL on the pickle stack because the payload uses BINPUT without
popping, so a resolver that ignores BINGET can still pass; modify the payload
bytes used in this test so that after the BINPUT opcode the original callable is
removed (e.g., add a POP immediately after BINPUT) and optionally add a filler
test case (like test_reduce_pattern_detects_memoized_callable) to ensure the
detection only succeeds when memo lookup via BINGET actually occurs; update the
payload variable in the test to include the POP and any needed filler so the
test enforces true memo recall behavior.
Member
Author
|
Added stronger differential validation to #692. What I verified against
What I added to lock that in:
Validation passed after the update:
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/scanners/test_pickle_scanner.py (2)
740-778:⚠️ Potential issue | 🟡 MinorTighten safe-builtin assertions beyond CRITICAL-only and single-check scope.
These tests still allow regressions where
builtins.set/slice/tuplefail in non-Global Module Reference Checkpaths. Assert there are noCheckStatus.FAILEDchecks mentioning the target builtin across all checks.Proposed test tightening
def test_safe_builtins_remain_allowlisted(self) -> None: """Safe reconstruction builtins must remain non-failing.""" for safe_builtin in ["set", "slice", "tuple"]: result = self._scan_bytes(self._craft_global_reduce_pickle("builtins", safe_builtin)) assert result.success - assert not result.has_errors, ( - f"Expected builtins.{safe_builtin} to remain non-failing, got: {[i.message for i in result.issues]}" - ) safe_global_checks = [check for check in result.checks if check.name == "Global Module Reference Check"] + failed_builtin_checks = [ + check + for check in result.checks + if check.status == CheckStatus.FAILED and f"builtins.{safe_builtin}" in check.message + ] assert any( check.status == CheckStatus.PASSED and f"builtins.{safe_builtin}" in check.message for check in safe_global_checks ), f"Expected passed Global Module Reference Check for builtins.{safe_builtin}" assert not any(check.status == CheckStatus.FAILED for check in safe_global_checks), ( f"Unexpected failed Global Module Reference Check for builtins.{safe_builtin}: " f"{[check.message for check in safe_global_checks]}" ) + assert not failed_builtin_checks, ( + f"Expected builtins.{safe_builtin} to stay non-failing across all checks, " + f"got: {[check.message for check in failed_builtin_checks]}" + ) def test_safe_import_only_builtins_remain_allowlisted(self) -> None: """Safe builtins must remain non-failing for import-only GLOBAL payloads.""" for safe_builtin in ["set", "slice", "tuple"]: result = self._scan_bytes(self._craft_global_only_pickle("builtins", safe_builtin)) assert result.success - assert not result.has_errors, ( - f"Expected import-only builtins.{safe_builtin} to remain non-failing, " - f"got: {[i.message for i in result.issues]}" - ) safe_global_checks = [check for check in result.checks if check.name == "Global Module Reference Check"] + failed_builtin_checks = [ + check + for check in result.checks + if check.status == CheckStatus.FAILED and f"builtins.{safe_builtin}" in check.message + ] assert any( check.status == CheckStatus.PASSED and f"builtins.{safe_builtin}" in check.message for check in safe_global_checks ), f"Expected passed import-only Global Module Reference Check for builtins.{safe_builtin}" assert not any(check.status == CheckStatus.FAILED for check in safe_global_checks), ( f"Unexpected failed import-only Global Module Reference Check for builtins.{safe_builtin}: " f"{[check.message for check in safe_global_checks]}" ) + assert not failed_builtin_checks, ( + f"Expected import-only builtins.{safe_builtin} to stay non-failing across all checks, " + f"got: {[check.message for check in failed_builtin_checks]}" + )Based on learnings, “Preserve or strengthen security detections; test both benign and malicious samples when adding scanner/feature changes.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/scanners/test_pickle_scanner.py` around lines 740 - 778, The tests test_safe_builtins_remain_allowlisted and test_safe_import_only_builtins_remain_allowlisted only verify the "Global Module Reference Check" and CRITICAL-only paths, allowing regressions where builtins like builtins.set/slice/tuple fail in other checks; update each test to also assert that across all result.checks there is no CheckStatus.FAILED entry whose message contains the target "builtins.<name>" (and keep the existing assertions that a passed global check mentions the builtin) so any failed check referencing the builtin will cause the test to fail.
816-819:⚠️ Potential issue | 🟠 MajorThis payload still does not force true memo recall via
BINGET.
BINPUTmemoizes but keeps the original callable on the stack, so scanners can pass without actually resolving memo recall. AddPOPimmediately afterBINPUTto makeBINGETresolution mandatory.Proposed payload fix
- payload = b"\x80\x02cbuiltins\nhasattr\nq\x01h\x01(tR." + payload = b"\x80\x02cbuiltins\nhasattr\nq\x010h\x01(tR."#!/bin/bash python - <<'PY' import pickletools payload_current = b"\x80\x02cbuiltins\nhasattr\nq\x01h\x01(tR." payload_fixed = b"\x80\x02cbuiltins\nhasattr\nq\x010h\x01(tR." print("Current payload disassembly:") pickletools.dis(payload_current) print("\nFixed payload disassembly:") pickletools.dis(payload_fixed) PYBased on learnings, “Preserve or strengthen security detections; test both benign and malicious samples when adding scanner/feature changes.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/scanners/test_pickle_scanner.py` around lines 816 - 819, The test payload in test_builtins_hasattr_binput_binget_recall_is_critical uses BINPUT but does not force memo recall via BINGET; insert a POP opcode immediately after the BINPUT in the payload (i.e., add the byte 0x30 right after the q\x01 sequence) so the scanner must resolve the memo via BINGET — update the payload variable in that test (payload = b"...") accordingly to include the POP byte.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/scanners/test_pickle_scanner.py`:
- Around line 740-778: The tests test_safe_builtins_remain_allowlisted and
test_safe_import_only_builtins_remain_allowlisted only verify the "Global Module
Reference Check" and CRITICAL-only paths, allowing regressions where builtins
like builtins.set/slice/tuple fail in other checks; update each test to also
assert that across all result.checks there is no CheckStatus.FAILED entry whose
message contains the target "builtins.<name>" (and keep the existing assertions
that a passed global check mentions the builtin) so any failed check referencing
the builtin will cause the test to fail.
- Around line 816-819: The test payload in
test_builtins_hasattr_binput_binget_recall_is_critical uses BINPUT but does not
force memo recall via BINGET; insert a POP opcode immediately after the BINPUT
in the payload (i.e., add the byte 0x30 right after the q\x01 sequence) so the
scanner must resolve the memo via BINGET — update the payload variable in that
test (payload = b"...") accordingly to include the POP byte.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4f770595-9372-4886-a81c-540534716c69
📒 Files selected for processing (1)
tests/scanners/test_pickle_scanner.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
builtins.hasattrand__builtin__.hasattrfrom the pickle safe-global allowlistset,slice, andtupleValidation
uv run ruff format modelaudit/ tests/uv run ruff check --fix modelaudit/ tests/uv run ruff check modelaudit/ tests/uv run ruff format --check modelaudit/ tests/uv run mypy modelaudit/uv run pytest tests/scanners/test_pickle_scanner.py -q -k "hasattr or builtins or pkgutil or uuid or multi_stream_benign_then_malicious"uv run pytest -n auto -m "not slow and not integration" --maxfail=1Notes
hasattris not promoted intoALWAYS_DANGEROUS_FUNCTIONS; removing it fromML_SAFE_GLOBALSis sufficient becausebuiltins/__builtin__already flow through the existing dangerous-module pathSummary by CodeRabbit
Bug Fixes
Tests
Documentation