Skip to content

Commit 4607d9e

Browse files
authored
chore(iast): reduce ssrf false positives (#13530)
This PR improves the accuracy of SSRF (Server-Side Request Forgery) vulnerability detection in the IAST module by reducing false positives, particularly when dealing with URL fragments. | URL Part | Tainted? | Safe Builder? | Encoded? | Report SSRF? | |---------------|---------------|------------------------|----------|--------------| | Protocol/Host/Port | Yes | N/A | N/A | Yes (HIGH) | | Path | Yes | Yes | Yes | No | | Path | Yes | No | No | Yes (MEDIUM) | | Query | Yes | Yes | N/A | No | | Query | Yes | No | N/A | Yes (MEDIUM) | | Fragment | Yes | N/A | N/A | No | ### Key Changes - Enhanced SSRF detection logic to properly handle URL fragments (#) - Added validation to ignore cases where tainted data only appears in URL fragments - Added test cases to verify fragment handling behavior ### Motivation URL fragments (parts after #) are client-side only and not sent to the server, making them irrelevant for SSRF vulnerabilities. The current implementation was generating false positives when tainted data appeared only in URL fragments, leading to unnecessary alerts. ### Testing Strategy - Added new test cases in `tests/appsec/iast/taint_sinks/test_ssrf.py` - Verified behavior with Django integration tests - Tested various URL fragment scenarios: - Tainted data in fragments only - Tainted data in both query params and fragments - Multiple fragments with tainted data ## 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)
1 parent c2038a8 commit 4607d9e

File tree

8 files changed

+187
-13
lines changed

8 files changed

+187
-13
lines changed

ddtrace/appsec/_iast/_patch_modules.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
from ddtrace.appsec._iast.secure_marks.sanitizers import path_traversal_sanitizer
77
from ddtrace.appsec._iast.secure_marks.sanitizers import sqli_sanitizer
88
from ddtrace.appsec._iast.secure_marks.sanitizers import xss_sanitizer
9+
from ddtrace.appsec._iast.secure_marks.validators import ssrf_validator
910
from ddtrace.appsec._iast.secure_marks.validators import unvalidated_redirect_validator
1011

1112

@@ -32,6 +33,7 @@ def patch_iast(patch_modules=IAST_PATCH):
3233
for module in (m for m, e in patch_modules.items() if e):
3334
when_imported("hashlib")(_on_import_factory(module, "ddtrace.appsec._iast.taint_sinks.%s", raise_errors=False))
3435

36+
# CMDI sanitizers
3537
when_imported("shlex")(
3638
lambda _: try_wrap_function_wrapper(
3739
"shlex",
@@ -40,6 +42,11 @@ def patch_iast(patch_modules=IAST_PATCH):
4042
)
4143
)
4244

45+
# SSRF
46+
when_imported("django.utils.http")(
47+
lambda _: try_wrap_function_wrapper("django.utils.http", "url_has_allowed_host_and_scheme", ssrf_validator)
48+
)
49+
4350
# SQL sanitizers
4451
when_imported("mysql.connector.conversion")(
4552
lambda _: try_wrap_function_wrapper(

ddtrace/appsec/_iast/secure_marks/validators.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,3 +93,18 @@ def unvalidated_redirect_validator(wrapped: Callable, instance: Any, args: Seque
9393
True if validation passed, False otherwise
9494
"""
9595
return create_validator(VulnerabilityType.UNVALIDATED_REDIRECT, wrapped, instance, args, kwargs)
96+
97+
98+
def ssrf_validator(wrapped: Callable, instance: Any, args: Sequence, kwargs: dict) -> bool:
99+
"""Validator for ssrf functions.
100+
101+
Args:
102+
wrapped: The original validator function
103+
instance: The instance the function is bound to (if any)
104+
args: Positional arguments
105+
kwargs: Keyword arguments
106+
107+
Returns:
108+
True if validation passed, False otherwise
109+
"""
110+
return create_validator(VulnerabilityType.SSRF, wrapped, instance, args, kwargs)

ddtrace/appsec/_iast/taint_sinks/ssrf.py

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,21 +2,19 @@
22

33
from ddtrace.appsec._constants import IAST_SPAN_TAGS
44
from ddtrace.appsec._iast._logs import iast_error
5+
from ddtrace.appsec._iast._logs import iast_propagation_sink_point_debug_log
56
from ddtrace.appsec._iast._metrics import _set_metric_iast_executed_sink
67
from ddtrace.appsec._iast._span_metrics import increment_iast_span_metric
78
from ddtrace.appsec._iast._taint_tracking import VulnerabilityType
9+
from ddtrace.appsec._iast._taint_tracking import get_ranges
810
from ddtrace.appsec._iast.constants import VULN_SSRF
911
from ddtrace.appsec._iast.taint_sinks._base import VulnerabilityBase
10-
from ddtrace.internal.logger import get_logger
1112
from ddtrace.internal.utils import ArgumentError
1213
from ddtrace.internal.utils import get_argument_value
1314
from ddtrace.internal.utils.importlib import func_name
1415
from ddtrace.settings.asm import config as asm_config
1516

1617

17-
log = get_logger(__name__)
18-
19-
2018
class SSRF(VulnerabilityBase):
2119
vulnerability_type = VULN_SSRF
2220
secure_mark = VulnerabilityType.SSRF
@@ -33,24 +31,41 @@ class SSRF(VulnerabilityBase):
3331

3432

3533
def _iast_report_ssrf(func: Callable, *args, **kwargs):
34+
"""
35+
Check and report potential SSRF (Server-Side Request Forgery) vulnerabilities in function calls.
36+
37+
This function analyzes calls to URL-handling functions to detect potential SSRF vulnerabilities.
38+
It checks if the URL argument is tainted (user-controlled) and reports it if conditions are met.
39+
URL fragments (parts after #) are handled specially - if all tainted parts are in the fragment,
40+
no vulnerability is reported.
41+
"""
3642
func_key = func_name(func)
3743
arg_pos, kwarg_name = _FUNC_TO_URL_ARGUMENT.get(func_key, (None, None))
3844
if arg_pos is None:
39-
log.debug("%s not found in list of functions supported for SSRF", func_key)
45+
iast_propagation_sink_point_debug_log("%s not found in list of functions supported for SSRF", func_key)
4046
return
4147

4248
try:
4349
kw = kwarg_name if kwarg_name else ""
4450
report_ssrf = get_argument_value(list(args), kwargs, arg_pos, kw)
4551
except ArgumentError:
46-
log.debug("Failed to get URL argument from _FUNC_TO_URL_ARGUMENT dict for function %s", func_key)
52+
iast_propagation_sink_point_debug_log(
53+
"Failed to get URL argument from _FUNC_TO_URL_ARGUMENT dict for function %s", func_key
54+
)
4755
return
48-
4956
if report_ssrf:
5057
if asm_config.is_iast_request_enabled:
5158
try:
5259
if SSRF.has_quota() and SSRF.is_tainted_pyobject(report_ssrf):
53-
SSRF.report(evidence_value=report_ssrf)
60+
valid_to_report = True
61+
fragment_start = report_ssrf.find("#")
62+
taint_ranges = get_ranges(report_ssrf)
63+
if fragment_start != -1:
64+
# If all taint ranges are in the fragment, do not report
65+
if all(r.start >= fragment_start for r in taint_ranges):
66+
valid_to_report = False
67+
if valid_to_report:
68+
SSRF.report(evidence_value=report_ssrf)
5469

5570
# Reports Span Metrics
5671
_set_metric_iast_executed_sink(SSRF.vulnerability_type)

tests/appsec/iast/taint_sinks/test_ssrf.py

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,6 @@ def _check_no_report_if_deduplicated(num_vuln_expected):
164164

165165
def test_ssrf_requests_deduplication(iast_context_deduplication_enabled):
166166
requests_patch()
167-
_end_iast_context_and_oce()
168167
try:
169168
import requests
170169
from requests.exceptions import ConnectionError # noqa: A004
@@ -187,7 +186,6 @@ def test_ssrf_requests_deduplication(iast_context_deduplication_enabled):
187186

188187
def test_ssrf_urllib3_deduplication(iast_context_deduplication_enabled):
189188
urllib3_patch()
190-
_end_iast_context_and_oce()
191189
try:
192190
for num_vuln_expected in [1, 0, 0]:
193191
_start_iast_context_and_oce()
@@ -209,7 +207,6 @@ def test_ssrf_urllib3_deduplication(iast_context_deduplication_enabled):
209207

210208
def test_ssrf_httplib_deduplication(iast_context_deduplication_enabled):
211209
httplib_patch()
212-
_end_iast_context_and_oce()
213210
try:
214211
import http.client
215212

@@ -233,7 +230,6 @@ def test_ssrf_httplib_deduplication(iast_context_deduplication_enabled):
233230

234231
def test_ssrf_webbrowser_deduplication(iast_context_deduplication_enabled):
235232
webbrowser_patch()
236-
_end_iast_context_and_oce()
237233
try:
238234
import webbrowser
239235

@@ -255,7 +251,6 @@ def test_ssrf_webbrowser_deduplication(iast_context_deduplication_enabled):
255251

256252
def test_ssrf_urllib_deduplication(iast_context_deduplication_enabled):
257253
urllib_patch()
258-
_end_iast_context_and_oce()
259254
try:
260255
import urllib.request
261256

tests/appsec/integrations/django_tests/conftest.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
from ddtrace.appsec._iast import enable_iast_propagation
88
from ddtrace.appsec._iast._patch_modules import patch_iast
99
from ddtrace.contrib.internal.django.patch import patch as django_patch
10+
from ddtrace.contrib.internal.requests.patch import patch as requests_patch
1011
from ddtrace.internal import core
1112
from ddtrace.trace import Pin
1213
from tests.appsec.iast.conftest import _end_iast_context_and_oce
@@ -31,6 +32,7 @@ def pytest_configure():
3132
), override_env(dict(_DD_IAST_PATCH_MODULES="tests.appsec.integrations")):
3233
settings.DEBUG = False
3334
patch_iast()
35+
requests_patch()
3436
django_patch()
3537
enable_iast_propagation()
3638
django.setup()

tests/appsec/integrations/django_tests/django_app/urls.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@ def shutdown(request):
6969
views.unvalidated_redirect_path_multiple_sources,
7070
name="unvalidated_redirect_path_multiple_sources",
7171
),
72+
handler("appsec/ssrf_requests/$", views.ssrf_requests, name="ssrf_requests"),
7273
handler("appsec/taint-checking-enabled/$", views.taint_checking_enabled_view, name="taint_checking_enabled_view"),
7374
handler(
7475
"appsec/taint-checking-disabled/$", views.taint_checking_disabled_view, name="taint_checking_disabled_view"

tests/appsec/integrations/django_tests/django_app/views.py

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,15 @@
44

55
import hashlib
66
from html import escape
7+
import json
78
import os
89
from pathlib import Path
910
from pathlib import PosixPath
1011
import shlex
1112
import subprocess
1213
from typing import Any
14+
import urllib
15+
from urllib.parse import quote
1316

1417
from django.db import connection
1518
from django.http import HttpResponse
@@ -18,6 +21,8 @@
1821
from django.shortcuts import render
1922
from django.utils.http import url_has_allowed_host_and_scheme
2023
from django.utils.safestring import mark_safe
24+
import requests
25+
from requests.exceptions import ConnectionError # noqa: A004
2126

2227
from ddtrace.appsec import _asm_request_context
2328
from ddtrace.appsec._iast._taint_tracking import OriginType
@@ -472,3 +477,65 @@ def signup(request):
472477
User.objects.create_user(username=login, password=passwd)
473478
return HttpResponse("OK", status=200)
474479
return HttpResponse("Error", status=400)
480+
481+
482+
def ssrf_requests(request):
483+
value = request.GET.get("url")
484+
option = request.GET.get("option")
485+
try:
486+
if option == "path":
487+
# label ssrf_requests_path
488+
_ = requests.get(f"http://localhost:8080/{value}", timeout=1)
489+
elif option == "protocol":
490+
# label ssrf_requests_protocol
491+
_ = requests.get(f"{value}://localhost:8080/", timeout=1)
492+
elif option == "host":
493+
# label ssrf_requests_host
494+
_ = requests.get(f"http://{value}:8080/", timeout=1)
495+
elif option == "query":
496+
# label ssrf_requests_query
497+
_ = requests.get(f"http://localhost:8080/?{value}", timeout=1)
498+
elif option == "query_with_fragment":
499+
# label ssrf_requests_query_with_fragment
500+
_ = requests.get(f"http://localhost:8080/?{value}", timeout=1)
501+
elif option == "port":
502+
# label ssrf_requests_port
503+
_ = requests.get(f"http://localhost:{value}/", timeout=1)
504+
elif option == "fragment1":
505+
_ = requests.get(f"http://localhost:8080/#section1={value}", timeout=1)
506+
elif option == "fragment2":
507+
_ = requests.get(f"http://localhost:8080/?param1=value1&param2=value2#section2={value}", timeout=1)
508+
elif option == "fragment3":
509+
_ = requests.get(
510+
"http://localhost:8080/path-to-something/object_identifier?"
511+
f"param1=value1&param2=value2#section3={value}",
512+
timeout=1,
513+
)
514+
elif option == "query_param":
515+
_ = requests.get("http://localhost:8080/", params={"param1": value}, timeout=1)
516+
elif option == "urlencode_single":
517+
params = urllib.parse.urlencode({"key1": value})
518+
_ = requests.get(f"http://localhost:8080/?{params}", timeout=1)
519+
elif option == "urlencode_multiple":
520+
params = urllib.parse.urlencode({"key1": value, "key2": "static_value", "key3": "another_value"})
521+
_ = requests.get(f"http://localhost:8080/?{params}", timeout=1)
522+
elif option == "urlencode_nested":
523+
nested_data = {"user": value, "filters": {"type": "report", "format": "json"}}
524+
params = urllib.parse.urlencode({"data": json.dumps(nested_data)})
525+
_ = requests.get(f"http://localhost:8080/?{params}", timeout=1)
526+
elif option == "urlencode_with_fragment":
527+
params = urllib.parse.urlencode({"search": value})
528+
_ = requests.get(f"http://localhost:8080/?{params}#results", timeout=1)
529+
elif option == "urlencode_doseq":
530+
params = urllib.parse.urlencode({"ids": [value, "id2", "id3"]}, doseq=True)
531+
_ = requests.get(f"http://localhost:8080/?{params}", timeout=1)
532+
elif option == "safe_host":
533+
if url_has_allowed_host_and_scheme(value, allowed_hosts={request.get_host()}):
534+
_ = requests.get(f"http://{value}:8080/", timeout=1)
535+
_ = requests.get(f"http://{value}:8080/", timeout=1)
536+
elif option == "safe_path":
537+
safe_path = quote(value)
538+
_ = requests.get(f"http://localhost:8080/{safe_path}", timeout=1)
539+
except ConnectionError:
540+
pass
541+
return HttpResponse("OK", status=200)

tests/appsec/integrations/django_tests/test_iast_django.py

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
from ddtrace.appsec._iast.constants import VULN_HEADER_INJECTION
1111
from ddtrace.appsec._iast.constants import VULN_INSECURE_COOKIE
1212
from ddtrace.appsec._iast.constants import VULN_SQL_INJECTION
13+
from ddtrace.appsec._iast.constants import VULN_SSRF
1314
from ddtrace.appsec._iast.constants import VULN_STACKTRACE_LEAK
1415
from ddtrace.appsec._iast.constants import VULN_UNVALIDATED_REDIRECT
1516
from ddtrace.settings.asm import config as asm_config
@@ -1467,3 +1468,74 @@ def test_django_iast_sampling_by_route_method(client, test_spans_2_vuln_per_requ
14671468
assert (
14681469
len(list_vulnerabilities) == 16
14691470
), f"Num vulnerabilities: ({len(list_vulnerabilities)}): {list_vulnerabilities}"
1471+
1472+
1473+
@pytest.mark.skipif(not asm_config._iast_supported, reason="Python version not supported by IAST")
1474+
def test_django_ssrf_safe_path(client, iast_span, tracer):
1475+
tainted_value = "path_param"
1476+
root_span, _ = _aux_appsec_get_root_span(
1477+
client, iast_span, tracer, url=f"/appsec/ssrf_requests/?url={tainted_value}"
1478+
)
1479+
loaded = root_span.get_tag(IAST.JSON)
1480+
assert loaded is None
1481+
1482+
1483+
@pytest.mark.parametrize(
1484+
("option", "url", "value_parts"),
1485+
[
1486+
("path", "url-path/", [{"value": "http://localhost:8080/"}, {"value": "url-path/", "source": 0}]),
1487+
("safe_path", "url-path/", None),
1488+
("protocol", "http", [{"value": "http", "source": 0}, {"value": "://localhost:8080/"}]),
1489+
("host", "localhost", [{"value": "http://"}, {"value": "localhost", "source": 0}, {"value": ":8080/"}]),
1490+
("urlencode_single", "value1", None),
1491+
("urlencode_multiple", "value1", None),
1492+
("urlencode_nested", "value1", None),
1493+
("urlencode_with_fragment", "value1", None),
1494+
("urlencode_doseq", "value1", None),
1495+
("safe_host", "localhost", None),
1496+
("port", "8080", [{"value": "http://localhost:"}, {"value": "8080", "source": 0}, {"value": "/"}]),
1497+
(
1498+
"query",
1499+
"param1=value1&param2=value2",
1500+
[
1501+
{"value": "http://localhost:8080/?"},
1502+
{"source": 0, "value": "param1="},
1503+
{"redacted": True, "source": 0, "pattern": "hijklm"},
1504+
],
1505+
),
1506+
(
1507+
"query_with_fragment",
1508+
"param1=value_with_%23hash%23&param2=value2",
1509+
[
1510+
{"value": "http://localhost:8080/?"},
1511+
{"source": 0, "value": "param1="},
1512+
{"redacted": True, "source": 0, "pattern": "hijklmnopqr"},
1513+
{"source": 0, "value": "#hash#"},
1514+
],
1515+
),
1516+
("fragment1", "fragment_value1", None),
1517+
("fragment2", "fragment_value1", None),
1518+
("fragment3", "fragment_value1", None),
1519+
("query_param", "param1=value1&param2=value2", None),
1520+
],
1521+
)
1522+
def test_django_ssrf_url(client, iast_span, tracer, option, url, value_parts):
1523+
root_span, response = _aux_appsec_get_root_span(
1524+
client, iast_span, tracer, url=f"/appsec/ssrf_requests/?option={option}&url={url}"
1525+
)
1526+
1527+
assert response.status_code == 200
1528+
assert response.content == b"OK"
1529+
1530+
if value_parts is None:
1531+
assert root_span.get_tag(IAST.JSON) is None
1532+
else:
1533+
loaded = json.loads(root_span.get_tag(IAST.JSON))
1534+
1535+
line, hash_value = get_line_and_hash(f"ssrf_requests_{option}", VULN_SSRF, filename=TEST_FILE)
1536+
1537+
assert loaded["vulnerabilities"][0]["type"] == VULN_SSRF
1538+
assert loaded["vulnerabilities"][0]["evidence"] == {"valueParts": value_parts}
1539+
assert loaded["vulnerabilities"][0]["location"]["path"] == TEST_FILE
1540+
assert loaded["vulnerabilities"][0]["location"]["line"] == line
1541+
assert loaded["vulnerabilities"][0]["hash"] == hash_value

0 commit comments

Comments
 (0)