-
Notifications
You must be signed in to change notification settings - Fork 6
fix: detect risky import-only pickle ML surfaces #696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -655,6 +655,31 @@ def _compute_pickle_length(path: str) -> int: | |
| "glob", | ||
| } | ||
|
|
||
| # Risky ML-specific import surfaces that must be flagged even when they appear | ||
| # as import-only GLOBAL/STACK_GLOBAL references (without immediate REDUCE). | ||
| RISKY_ML_MODULE_PREFIXES: tuple[str, ...] = ( | ||
| "torch.jit", | ||
| "torch._dynamo", | ||
| "torch._inductor", | ||
| "numpy.f2py", | ||
| "numpy.distutils", | ||
| ) | ||
|
|
||
| RISKY_ML_EXACT_REFS: set[tuple[str, str]] = { | ||
| ("torch", "compile"), | ||
| ("torch.storage", "_load_from_bytes"), | ||
| } | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
| def _split_parent_child_ref(prefix: str) -> tuple[str, str]: | ||
| parent, _separator, child = prefix.rpartition(".") | ||
| return parent, child | ||
|
|
||
|
|
||
| RISKY_ML_PARENT_CHILD_REFS: frozenset[tuple[str, str]] = frozenset( | ||
| _split_parent_child_ref(prefix) for prefix in RISKY_ML_MODULE_PREFIXES | ||
| ) | ||
|
Comment on lines
+658
to
+681
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: Yes—for the C-accelerated For protocols < 4, it does not do dotted traversal; it only looks up a single attribute named exactly Citations:
🏁 Script executed: cd modelaudit && wc -l scanners/pickle_scanner.pyRepository: promptfoo/modelaudit Length of output: 95 🏁 Script executed: cd modelaudit && sed -n '658,681p' scanners/pickle_scanner.pyRepository: promptfoo/modelaudit Length of output: 776 🏁 Script executed: cd modelaudit && sed -n '1933,1950p' scanners/pickle_scanner.pyRepository: promptfoo/modelaudit Length of output: 772 🏁 Script executed: cd modelaudit && sed -n '1940,1960p' scanners/pickle_scanner.pyRepository: promptfoo/modelaudit Length of output: 791 🏁 Script executed: cd modelaudit && grep -n "pickle.HIGHEST_PROTOCOL\|protocols\|protocol" scanners/pickle_scanner.py | head -30Repository: promptfoo/modelaudit Length of output: 2266 🏁 Script executed: cd modelaudit && sed -n '2000,2010p' scanners/pickle_scanner.pyRepository: promptfoo/modelaudit Length of output: 615 🏁 Script executed: cd modelaudit && find . -name "*test*pickle*" -type f 2>/dev/null | head -10Repository: promptfoo/modelaudit Length of output: 46 🏁 Script executed: cd modelaudit && rg "_is_risky_ml_import" --type py -A 5 -B 5Repository: promptfoo/modelaudit Length of output: 2216 🏁 Script executed: cd modelaudit && rg "torch.*compile\.__globals__\|torch\.storage\._load_from_bytes\.__" tests/ --type pyRepository: promptfoo/modelaudit Length of output: 110 🏁 Script executed: cd modelaudit && grep -r "compile\.__globals__\|_load_from_bytes\.__" tests/ --include="*.py"Repository: promptfoo/modelaudit Length of output: 103 🏁 Script executed: git ls-files | grep -i test | grep -i pickle | head -20Repository: promptfoo/modelaudit Length of output: 1047 🏁 Script executed: git ls-files | grep -i test | head -30Repository: promptfoo/modelaudit Length of output: 1512 🏁 Script executed: cd modelaudit && rg "torch.*compile" --type py | grep -i test | head -10Repository: promptfoo/modelaudit Length of output: 46 🏁 Script executed: find . -path ./node_modules -prune -o -name "*test*pickle*scanner*" -type f -print 2>/dev/nullRepository: promptfoo/modelaudit Length of output: 103 🏁 Script executed: cd modelaudit && find .. -name "*test*" -type f -path "*/scanners/*" 2>/dev/null | head -10Repository: promptfoo/modelaudit Length of output: 492 🏁 Script executed: git ls-files | grep -E "test.*pickle.*scanner|scanner.*test.*pickle" | head -10Repository: promptfoo/modelaudit Length of output: 101 🏁 Script executed: cd modelaudit && git ls-files | grep test | grep scannerRepository: promptfoo/modelaudit Length of output: 46 🏁 Script executed: wc -l tests/scanners/test_pickle_scanner.pyRepository: promptfoo/modelaudit Length of output: 106 🏁 Script executed: grep -n "torch.*compile\|_is_risky_ml_import\|RISKY_ML" tests/scanners/test_pickle_scanner.py | head -30Repository: promptfoo/modelaudit Length of output: 352 🏁 Script executed: grep -n "def test.*risky\|def test.*ml" tests/scanners/test_pickle_scanner.pyRepository: promptfoo/modelaudit Length of output: 972 🏁 Script executed: sed -n '1,100p' tests/scanners/test_pickle_scanner.pyRepository: promptfoo/modelaudit Length of output: 3367 🏁 Script executed: sed -n '1603,1643p' tests/scanners/test_pickle_scanner.pyRepository: promptfoo/modelaudit Length of output: 2057 🏁 Script executed: sed -n '1750,1779p' tests/scanners/test_pickle_scanner.pyRepository: promptfoo/modelaudit Length of output: 1190 🏁 Script executed: sed -n '1644,1670p' tests/scanners/test_pickle_scanner.pyRepository: promptfoo/modelaudit Length of output: 1248 🏁 Script executed: sed -n '1590,1610p' tests/scanners/test_pickle_scanner.pyRepository: promptfoo/modelaudit Length of output: 944 🏁 Script executed: grep -n "compile.__\|__globals__\|__code__" tests/scanners/test_pickle_scanner.pyRepository: promptfoo/modelaudit Length of output: 46 🏁 Script executed: rg "torch.*compile.__" --type pyRepository: promptfoo/modelaudit Length of output: 46 🏁 Script executed: sed -n '2732,2760p' modelaudit/scanners/pickle_scanner.pyRepository: promptfoo/modelaudit Length of output: 1430 🏁 Script executed: grep -n "def is_dangerous_reduce_pattern\|_is_dangerous_ref" modelaudit/scanners/pickle_scanner.py | head -20Repository: promptfoo/modelaudit Length of output: 386 🏁 Script executed: sed -n '2643,2660p' modelaudit/scanners/pickle_scanner.pyRepository: promptfoo/modelaudit Length of output: 768 Treat exact risky refs as dotted prefixes—fix bypass via The current Add a check before the tuple split to test whether any prefix of the full dotted reference matches an exact ref: +RISKY_ML_EXACT_FULL_REFS: frozenset[str] = frozenset(
+ f"{module}.{name}" for module, name in RISKY_ML_EXACT_REFS
+)
+
def _is_risky_ml_import(mod: str, func: str) -> bool:
"""Return True when module/function matches risky ML import policy."""
full_ref = f"{mod}.{func}" if func else mod
parts = full_ref.split(".")
+ for i in range(1, len(parts) + 1):
+ candidate = ".".join(parts[:i])
+ if candidate in RISKY_ML_EXACT_FULL_REFS:
+ return True
+
for i in range(1, len(parts)):
candidate_mod = ".".join(parts[:i])
candidate_func = ".".join(parts[i:])
if (candidate_mod, candidate_func) in RISKY_ML_EXACT_REFS:
return TrueAdd a test case for 🤖 Prompt for AI Agents |
||
|
|
||
|
|
||
| def _is_dangerous_module(mod: str) -> bool: | ||
| """Check if module is in ALWAYS_DANGEROUS_MODULES (exact or prefix match). | ||
|
|
@@ -1905,6 +1930,26 @@ def _is_safe_ml_global(mod: str, func: str) -> bool: | |
| return False | ||
|
|
||
|
|
||
| def _is_risky_ml_import(mod: str, func: str) -> bool: | ||
| """Return True when module/function matches risky ML import policy.""" | ||
| full_ref = f"{mod}.{func}" if func else mod | ||
| parts = full_ref.split(".") | ||
|
|
||
| for i in range(1, len(parts)): | ||
| candidate_mod = ".".join(parts[:i]) | ||
| candidate_func = ".".join(parts[i:]) | ||
| if (candidate_mod, candidate_func) in RISKY_ML_EXACT_REFS: | ||
| return True | ||
| if (candidate_mod, candidate_func) in RISKY_ML_PARENT_CHILD_REFS: | ||
| return True | ||
| if any( | ||
| candidate_mod == prefix or candidate_mod.startswith(f"{prefix}.") for prefix in RISKY_ML_MODULE_PREFIXES | ||
| ): | ||
| return True | ||
|
|
||
| return False | ||
|
|
||
|
|
||
| def _is_copyreg_extension_ref(mod: str) -> bool: | ||
| """Return True when a reference came from an EXT opcode extension lookup.""" | ||
| return mod == COPYREG_EXTENSION_MODULE | ||
|
|
@@ -1954,6 +1999,13 @@ def _is_actually_dangerous_global(mod: str, func: str, ml_context: dict) -> bool | |
| logger.warning(f"Extension-registry callable detected via EXT opcode: {full_ref}") | ||
| return True | ||
|
|
||
| # STEP 0.5: Risky ML imports should be flagged even in import-only payloads. | ||
| # These are intentionally separate from the broad ML safe allowlist because | ||
| # they map to runtime loading/compilation pathways with elevated risk. | ||
| if _is_risky_ml_import(mod, func): | ||
| logger.warning(f"Risky ML import detected: {full_ref}") | ||
| return True | ||
|
Comment on lines
+2002
to
+2007
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mirror the risky-ML short-circuit inside Line 2748 still returns early on 🔧 Proposed fix def _is_dangerous_ref(mod: str, func: str) -> bool:
"""Check if a module.function reference is dangerous enough to flag."""
if _is_copyreg_extension_ref(mod):
return True
+ if _is_risky_ml_import(mod, func):
+ return True
full_ref = f"{mod}.{func}"Based on learnings, "Preserve or strengthen security detections; test both benign and malicious samples when adding scanner/feature changes". Also applies to: 2734-2751 🤖 Prompt for AI Agents |
||
|
|
||
| # STEP 1: ALWAYS flag dangerous functions first (no exceptions, no allowlist override) | ||
| # This MUST come before the ML_SAFE_GLOBALS check to prevent bypass attacks | ||
| # where an attacker places dangerous functions (e.g., operator.attrgetter) in a | ||
|
|
@@ -2588,6 +2640,10 @@ def is_suspicious_global(mod: str, func: str) -> bool: | |
| First checks against ML_SAFE_GLOBALS allowlist to reduce false positives | ||
| for legitimate ML framework operations. | ||
| """ | ||
| # STEP 0: Always flag risky ML imports before any allowlist checks. | ||
| if _is_risky_ml_import(mod, func): | ||
| return True | ||
|
|
||
| # STEP 1: Check ML_SAFE_GLOBALS allowlist first | ||
| # If the module.function is in the safe list, it's not suspicious | ||
| if mod in ML_SAFE_GLOBALS: | ||
|
|
@@ -5055,7 +5111,7 @@ def get_depth(x): | |
| 0, | ||
| ), | ||
| }, | ||
| why=get_import_explanation(module_name) | ||
| why=get_import_explanation(f"{module_name}.{func_name}") | ||
| if module_name | ||
| else "A dangerous pattern was detected that could execute arbitrary code during unpickling.", | ||
| ) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate
### Fixedsection violates Keep a Changelog format.There are two
### Fixedsections under[Unreleased](lines 82 and 124). The Keep a Changelog format requires each section type to appear only once per version. Merge this section's entries into the existing### Fixedsection at line 82.🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 124-124: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 Prompt for AI Agents