Skip to content

Commit 7696cf3

Browse files
fix(iast): avoid reporting line number 0 [backport 2.5] (#8553)
Backport 84b3d5e from #8549 to 2.5. IAST: This fix addresses an issue where a vulnerability would be reported at line 0 if we couldn't extract the proper line number, whereas the default line number should be -1. ## 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] Risks are described (performance impact, potential for breakage, maintainability) - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] [Library release note guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html) are followed or label `changelog/no-changelog` is set - [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)) - [x] If this PR changes the public interface, I've notified `@DataDog/apm-tees`. - [x] If change 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`. ## Reviewer Checklist - [x] Title is accurate - [x] All changes are related to the pull request's stated goal - [x] Description motivates each change - [x] Avoids breaking [API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces) changes - [x] Testing strategy adequately addresses listed risks - [x] Change is maintainable (easy to change, telemetry, documentation) - [x] Release note makes sense to a user of the library - [x] Author has 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) Co-authored-by: Federico Mon <[email protected]>
1 parent 1f58d14 commit 7696cf3

File tree

6 files changed

+45
-9
lines changed

6 files changed

+45
-9
lines changed

ddtrace/appsec/_iast/_stacktrace.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -122,8 +122,8 @@ get_file_and_line(PyObject* Py_UNUSED(module), PyObject* cwd_obj)
122122
return result;
123123

124124
exit_0:; // fix: "a label can only be part of a statement and a declaration is not a statement" error
125-
// Return "", 0
126-
PyObject* line_obj = Py_BuildValue("i", 0);
125+
// Return "", -1
126+
PyObject* line_obj = Py_BuildValue("i", -1);
127127
filename_o = PyUnicode_FromString("");
128128
result = PyTuple_Pack(2, filename_o, line_obj);
129129
Py_DecRef(cwd_bytes);

ddtrace/appsec/_iast/taint_sinks/_base.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,9 @@ def wrapper(wrapped, instance, args, kwargs):
9494
@classmethod
9595
@taint_sink_deduplication
9696
def _prepare_report(cls, span, vulnerability_type, evidence, file_name, line_number, sources):
97+
if line_number is not None and (line_number == 0 or line_number < -1):
98+
line_number = -1
99+
97100
report = core.get_item(IAST.CONTEXT_KEY, span=span)
98101
if report:
99102
report.vulnerabilities.add(
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
fixes:
3+
- |
4+
Vulnerability Management for Code-level (IAST): This fix addresses an issue where a vulnerability would be reported at line 0 if we couldn't extract the proper line number, whereas the default line number should be -1.

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,13 @@
55
import os
66

77

8-
def parametrized_week_hash(hash_func, method):
8+
def parametrized_weak_hash(hash_func, method):
99
import hashlib
1010

1111
m = getattr(hashlib, hash_func)()
1212
m.update(b"Nobody inspects")
1313
m.update(b" the spammish repetition")
14-
# label parametrized_week_hash
14+
# label parametrized_weak_hash
1515
getattr(m, method)()
1616

1717

tests/appsec/iast/iast_utils.py

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,13 @@ def get_line(label, filename=None):
1111
raise AssertionError("label %s not found" % label)
1212

1313

14-
def get_line_and_hash(label, vuln_type, filename=None):
14+
def get_line_and_hash(label, vuln_type, filename=None, fixed_line=None):
1515
"""return the line number and the associated vulnerability hash for `label` and source file `filename`"""
1616

17-
line = get_line(label, filename=filename)
17+
if fixed_line is not None:
18+
line = fixed_line
19+
else:
20+
line = get_line(label, filename=filename)
1821
rep = "Vulnerability(type='%s', location=Location(path='%s', line=%s))" % (vuln_type, filename, line)
1922
hash_value = zlib.crc32(rep.encode())
2023

tests/appsec/iast/taint_sinks/test_weak_hash.py

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
from ddtrace.appsec._iast.taint_sinks.weak_hash import unpatch_iast
1010
from ddtrace.internal import core
1111
from tests.appsec.iast.fixtures.taint_sinks.weak_algorithms import hashlib_new
12-
from tests.appsec.iast.fixtures.taint_sinks.weak_algorithms import parametrized_week_hash
12+
from tests.appsec.iast.fixtures.taint_sinks.weak_algorithms import parametrized_weak_hash
1313
from tests.appsec.iast.iast_utils import get_line_and_hash
1414
from tests.utils import override_env
1515

@@ -28,10 +28,10 @@
2828
],
2929
)
3030
def test_weak_hash_hashlib(iast_span_defaults, hash_func, method):
31-
parametrized_week_hash(hash_func, method)
31+
parametrized_weak_hash(hash_func, method)
3232

3333
line, hash_value = get_line_and_hash(
34-
"parametrized_week_hash", VULN_INSECURE_HASHING_TYPE, filename=WEAK_ALGOS_FIXTURES_PATH
34+
"parametrized_weak_hash", VULN_INSECURE_HASHING_TYPE, filename=WEAK_ALGOS_FIXTURES_PATH
3535
)
3636

3737
span_report = core.get_item(IAST.CONTEXT_KEY, span=iast_span_defaults)
@@ -42,6 +42,32 @@ def test_weak_hash_hashlib(iast_span_defaults, hash_func, method):
4242
assert list(span_report.vulnerabilities)[0].hash == hash_value
4343

4444

45+
@pytest.mark.parametrize(
46+
"hash_func,method,fake_line",
47+
[
48+
("md5", "hexdigest", 0),
49+
("md5", "hexdigest", -100),
50+
("md5", "hexdigest", -1),
51+
],
52+
)
53+
def test_ensure_line_reported_is_minus_one_for_edge_cases(iast_span_defaults, hash_func, method, fake_line):
54+
with mock.patch(
55+
"ddtrace.appsec._iast.taint_sinks._base.get_info_frame", return_value=(WEAK_ALGOS_FIXTURES_PATH, fake_line)
56+
):
57+
parametrized_weak_hash(hash_func, method)
58+
59+
_, hash_value = get_line_and_hash(
60+
"parametrized_weak_hash", VULN_INSECURE_HASHING_TYPE, filename=WEAK_ALGOS_FIXTURES_PATH, fixed_line=-1
61+
)
62+
63+
span_report = core.get_item(IAST.CONTEXT_KEY, span=iast_span_defaults)
64+
assert list(span_report.vulnerabilities)[0].type == VULN_INSECURE_HASHING_TYPE
65+
assert list(span_report.vulnerabilities)[0].location.path == WEAK_ALGOS_FIXTURES_PATH
66+
assert list(span_report.vulnerabilities)[0].location.line == -1
67+
assert list(span_report.vulnerabilities)[0].evidence.value == hash_func
68+
assert list(span_report.vulnerabilities)[0].hash == hash_value
69+
70+
4571
@pytest.mark.parametrize("hash_func", ["md5", "sha1"])
4672
def test_weak_hash_hashlib_no_digest(iast_span_md5_and_sha1_configured, hash_func):
4773
import hashlib

0 commit comments

Comments
 (0)