Skip to content

Commit ae55dd3

Browse files
Fix run_hook() missing several pickle entry points (GHSA-wccx-j62j-r448)
run_hook() only hooked pickle.load and pickle.Unpickler, leaving pickle.loads, _pickle.load, and _pickle.loads unprotected. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent ffac347 commit ae55dd3

File tree

4 files changed

+31
-3
lines changed

4 files changed

+31
-3
lines changed

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ raises an `UnsafeFileError` exception if malicious content is detected in the fi
8787
#### Option 1 (recommended): check safety of all pickle files loaded
8888

8989
```python
90-
# This enforces safety checks every time pickle.load() is used
90+
# This enforces safety checks every time pickle is used to deserialize
9191
fickling.always_check_safety()
9292

9393
# Attempt to load an unsafe file now raises an exception

fickling/hook.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,11 @@ def load(self):
3131

3232
def run_hook():
3333
"""Replace pickle.load() and pickle.Unpickler by fickling's safe versions"""
34-
# Hook the function
34+
# Hook functions
3535
pickle.load = loader.load
36+
_pickle.load = loader.load
37+
pickle.loads = loader.loads
38+
_pickle.loads = loader.loads
3639

3740
# Hook the Unpickler class
3841
pickle.Unpickler = FicklingSafetyUnpickler

fickling/loader.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
11
import pickle
22
from io import BytesIO
33

4+
# Save the original pickle.loads before any hooks can replace it.
5+
# loader.load() must use the real pickle.loads for final deserialization,
6+
# otherwise hooking pickle.loads causes infinite recursion.
7+
_original_pickle_loads = pickle.loads
8+
49
from fickling.analysis import Severity, check_safety
510
from fickling.exception import UnsafeFileError
611
from fickling.fickle import Pickled
@@ -21,7 +26,7 @@ def load(
2126
# We don't do pickle.load(file) because it could allow for a race
2227
# condition where the file we check is not the same that gets
2328
# loaded after the analysis.
24-
return pickle.loads(pickled_data.dumps(), *args, **kwargs)
29+
return _original_pickle_loads(pickled_data.dumps(), *args, **kwargs)
2530
if pickled_data.has_invalid_opcode:
2631
raise UnsafeFileError(
2732
file,

test/test_hook.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import _pickle
12
import io
23
import pickle
34
import unittest
@@ -25,6 +26,9 @@ def setUp(self):
2526
# Set up global fickling hook
2627
hook.run_hook()
2728

29+
def tearDown(self):
30+
hook.remove_hook()
31+
2832
def test_safe_pickle(self):
2933
# Fickling can check a pickle file for safety prior to running it
3034
test_list = [1, 2, 3]
@@ -44,3 +48,19 @@ def test_unsafe_pickle(self):
4448
pass
4549
else:
4650
self.fail(e)
51+
52+
# https://github.com/trailofbits/fickling/security/advisories/GHSA-wccx-j62j-r448
53+
def test_run_hook_covers_all_entry_points(self):
54+
data = pickle.dumps(Payload())
55+
cases = {
56+
"pickle.load": lambda: pickle.load(io.BytesIO(data)),
57+
"pickle.loads": lambda: pickle.loads(data),
58+
"pickle.Unpickler": lambda: pickle.Unpickler(io.BytesIO(data)).load(),
59+
"_pickle.load": lambda: _pickle.load(io.BytesIO(data)),
60+
"_pickle.loads": lambda: _pickle.loads(data),
61+
"_pickle.Unpickler": lambda: _pickle.Unpickler(io.BytesIO(data)).load(),
62+
}
63+
for name, call in cases.items():
64+
with self.subTest(entry_point=name):
65+
with self.assertRaises(UnsafeFileError, msg=f"{name} was not intercepted"):
66+
call()

0 commit comments

Comments
 (0)