Skip to content

Commit fcf5dbb

Browse files
authored
fix(iast): remove logs metrics noise [backport 1.20] (#7550)
Backport 5dae3e3 from #7546 to 1.20. IAST Aspects report a log metric each time an exception is raised. We noticed an application that wants to raise and control exceptions, such as in this dummy example: ```python my_data = {"element1": 1} is_element2_in_my_data = False try: my_data["element1"] is_element2_in_my_data = True except KeyError: pass ``` Those operations should not log a metric. ## Checklist - [x] Change(s) are motivated and described in the PR description. - [x] Testing strategy is described if automated tests are not included in the PR. - [x] Risk is outlined (performance impact, potential for breakage, maintainability, etc). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed. If no release note is required, add label `changelog/no-changelog`. - [x] Documentation is included (in-code, generated user docs, [public corp docs](https://github.com/DataDog/documentation/)). - [x] Backport labels are set (if [applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)) ## Reviewer Checklist - [x] Title is accurate. - [x] No unnecessary changes are introduced. - [x] Description motivates each change. - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes unless absolutely necessary. - [x] Testing strategy adequately addresses listed risk(s). - [x] Change is maintainable (easy to change, telemetry, documentation). - [x] Release note makes sense to a user of the library. - [x] Reviewer has explicitly acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment. - [x] 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) - [x] If this PR touches code that signs or publishes builds or packages, or handles credentials of any kind, I've requested a review from `@DataDog/security-design-and-guidance`. - [x] This PR doesn't touch any of that.
1 parent 0db0d25 commit fcf5dbb

File tree

6 files changed

+109
-28
lines changed

6 files changed

+109
-28
lines changed

ddtrace/appsec/iast/_taint_tracking/aspects.py

Lines changed: 41 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ def add_aspect(op1, op2):
5151
def str_aspect(*args, **kwargs):
5252
# type: (Any, Any) -> str
5353
result = builtin_str(*args, **kwargs)
54+
5455
if isinstance(args[0], TEXT_TYPES) and is_pyobject_tainted(args[0]):
5556
try:
5657
if isinstance(args[0], (bytes, bytearray)):
@@ -153,39 +154,46 @@ def build_string_aspect(*args): # type: (List[Any]) -> str
153154

154155
def ljust_aspect(candidate_text, *args, **kwargs):
155156
# type: (Any, Any, Any) -> Union[str, bytes, bytearray]
157+
158+
result = candidate_text.ljust(*args, **kwargs)
159+
156160
if not isinstance(candidate_text, TEXT_TYPES):
157-
return candidate_text.ljust(*args, **kwargs)
161+
return result
162+
158163
try:
159164
ranges_new = get_ranges(candidate_text)
160165
fillchar = parse_params(1, "fillchar", " ", *args, **kwargs)
161166
fillchar_ranges = get_ranges(fillchar)
162167
if ranges_new is None or (not ranges_new and not fillchar_ranges):
163-
return candidate_text.ljust(*args, **kwargs)
168+
return result
164169

165170
if fillchar_ranges:
166171
# Can only be one char, so we create one range to cover from the start to the end
167172
ranges_new = ranges_new + [shift_taint_range(fillchar_ranges[0], len(candidate_text))]
168173

169-
res = candidate_text.ljust(parse_params(0, "width", None, *args, **kwargs), fillchar)
170-
taint_pyobject_with_ranges(res, ranges_new)
171-
return res
174+
new_result = candidate_text.ljust(parse_params(0, "width", None, *args, **kwargs), fillchar)
175+
taint_pyobject_with_ranges(new_result, ranges_new)
176+
return new_result
172177
except Exception as e:
173178
_set_iast_error_metric("IAST propagation error. ljust_aspect. {}".format(e))
174-
return candidate_text.ljust(*args, **kwargs)
179+
return result
175180

176181

177182
def zfill_aspect(candidate_text, *args, **kwargs):
178183
# type: (Any, Any, Any) -> Any
184+
185+
result = candidate_text.zfill(*args, **kwargs)
186+
179187
if not isinstance(candidate_text, TEXT_TYPES):
180-
return candidate_text.zfill(*args, **kwargs)
188+
return result
189+
181190
try:
182191
ranges_orig = get_ranges(candidate_text)
183192
if not ranges_orig:
184-
return candidate_text.zfill(*args, **kwargs)
193+
return result
185194
prefix = candidate_text[0] in ("-", "+")
186-
res = candidate_text.zfill(*args, **kwargs)
187195

188-
difflen = len(res) - len(candidate_text)
196+
difflen = len(result) - len(candidate_text)
189197
ranges_new = [] # type: List[TaintRange]
190198
ranges_new_append = ranges_new.append
191199
ranges_new_extend = ranges_new.extend
@@ -200,25 +208,27 @@ def zfill_aspect(candidate_text, *args, **kwargs):
200208
TaintRange(start=r.start + difflen + 1, length=r.length - 1, source=r.source),
201209
]
202210
)
203-
taint_pyobject_with_ranges(res, tuple(ranges_new))
204-
return res
211+
taint_pyobject_with_ranges(result, tuple(ranges_new))
205212
except Exception as e:
206213
_set_iast_error_metric("IAST propagation error. format_aspect. {}".format(e))
207-
return candidate_text.zfill(*args, **kwargs)
214+
return result
208215

209216

210217
def format_aspect(
211218
candidate_text, # type: str
212219
*args, # type: List[Any]
213220
**kwargs # type: Dict[str, Any]
214221
): # type: (...) -> str
222+
result = candidate_text.format(*args, **kwargs)
223+
215224
if not isinstance(candidate_text, TEXT_TYPES):
216-
return candidate_text.format(*args, **kwargs)
225+
return result
226+
217227
try:
218228
params = tuple(args) + tuple(kwargs.values())
219229
ranges_orig, candidate_text_ranges = are_all_text_all_ranges(candidate_text, params)
220230
if not ranges_orig:
221-
return candidate_text.format(*args, **kwargs)
231+
return result
222232

223233
new_template = as_formatted_evidence(
224234
candidate_text, candidate_text_ranges, tag_mapping_function=TagMappingMode.Mapper
@@ -232,24 +242,25 @@ def format_aspect(
232242
new_args = list(map(fun, args))
233243

234244
new_kwargs = {key: fun(value) for key, value in iteritems(kwargs)}
235-
result = _convert_escaped_text_to_tainted_text(
245+
new_result = _convert_escaped_text_to_tainted_text(
236246
new_template.format(*new_args, **new_kwargs),
237247
ranges_orig=ranges_orig,
238248
)
239-
if result != candidate_text.format(*args):
249+
if new_result != result:
240250
raise Exception(
241251
"format_aspect result %s is different to candidate_text.format %s"
242252
% (result, candidate_text.format(*args))
243253
)
244-
return result
254+
return new_result
245255
except Exception as e:
246256
_set_iast_error_metric("IAST propagation error. format_aspect. {}".format(e))
247-
return candidate_text.format(*args, **kwargs)
257+
return result
248258

249259

250260
def format_map_aspect(candidate_text, *args, **kwargs): # type: (str, Any, Any) -> str
251261
if not isinstance(candidate_text, TEXT_TYPES):
252262
return candidate_text.format_map(*args, **kwargs)
263+
253264
try:
254265
mapping = parse_params(0, "mapping", None, *args, **kwargs)
255266
mapping_tuple = tuple(mapping if not isinstance(mapping, dict) else mapping.values())
@@ -281,6 +292,7 @@ def format_map_aspect(candidate_text, *args, **kwargs): # type: (str, Any, Any)
281292
def repr_aspect(*args, **kwargs):
282293
# type: (Any, Any) -> Any
283294
result = repr(*args, **kwargs)
295+
284296
if isinstance(args[0], TEXT_TYPES) and is_pyobject_tainted(args[0]):
285297
try:
286298
if isinstance(args[0], (bytes, bytearray)):
@@ -379,27 +391,33 @@ def incremental_translation(self, incr_coder, funcode, empty):
379391

380392

381393
def decode_aspect(self, *args, **kwargs):
394+
result = self.decode(*args, **kwargs)
395+
382396
if not is_pyobject_tainted(self) or not isinstance(self, bytes):
383-
return self.decode(*args, **kwargs)
397+
return result
398+
384399
try:
385400
codec = args[0] if args else "utf-8"
386401
inc_dec = codecs.getincrementaldecoder(codec)(**kwargs)
387402
return incremental_translation(self, inc_dec, inc_dec.decode, "")
388403
except Exception as e:
389404
_set_iast_error_metric("IAST propagation error. decode_aspect. {}".format(e))
390-
return self.decode(*args, **kwargs)
405+
return result
391406

392407

393408
def encode_aspect(self, *args, **kwargs):
409+
result = self.encode(*args, **kwargs)
410+
394411
if not is_pyobject_tainted(self) or not isinstance(self, str):
395-
return self.encode(*args, **kwargs)
412+
return result
413+
396414
try:
397415
codec = args[0] if args else "utf-8"
398416
inc_enc = codecs.getincrementalencoder(codec)(**kwargs)
399417
return incremental_translation(self, inc_enc, inc_enc.encode, b"")
400418
except Exception as e:
401419
_set_iast_error_metric("IAST propagation error. encode_aspect. {}".format(e))
402-
return self.encode(*args, **kwargs)
420+
return result
403421

404422

405423
def upper_aspect(candidate_text, *args, **kwargs): # type: (Any, Any, Any) -> TEXT_TYPE

tests/appsec/iast/aspects/test_encode_decode_aspect.py

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,20 @@
33
import pytest
44

55
from ddtrace.appsec.iast._utils import _is_python_version_supported as python_supported_by_iast
6+
from tests.appsec.iast.aspects.conftest import _iast_patched_module
67

78

89
try:
910
from ddtrace.appsec.iast._taint_tracking import OriginType
11+
from ddtrace.appsec.iast._taint_tracking import taint_pyobject
1012
except (ImportError, AttributeError):
1113
pytest.skip("IAST not supported for this Python version", allow_module_level=True)
1214

1315

16+
if python_supported_by_iast():
17+
mod = _iast_patched_module("tests.appsec.iast.fixtures.aspects.str_methods")
18+
19+
1420
def catch_all(fun, args, kwargs):
1521
try:
1622
return True, fun(*args, **kwargs)
@@ -109,3 +115,33 @@ def test_encode_and_add_aspect(infix, args, kwargs, should_be_tainted, prefix, s
109115
assert list_ranges[0].start == len(prefix.encode(*args, **kwargs))
110116
len_infix = len(infix.encode(*args, **kwargs))
111117
assert list_ranges[0].length == len_infix
118+
119+
120+
@pytest.mark.skipif(not python_supported_by_iast(), reason="Python version not supported by IAST")
121+
def test_encode_error_and_no_log_metric(mock_telemetry_lifecycle_writer):
122+
string_input = taint_pyobject(
123+
pyobject="abcde",
124+
source_name="test_add_aspect_tainting_left_hand",
125+
source_value="abcde",
126+
source_origin=OriginType.PARAMETER,
127+
)
128+
with pytest.raises(LookupError):
129+
mod.do_encode(string_input, "encoding-not-exists")
130+
131+
list_metrics_logs = list(mock_telemetry_lifecycle_writer._logs)
132+
assert len(list_metrics_logs) == 0
133+
134+
135+
@pytest.mark.skipif(not python_supported_by_iast(), reason="Python version not supported by IAST")
136+
def test_dencode_error_and_no_log_metric(mock_telemetry_lifecycle_writer):
137+
string_input = taint_pyobject(
138+
pyobject=b"abcde",
139+
source_name="test_add_aspect_tainting_left_hand",
140+
source_value="abcde",
141+
source_origin=OriginType.PARAMETER,
142+
)
143+
with pytest.raises(LookupError):
144+
mod.do_decode(string_input, "decoding-not-exists")
145+
146+
list_metrics_logs = list(mock_telemetry_lifecycle_writer._logs)
147+
assert len(list_metrics_logs) == 0

tests/appsec/iast/aspects/test_format_aspect_fixtures.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,3 +224,10 @@ def test_format_when_tainted_template_range_special_template_then_tainted_result
224224
# escaped_expected_result="a:+-<input2>aaaa<input2>-+:a parameter",
225225
# )
226226
pass
227+
228+
def test_format_key_error_and_no_log_metric(self, mock_telemetry_lifecycle_writer):
229+
with pytest.raises(KeyError):
230+
mod.do_format_key_error("test1")
231+
232+
list_metrics_logs = list(mock_telemetry_lifecycle_writer._logs)
233+
assert len(list_metrics_logs) == 0

tests/appsec/iast/aspects/test_str_aspect.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,14 @@ def test_aspect_ljust_str_tainted(self):
112112
ljusted = mod.do_ljust(string_input, 4) # pylint: disable=no-member
113113
assert as_formatted_evidence(ljusted) == ":+-foo-+: "
114114

115+
def test_aspect_ljust_error_and_no_log_metric(self, mock_telemetry_lifecycle_writer):
116+
string_input = create_taint_range_with_format(":+-foo-+:")
117+
with pytest.raises(TypeError):
118+
mod.do_ljust(string_input, "aaaaa")
119+
120+
list_metrics_logs = list(mock_telemetry_lifecycle_writer._logs)
121+
assert len(list_metrics_logs) == 0
122+
115123
def test_zfill(self):
116124
# Not tainted
117125
string_input = "-1234"
@@ -131,6 +139,14 @@ def test_zfill(self):
131139
res = mod.do_zfill(string_input, 7) # pylint: disable=no-member
132140
assert as_formatted_evidence(res) == "00:+-012-+:34"
133141

142+
def test_aspect_zfill_error_and_no_log_metric(self, mock_telemetry_lifecycle_writer):
143+
string_input = create_taint_range_with_format(":+-foo-+:")
144+
with pytest.raises(TypeError):
145+
mod.do_zfill(string_input, "aaaaa")
146+
147+
list_metrics_logs = list(mock_telemetry_lifecycle_writer._logs)
148+
assert len(list_metrics_logs) == 0
149+
134150
def test_format(self):
135151
# type: () -> None
136152
string_input = "foo"

tests/appsec/iast/fixtures/aspects/str_methods.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -845,6 +845,10 @@ def do_format_map(template, mapping): # type: (str, Dict[str, Any]) -> str
845845
return template.format_map(mapping)
846846

847847

848+
def do_format_key_error(param1): # type: (str, Dict[str, Any]) -> str
849+
return "Test {param1}, {param2}".format(param1=param1) # noqa
850+
851+
848852
def do_join(s, iterable):
849853
# type: (str, Iterable) -> str
850854
return s.join(iterable)

tests/appsec/test_psycopg2.py

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,11 @@
55

66

77
try:
8-
from ddtrace.appsec._iast import oce
9-
from ddtrace.appsec._iast._taint_tracking import OriginType
10-
from ddtrace.appsec._iast._taint_tracking import create_context
11-
from ddtrace.appsec._iast._taint_tracking import is_pyobject_tainted
12-
from ddtrace.appsec._iast._taint_utils import LazyTaintList
8+
from ddtrace.appsec.iast import oce
9+
from ddtrace.appsec.iast._taint_tracking import OriginType
10+
from ddtrace.appsec.iast._taint_tracking import create_context
11+
from ddtrace.appsec.iast._taint_tracking import is_pyobject_tainted
12+
from ddtrace.appsec.iast._taint_utils import LazyTaintList
1313
except (ImportError, AttributeError):
1414
pytest.skip("IAST not supported for this Python version", allow_module_level=True)
1515

0 commit comments

Comments
 (0)