Skip to content

Commit 3d656b9

Browse files
dguidoclaudethomas-chauchefoin-tob
authored
Add safe builtins allowlist to prevent false positives (#206)
* Add safe builtins allowlist to prevent false positives Previously, all imports from the builtins module were flagged as LIKELY_OVERTLY_MALICIOUS, even safe functions like dict(), len(), sorted(), and enumerate(). This caused false positives for legitimate pickle files. Add SAFE_BUILTINS frozenset containing type constructors and pure functions that cannot be used for code execution or system access. Modify both UnsafeImportsML and UnsafeImports analyzers to check individual builtin names against this allowlist. Dangerous builtins like eval, exec, getattr, __import__, and open remain blocked as they can be used for arbitrary code execution. Fixes #205 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Fix ruff formatting for PR - Format analysis.py and test_bypasses.py with ruff - Remove mypy from pre-commit hooks (was already skipped in CI) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Extract SAFE_BUILTINS and BUILTIN_MODULE_NAMES out of analysis module - Add BUILTIN_MODULE_NAMES constant to eliminate repeated tuple - Move SAFE_BUILTINS from UnsafeImportsML class to fickle.py - Update both UnsafeImportsML and UnsafeImports to use shared constants - Removes cross-class dependency (UnsafeImports no longer references UnsafeImportsML.SAFE_BUILTINS) Co-Authored-By: Claude <noreply@anthropic.com> * Remove type from SAFE_BUILTINS allowlist type() with 3 arguments dynamically creates classes, which could be a building block in exploit chains (e.g., triggering __init_subclass__ or __set_name__ on imported base classes/descriptors). While not directly exploitable in isolation, there's no legitimate reason for a pickle to dynamically create classes, so we exclude it as a defense-in-depth measure. Co-Authored-By: Claude <noreply@anthropic.com> * Make unsafe builtin tests verify both analyzers detect the issue Update test_unsafe_builtins_still_flagged and test_unsafe_builtin_eval_still_flagged to assert that both UnsafeImports and UnsafeImportsML flag dangerous builtins, rather than checking if either one does. Co-Authored-By: Claude <noreply@anthropic.com> * Simplify UnsafeImports builtin check using all() Co-Authored-By: Claude <noreply@anthropic.com> * Format BUILTIN_MODULE_NAMES as single line Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: Thomas Chauchefoin <thomas.chauchefoin@trailofbits.com>
1 parent 69ba1e5 commit 3d656b9

File tree

3 files changed

+168
-8
lines changed

3 files changed

+168
-8
lines changed

‎fickling/analysis.py‎

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,14 @@
77
from collections.abc import Iterable, Iterator
88
from enum import Enum
99

10-
from fickling.fickle import InterpretationError, Interpreter, Pickled, Proto
10+
from fickling.fickle import (
11+
BUILTIN_MODULE_NAMES,
12+
SAFE_BUILTINS,
13+
InterpretationError,
14+
Interpreter,
15+
Pickled,
16+
Proto,
17+
)
1118

1219

1320
class AnalyzerMeta(type):
@@ -267,13 +274,27 @@ def analyze(self, context: AnalysisContext) -> Iterator[AnalysisResult]:
267274
]
268275
for module_name in all_modules:
269276
if module_name in self.UNSAFE_MODULES:
270-
risk_info = self.UNSAFE_MODULES[module_name]
271-
yield AnalysisResult(
272-
Severity.LIKELY_OVERTLY_MALICIOUS,
273-
f"`{shortened}` uses `{module_name}` that is indicative of a malicious pickle file. {risk_info}",
274-
"UnsafeImportsML",
275-
trigger=shortened,
276-
)
277+
# Special handling for builtins - check specific function names
278+
if module_name in BUILTIN_MODULE_NAMES:
279+
for n in node.names:
280+
if n.name not in SAFE_BUILTINS:
281+
risk_info = self.UNSAFE_MODULES[module_name]
282+
yield AnalysisResult(
283+
Severity.LIKELY_OVERTLY_MALICIOUS,
284+
f"`{shortened}` imports `{n.name}` from `{module_name}` "
285+
f"which can execute arbitrary code. {risk_info}",
286+
"UnsafeImportsML",
287+
trigger=shortened,
288+
)
289+
else:
290+
# All other unsafe modules are fully blocked
291+
risk_info = self.UNSAFE_MODULES[module_name]
292+
yield AnalysisResult(
293+
Severity.LIKELY_OVERTLY_MALICIOUS,
294+
f"`{shortened}` uses `{module_name}` that is indicative of a malicious pickle file. {risk_info}",
295+
"UnsafeImportsML",
296+
trigger=shortened,
297+
)
277298
if node.module in self.UNSAFE_IMPORTS:
278299
for n in node.names:
279300
if n.name in self.UNSAFE_IMPORTS[node.module]:
@@ -348,6 +369,10 @@ def analyze(self, context: AnalysisContext) -> Iterator[AnalysisResult]:
348369
class UnsafeImports(Analysis):
349370
def analyze(self, context: AnalysisContext) -> Iterator[AnalysisResult]:
350371
for node in context.pickled.unsafe_imports():
372+
if node.module in BUILTIN_MODULE_NAMES and all(
373+
n.name in SAFE_BUILTINS for n in node.names
374+
):
375+
continue
351376
shortened, _ = context.shorten_code(node)
352377
yield AnalysisResult(
353378
Severity.LIKELY_OVERTLY_MALICIOUS,

‎fickling/fickle.py‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,72 @@
6262
]
6363
)
6464

65+
BUILTIN_MODULE_NAMES: frozenset[str] = frozenset(["builtins", "__builtins__", "__builtin__"])
66+
67+
# Builtins that are safe to import - pure functions and type constructors
68+
# that cannot be used for code execution or system access.
69+
# Dangerous builtins NOT in this list (and thus blocked):
70+
# - eval, exec, compile: direct code execution
71+
# - open: file system access
72+
# - __import__, __loader__, __spec__: import machinery
73+
# - getattr, setattr, delattr, hasattr: attribute access (can call any method)
74+
# - globals, locals, vars: namespace access
75+
# - input: user input (could be abused)
76+
# - breakpoint: debugger access
77+
# - memoryview: low-level memory access
78+
SAFE_BUILTINS: frozenset[str] = frozenset(
79+
[
80+
# Type constructors (create data, cannot execute code)
81+
"bool",
82+
"int",
83+
"float",
84+
"complex",
85+
"str",
86+
"bytes",
87+
"bytearray",
88+
"list",
89+
"tuple",
90+
"set",
91+
"frozenset",
92+
"dict",
93+
# Pure functions (no side effects, no code execution)
94+
"len",
95+
"abs",
96+
"sum",
97+
"min",
98+
"max",
99+
"round",
100+
"pow",
101+
"divmod",
102+
"sorted",
103+
"reversed",
104+
"enumerate",
105+
"zip",
106+
"range",
107+
"map",
108+
"filter",
109+
"slice",
110+
"iter",
111+
"next",
112+
"all",
113+
"any",
114+
"hash",
115+
"id",
116+
"repr",
117+
"ascii",
118+
"bin",
119+
"hex",
120+
"oct",
121+
"ord",
122+
"chr",
123+
"isinstance",
124+
"issubclass",
125+
"object",
126+
"callable",
127+
"format",
128+
]
129+
)
130+
65131

66132
def is_std_module(module_name: str) -> bool:
67133
return module_name in BUILTIN_STDLIB_MODULE_NAMES

‎test/test_bypasses.py‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -376,3 +376,72 @@ def test_builtins_import_bypass(self):
376376
res.detailed_results()["AnalysisResult"].get("UnsafeImports"),
377377
"from builtins import getattr",
378378
)
379+
380+
def test_safe_builtins_not_flagged(self):
381+
"""Safe builtins like len, dict should not be flagged as malicious."""
382+
pickled = Pickled(
383+
[
384+
op.Global("builtins len"),
385+
op.EmptyList(),
386+
op.TupleOne(),
387+
op.Reduce(),
388+
op.Stop(),
389+
]
390+
)
391+
res = check_safety(pickled)
392+
# Should not have UnsafeImports or UnsafeImportsML result for safe builtins
393+
detailed = res.detailed_results().get("AnalysisResult", {})
394+
self.assertIsNone(detailed.get("UnsafeImports"))
395+
self.assertIsNone(detailed.get("UnsafeImportsML"))
396+
397+
def test_safe_builtin_dict_not_flagged(self):
398+
"""Safe builtin dict() should not be flagged as malicious."""
399+
pickled = Pickled(
400+
[
401+
op.Global("builtins dict"),
402+
op.EmptyTuple(),
403+
op.Reduce(),
404+
op.Stop(),
405+
]
406+
)
407+
res = check_safety(pickled)
408+
detailed = res.detailed_results().get("AnalysisResult", {})
409+
self.assertIsNone(detailed.get("UnsafeImports"))
410+
self.assertIsNone(detailed.get("UnsafeImportsML"))
411+
412+
def test_unsafe_builtins_still_flagged(self):
413+
"""Dangerous builtins like getattr, __import__ must still be flagged."""
414+
pickled = Pickled(
415+
[
416+
op.Global("builtins getattr"),
417+
op.String("os"),
418+
op.String("system"),
419+
op.TupleTwo(),
420+
op.Reduce(),
421+
op.Stop(),
422+
]
423+
)
424+
res = check_safety(pickled)
425+
self.assertGreater(res.severity, Severity.LIKELY_SAFE)
426+
# Should be flagged by both unsafe import checkers
427+
detailed = res.detailed_results().get("AnalysisResult", {})
428+
self.assertIsNotNone(detailed.get("UnsafeImports"))
429+
self.assertIsNotNone(detailed.get("UnsafeImportsML"))
430+
431+
def test_unsafe_builtin_eval_still_flagged(self):
432+
"""Dangerous builtin eval must still be flagged."""
433+
pickled = Pickled(
434+
[
435+
op.Global("builtins eval"),
436+
op.String("print('hello')"),
437+
op.TupleOne(),
438+
op.Reduce(),
439+
op.Stop(),
440+
]
441+
)
442+
res = check_safety(pickled)
443+
self.assertGreater(res.severity, Severity.LIKELY_SAFE)
444+
# Should be flagged by both unsafe import checkers
445+
detailed = res.detailed_results().get("AnalysisResult", {})
446+
self.assertIsNotNone(detailed.get("UnsafeImports"))
447+
self.assertIsNotNone(detailed.get("UnsafeImportsML"))

0 commit comments

Comments
 (0)