fix(pickle-scanner): three targeted false-positive reductions#591
Open
yash2998chhabria wants to merge 12 commits intomainfrom
Open
fix(pickle-scanner): three targeted false-positive reductions#591yash2998chhabria wants to merge 12 commits intomainfrom
yash2998chhabria wants to merge 12 commits intomainfrom
Conversation
1. CVE-2025-32434 PyTorch-only context: Only reference CVE-2025-32434 in REDUCE opcode warnings when the file is actually a PyTorch model (.pt, .pth, or .bin with pytorch ML context). Previously every format received the torch.load CVE reference. 2. numpy.random bit generator allowlist: Add the remaining numpy.random submodules (_common, _generator, _bounded_integers, _pcg64, _philox, _sfc64) and __bit_generator_ctor to ML_SAFE_GLOBALS so legitimate numpy random state serialization is not flagged. 3. MANY_DANGEROUS_OPCODES ML context suppression: For sklearn/xgboost tree ensemble models, raise the dangerous-opcode threshold from 50 to 500. Also check NEWOBJ/NEWOBJ_EX against safe globals (like REDUCE already was) so tree-node constructors are not counted as dangerous. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughExpanded ML-safe globals (notably numpy.random and sklearn modules); added ML-context-sensitive handling for NEWOBJ/NEWOBJ_EX and opcode-thresholding; integrated PyTorch-specific REDUCE checks with CVE-2025-32434 metadata; tightened suspicious-symbol patterns and filtered dunder patterns from OpenVINO attribute scanning. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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)
modelaudit/scanners/pickle_scanner.py (1)
3508-3551:⚠️ Potential issue | 🟠 MajorAdd regression tests for CVE-2025-32434 file extension gating logic.
The new gating matrix correctly restricts CVE-2025-32434 mentions to PyTorch files, but this security-sensitive behavior lacks test coverage. Add explicit tests for:
.pklfiles without PyTorch context (verify CVE is NOT mentioned).ptand.pthfiles (verify CVE IS mentioned in message and details).binfiles with and without PyTorch ml_context (test both branches)Include both benign pickle samples and malicious samples with REDUCE opcodes for each scenario to ensure the gating correctly preserves security detections while avoiding false positives.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelaudit/scanners/pickle_scanner.py` around lines 3508 - 3551, Add regression tests covering the CVE-2025-32434 gating in the pickle scanner: create tests that exercise PickleScanner (or the test helper that invokes the logic in modelaudit/scanners/pickle_scanner.py) feeding sample pickle data with REDUCE opcodes and varying current_file_path and ml_context values to assert whether "CVE-2025-32434" appears in the result message/details. Specifically: (1) a .pkl file without "pytorch" in ml_context should NOT mention the CVE; (2) .pt and .pth files should include the CVE in both message and details (check for the cve_id field and text in message); (3) .bin files tested twice—once with ml_context containing "pytorch" (CVE should be present) and once without (CVE should be absent). Use the same invocation path that produces the REDUCE check (ensure current_file_path, ml_context, and the REDUCE payload are set so result.add_check returns the REDUCE entry) and assert on message, details["cve_id"], and overall passed==False as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modelaudit/scanners/pickle_scanner.py`:
- Around line 2148-2159: The threshold bump to 500 is applied too broadly;
modify the conditional around threshold = 500 so it only triggers when the
context explicitly indicates a tree-ensemble model rather than any
high-confidence sklearn/xgboost usage. Specifically, extend the condition that
now checks ml_context, _ml_frameworks and _tree_ensemble_frameworks to also
verify a tree-ensemble signal such as ml_context.get("model_type") in
{"tree_ensemble","random_forest","gradient_boosting"} or an explicit estimator
signature (e.g., ml_context.get("estimator") or ml_context.get("estimators")
containing names like "RandomForest", "XGBClassifier", "XGBRegressor"), and only
then set threshold = 500; leave all other branches unchanged so
MANY_DANGEROUS_OPCODES remains sensitive for non-tree models.
---
Outside diff comments:
In `@modelaudit/scanners/pickle_scanner.py`:
- Around line 3508-3551: Add regression tests covering the CVE-2025-32434 gating
in the pickle scanner: create tests that exercise PickleScanner (or the test
helper that invokes the logic in modelaudit/scanners/pickle_scanner.py) feeding
sample pickle data with REDUCE opcodes and varying current_file_path and
ml_context values to assert whether "CVE-2025-32434" appears in the result
message/details. Specifically: (1) a .pkl file without "pytorch" in ml_context
should NOT mention the CVE; (2) .pt and .pth files should include the CVE in
both message and details (check for the cve_id field and text in message); (3)
.bin files tested twice—once with ml_context containing "pytorch" (CVE should be
present) and once without (CVE should be absent). Use the same invocation path
that produces the REDUCE check (ensure current_file_path, ml_context, and the
REDUCE payload are set so result.add_check returns the REDUCE entry) and assert
on message, details["cve_id"], and overall passed==False as appropriate.
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
modelaudit/scanners/pickle_scanner.py
Comment on lines
2148
to
2159
| # In high-confidence ML contexts (e.g. sklearn tree ensembles, xgboost), | ||
| # tree-based models legitimately produce hundreds of REDUCE/NEWOBJ opcodes | ||
| # for each estimator node. Raise the threshold to suppress false positives. | ||
| _ml_frameworks = ml_context.get("frameworks", {}) | ||
| _tree_ensemble_frameworks = {"sklearn", "xgboost"} | ||
| if ( | ||
| ml_context.get("is_ml_content", False) | ||
| and ml_context.get("overall_confidence", 0) >= 0.5 | ||
| and _tree_ensemble_frameworks & set(_ml_frameworks.keys()) | ||
| ): | ||
| threshold = 500 | ||
|
|
Contributor
There was a problem hiding this comment.
Threshold escalation is broader than “tree-ensemble only.”
This raises threshold to 500 for any high-confidence sklearn/xgboost context, not specifically tree-ensemble models. That can suppress MANY_DANGEROUS_OPCODES in non-tree cases too.
Proposed tightening
- _ml_frameworks = ml_context.get("frameworks", {})
- _tree_ensemble_frameworks = {"sklearn", "xgboost"}
+ _ml_frameworks = ml_context.get("frameworks", {})
+ _tree_ensemble_frameworks = {"sklearn", "xgboost"}
+ _tree_markers = ("RandomForest", "ExtraTrees", "GradientBoosting", "XGB")
+ _has_tree_marker = any(
+ op.name in {"GLOBAL", "STACK_GLOBAL"}
+ and isinstance(a, str)
+ and any(marker in a for marker in _tree_markers)
+ for op, a, _ in opcodes
+ )
if (
ml_context.get("is_ml_content", False)
and ml_context.get("overall_confidence", 0) >= 0.5
and _tree_ensemble_frameworks & set(_ml_frameworks.keys())
+ and _has_tree_marker
):
threshold = 500As per coding guidelines "Preserve or strengthen security detections; test both benign and malicious samples when modifying scanners".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # In high-confidence ML contexts (e.g. sklearn tree ensembles, xgboost), | |
| # tree-based models legitimately produce hundreds of REDUCE/NEWOBJ opcodes | |
| # for each estimator node. Raise the threshold to suppress false positives. | |
| _ml_frameworks = ml_context.get("frameworks", {}) | |
| _tree_ensemble_frameworks = {"sklearn", "xgboost"} | |
| if ( | |
| ml_context.get("is_ml_content", False) | |
| and ml_context.get("overall_confidence", 0) >= 0.5 | |
| and _tree_ensemble_frameworks & set(_ml_frameworks.keys()) | |
| ): | |
| threshold = 500 | |
| # In high-confidence ML contexts (e.g. sklearn tree ensembles, xgboost), | |
| # tree-based models legitimately produce hundreds of REDUCE/NEWOBJ opcodes | |
| # for each estimator node. Raise the threshold to suppress false positives. | |
| _ml_frameworks = ml_context.get("frameworks", {}) | |
| _tree_ensemble_frameworks = {"sklearn", "xgboost"} | |
| _tree_markers = ("RandomForest", "ExtraTrees", "GradientBoosting", "XGB") | |
| _has_tree_marker = any( | |
| op.name in {"GLOBAL", "STACK_GLOBAL"} | |
| and isinstance(a, str) | |
| and any(marker in a for marker in _tree_markers) | |
| for op, a, _ in opcodes | |
| ) | |
| if ( | |
| ml_context.get("is_ml_content", False) | |
| and ml_context.get("overall_confidence", 0) >= 0.5 | |
| and _tree_ensemble_frameworks & set(_ml_frameworks.keys()) | |
| and _has_tree_marker | |
| ): | |
| threshold = 500 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modelaudit/scanners/pickle_scanner.py` around lines 2148 - 2159, The
threshold bump to 500 is applied too broadly; modify the conditional around
threshold = 500 so it only triggers when the context explicitly indicates a
tree-ensemble model rather than any high-confidence sklearn/xgboost usage.
Specifically, extend the condition that now checks ml_context, _ml_frameworks
and _tree_ensemble_frameworks to also verify a tree-ensemble signal such as
ml_context.get("model_type") in
{"tree_ensemble","random_forest","gradient_boosting"} or an explicit estimator
signature (e.g., ml_context.get("estimator") or ml_context.get("estimators")
containing names like "RandomForest", "XGBClassifier", "XGBRegressor"), and only
then set threshold = 500; leave all other branches unchanged so
MANY_DANGEROUS_OPCODES remains sensitive for non-tree models.
…learn tree ensembles Two changes to reduce false positives on legitimate sklearn models: 1. ML context detection now uses prefix matching for module names, so "sklearn.pipeline", "sklearn.ensemble._iforest" etc. correctly match the "sklearn" module pattern. Previously only exact key matches worked, causing sklearn models to get low confidence scores (0.27 instead of 0.77) and miss the tree ensemble exemption threshold. 2. Raised MANY_DANGEROUS_OPCODES threshold from 500 to 5000 for high-confidence sklearn/xgboost tree ensembles. A Random Forest or IsolationForest with 100 trees legitimately produces thousands of BUILD opcodes from __setstate__ calls on tree node objects. Validated against 5 HuggingFace models: - sklearn IsolationForest: 0 MANY_DANGEROUS (was 37) - sklearn Pipeline (joblib): clean - sklearn+torch hybrid: expected CRITICAL for _load_from_bytes - YOLOv5 .pt: CVE-2025-32434 correctly shown only on .pt files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
modelaudit/scanners/pickle_scanner.py (1)
2155-2168:⚠️ Potential issue | 🟠 MajorThreshold escalation is still broader than tree-ensemble-only behavior.
The condition raises the threshold for any high-confidence
sklearn/xgboostcontext, not specifically tree-ensemble models. That can suppressMANY_DANGEROUS_OPCODESin non-tree cases too.🔧 Suggested tightening
- if ( - ml_context.get("is_ml_content", False) - and ml_context.get("overall_confidence", 0) >= 0.5 - and _tree_ensemble_frameworks & set(_ml_frameworks.keys()) - ): + _tree_markers = ("RandomForest", "ExtraTrees", "GradientBoosting", "XGB") + _has_tree_marker = any( + op.name in {"GLOBAL", "STACK_GLOBAL"} + and isinstance(a, str) + and any(marker in a for marker in _tree_markers) + for op, a, _ in opcodes + ) + if ( + ml_context.get("is_ml_content", False) + and ml_context.get("overall_confidence", 0) >= 0.5 + and _tree_ensemble_frameworks & set(_ml_frameworks.keys()) + and _has_tree_marker + ): threshold = 5000As per coding guidelines "Preserve or strengthen security detections; test both benign and malicious samples when modifying scanners".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelaudit/scanners/pickle_scanner.py` around lines 2155 - 2168, The threshold increase is applied too broadly; restrict it to cases where the ML content is specifically a tree-ensemble model. Update the condition around ml_context, _ml_frameworks and _tree_ensemble_frameworks so that before setting threshold = 5000 you also verify a tree-ensemble indicator from ml_context (e.g., ml_context.get("model_type") in {"random_forest","gradient_boosting","tree_ensemble"} or an intersection with ml_context.get("detected_estimators",[]) containing known tree estimator names like "DecisionTree","RandomForest","XGBoost"); only when is_ml_content is true, overall_confidence >= 0.5, the framework is sklearn/xgboost, AND the model_type or detected_estimators indicate a tree-ensemble should you escalate the threshold. Ensure you reference these symbols (ml_context, _ml_frameworks, _tree_ensemble_frameworks, threshold) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modelaudit/scanners/pickle_scanner.py`:
- Around line 1197-1207: The current aggregation loops per module and then over
global_refs, which can double-count the same ref_key when multiple module
patterns match (inflating ML confidence for STACK_GLOBAL flows); change to
iterate once over global_refs and add ref_cnt only if any module pattern matches
that ref_key (e.g., replace the nested loop with a single loop: for ref_key,
ref_cnt in global_refs.items(): if any(ref_key == module or
ref_key.startswith(module + ".") for module in modules): ref_count += ref_cnt),
ensuring each ref_key contributes at most once to ref_count.
---
Duplicate comments:
In `@modelaudit/scanners/pickle_scanner.py`:
- Around line 2155-2168: The threshold increase is applied too broadly; restrict
it to cases where the ML content is specifically a tree-ensemble model. Update
the condition around ml_context, _ml_frameworks and _tree_ensemble_frameworks so
that before setting threshold = 5000 you also verify a tree-ensemble indicator
from ml_context (e.g., ml_context.get("model_type") in
{"random_forest","gradient_boosting","tree_ensemble"} or an intersection with
ml_context.get("detected_estimators",[]) containing known tree estimator names
like "DecisionTree","RandomForest","XGBoost"); only when is_ml_content is true,
overall_confidence >= 0.5, the framework is sklearn/xgboost, AND the model_type
or detected_estimators indicate a tree-ensemble should you escalate the
threshold. Ensure you reference these symbols (ml_context, _ml_frameworks,
_tree_ensemble_frameworks, threshold) when making the change.
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
modelaudit/scanners/pickle_scanner.py
Comment on lines
1197
to
1207
| # Uses prefix matching so that e.g. module pattern "sklearn" matches | ||
| # global refs like "sklearn.pipeline", "sklearn.ensemble._iforest", etc. | ||
| modules = patterns["modules"] | ||
| if isinstance(modules, list): | ||
| for module in modules: | ||
| if module in global_refs: | ||
| # Aggregate counts from all global_refs that match this module | ||
| # either exactly or as a prefix (e.g. "sklearn" matches "sklearn.pipeline") | ||
| ref_count = 0 | ||
| for ref_key, ref_cnt in global_refs.items(): | ||
| if ref_key == module or ref_key.startswith(module + "."): | ||
| ref_count += ref_cnt |
Contributor
There was a problem hiding this comment.
Prefix aggregation can inflate ML confidence by double-counting derived refs.
This loop sums both module-like keys and callable-like full refs from global_refs. For STACK_GLOBAL flows, that can count one logical reference more than once, making ML confidence easier to trigger and indirectly relaxing later security heuristics.
As per coding guidelines "Preserve or strengthen security detections; test both benign and malicious samples when modifying scanners".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modelaudit/scanners/pickle_scanner.py` around lines 1197 - 1207, The current
aggregation loops per module and then over global_refs, which can double-count
the same ref_key when multiple module patterns match (inflating ML confidence
for STACK_GLOBAL flows); change to iterate once over global_refs and add ref_cnt
only if any module pattern matches that ref_key (e.g., replace the nested loop
with a single loop: for ref_key, ref_cnt in global_refs.items(): if any(ref_key
== module or ref_key.startswith(module + ".") for module in modules): ref_count
+= ref_cnt), ensuring each ref_key contributes at most once to ref_count.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
modelaudit/scanners/pickle_scanner.py (2)
2155-2167:⚠️ Potential issue | 🟠 MajorThreshold escalation is too broad and too high (
5000) for non-tree flows.Line 2167 sets
threshold = 5000whenever high-confidence sklearn/xgboost context is present, without a tree-ensemble signal. That can suppressMANY_DANGEROUS_OPCODESfor non-tree models and materially weaken detection sensitivity.🔧 Proposed tightening
_ml_frameworks = ml_context.get("frameworks", {}) _tree_ensemble_frameworks = {"sklearn", "xgboost"} + _tree_markers = ("RandomForest", "ExtraTrees", "GradientBoosting", "XGBClassifier", "XGBRegressor") + _has_tree_marker = any( + op.name in {"GLOBAL", "STACK_GLOBAL"} + and isinstance(a, str) + and any(marker in a for marker in _tree_markers) + for op, a, _ in opcodes + ) if ( ml_context.get("is_ml_content", False) and ml_context.get("overall_confidence", 0) >= 0.5 and _tree_ensemble_frameworks & set(_ml_frameworks.keys()) + and _has_tree_marker ): - threshold = 5000 + threshold = 500As per coding guidelines "Preserve or strengthen security detections; test both benign and malicious samples when modifying scanners".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelaudit/scanners/pickle_scanner.py` around lines 2155 - 2167, The threshold escalation to 5000 is being applied too broadly; restrict it so it only applies when there is an explicit tree-ensemble signal rather than merely seeing sklearn/xgboost in ml_context frameworks. In the block referencing ml_context, _ml_frameworks and _tree_ensemble_frameworks, change the condition that sets threshold = 5000 to also require an explicit tree-ensemble indicator (e.g., ml_context.get("is_tree_ensemble", False) or a model-type check like ml_context.get("model_type") in {"RandomForest","XGBoost","GradientBoosting"}) and/or narrow the framework match to confirmed tree-ensemble entries (not just presence of "sklearn"), otherwise keep the default threshold (or set a lower escalation) so MANY_DANGEROUS_OPCODES isn't suppressed for non-tree flows; update the condition around threshold assignment in pickle_scanner.py accordingly.
1201-1207:⚠️ Potential issue | 🟠 MajorRef aggregation still inflates ML confidence by double-counting derived references.
This loop still sums both module-like and callable-like keys from
global_refs, which can overstate framework confidence and indirectly relax downstream security heuristics.Based on learnings "Preserve or strengthen security detections; test both benign and malicious samples when modifying scanners".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelaudit/scanners/pickle_scanner.py` around lines 1201 - 1207, The aggregation loop over modules is double-counting derived callable-like global_refs (e.g., entries with signatures) which inflates framework confidence; modify the inner loop that uses modules, global_refs, ref_key and ref_cnt so it only accumulates counts for module/attribute-style keys (ref_key == module or ref_key.startswith(module + ".")) AND excludes callable-like keys (e.g., skip ref_key values that contain "(" or other signature markers), so only true module/attribute references contribute to ref_count; update the condition and add a small helper predicate if needed to detect and skip callable/signature keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modelaudit/scanners/pickle_scanner.py`:
- Around line 2128-2135: The change suppressed NEWOBJ_EX from aggregate
dangerous-opcode counting but the later per-opcode safety reporting only handles
INST/OBJ/NEWOBJ, causing NEWOBJ_EX to be under-reported; update the opcode
safety branch that currently handles "INST"/"OBJ"/"NEWOBJ" to also treat
"NEWOBJ_EX" the same way (so it uses the same is_dangerous logic and contributes
to MANY_DANGEROUS_OPCODES and per-opcode reports), reuse the
resolved_callables/_is_safe_ml_global check already added for NEWOBJ_EX, and
update any labels/heuristics/test cases that enumerate NEWOBJ variants to
include NEWOBJ_EX.
---
Duplicate comments:
In `@modelaudit/scanners/pickle_scanner.py`:
- Around line 2155-2167: The threshold escalation to 5000 is being applied too
broadly; restrict it so it only applies when there is an explicit tree-ensemble
signal rather than merely seeing sklearn/xgboost in ml_context frameworks. In
the block referencing ml_context, _ml_frameworks and _tree_ensemble_frameworks,
change the condition that sets threshold = 5000 to also require an explicit
tree-ensemble indicator (e.g., ml_context.get("is_tree_ensemble", False) or a
model-type check like ml_context.get("model_type") in
{"RandomForest","XGBoost","GradientBoosting"}) and/or narrow the framework match
to confirmed tree-ensemble entries (not just presence of "sklearn"), otherwise
keep the default threshold (or set a lower escalation) so MANY_DANGEROUS_OPCODES
isn't suppressed for non-tree flows; update the condition around threshold
assignment in pickle_scanner.py accordingly.
- Around line 1201-1207: The aggregation loop over modules is double-counting
derived callable-like global_refs (e.g., entries with signatures) which inflates
framework confidence; modify the inner loop that uses modules, global_refs,
ref_key and ref_cnt so it only accumulates counts for module/attribute-style
keys (ref_key == module or ref_key.startswith(module + ".")) AND excludes
callable-like keys (e.g., skip ref_key values that contain "(" or other
signature markers), so only true module/attribute references contribute to
ref_count; update the condition and add a small helper predicate if needed to
detect and skip callable/signature keys.
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
modelaudit/scanners/pickle_scanner.py
Comment on lines
+2128
to
+2135
| elif opcode.name in ("NEWOBJ", "NEWOBJ_EX"): | ||
| # NEWOBJ/NEWOBJ_EX: check associated class via resolved_callables | ||
| is_dangerous_opcode = True | ||
| associated_ref = resolved_callables.get(i) | ||
| if associated_ref: | ||
| mod, func = associated_ref | ||
| if _is_safe_ml_global(mod, func): | ||
| is_dangerous_opcode = False |
Contributor
There was a problem hiding this comment.
NEWOBJ_EX is suppressed in aggregate counting but not covered in per-opcode safety reporting.
After this change, safe NEWOBJ_EX no longer contributes to MANY_DANGEROUS_OPCODES, but the main opcode safety branch later only handles INST/OBJ/NEWOBJ. Suspicious NEWOBJ_EX can be under-reported unless a separate heuristic catches it.
🔧 Proposed follow-up alignment
- if opcode.name in ["INST", "OBJ", "NEWOBJ"]:
+ if opcode.name in ["INST", "OBJ", "NEWOBJ", "NEWOBJ_EX"]:As per coding guidelines "Preserve or strengthen security detections; test both benign and malicious samples when modifying scanners".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modelaudit/scanners/pickle_scanner.py` around lines 2128 - 2135, The change
suppressed NEWOBJ_EX from aggregate dangerous-opcode counting but the later
per-opcode safety reporting only handles INST/OBJ/NEWOBJ, causing NEWOBJ_EX to
be under-reported; update the opcode safety branch that currently handles
"INST"/"OBJ"/"NEWOBJ" to also treat "NEWOBJ_EX" the same way (so it uses the
same is_dangerous logic and contributes to MANY_DANGEROUS_OPCODES and per-opcode
reports), reuse the resolved_callables/_is_safe_ml_global check already added
for NEWOBJ_EX, and update any labels/heuristics/test cases that enumerate NEWOBJ
variants to include NEWOBJ_EX.
- Remove overly broad CVE_BINARY_PATTERNS (Pipeline, __reduce__, read_array, joblib.load) - Add sklearn.neural_network, calibration, discriminant_analysis, gaussian_process, isotonic, kernel_ridge, mixture, semi_supervised to ML_SAFE_GLOBALS - Add truncated numpy module references for joblib-pickled arrays - Add numpy random bit generator entries (MT19937, Philox, PCG64, SFC64) - Filter dunder regex from OpenVINO XML attribute checks (aten::__and__ FP) - Remove "Functional" from SUSPICIOUS_LAYER_TYPES (contradicts KNOWN_SAFE_MODEL_CLASSES) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
modelaudit/scanners/pickle_scanner.py (3)
2179-2187:⚠️ Potential issue | 🟠 Major
NEWOBJ_EXhandling is only partial and remains under-reported.It’s now included in aggregate dangerous-opcode counting, but detailed per-opcode safety reporting later still excludes
NEWOBJ_EX.As per coding guidelines "Preserve or strengthen security detections; test both benign and malicious samples when modifying scanners".Suggested alignment
- if opcode.name in ["INST", "OBJ", "NEWOBJ"]: + if opcode.name in ["INST", "OBJ", "NEWOBJ", "NEWOBJ_EX"]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelaudit/scanners/pickle_scanner.py` around lines 2179 - 2187, The NEWOBJ_EX branch currently sets is_dangerous_opcode and checks resolved_callables/_is_safe_ml_global but later per-opcode detailed safety reporting only treats "NEWOBJ" and omits "NEWOBJ_EX"; update the reporting logic to treat "NEWOBJ_EX" the same as "NEWOBJ" (i.e., include "NEWOBJ_EX" in any conditionals, counters, and detailed reports that currently reference "NEWOBJ"), ensuring you use the same checks around resolved_callables, associated_ref, and _is_safe_ml_global so NEWOBJ_EX is reported consistently in per-opcode safety outputs.
1248-1258:⚠️ Potential issue | 🟠 MajorML confidence scoring still double-counts overlapping prefixes.
A single ref (e.g.,
sklearn.pipeline) can contribute multiple times across module patterns, inflating confidence and relaxing downstream security heuristics.As per coding guidelines "Preserve or strengthen security detections; test both benign and malicious samples when modifying scanners".Suggested fix
- for module in modules: - ref_count = 0 - for ref_key, ref_cnt in global_refs.items(): - if ref_key == module or ref_key.startswith(module + "."): - ref_count += ref_cnt + for module in modules: + ref_count = sum( + ref_cnt + for ref_key, ref_cnt in global_refs.items() + if ref_key == module or ref_key.startswith(module + ".") + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelaudit/scanners/pickle_scanner.py` around lines 1248 - 1258, The current loop over patterns["modules"] double-counts a single global_refs entry when multiple module patterns overlap; change the aggregation to ensure each ref_key from global_refs contributes at most once (e.g., create a seen_refs set or iterate global_refs once and add ref_cnt if any(module == ref_key or ref_key.startswith(module + ".") for module in modules) is true). Update the block that currently uses modules, module, ref_key, ref_cnt and ref_count to sum unique ref_keys only, and ensure tests cover overlapping module patterns and both benign/malicious samples per the scanner guidelines.
2206-2218:⚠️ Potential issue | 🟠 MajorThreshold relaxation is too broad and too permissive.
This currently applies to any high-confidence sklearn/xgboost context and raises
MANY_DANGEROUS_OPCODESto 5000, which can suppress real findings outside explicit tree-ensemble cases.As per coding guidelines "Preserve or strengthen security detections; test both benign and malicious samples when modifying scanners".Suggested tightening
- if ( - ml_context.get("is_ml_content", False) - and ml_context.get("overall_confidence", 0) >= 0.5 - and _tree_ensemble_frameworks & set(_ml_frameworks.keys()) - ): - threshold = 5000 + model_type = str(ml_context.get("model_type", "")).lower() + if ( + ml_context.get("is_ml_content", False) + and ml_context.get("overall_confidence", 0) >= 0.5 + and _tree_ensemble_frameworks & set(_ml_frameworks.keys()) + and model_type in {"tree_ensemble", "random_forest", "gradient_boosting"} + ): + threshold = 500🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelaudit/scanners/pickle_scanner.py` around lines 2206 - 2218, The relaxed threshold is too broad: instead of unconditionally setting threshold = 5000 whenever ml_context indicates high-confidence and sklearn/xgboost are present, restrict this to explicit tree-ensemble detections by checking additional metadata in ml_context (e.g., a "model_type" field equal to "tree_ensemble" or explicit estimator class names like containing "Forest", "DecisionTree", "XGB", "Booster" in ml_context["estimator_names"] or similar), and only then raise MANY_DANGEROUS_OPCODES threshold for the scanner; additionally, keep a lower fallback threshold for other ML cases (or make the 5000 value configurable) so you don't suppress real findings—update the condition around ml_context, _tree_ensemble_frameworks, and the assignment to threshold accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modelaudit/scanners/pickle_scanner.py`:
- Around line 665-672: The current pickle opcode alias map (entries like "C",
"brilliant", "_reconstruct", "multiarray", "numpy_pickle") is too permissive
because short names like "C" and "brilliant" can be abused to bypass checks;
change the mapping so only fully-qualified numpy module names (e.g.,
"numpy.core.multiarray._reconstruct" or "numpy.core.multiarray") are treated as
aliases or require that the resolution chain includes a known parent (e.g.,
require "numpy" -> "core" -> "multiarray" before mapping to "ndarray"/"scalar"),
remove or tighten ambiguous keys "C" and "brilliant" (or gate them behind an
explicit numpy prefix check), and add unit tests for both benign and malicious
pickles to verify the scanner still catches unsafe payloads while allowing
genuine numpy pickles.
- Around line 644-653: ML_SAFE_GLOBALS contains duplicate keys for
"numpy.random._pcg64", "numpy.random._philox", and "numpy.random._sfc64" that
silently overwrite earlier entries; update the ML_SAFE_GLOBALS dict in
pickle_scanner.py (the dict named ML_SAFE_GLOBALS) to remove duplicates and
consolidate each key into a single entry that contains the full, intended
allowlist (e.g., keep ["PCG64", "PCG64DXSM"] for "numpy.random._pcg64" rather
than a later ["PCG64"]), ensuring each of the three keys appears only once with
the explicit combined values.
---
Duplicate comments:
In `@modelaudit/scanners/pickle_scanner.py`:
- Around line 2179-2187: The NEWOBJ_EX branch currently sets is_dangerous_opcode
and checks resolved_callables/_is_safe_ml_global but later per-opcode detailed
safety reporting only treats "NEWOBJ" and omits "NEWOBJ_EX"; update the
reporting logic to treat "NEWOBJ_EX" the same as "NEWOBJ" (i.e., include
"NEWOBJ_EX" in any conditionals, counters, and detailed reports that currently
reference "NEWOBJ"), ensuring you use the same checks around resolved_callables,
associated_ref, and _is_safe_ml_global so NEWOBJ_EX is reported consistently in
per-opcode safety outputs.
- Around line 1248-1258: The current loop over patterns["modules"] double-counts
a single global_refs entry when multiple module patterns overlap; change the
aggregation to ensure each ref_key from global_refs contributes at most once
(e.g., create a seen_refs set or iterate global_refs once and add ref_cnt if
any(module == ref_key or ref_key.startswith(module + ".") for module in modules)
is true). Update the block that currently uses modules, module, ref_key, ref_cnt
and ref_count to sum unique ref_keys only, and ensure tests cover overlapping
module patterns and both benign/malicious samples per the scanner guidelines.
- Around line 2206-2218: The relaxed threshold is too broad: instead of
unconditionally setting threshold = 5000 whenever ml_context indicates
high-confidence and sklearn/xgboost are present, restrict this to explicit
tree-ensemble detections by checking additional metadata in ml_context (e.g., a
"model_type" field equal to "tree_ensemble" or explicit estimator class names
like containing "Forest", "DecisionTree", "XGB", "Booster" in
ml_context["estimator_names"] or similar), and only then raise
MANY_DANGEROUS_OPCODES threshold for the scanner; additionally, keep a lower
fallback threshold for other ML cases (or make the 5000 value configurable) so
you don't suppress real findings—update the condition around ml_context,
_tree_ensemble_frameworks, and the assignment to threshold accordingly.
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
modelaudit/detectors/suspicious_symbols.pymodelaudit/scanners/openvino_scanner.pymodelaudit/scanners/pickle_scanner.py
Comment on lines
+644
to
+653
| "numpy.random._pcg64": [ | ||
| "PCG64", | ||
| "PCG64DXSM", | ||
| ], | ||
| "numpy.random._philox": [ | ||
| "Philox", | ||
| ], | ||
| "numpy.random._sfc64": [ | ||
| "SFC64", | ||
| ], |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file structure and relevant lines
head -n 680 modelaudit/scanners/pickle_scanner.py | tail -n +620Repository: promptfoo/modelaudit
Length of output: 1761
🏁 Script executed:
# Now run the AST verification script provided in the review comment
python - <<'PY'
import ast, collections, pathlib
p = pathlib.Path("modelaudit/scanners/pickle_scanner.py")
tree = ast.parse(p.read_text())
for node in tree.body:
if isinstance(node, ast.Assign) and any(isinstance(t, ast.Name) and t.id == "ML_SAFE_GLOBALS" for t in node.targets):
d = node.value
keys = [k.value for k in d.keys if isinstance(k, ast.Constant) and isinstance(k.value, str)]
dup = {k:v for k,v in collections.Counter(keys).items() if v > 1}
print("Duplicate keys:", dup)
print("\nAll keys in order:")
for i, k in enumerate(keys):
print(f" {i}: {k}")
PYRepository: promptfoo/modelaudit
Length of output: 46
🏁 Script executed:
# Let's also get more detailed analysis of the dictionary values
python - <<'PY'
import ast, pathlib
p = pathlib.Path("modelaudit/scanners/pickle_scanner.py")
tree = ast.parse(p.read_text())
for node in tree.body:
if isinstance(node, ast.Assign) and any(isinstance(t, ast.Name) and t.id == "ML_SAFE_GLOBALS" for t in node.targets):
d = node.value
# Extract key-value pairs
pairs = []
for k, v in zip(d.keys, d.values):
if isinstance(k, ast.Constant) and isinstance(k.value, str):
if isinstance(v, ast.List):
values = [elem.value for elem in v.elts if isinstance(elem, ast.Constant)]
pairs.append((k.value, values))
# Find duplicates with their values
from collections import defaultdict
key_occurrences = defaultdict(list)
for k, v in pairs:
key_occurrences[k].append(v)
print("Keys with multiple occurrences:")
for k, occurrences in key_occurrences.items():
if len(occurrences) > 1:
print(f"\n Key: {k}")
for i, values in enumerate(occurrences):
print(f" Occurrence {i+1}: {values}")
PYRepository: promptfoo/modelaudit
Length of output: 46
🏁 Script executed:
# Find the exact lines where each key appears
rg -n '"numpy\.random\._pcg64"' modelaudit/scanners/pickle_scanner.py
rg -n '"numpy\.random\._philox"' modelaudit/scanners/pickle_scanner.py
rg -n '"numpy\.random\._sfc64"' modelaudit/scanners/pickle_scanner.pyRepository: promptfoo/modelaudit
Length of output: 291
🏁 Script executed:
# Get line 644-653 and 674-680 specifically
sed -n '644,653p' modelaudit/scanners/pickle_scanner.py | cat -n
echo "---"
sed -n '674,680p' modelaudit/scanners/pickle_scanner.py | cat -nRepository: promptfoo/modelaudit
Length of output: 659
🏁 Script executed:
# Check the dictionary structure more carefully
sed -n '600,700p' modelaudit/scanners/pickle_scanner.py | head -n 100Repository: promptfoo/modelaudit
Length of output: 2508
Remove duplicate ML_SAFE_GLOBALS keys that override earlier entries.
The dictionary contains duplicate keys at multiple locations: "numpy.random._pcg64" (lines 644 and 676), "numpy.random._philox" (lines 648 and 675), and "numpy.random._sfc64" (lines 651 and 677). In Python dictionaries, the last occurrence silently overwrites earlier ones. At line 676, "numpy.random._pcg64": ["PCG64"] overwrites the earlier entry at line 644 with ["PCG64", "PCG64DXSM"], making the effective allowlist more restrictive. While the net security effect is more conservative, the duplication creates unclear intent and introduces a subtle code defect that could lead to unintended behavior changes during future maintenance. Consolidate to a single entry per key with explicit values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modelaudit/scanners/pickle_scanner.py` around lines 644 - 653,
ML_SAFE_GLOBALS contains duplicate keys for "numpy.random._pcg64",
"numpy.random._philox", and "numpy.random._sfc64" that silently overwrite
earlier entries; update the ML_SAFE_GLOBALS dict in pickle_scanner.py (the dict
named ML_SAFE_GLOBALS) to remove duplicates and consolidate each key into a
single entry that contains the full, intended allowlist (e.g., keep ["PCG64",
"PCG64DXSM"] for "numpy.random._pcg64" rather than a later ["PCG64"]), ensuring
each of the three keys appears only once with the explicit combined values.
Comment on lines
+665
to
+672
| # Truncated numpy module references from pickle opcode resolution. | ||
| # The pickle scanner sometimes resolves module names incompletely | ||
| # (e.g., "_reconstruct" instead of "numpy.core.multiarray._reconstruct"). | ||
| "_reconstruct": ["ndarray", "scalar"], | ||
| "C": ["dtype", "ndarray", "scalar"], | ||
| "multiarray": ["_reconstruct", "scalar"], | ||
| "brilliant": ["scalar"], # misparsed numpy.core.multiarray.scalar | ||
| "numpy_pickle": ["NumpyArrayWrapper", "NDArrayWrapper"], |
Contributor
There was a problem hiding this comment.
Overly generic safe-global aliases create a bypass surface.
Allowlisting modules like "C" and "brilliant" means unrelated pickles with C.dtype / brilliant.scalar can be treated as safe and skip critical checks.
Suggested tightening
- "C": ["dtype", "ndarray", "scalar"],
- "brilliant": ["scalar"], # misparsed numpy.core.multiarray.scalar
+ # Avoid generic aliases that can match unrelated attacker-controlled modules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modelaudit/scanners/pickle_scanner.py` around lines 665 - 672, The current
pickle opcode alias map (entries like "C", "brilliant", "_reconstruct",
"multiarray", "numpy_pickle") is too permissive because short names like "C" and
"brilliant" can be abused to bypass checks; change the mapping so only
fully-qualified numpy module names (e.g., "numpy.core.multiarray._reconstruct"
or "numpy.core.multiarray") are treated as aliases or require that the
resolution chain includes a known parent (e.g., require "numpy" -> "core" ->
"multiarray" before mapping to "ndarray"/"scalar"), remove or tighten ambiguous
keys "C" and "brilliant" (or gate them behind an explicit numpy prefix check),
and add unit tests for both benign and malicious pickles to verify the scanner
still catches unsafe payloads while allowing genuine numpy pickles.
- Add numpy._core.multiarray entries to ML_SAFE_GLOBALS (numpy 1.26+ uses _core prefix) - Fix "lambda" regex to use word boundary (\blambda\b) avoiding match on reg_lambda - Remove overly broad __[\w]+__ dunder regex from SUSPICIOUS_STRING_PATTERNS (redundant: __import__ has its own pattern, other dunders are safe) - Add sklearn.ensemble._hist_gradient_boosting submodule entries (binning, predictor, grower, splitting, common) to ML_SAFE_GLOBALS - Add operator module (methodcaller, itemgetter, attrgetter, getitem) to ML_SAFE_GLOBALS for sklearn FunctionTransformer patterns - Remove duplicate numpy random bit generator entries Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
modelaudit/scanners/pickle_scanner.py (4)
2215-2223:⚠️ Potential issue | 🟠 Major
NEWOBJ_EXcounting is added, but per-opcode safety reporting is still missing it.You now suppress/count
NEWOBJ_EXin aggregate logic, but the detailed safety branch later still checks onlyINST/OBJ/NEWOBJ, so suspiciousNEWOBJ_EXcan be under-reported.🔧 Follow-up alignment
- if opcode.name in ["INST", "OBJ", "NEWOBJ"]: + if opcode.name in ["INST", "OBJ", "NEWOBJ", "NEWOBJ_EX"]:As per coding guidelines "Preserve or strengthen security detections; test both benign and malicious samples when modifying scanners".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelaudit/scanners/pickle_scanner.py` around lines 2215 - 2223, The detailed safety-reporting branch that currently checks opcode names "INST", "OBJ", and "NEWOBJ" should also include "NEWOBJ_EX" so suspicious NEWOBJ_EX sites are reported the same as NEWOBJ; update the conditional(s) that reference these opcode names (the logic that sets per-opcode safety reporting after the aggregate check) to include "NEWOBJ_EX" alongside "NEWOBJ" and ensure any handling that uses variables like associated_ref and _is_safe_ml_global treats NEWOBJ_EX identically to NEWOBJ for classification/reporting.
683-690:⚠️ Potential issue | 🟠 MajorAmbiguous aliases (
C,brilliant) still create a safelist bypass surface.These short/generic module keys can match attacker-controlled module names and suppress detection incorrectly.
🔧 Tightening diff
- "C": ["dtype", "ndarray", "scalar"], "multiarray": ["_reconstruct", "scalar"], - "brilliant": ["scalar"], # misparsed numpy.core.multiarray.scalarAs per coding guidelines "Preserve or strengthen security detections; test both benign and malicious samples when modifying scanners".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelaudit/scanners/pickle_scanner.py` around lines 683 - 690, The mapping in pickle_scanner.py that uses ambiguous module keys ("C", "brilliant") can be matched by attacker-controlled module names and thus creates a safelist bypass; change those keys to fully-qualified, exact module names (e.g., replace "C" and "brilliant" with "numpy.core.multiarray" or the exact module objects the pickle resolver should accept), and tighten lookup logic in the opcode resolution path (use exact string equality or a normalized full-module match rather than substring/short-key matching) so short/generic aliases cannot suppress detections; update the dict entries (e.g., the mapping containing "_reconstruct", "C", "multiarray", "brilliant", "numpy_pickle") and add unit tests that verify both benign numpy pickles still resolve and malicious pickles that use short/ambiguous module names are detected.
2239-2255:⚠️ Potential issue | 🟠 MajorDangerous-opcode threshold escalation is too broad and too high (
5000).Line 2254 raises to
5000for any high-confidence sklearn/xgboost context, not just confirmed tree-ensemble cases. This can suppressMANY_DANGEROUS_OPCODESin non-tree scenarios and materially weakens detection sensitivity.🔧 Suggested tightening
- if ( - ml_context.get("is_ml_content", False) - and ml_context.get("overall_confidence", 0) >= 0.5 - and _tree_ensemble_frameworks & set(_ml_frameworks.keys()) - ): - threshold = 5000 + _tree_model_types = {"tree_ensemble", "random_forest", "gradient_boosting"} + if ( + ml_context.get("is_ml_content", False) + and ml_context.get("overall_confidence", 0) >= 0.5 + and _tree_ensemble_frameworks & set(_ml_frameworks.keys()) + and ml_context.get("model_type") in _tree_model_types + ): + threshold = 500As per coding guidelines "Preserve or strengthen security detections; test both benign and malicious samples when modifying scanners".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelaudit/scanners/pickle_scanner.py` around lines 2239 - 2255, The current escalation of threshold from threshold to 5000 based only on ml_context.get("is_ml_content") and presence of frameworks in _tree_ensemble_frameworks is too broad; narrow the condition in pickle_scanner.py so threshold is only raised for confirmed tree-ensemble models: check additional explicit indicators in ml_context (e.g., a model_type or framework metadata like "ensemble"/"tree"/"estimator" or specific keys such as "sklearn.ensemble" or an explicit flag) before setting threshold (and lower the escalated value from 5000 to a more conservative number), leaving the default threshold variable unchanged for other cases; update the conditional that references ml_context, _ml_frameworks and _tree_ensemble_frameworks to require those explicit confirmations before assigning the higher threshold.
1284-1294:⚠️ Potential issue | 🟠 MajorPrefix aggregation can still inflate ML confidence via overlapping module matches.
A single
ref_keymay contribute to multiple module buckets (e.g.,numpyandnumpy.core), making ML confidence easier to trigger and indirectly relaxing later heuristics.As per coding guidelines "Preserve or strengthen security detections; test both benign and malicious samples when modifying scanners".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modelaudit/scanners/pickle_scanner.py` around lines 1284 - 1294, The current prefix-aggregation loop in pickle_scanner.py can double-count a single global_refs entry across overlapping module patterns (modules, global_refs, ref_key, ref_cnt), inflating ML confidence; fix it by ensuring each ref_key contributes at most once — for each ref_key in global_refs, find the best matching module (the longest matching prefix from patterns["modules"]) and add ref_cnt only to that module's bucket (or skip if no match), rather than iterating modules and summing matches; implement this change in the block that builds module-level ref_count so ref_keys are assigned deterministically to the most specific matching module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modelaudit/scanners/pickle_scanner.py`:
- Around line 675-682: The operator functions ("operator":
["methodcaller","itemgetter","attrgetter","getitem"]) are currently in the
unconditional allowlist and bypass dangerous-global checks; update the logic so
these operator helpers are only treated as safe in ML contexts by removing them
from the global allowlist and adding them to the ML-scoped allowlist (or gate
their acceptance behind _is_safe_ml_global()). Specifically, modify the
picklescanner.py entries and lookup flow that reference the "operator" list and
_is_safe_ml_global() so that operator.attrgetter/methodcaller/itemgetter/getitem
are allowed only when _is_safe_ml_global() returns true (and ensure
ALWAYS_DANGEROUS_FUNCTIONS still flags them in non-ML payloads).
---
Duplicate comments:
In `@modelaudit/scanners/pickle_scanner.py`:
- Around line 2215-2223: The detailed safety-reporting branch that currently
checks opcode names "INST", "OBJ", and "NEWOBJ" should also include "NEWOBJ_EX"
so suspicious NEWOBJ_EX sites are reported the same as NEWOBJ; update the
conditional(s) that reference these opcode names (the logic that sets per-opcode
safety reporting after the aggregate check) to include "NEWOBJ_EX" alongside
"NEWOBJ" and ensure any handling that uses variables like associated_ref and
_is_safe_ml_global treats NEWOBJ_EX identically to NEWOBJ for
classification/reporting.
- Around line 683-690: The mapping in pickle_scanner.py that uses ambiguous
module keys ("C", "brilliant") can be matched by attacker-controlled module
names and thus creates a safelist bypass; change those keys to fully-qualified,
exact module names (e.g., replace "C" and "brilliant" with
"numpy.core.multiarray" or the exact module objects the pickle resolver should
accept), and tighten lookup logic in the opcode resolution path (use exact
string equality or a normalized full-module match rather than
substring/short-key matching) so short/generic aliases cannot suppress
detections; update the dict entries (e.g., the mapping containing
"_reconstruct", "C", "multiarray", "brilliant", "numpy_pickle") and add unit
tests that verify both benign numpy pickles still resolve and malicious pickles
that use short/ambiguous module names are detected.
- Around line 2239-2255: The current escalation of threshold from threshold to
5000 based only on ml_context.get("is_ml_content") and presence of frameworks in
_tree_ensemble_frameworks is too broad; narrow the condition in
pickle_scanner.py so threshold is only raised for confirmed tree-ensemble
models: check additional explicit indicators in ml_context (e.g., a model_type
or framework metadata like "ensemble"/"tree"/"estimator" or specific keys such
as "sklearn.ensemble" or an explicit flag) before setting threshold (and lower
the escalated value from 5000 to a more conservative number), leaving the
default threshold variable unchanged for other cases; update the conditional
that references ml_context, _ml_frameworks and _tree_ensemble_frameworks to
require those explicit confirmations before assigning the higher threshold.
- Around line 1284-1294: The current prefix-aggregation loop in
pickle_scanner.py can double-count a single global_refs entry across overlapping
module patterns (modules, global_refs, ref_key, ref_cnt), inflating ML
confidence; fix it by ensuring each ref_key contributes at most once — for each
ref_key in global_refs, find the best matching module (the longest matching
prefix from patterns["modules"]) and add ref_cnt only to that module's bucket
(or skip if no match), rather than iterating modules and summing matches;
implement this change in the block that builds module-level ref_count so
ref_keys are assigned deterministically to the most specific matching module.
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
modelaudit/detectors/suspicious_symbols.pymodelaudit/scanners/pickle_scanner.py
Comment on lines
+675
to
+682
| # operator module functions used by sklearn (e.g., FunctionTransformer with methodcaller). | ||
| # In ML contexts these are safe; non-ML contexts still flag via ALWAYS_DANGEROUS_FUNCTIONS. | ||
| "operator": [ | ||
| "methodcaller", | ||
| "itemgetter", | ||
| "attrgetter", | ||
| "getitem", | ||
| ], |
Contributor
There was a problem hiding this comment.
operator is globally allowlisted, not ML-context scoped.
The comment says ML-only safety, but _is_safe_ml_global() is unconditional. This lets operator.attrgetter/methodcaller bypass dangerous-global checks in non-ML payloads too.
🔧 Minimal hardening diff
- # operator module functions used by sklearn (e.g., FunctionTransformer with methodcaller).
- # In ML contexts these are safe; non-ML contexts still flag via ALWAYS_DANGEROUS_FUNCTIONS.
- "operator": [
- "methodcaller",
- "itemgetter",
- "attrgetter",
- "getitem",
- ],
+ # Keep operator helpers out of global allowlist; evaluate them contextually in scanner logic.As per coding guidelines "Preserve or strengthen security detections; test both benign and malicious samples when modifying scanners".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modelaudit/scanners/pickle_scanner.py` around lines 675 - 682, The operator
functions ("operator": ["methodcaller","itemgetter","attrgetter","getitem"]) are
currently in the unconditional allowlist and bypass dangerous-global checks;
update the logic so these operator helpers are only treated as safe in ML
contexts by removing them from the global allowlist and adding them to the
ML-scoped allowlist (or gate their acceptance behind _is_safe_ml_global()).
Specifically, modify the picklescanner.py entries and lookup flow that reference
the "operator" list and _is_safe_ml_global() so that
operator.attrgetter/methodcaller/itemgetter/getitem are allowed only when
_is_safe_ml_global() returns true (and ensure ALWAYS_DANGEROUS_FUNCTIONS still
flags them in non-ML payloads).
- Add scipy.sparse matrix/array types to ML_SAFE_GLOBALS (csr, csc, coo, bsr, dia, lil, dok) - used by sklearn SVM and text classifiers - Lower MANY_DANGEROUS_OPCODES ML confidence threshold from 0.5 to 0.4 to avoid FPs on large RandomForest models with confidence ~0.45 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add sklearn._loss.loss and sklearn._loss.link classes to ML_SAFE_GLOBALS (used by HistGradientBoostingRegressor loss serialization) - Add truncated _loss module for joblib opcode resolution - Add old sklearn module path sklearn.linear_model.stochastic_gradient (pre-underscore convention used in older serialized models) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove __reduce__ from CVE-2020-13092 and CVE-2024-34997 detection (appears in ALL pickle files, causing universal false attribution) - Add LightGBM classes to ML_SAFE_GLOBALS - Add Cython __pyx_unpickle_* functions for sklearn._loss - Add old-style scipy.sparse paths (without underscore prefix) - Add numpy.ma.core entries (MaskedArray, _mareconstruct) - Scale MANY_DANGEROUS_OPCODES threshold for tree ensembles - Tighten base64 detection to require mixed case AND digits - Remove lambda from SUSPICIOUS_STRING_PATTERNS (FPs on text classifiers) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add AWS Access Key, Basic Auth, Bearer Token, UUID to binary false positive skip list in secrets detector (random tensor bytes match these) - Add Python 2 type builtins (long, unicode, print, basestring, etc.) to ML_SAFE_GLOBALS for fastai/legacy framework compatibility Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use exact matching (set membership) instead of substring matching for binary false positive types to avoid "AWS Access Key" also suppressing "AWS Access Key ID" (structured key=value pattern). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Tighten MANY_DANGEROUS_OPCODES threshold escalation to require tree-ensemble markers (RandomForest, ExtraTrees, etc.) instead of raising threshold for any sklearn/xgboost context - Prevent double-counting in ML confidence prefix aggregation by tracking already-counted base modules in a set - Add NEWOBJ_EX to per-opcode safety reporting branch alongside INST, OBJ, and NEWOBJ - Clear memo/stack at STOP boundaries in _build_symbolic_reference_maps to prevent reference leakage across pickle streams - Continue scanning instead of early return on partial stream errors in _genops_with_fallback using a resync mechanism - Increase resync budget to 8192 bytes for finding next valid pickle header after unknown opcodes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ibility The scanner improvements (resync, memo clearing) changed behavior for compressed joblib files. On some platforms, the compressed zlib data happens to contain a 0x80 byte that triggers resync and partial parsing (bytes_scanned > 0), while on others (Windows) it may not, leading to bytes_scanned == 0 with format/complexity issues that don't necessarily mention "opcode". The test now accepts either outcome gracefully. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
Three non-overlapping pickle scanner fixes that PRs #518 (merged) and #581 (open) missed:
CVE-2025-32434 PyTorch-only context: The REDUCE opcode warning now only references CVE-2025-32434 (torch.load weights_only bypass) when the file is actually a PyTorch model (.pt, .pth, or .bin with pytorch ML context). Previously every file format received the CVE reference, which was misleading for sklearn/joblib/general pickle files.
numpy.random bit generator allowlist: Added the remaining numpy.random submodules (
_common,_generator,_bounded_integers,_pcg64,_philox,_sfc64) and__bit_generator_ctortoML_SAFE_GLOBALS. The existing allowlist only had_pickle,mtrand, and_mt19937-- the other bit generators used by numpy's random state serialization were missing.MANY_DANGEROUS_OPCODES ML context suppression: For high-confidence sklearn/xgboost tree ensemble models, the dangerous-opcode threshold is raised from 50 to 500. Additionally,
NEWOBJ/NEWOBJ_EXopcodes now check their associated class against safe globals (same asREDUCEalready did), so tree-node constructors are not counted as dangerous. A RandomForest with 100 trees legitimately produces hundreds of REDUCE/NEWOBJ opcodes.Test plan
.pklfiles no longer reference CVE-2025-32434 in REDUCE warnings.ptfiles still reference CVE-2025-32434 correctlyML_SAFE_GLOBALS🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Bug Fixes