Skip to content

Commit 9384499

Browse files
fix(iast): report cookies [backport 2.2] (#7639)
Backport 4309dae from #7599 to 2.2. IAST is independent of Appsec; `iast_enabled` doesn't require `asm_enabled`. With that, this PR introduces a small refactor when both IAST and ASM check the request cookies. ## 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. Co-authored-by: Alberto Vara <[email protected]>
1 parent 57653ee commit 9384499

File tree

4 files changed

+73
-50
lines changed

4 files changed

+73
-50
lines changed

ddtrace/contrib/trace_utils.py

Lines changed: 36 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -515,43 +515,42 @@ def set_http_meta(
515515
if retries_remain is not None:
516516
span.set_tag_str(http.RETRIES_REMAIN, str(retries_remain))
517517

518-
if asm_config._asm_enabled:
519-
from ddtrace.appsec._iast._utils import _is_iast_enabled
520-
521-
if _is_iast_enabled():
522-
from ddtrace.appsec._iast.taint_sinks.insecure_cookie import asm_check_cookies
523-
524-
if request_cookies:
525-
asm_check_cookies(request_cookies)
526-
527-
if response_cookies:
528-
asm_check_cookies(response_cookies)
529-
530-
if span.span_type == SpanTypes.WEB:
531-
from ddtrace.appsec._asm_request_context import set_waf_address
532-
from ddtrace.appsec._constants import SPAN_DATA_NAMES
533-
534-
status_code = str(status_code) if status_code is not None else None
535-
536-
addresses = {
537-
k: v
538-
for k, v in [
539-
(SPAN_DATA_NAMES.REQUEST_URI_RAW, raw_uri),
540-
(SPAN_DATA_NAMES.REQUEST_METHOD, method),
541-
(SPAN_DATA_NAMES.REQUEST_COOKIES, request_cookies),
542-
(SPAN_DATA_NAMES.REQUEST_QUERY, parsed_query),
543-
(SPAN_DATA_NAMES.REQUEST_HEADERS_NO_COOKIES, request_headers),
544-
(SPAN_DATA_NAMES.RESPONSE_HEADERS_NO_COOKIES, response_headers),
545-
(SPAN_DATA_NAMES.RESPONSE_STATUS, status_code),
546-
(SPAN_DATA_NAMES.REQUEST_PATH_PARAMS, request_path_params),
547-
(SPAN_DATA_NAMES.REQUEST_BODY, request_body),
548-
(SPAN_DATA_NAMES.REQUEST_HTTP_IP, request_ip),
549-
(SPAN_DATA_NAMES.REQUEST_ROUTE, route),
550-
]
551-
if v is not None
552-
}
553-
for k, v in addresses.items():
554-
set_waf_address(k, v, span)
518+
from ddtrace.appsec._iast._utils import _is_iast_enabled
519+
520+
if _is_iast_enabled():
521+
from ddtrace.appsec._iast.taint_sinks.insecure_cookie import asm_check_cookies
522+
523+
if request_cookies:
524+
asm_check_cookies(request_cookies)
525+
526+
if response_cookies:
527+
asm_check_cookies(response_cookies)
528+
529+
if asm_config._asm_enabled and span.span_type == SpanTypes.WEB:
530+
from ddtrace.appsec._asm_request_context import set_waf_address
531+
from ddtrace.appsec._constants import SPAN_DATA_NAMES
532+
533+
status_code = str(status_code) if status_code is not None else None
534+
535+
addresses = {
536+
k: v
537+
for k, v in [
538+
(SPAN_DATA_NAMES.REQUEST_URI_RAW, raw_uri),
539+
(SPAN_DATA_NAMES.REQUEST_METHOD, method),
540+
(SPAN_DATA_NAMES.REQUEST_COOKIES, request_cookies),
541+
(SPAN_DATA_NAMES.REQUEST_QUERY, parsed_query),
542+
(SPAN_DATA_NAMES.REQUEST_HEADERS_NO_COOKIES, request_headers),
543+
(SPAN_DATA_NAMES.RESPONSE_HEADERS_NO_COOKIES, response_headers),
544+
(SPAN_DATA_NAMES.RESPONSE_STATUS, status_code),
545+
(SPAN_DATA_NAMES.REQUEST_PATH_PARAMS, request_path_params),
546+
(SPAN_DATA_NAMES.REQUEST_BODY, request_body),
547+
(SPAN_DATA_NAMES.REQUEST_HTTP_IP, request_ip),
548+
(SPAN_DATA_NAMES.REQUEST_ROUTE, route),
549+
]
550+
if v is not None
551+
}
552+
for k, v in addresses.items():
553+
set_waf_address(k, v, span)
555554

556555
if route is not None:
557556
span.set_tag_str(http.ROUTE, route)
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
fixes:
3+
- |
4+
Vulnerability Management for Code-level (IAST): Generates cookies vulnerabilities report if IAST is enabled.
5+
Before this fix, Cookies vulnerabilities were only generated if both IAST and Appsec were enabled.

tests/contrib/django/test_django_appsec_iast.py

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
from ddtrace.appsec._iast import oce
99
from ddtrace.appsec._iast._patch_modules import patch_iast
1010
from ddtrace.appsec._iast._utils import _is_python_version_supported as python_supported_by_iast
11+
from ddtrace.appsec._iast.constants import VULN_SQL_INJECTION
1112
from ddtrace.internal.compat import urlencode
1213
from ddtrace.settings.asm import config as asm_config
1314
from tests.appsec.iast.iast_utils import get_line_and_hash
@@ -336,20 +337,27 @@ def test_django_tainted_user_agent_iast_enabled_sqli_http_cookies_name(client, t
336337
url="/appsec/sqli_http_request_cookie_name/",
337338
cookies={"master": "test/1.2.3"},
338339
)
339-
vuln_type = "SQL_INJECTION"
340340

341341
loaded = json.loads(root_span.get_tag(IAST.JSON))
342342

343-
line, hash_value = get_line_and_hash("iast_enabled_sqli_http_cookies_name", vuln_type, filename=TEST_FILE)
343+
line, hash_value = get_line_and_hash(
344+
"iast_enabled_sqli_http_cookies_name", VULN_SQL_INJECTION, filename=TEST_FILE
345+
)
346+
347+
vulnerability = False
348+
for vuln in loaded["vulnerabilities"]:
349+
if vuln["type"] == VULN_SQL_INJECTION:
350+
vulnerability = vuln
351+
352+
assert vulnerability, "No {} reported".format(VULN_SQL_INJECTION)
344353

345354
assert loaded["sources"] == [{"origin": "http.request.cookie.name", "name": "master", "value": "master"}]
346-
assert loaded["vulnerabilities"][0]["type"] == vuln_type
347-
assert loaded["vulnerabilities"][0]["hash"] == hash_value
348-
assert loaded["vulnerabilities"][0]["evidence"] == {
355+
assert vulnerability["hash"] == hash_value
356+
assert vulnerability["evidence"] == {
349357
"valueParts": [{"value": "SELECT 1 FROM sqlite_"}, {"source": 0, "value": "master"}]
350358
}
351-
assert loaded["vulnerabilities"][0]["location"]["path"] == TEST_FILE
352-
assert loaded["vulnerabilities"][0]["location"]["line"] == line
359+
assert vulnerability["location"]["path"] == TEST_FILE
360+
assert vulnerability["location"]["line"] == line
353361

354362
assert response.status_code == 200
355363
assert response.content == b"test/1.2.3"
@@ -393,14 +401,20 @@ def test_django_tainted_user_agent_iast_enabled_sqli_http_cookies_value(client,
393401

394402
line, hash_value = get_line_and_hash("iast_enabled_sqli_http_cookies_value", vuln_type, filename=TEST_FILE)
395403

404+
vulnerability = False
405+
for vuln in loaded["vulnerabilities"]:
406+
if vuln["type"] == VULN_SQL_INJECTION:
407+
vulnerability = vuln
408+
409+
assert vulnerability, "No {} reported".format(VULN_SQL_INJECTION)
396410
assert loaded["sources"] == [{"origin": "http.request.cookie.value", "name": "master", "value": "master"}]
397-
assert loaded["vulnerabilities"][0]["type"] == "SQL_INJECTION"
398-
assert loaded["vulnerabilities"][0]["hash"] == hash_value
399-
assert loaded["vulnerabilities"][0]["evidence"] == {
411+
assert vulnerability["type"] == "SQL_INJECTION"
412+
assert vulnerability["hash"] == hash_value
413+
assert vulnerability["evidence"] == {
400414
"valueParts": [{"value": "SELECT 1 FROM sqlite_"}, {"source": 0, "value": "master"}]
401415
}
402-
assert loaded["vulnerabilities"][0]["location"]["line"] == line
403-
assert loaded["vulnerabilities"][0]["location"]["path"] == TEST_FILE
416+
assert vulnerability["location"]["line"] == line
417+
assert vulnerability["location"]["path"] == TEST_FILE
404418

405419
assert response.status_code == 200
406420
assert response.content == b"master"

tests/contrib/flask/test_flask_appsec_iast.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -371,7 +371,12 @@ def test_sqli():
371371
VULN_SQL_INJECTION,
372372
filename=TEST_FILE_PATH,
373373
)
374-
vulnerability = loaded["vulnerabilities"][0]
374+
vulnerability = False
375+
for vuln in loaded["vulnerabilities"]:
376+
if vuln["type"] == VULN_SQL_INJECTION:
377+
vulnerability = vuln
378+
379+
assert vulnerability, "No {} reported".format(VULN_SQL_INJECTION)
375380
assert vulnerability["type"] == VULN_SQL_INJECTION
376381
assert vulnerability["evidence"] == {
377382
"valueParts": [{"value": "SELECT 1 FROM "}, {"value": "sqlite_master", "source": 0}]

0 commit comments

Comments
 (0)