Skip to content

Commit c69c06c

Browse files
committed
Security fix: Prevent class pollution and remote code execution in Delta
- Add validation to prevent traversing dunder attributes via check_elem() - Harden Delta class against malicious pickle payloads - Make SAFE_TO_IMPORT a frozenset for immutability - Add comprehensive security tests in test_security.py - Prevent access to __globals__ and other dangerous attributes
1 parent b639fec commit c69c06c

File tree

4 files changed

+149
-3
lines changed

4 files changed

+149
-3
lines changed

deepdiff/delta.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
)
1818
from deepdiff.path import (
1919
_path_to_elements, _get_nested_obj, _get_nested_obj_and_force,
20-
GET, GETATTR, parse_path, stringify_path,
20+
GET, GETATTR, check_elem, parse_path, stringify_path,
2121
)
2222
from deepdiff.anyset import AnySet
2323
from deepdiff.summarize import summarize
@@ -237,6 +237,11 @@ def _get_elem_and_compare_to_old_value(
237237
forced_old_value=None,
238238
next_element=None,
239239
):
240+
try:
241+
check_elem(elem)
242+
except ValueError as error:
243+
self._raise_or_log(UNABLE_TO_GET_ITEM_MSG.format(path_for_err_reporting, error))
244+
return not_found
240245
# if forced_old_value is not None:
241246
try:
242247
if action == GET:
@@ -536,6 +541,7 @@ def _get_elements_and_details(self, path):
536541
obj = self
537542
# obj = self.get_nested_obj(obj=self, elements=elements[:-1])
538543
elem, action = elements[-1] # type: ignore
544+
check_elem(elem)
539545
except Exception as e:
540546
self._raise_or_log(UNABLE_TO_GET_ITEM_MSG.format(path, e))
541547
return None

deepdiff/path.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,7 @@ def _path_to_elements(path, root_element=DEFAULT_FIRST_ELEMENT):
117117

118118
def _get_nested_obj(obj, elements, next_element=None):
119119
for (elem, action) in elements:
120+
check_elem(elem)
120121
if action == GET:
121122
obj = obj[elem]
122123
elif action == GETATTR:
@@ -134,11 +135,17 @@ def _guess_type(elements, elem, index, next_element):
134135
return {}
135136

136137

138+
def check_elem(elem):
139+
if isinstance(elem, str) and elem.startswith("__") and elem.endswith("__"):
140+
raise ValueError("traversing dunder attributes is not allowed")
141+
142+
137143
def _get_nested_obj_and_force(obj, elements, next_element=None):
138144
prev_elem = None
139145
prev_action = None
140146
prev_obj = obj
141147
for index, (elem, action) in enumerate(elements):
148+
check_elem(elem)
142149
_prev_obj = obj
143150
if action == GET:
144151
try:

deepdiff/serialization.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ class UnsupportedFormatErr(TypeError):
5959
DELTA_IGNORE_ORDER_NEEDS_REPETITION_REPORT = 'report_repetition must be set to True when ignore_order is True to create the delta object.'
6060
DELTA_ERROR_WHEN_GROUP_BY = 'Delta can not be made when group_by is used since the structure of data is modified from the original form.'
6161

62-
SAFE_TO_IMPORT = {
62+
SAFE_TO_IMPORT = frozenset({
6363
'builtins.range',
6464
'builtins.complex',
6565
'builtins.set',
@@ -95,7 +95,7 @@ class UnsupportedFormatErr(TypeError):
9595
'ipaddress.IPv4Address',
9696
'ipaddress.IPv6Address',
9797
'collections.abc.KeysView',
98-
}
98+
})
9999

100100

101101
TYPE_STR_TO_TYPE = {

tests/test_security.py

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
import os
2+
import pickle
3+
import pytest
4+
from deepdiff import Delta
5+
from deepdiff.helper import Opcode
6+
from deepdiff.serialization import ForbiddenModule
7+
8+
9+
class TestDeltaClassPollution:
10+
11+
def test_builtins_int(self):
12+
13+
pollute_int = pickle.dumps(
14+
{
15+
"values_changed": {"root['tmp']": {"new_value": Opcode("", 0, 0, 0, 0)}},
16+
"dictionary_item_added": {
17+
(
18+
("root", "GETATTR"),
19+
("tmp", "GET"),
20+
("__repr__", "GETATTR"),
21+
("__globals__", "GETATTR"),
22+
("__builtins__", "GET"),
23+
("int", "GET"),
24+
): "no longer a class"
25+
},
26+
}
27+
)
28+
29+
assert isinstance(pollute_int, bytes)
30+
31+
# ------------[ Exploit ]------------
32+
# This could be some example, vulnerable, application.
33+
# The inputs above could be sent via HTTP, for example.
34+
35+
36+
# Existing dictionary; it is assumed that it contains
37+
# at least one entry, otherwise a different Delta needs to be
38+
# applied first, adding an entry to the dictionary.
39+
mydict = {"tmp": "foobar"}
40+
41+
# Before pollution
42+
assert 42 == int("41") + 1
43+
44+
# Apply Delta to mydict
45+
result = mydict + Delta(pollute_int)
46+
47+
assert 1337 == int("1337")
48+
49+
def test_remote_code_execution(self):
50+
if os.path.exists('/tmp/pwned'):
51+
os.remove('/tmp/pwned')
52+
53+
pollute_safe_to_import = pickle.dumps(
54+
{
55+
"values_changed": {"root['tmp']": {"new_value": Opcode("", 0, 0, 0, 0)}},
56+
"set_item_added": {
57+
(
58+
("root", "GETATTR"),
59+
("tmp", "GET"),
60+
("__repr__", "GETATTR"),
61+
("__globals__", "GETATTR"),
62+
("sys", "GET"),
63+
("modules", "GETATTR"),
64+
("deepdiff.serialization", "GET"),
65+
("SAFE_TO_IMPORT", "GETATTR"),
66+
): set(["posix.system"])
67+
},
68+
}
69+
)
70+
71+
# From https://davidhamann.de/2020/04/05/exploiting-python-pickle/
72+
class RCE:
73+
def __reduce__(self):
74+
cmd = "id > /tmp/pwned"
75+
return os.system, (cmd,)
76+
77+
# Wrap object with dictionary so that Delta does not crash
78+
rce_pickle = pickle.dumps({"_": RCE()})
79+
80+
assert isinstance(pollute_safe_to_import, bytes)
81+
assert isinstance(rce_pickle, bytes)
82+
83+
# ------------[ Exploit ]------------
84+
# This could be some example, vulnerable, application.
85+
# The inputs above could be sent via HTTP, for example.
86+
87+
# Existing dictionary; it is assumed that it contains
88+
# at least one entry, otherwise a different Delta needs to be
89+
# applied first, adding an entry to the dictionary.
90+
mydict = {"tmp": "foobar"}
91+
92+
# Apply Delta to mydict
93+
with pytest.raises(ValueError) as exc_info:
94+
mydict + Delta(pollute_safe_to_import)
95+
assert "traversing dunder attributes is not allowed" == str(exc_info.value)
96+
97+
with pytest.raises(ForbiddenModule) as exc_info:
98+
Delta(rce_pickle) # no need to apply this Delta
99+
assert "Module 'posix.system' is forbidden. You need to explicitly pass it by passing a safe_to_import parameter" == str(exc_info.value)
100+
101+
assert not os.path.exists('/tmp/pwned'), "We should not have created this file"
102+
103+
def test_delta_should_not_access_globals(self):
104+
105+
pollute_global = pickle.dumps(
106+
{
107+
"dictionary_item_added": {
108+
(
109+
("root", "GETATTR"),
110+
("myfunc", "GETATTR"),
111+
("__globals__", "GETATTR"),
112+
("PWNED", "GET"),
113+
): 1337
114+
}
115+
}
116+
)
117+
118+
119+
# demo application
120+
class Foo:
121+
def __init__(self):
122+
pass
123+
124+
def myfunc(self):
125+
pass
126+
127+
128+
PWNED = False
129+
delta = Delta(pollute_global)
130+
assert PWNED is False
131+
b = Foo() + delta
132+
133+
assert PWNED is False

0 commit comments

Comments
 (0)