Skip to content

Commit b692d40

Browse files
fix(iast): eval wrapping error [backport 3.10] (#13997)
Backport fe421d2 from #13976 to 3.10. Python treats eval() differently depending on whether the 'locals' argument is provided. If only 'globals' is given, any new functions or variables are stored in the globals. If both 'globals' and 'locals' are given, new definitions go into locals instead. To ensure we can access any functions created by eval(), we copy them from locals to globals when needed. ## Checklist - [x] PR author has checked that all the criteria below are met - The PR description includes an overview of the change - The PR description articulates the motivation for the change - The change includes tests OR the PR description describes a testing strategy - The PR description notes risks associated with the change, if any - Newly-added code is easy to change - The change follows the [library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) - The change includes or references documentation updates if necessary - Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Reviewer has checked that all the criteria below are met - Title is accurate - All changes are related to the pull request's stated goal - Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - Testing strategy adequately addresses listed risks - Newly-added code is easy to change - Release note makes sense to a user of the library - If necessary, author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment - Backport labels are set in a manner that is consistent with the [release branch maintenance policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting) Co-authored-by: Alberto Vara <[email protected]>
1 parent 8d3e0d3 commit b692d40

File tree

11 files changed

+200
-22
lines changed

11 files changed

+200
-22
lines changed

ddtrace/appsec/_iast/taint_sinks/code_injection.py

Lines changed: 41 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
from ddtrace.appsec._constants import IAST
55
from ddtrace.appsec._constants import IAST_SPAN_TAGS
66
from ddtrace.appsec._iast._logs import iast_error
7+
from ddtrace.appsec._iast._logs import iast_propagation_sink_point_debug_log
78
from ddtrace.appsec._iast._metrics import _set_metric_iast_executed_sink
89
from ddtrace.appsec._iast._metrics import _set_metric_iast_instrumented_sink
910
from ddtrace.appsec._iast._patch_modules import WrapFunctonsForIAST
@@ -32,7 +33,6 @@ def patch():
3233
# TODO: wrap exec functions is very dangerous because it needs and modifies locals and globals from the original
3334
# function
3435
# iast_funcs.wrap_function("builtins", "exec", _iast_coi)
35-
3636
iast_funcs.patch()
3737

3838
_set_metric_iast_instrumented_sink(VULN_CODE_INJECTION)
@@ -47,27 +47,49 @@ def _iast_coi(wrapped, instance, args, kwargs):
4747
if len(args) >= 1:
4848
_iast_report_code_injection(args[0])
4949

50-
caller_frame = None
51-
if len(args) > 1:
52-
func_globals = args[1]
53-
elif kwargs.get("globals"):
54-
func_globals = kwargs.get("globals")
55-
else:
56-
frames = inspect.currentframe()
57-
caller_frame = frames.f_back
58-
func_globals = caller_frame.f_globals
59-
60-
if len(args) > 2:
61-
func_locals = args[2]
62-
elif kwargs.get("locals"):
63-
func_locals = kwargs.get("locals")
64-
else:
65-
if caller_frame is None:
50+
try:
51+
caller_frame = None
52+
if len(args) > 1:
53+
func_globals = args[1]
54+
elif kwargs.get("globals"):
55+
func_globals = kwargs.get("globals")
56+
else:
6657
frames = inspect.currentframe()
6758
caller_frame = frames.f_back
68-
func_locals = caller_frame.f_locals
59+
func_globals = caller_frame.f_globals
60+
61+
func_locals_copy_to_check = None
62+
if len(args) > 2:
63+
func_locals = args[2]
64+
elif kwargs.get("locals"):
65+
func_locals = kwargs.get("locals")
66+
else:
67+
if caller_frame is None:
68+
frames = inspect.currentframe()
69+
caller_frame = frames.f_back
70+
func_locals = caller_frame.f_locals
71+
func_locals_copy_to_check = func_locals.copy() if func_locals else None
72+
except Exception as e:
73+
iast_propagation_sink_point_debug_log(f"Error in _iast_code_injection. {e}")
74+
return wrapped(*args, **kwargs)
75+
76+
res = wrapped(args[0], func_globals, func_locals)
77+
78+
# We need to perform this `func_locals_copy_to_check` check because of how Python handles `eval()` depending
79+
# on whether `locals` is provided. If we have code like `def evaluate(n): return n` and we call
80+
# `eval(code, my_globals)`, the new function will be stored in `my_globals["evaluate"]`. However, if we call
81+
# `eval(code, my_globals, my_locals)`, then the function will be stored in `my_locals["evaluate"]`. So, if `eval()`
82+
# is called without a `locals` argument, we need to transfer the newly created code from the local
83+
# context to the global one.
84+
try:
85+
if func_locals_copy_to_check is not None:
86+
diff_keys = set(func_locals) - set(func_locals_copy_to_check)
87+
for key in diff_keys:
88+
func_globals[key] = func_locals[key]
89+
except Exception as e:
90+
iast_propagation_sink_point_debug_log(f"Error in _iast_code_injection. {e}")
6991

70-
return wrapped(args[0], func_globals, func_locals)
92+
return res
7193

7294

7395
def _iast_report_code_injection(code_string: Text):

hatch.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -316,6 +316,7 @@ dependencies = [
316316
"pytest-xdist",
317317
"hypothesis",
318318
"requests",
319+
"babel",
319320
"SQLAlchemy",
320321
"psycopg2-binary~=2.9.9",
321322
"pymysql",
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
---
2+
fixes:
3+
- |
4+
Code Security (IAST): Improved compatibility with eval() when used with custom globals and locals. When
5+
instrumenting eval(), Python behaves differently depending on whether locals is passed. If both globals and
6+
locals are provided, new functions are stored in the locals dictionary. This fix ensures any dynamically
7+
defined functions (e.g., via eval(code, globals, locals)) are accessible by copying them from locals to
8+
globals when necessary. This resolves issues with third-party libraries (like babel) that rely on this behavior.

tests/appsec/app.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
from tests.appsec.iast_packages.packages.pkg_annotated_types import pkg_annotated_types
2323
from tests.appsec.iast_packages.packages.pkg_asn1crypto import pkg_asn1crypto
2424
from tests.appsec.iast_packages.packages.pkg_attrs import pkg_attrs
25+
from tests.appsec.iast_packages.packages.pkg_babel import pkg_babel
2526
from tests.appsec.iast_packages.packages.pkg_beautifulsoup4 import pkg_beautifulsoup4
2627
from tests.appsec.iast_packages.packages.pkg_cachetools import pkg_cachetools
2728
from tests.appsec.iast_packages.packages.pkg_certifi import pkg_certifi
@@ -101,6 +102,7 @@
101102
app.register_blueprint(pkg_annotated_types)
102103
app.register_blueprint(pkg_asn1crypto)
103104
app.register_blueprint(pkg_attrs)
105+
app.register_blueprint(pkg_babel)
104106
app.register_blueprint(pkg_beautifulsoup4)
105107
app.register_blueprint(pkg_cachetools)
106108
app.register_blueprint(pkg_certifi)

tests/appsec/iast/fixtures/taint_sinks/code_injection.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,11 @@ def pt_eval_lambda_globals(origin_string):
3838
return r
3939

4040

41+
def pt_eval_add_data_to_global(origin_string, globals_namespace, locals_namespace):
42+
code = compile(origin_string, "<rule>", "exec")
43+
eval(code, globals_namespace, locals_namespace)
44+
45+
4146
def pt_literal_eval(origin_string):
4247
r = literal_eval(origin_string)
4348
return r

tests/appsec/iast/taint_sinks/test_code_injection.py

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,6 @@ def test_code_injection_eval_globals_locals_override(iast_context_defaults):
113113

114114
def test_code_injection_eval_lambda(iast_context_defaults):
115115
"""Validate globals and locals of the function"""
116-
mod = _iast_patched_module("tests.appsec.iast.fixtures.taint_sinks.code_injection")
117116

118117
def pt_eval_lambda_no_tainted(fun):
119118
return eval("lambda v,fun=fun:not fun(v)")
@@ -126,7 +125,6 @@ def is_true_no_tainted(value):
126125

127126
def test_code_injection_eval_globals_kwargs_lambda(iast_context_defaults):
128127
"""Validate globals and locals of the function"""
129-
130128
code_string = "square(5)"
131129

132130
tainted_string = taint_pyobject(
@@ -151,7 +149,6 @@ def test_code_injection_eval_globals_kwargs_lambda(iast_context_defaults):
151149

152150

153151
def test_code_injection_literal_eval(iast_context_defaults):
154-
mod = _iast_patched_module("tests.appsec.iast.fixtures.taint_sinks.code_injection")
155152
code_string = "[1, 2, 3]"
156153

157154
tainted_string = taint_pyobject(
@@ -162,3 +159,43 @@ def test_code_injection_literal_eval(iast_context_defaults):
162159
data = get_iast_reporter()
163160

164161
assert data is None
162+
163+
164+
def test_code_injection_eval_add_data_to_global(iast_context_defaults):
165+
mod = _iast_patched_module("tests.appsec.iast.fixtures.taint_sinks.code_injection")
166+
code_string = """
167+
def evaluate(n):
168+
return func(n)
169+
"""
170+
171+
tainted_string = taint_pyobject(
172+
code_string, source_name="path", source_value=code_string, source_origin=OriginType.PATH
173+
)
174+
namespace_globals = {}
175+
namespace_locals = None
176+
mod.pt_eval_add_data_to_global(tainted_string, namespace_globals, namespace_locals)
177+
assert namespace_globals["evaluate"]
178+
data = get_iast_reporter()
179+
180+
assert data is None
181+
182+
183+
def test_code_injection_eval_add_data_to_local(iast_context_defaults):
184+
mod = _iast_patched_module("tests.appsec.iast.fixtures.taint_sinks.code_injection")
185+
code_string = """
186+
def evaluate(n):
187+
return func(n)
188+
"""
189+
190+
tainted_string = taint_pyobject(
191+
code_string, source_name="path", source_value=code_string, source_origin=OriginType.PATH
192+
)
193+
namespace_globals = {}
194+
namespace_locals = {"var1": "value1", "var2": "value2"}
195+
mod.pt_eval_add_data_to_global(tainted_string, namespace_globals, namespace_locals)
196+
assert namespace_locals["evaluate"]
197+
with pytest.raises(KeyError):
198+
_ = namespace_globals["evaluate"]
199+
data = get_iast_reporter()
200+
201+
assert data is None
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
"""
2+
idna==3.6
3+
4+
https://pypi.org/project/idna/
5+
"""
6+
7+
from flask import Blueprint
8+
from flask import request
9+
10+
from .utils import ResultResponse
11+
12+
13+
pkg_babel = Blueprint("package_babel", __name__)
14+
15+
16+
@pkg_babel.route("/babel")
17+
def pkg_babel_view():
18+
from babel.plural import to_python
19+
20+
response = ResultResponse(request.args.get("package_param"))
21+
func = to_python({"one": "n is 1", "few": "n in 2..4"})
22+
23+
response.result1 = func(int(request.args.get("package_param")))
24+
return response.json()
25+
26+
27+
@pkg_babel.route("/babel_propagation")
28+
def pkg_babel_propagation_view():
29+
from babel import Locale
30+
31+
from ddtrace.appsec._iast._taint_tracking._taint_objects_base import is_pyobject_tainted
32+
33+
response = ResultResponse(request.args.get("package_param"))
34+
if not is_pyobject_tainted(response.package_param):
35+
response.result1 = "Error: package_param is not tainted"
36+
return response.json()
37+
38+
response.result1 = Locale("en", "US").currency_formats["standard"]
39+
response.result1 = "OK" if is_pyobject_tainted(response.package_param) else "Error: result is not tainted"
40+
return response.json()

tests/appsec/iast_packages/test_packages.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -918,6 +918,15 @@ def create_venv(self):
918918
import_name="annotated_types",
919919
skip_python_version=[(3, 8)],
920920
),
921+
PackageForTesting(
922+
"babel",
923+
"2.17.0",
924+
"15",
925+
"other",
926+
"",
927+
test_import=False,
928+
test_propagation=True,
929+
),
921930
]
922931

923932
# Sort by name so it's easier to infer the progress of the tests
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
import types
2+
3+
4+
code1 = """
5+
def evaluate_locale():
6+
from babel import Locale
7+
response = Locale('en', 'US').currency_formats['standard']
8+
return response
9+
"""
10+
11+
12+
def babel_locale():
13+
module_name = "test_babel"
14+
compiled_code = compile(code1, "tests/appsec/integrations/packages_tests/", mode="exec")
15+
module_changed = types.ModuleType(module_name)
16+
exec(compiled_code, module_changed.__dict__) # define evaluate
17+
result = eval("evaluate_locale()", module_changed.__dict__) # llama evaluate
18+
return f"OK:{result}"
19+
20+
21+
def babel_to_python(n):
22+
def evaluate_to_python(n):
23+
from babel.plural import to_python
24+
25+
func = to_python({"one": "n is 1", "few": "n in 2..4"})
26+
return func(n)
27+
28+
return f"OK:{evaluate_to_python(n)}"

tests/appsec/integrations/packages_tests/conftest.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import pytest
22

3+
from ddtrace.appsec._iast.taint_sinks.code_injection import patch as code_injection_patch
34
from ddtrace.contrib.internal.psycopg.patch import patch as psycopg_patch
45
from ddtrace.contrib.internal.psycopg.patch import unpatch as psycopg_unpatch
56
from ddtrace.contrib.internal.sqlalchemy.patch import patch as sqlalchemy_patch
@@ -19,6 +20,7 @@ def iast_create_context():
1920
sqlalchemy_patch()
2021
psycopg_patch()
2122
sqli_sqlite_patch()
23+
code_injection_patch()
2224
_start_iast_context_and_oce()
2325
yield
2426
_end_iast_context_and_oce()

0 commit comments

Comments
 (0)