diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index d0e05d1..3893daf 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -19,4 +19,4 @@ owner. -- **Advait Varhade** ([@AdvaitVarhade](https://github.com/AdvaitVarhade)) — SSRF (CWE-918) detection rule for JavaScript/Node ([#73](https://github.com/Signetry/core/pull/73)) +- **Advait Varhade** ([@AdvaitVarhade](https://github.com/AdvaitVarhade)) — SSRF (CWE-918) detection rule for JavaScript/Node ([#73](https://github.com/Signetry/core/pull/73)); SSRF URL-argument resolution for keyword + positional calls, plus httpx/aiohttp coverage ([#89](https://github.com/Signetry/core/pull/89)) diff --git a/signetry_core/pipeline/findings/deterministic.py b/signetry_core/pipeline/findings/deterministic.py index fa7efb4..011964e 100644 --- a/signetry_core/pipeline/findings/deterministic.py +++ b/signetry_core/pipeline/findings/deterministic.py @@ -276,7 +276,11 @@ def visit_Call(self, node: ast.Call) -> None: # noqa: C901 - dispatch table by "requests.delete", "requests.head", "requests.options", "requests.request", "httpx.get", "httpx.post", "httpx.put", "httpx.patch", "httpx.delete", "httpx.head", "httpx.options", "httpx.request", - "urllib.request.urlopen", "urllib.request.Request", + # Sink only. urllib.request.Request() merely *builds* a request; taint + # propagates through `req = Request(tainted)`, so urlopen(req) already + # flags the flow. Listing the constructor too reported one SSRF twice, + # on adjacent lines, where dedup (file, line, category) cannot collapse it. + "urllib.request.urlopen", "aiohttp.request", ) if target in ssrf_targets: diff --git a/tests/test_findings_engine.py b/tests/test_findings_engine.py index 326385c..c4f4dd9 100644 --- a/tests/test_findings_engine.py +++ b/tests/test_findings_engine.py @@ -311,6 +311,23 @@ def test_detects_ssrf_keyword_and_positional_variants(): assert "ssrf" in _cats(src_urllib_req) +def test_urllib_request_flow_is_reported_once_at_the_sink(): + # Request() only *builds* the request; taint propagates through the assignment + # so urlopen() catches the flow. Listing the constructor as a sink too reported + # one SSRF on two adjacent lines, which dedup (file, line, category) cannot + # collapse. Exactly one finding, on the urlopen line. + src = ( + "import urllib.request, flask\n" + "def f():\n" + " dest = flask.request.args.get('dest')\n" + " req = urllib.request.Request(dest)\n" + " return urllib.request.urlopen(req).read()\n" + ) + ssrf = [f for f in scan_source("x.py", src) if f.category == "ssrf"] + assert len(ssrf) == 1, [(f.rule_id, f.line) for f in ssrf] + assert ssrf[0].line == 5 + + def test_no_false_positive_on_constant_url_with_tainted_kwargs(): src_clean_kwargs = ( "import requests, flask\n"