diff --git a/CHANGELOG.md b/CHANGELOG.md index daaf788..11698dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Precision + +- `open-redirect` judges a flow on the destination a browser resolves from the target's constant text, with its own criteria rather than SSRF's. A reference on the current site keeps the browser there, so the flow is refuted. Such a reference is `/` followed by a character that is not a slash, a backslash, a space or a control character, as in `"/profile/" + id`; or `?`, `#`, or a relative path whose first segment holds no `:`. A fixed absolute host makes the flow a hotspot, since the input chooses the path, where that host may redirect again. Everything else stays a vulnerability, including `"/" + next`: `//evil.com` is another host. The URL facts both rules read are one shared proof, `coretrace_python.taint.urls`, and `ssrf` now reads it too, with the same verdicts. The regression corpus is unchanged: its three `open-redirect` findings redirect to values built without constant text (#168). + ## 0.14.0 (2026-09-26) ### Precision diff --git a/docs/plugins.md b/docs/plugins.md index c1d453a..ec46911 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -117,7 +117,9 @@ that must not leave the application through a log, a third party or plain storag A rule may know more about its own sinks than the refutation does: `judge(ctx, function, flow, verdict)` returns the rule's verdict on a flow, given the refutation's, and by -default returns it unchanged. `ssrf` reads there how the URL is built. The refutation's +default returns it unchanged. `ssrf` and `open-redirect` read there what the URL's +constant text proves, a proof they share (`coretrace_python.taint.urls`) and each +judges by its own criteria. The refutation's verdict, which other consumers of the flow such as the advisory correlation read, does not change; a rule reading more analyses declares them in its `requires`. diff --git a/docs/usage.md b/docs/usage.md index 587d634..d9375a0 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -117,7 +117,7 @@ variable is not treated that way yet. | `ssrf` | The URL of an HTTP client request (Requests, httpx, `urllib`), by position or as `url=`; the body, headers and query parameters are not the destination. How `+` and f-strings build the URL can prove part of its destination. When constant text fixes `scheme://host` and ends the authority with `/`, `?` or `#` before any input (`"https://api.example.com/users/" + id`, or a module-level string bound once and never rebound), the flow is a hotspot: the input still chooses the path, which may reach a sensitive resource, and a redirect may lead elsewhere. With the path fixed too, the input reaching the query or fragment only, and the call disabling redirects (`allow_redirects=False`, `follow_redirects=False`), the destination is fixed and the flow is refuted. Anything unproven stays a vulnerability: a base of unknown value (imported, a call result), a constant leaving the authority open (`"https:/"`, no `/` after the host, a variable port), `urljoin`, `%`, `format`, a URL built in a helper function. | | `xss` | An HTTP response body built without escaping. A Django template rendered by `render_to_string` escapes, where the analyzer can establish it (below). | | `insecure-deserialization` | `pickle`, `marshal`, `dill`, `jsonpickle`, `shelve`, and the YAML loaders (`yaml.load`, `load_all`, `full_load`, `unsafe_load` and their several-document versions); `yaml.load` and `load_all` given a safe loader (`SafeLoader`, `BaseLoader`) are not. | -| `open-redirect` | An HTTP redirect target. | +| `open-redirect` | An HTTP redirect target, judged on the destination a browser resolves from the target's constant text, as `+` and f-strings build it. A reference on the current site keeps the browser there, where any further redirect is the project's own code, analysed on its own, so the flow is refuted. Such a reference is `/` followed by a character that is not a slash, a backslash, a space or a control character (`"/profile/" + id`), `?`, `#`, or a relative path whose first segment ends in the text without a `:`. A fixed absolute host (`"https://example.com/" + path`) sends the browser there, but the input chooses the path, where that host may redirect again: a hotspot. Anything else stays a vulnerability. That covers `"/" + next`, since `//evil.com` is another host, a host left open, a base of unknown value and a target built in a helper function. | | `plaintext-credential-storage` | A parameter named like a password stored in a database without hashing. Medium confidence, since a name is a hint. | | `exploitable-vulnerability` | An API affected by an advisory of a vulnerable requirement, see Dependencies. | diff --git a/src/coretrace_python/bundled/security/open_redirect/open_redirect.py b/src/coretrace_python/bundled/security/open_redirect/open_redirect.py index 9a2c6a9..37c83d3 100644 --- a/src/coretrace_python/bundled/security/open_redirect/open_redirect.py +++ b/src/coretrace_python/bundled/security/open_redirect/open_redirect.py @@ -1,12 +1,26 @@ -"""Open redirect: attacker-controlled input reaching a open redirect sink.""" +"""Open redirect: attacker-controlled input reaching a open redirect sink. + +The rule draws its verdict from what the target's constant text proves (``taint.urls``), +as a browser resolves it. A reference on the current site (``/profile/``, ``?page=``, +``profile/``) keeps the browser on the site, where any further redirect is the project's +own code, analysed on its own: refuted. A fixed absolute host sends the browser there, +but the input chooses the path, where that host may redirect again: a hotspot. Anything +unproven, ``"/" + next`` included, stays a vulnerability. +""" from __future__ import annotations from typing import ClassVar +from coretrace_python.abstract.strings import ModuleStringsAnalysis +from coretrace_python.analysis import AnyAnalysis from coretrace_python.findings import Severity -from coretrace_python.plugins import TaintDetector -from coretrace_python.taint import TaintKind +from coretrace_python.findings.refutation import Status, Verdict +from coretrace_python.hir import nodes +from coretrace_python.ir.ssa import SSAAnalysis +from coretrace_python.plugins import PluginContext, TaintDetector +from coretrace_python.taint import TaintFlow, TaintKind +from coretrace_python.taint.urls import flow_url class OpenRedirectPlugin(TaintDetector): @@ -15,3 +29,19 @@ class OpenRedirectPlugin(TaintDetector): kind: ClassVar[TaintKind] = TaintKind.REDIRECT severity: ClassVar[Severity] = Severity.MEDIUM title: ClassVar[str] = "Open redirect" + requires: ClassVar[frozenset[AnyAnalysis]] = TaintDetector.requires | {SSAAnalysis, ModuleStringsAnalysis} + + def judge(self, ctx: PluginContext, function: nodes.Function, flow: TaintFlow, verdict: Verdict) -> Verdict: + ssa = ctx.get(SSAAnalysis, function) + defs = {i.result: i for block in ssa.blocks for i in block.instructions if i.result is not None} + target = flow_url(flow, defs, ctx.get(ModuleStringsAnalysis)) + if target.relative: + return Verdict(flow, Status.REFUTED, f"the browser stays on the site: the target starts with {target.text!r}") + if target.origin is not None and verdict.status is Status.VULNERABILITY: + return Verdict( + flow, + Status.HOTSPOT, + f"the browser goes to the host fixed by {target.origin!r}, but the input chooses the path, " + "where that host may redirect again", + ) + return verdict diff --git a/src/coretrace_python/bundled/security/open_redirect/plugin.toml b/src/coretrace_python/bundled/security/open_redirect/plugin.toml index db3564c..9b32ff2 100644 --- a/src/coretrace_python/bundled/security/open_redirect/plugin.toml +++ b/src/coretrace_python/bundled/security/open_redirect/plugin.toml @@ -1,7 +1,7 @@ name = "open-redirect" version = "1.0.0" plugin_api = ">=1,<2" -requires = ["taint.flows", "findings.refutation"] +requires = ["taint.flows", "findings.refutation", "ir.ssa", "abstract.module_strings"] provides = ["vulnerability.open-redirect"] [entrypoint] diff --git a/src/coretrace_python/bundled/security/ssrf/ssrf.py b/src/coretrace_python/bundled/security/ssrf/ssrf.py index bc57564..aa4ed5a 100644 --- a/src/coretrace_python/bundled/security/ssrf/ssrf.py +++ b/src/coretrace_python/bundled/security/ssrf/ssrf.py @@ -1,21 +1,18 @@ """Server-side request forgery: attacker-controlled input reaching a SSRF sink. -How the URL is built, with ``+`` and f-strings, can prove part of its destination. Constant -text fixing ``scheme://host`` and ending the authority with ``/``, ``?`` or ``#`` before -any input keeps the input from choosing the host; the input still chooses the path, which -may reach a sensitive resource of that host, and a redirect may lead elsewhere, so the -flow is a hotspot. With the path fixed too, the input reaching the query or fragment -only, and redirects disabled in the call, the destination is fixed and the flow is -refuted. Anything unproven, a base of unknown value or a URL built in a helper function, -stays a vulnerability. +The rule draws its verdict from what the URL's constant text proves (``taint.urls``). +A fixed host keeps the input from choosing it, but the input still chooses the path, +which may reach a sensitive resource of that host, and a redirect may lead elsewhere: a +hotspot. With the path fixed too, the input reaching the query or fragment only, and +redirects disabled in the call, the destination is fixed: refuted. Anything unproven, +a base of unknown value or a URL built in a helper function, stays a vulnerability. """ from __future__ import annotations -import re from typing import ClassVar -from coretrace_python.abstract.strings import ModuleStringsAnalysis, leading_text +from coretrace_python.abstract.strings import ModuleStringsAnalysis from coretrace_python.analysis import AnyAnalysis from coretrace_python.findings import Severity from coretrace_python.findings.refutation import Status, Verdict @@ -23,15 +20,7 @@ from coretrace_python.ir.ssa import SSAAnalysis from coretrace_python.plugins import PluginContext, TaintDetector from coretrace_python.taint import TaintFlow, TaintKind - -# ``scheme://`` and an authority ended by ``/``, ``?`` or ``#``. A backslash, which some -# URL parsers take for a slash, does not end it here. -_FIXED_HOST = re.compile(r"[A-Za-z][A-Za-z0-9+.-]*://[^/?#\\]+[/?#]") -# The same with the path fixed too: what follows is query or fragment. -_FIXED_PATH = re.compile(r"[A-Za-z][A-Za-z0-9+.-]*://[^/?#\\]+(?:/[^?#]*)?[?#]") -# A client honours the one of these keywords it knows and rejects the other with a -# ``TypeError``: either way, the call follows no redirect. -_NO_REDIRECTS = ("allow_redirects", "follow_redirects") +from coretrace_python.taint.urls import flow_url, redirects_disabled class SsrfPlugin(TaintDetector): @@ -43,22 +32,18 @@ class SsrfPlugin(TaintDetector): requires: ClassVar[frozenset[AnyAnalysis]] = TaintDetector.requires | {SSAAnalysis, ModuleStringsAnalysis} def judge(self, ctx: PluginContext, function: nodes.Function, flow: TaintFlow, verdict: Verdict) -> Verdict: - if flow.through is not None: - return verdict # the URL is built in the callee ssa = ctx.get(SSAAnalysis, function) defs = {i.result: i for block in ssa.blocks for i in block.instructions if i.result is not None} - text, _ = leading_text(flow.argument, defs, ctx.get(ModuleStringsAnalysis)) - host = _FIXED_HOST.match(text) - if host is None: + url = flow_url(flow, defs, ctx.get(ModuleStringsAnalysis)) + if url.origin is None: return verdict - arguments = flow.sink_arguments - no_redirects = arguments is not None and any(arguments.given(name) == (True, "False") for name in _NO_REDIRECTS) - if _FIXED_PATH.match(text) is not None: + no_redirects = redirects_disabled(flow.sink_arguments) + if url.path_fixed: if no_redirects: - return Verdict(flow, Status.REFUTED, f"the destination is fixed by {text!r} and redirects are disabled") + return Verdict(flow, Status.REFUTED, f"the destination is fixed by {url.text!r} and redirects are disabled") left = "a redirect may lead elsewhere" else: left = "the input chooses the path" + ("" if no_redirects else ", and a redirect may lead elsewhere") if verdict.status is not Status.VULNERABILITY: return verdict - return Verdict(flow, Status.HOTSPOT, f"the host is fixed by {host.group(0)!r}, but {left}") + return Verdict(flow, Status.HOTSPOT, f"the host is fixed by {url.origin!r}, but {left}") diff --git a/src/coretrace_python/taint/urls.py b/src/coretrace_python/taint/urls.py new file mode 100644 index 0000000..2ef4716 --- /dev/null +++ b/src/coretrace_python/taint/urls.py @@ -0,0 +1,70 @@ +"""What the constant text a URL starts with proves about its destination, and whether a +call follows redirects: one proof that the rules reading URLs share, each drawing its +own verdict from it. The text is what ``+`` and f-strings build before any other value +(``abstract.strings``); the proof says nothing about the rest of the URL.""" + +from __future__ import annotations + +import re +from collections.abc import Mapping +from dataclasses import dataclass + +from coretrace_python.abstract.strings import leading_text +from coretrace_python.interprocedural import Arguments +from coretrace_python.ir.model import Instruction, Value +from coretrace_python.taint.engine import TaintFlow + +# ``scheme://`` and an authority ended by ``/``, ``?`` or ``#``. A backslash, which some +# URL parsers take for a slash, does not end it here. +_ORIGIN = re.compile(r"[A-Za-z][A-Za-z0-9+.-]*://[^/?#\\]+[/?#]") +# The same with the path fixed too: what follows is query or fragment. +_PATH = re.compile(r"[A-Za-z][A-Za-z0-9+.-]*://[^/?#\\]+(?:/[^?#]*)?[?#]") +# A reference a browser resolves on the current site: ``/`` then a character that is not +# a slash, a backslash, a space or a control character (``//`` and ``/\`` name another +# host, and a browser drops tabs and newlines), a query or a fragment, or a relative path +# whose first segment ends in the text without a ``:`` that would make it a scheme. +_RELATIVE = re.compile(r"/[^/\\\x00-\x20\x7f]|[?#]|[^/\\?#:\x00-\x20\x7f][^/\\?#:]*[/?#]") +# A client honours the one of these keywords it knows and rejects the other with a +# ``TypeError``: either way, the call follows no redirect. +_NO_REDIRECTS = ("allow_redirects", "follow_redirects") + + +@dataclass(frozen=True) +class UrlProof: + """What the constant ``text`` a URL starts with proves: the ``origin`` it fixes + (``scheme://authority`` and the character ending it), whether it fixes the path + too (``path_fixed``), and whether it starts a reference a browser resolves on the + current site (``relative``).""" + + text: str + origin: str | None = None + path_fixed: bool = False + relative: bool = False + + +def url_proof(text: str) -> UrlProof: + origin = _ORIGIN.match(text) + return UrlProof( + text, + origin.group(0) if origin is not None else None, + _PATH.match(text) is not None, + _RELATIVE.match(text) is not None, + ) + + +def flow_url(flow: TaintFlow, defs: Mapping[Value, Instruction], strings: Mapping[str, str]) -> UrlProof: + """The proof for the value ``flow`` passes to its sink, ``defs`` defining the values + of the function making the call. A flow reaching its sink in a callee proves nothing: + the value is built there.""" + + if flow.through is not None: + return UrlProof("") + text, _ = leading_text(flow.argument, defs, strings) + return url_proof(text) + + +def redirects_disabled(arguments: Arguments | None) -> bool: + """Whether the call disables redirects explicitly (``allow_redirects=False`` or + ``follow_redirects=False``); absent, a variable or unpacked options do not.""" + + return arguments is not None and any(arguments.given(name) == (True, "False") for name in _NO_REDIRECTS) diff --git a/tests/test_open_redirect_destinations.py b/tests/test_open_redirect_destinations.py new file mode 100644 index 0000000..b7532d9 --- /dev/null +++ b/tests/test_open_redirect_destinations.py @@ -0,0 +1,116 @@ +"""An open-redirect flow is judged on the destination a browser resolves from the target, +as far as ``+`` and f-strings build it, with its own criteria rather than SSRF's. + +- A target whose constant text starts a reference a browser resolves on the current site + (``/profile/``, ``?page=``, ``profile/``) keeps the browser on the site, where any + further redirect is the project's own code, analysed on its own: refuted. +- A target whose constant text fixes an absolute ``scheme://host`` sends the browser to + that host, but the input chooses the path, where that host may redirect again: a + hotspot. +- Anything else stays a vulnerability: ``"/" + next`` (``//evil.com`` is another host), + a backslash or a control character after the slash, a host left open, an unknown base, + a target built in a helper function. +""" + +from __future__ import annotations + +import pytest + +from coretrace_python import engine +from coretrace_python.findings import Finding +from coretrace_python.source import SourceManager + +PLUGINS = [engine.BUNDLED_PLUGINS] + +FLASK = ( + "from flask import Flask, redirect, request\n\n" + "app = Flask(__name__)\n" + "HOME = '/account/'\n\n" + "@app.route('/go/')\n" + "def go(uid):\n" + " return {call}\n" +) + + +def check(text: str) -> tuple[Finding, ...]: + return engine.check(SourceManager().add_source("app.py", text), PLUGINS) + + +def redirects(text: str) -> list[tuple[int, str]]: + return [(f.span.start_line, f.metadata["verdict"]) for f in check(text) if f.rule_id == "open-redirect"] + + +@pytest.mark.parametrize( + "call", + [ + "redirect('/profile/' + uid)", + "redirect(f'/users/{uid}/edit')", + "redirect('?page=' + uid)", + "redirect('#' + uid)", + "redirect('profile/' + uid)", + "redirect(HOME + uid)", + ], +) +def test_a_target_on_the_current_site_is_no_open_redirect(call: str) -> None: + assert redirects(FLASK.format(call=call)) == [] + + +@pytest.mark.parametrize( + "call", + [ + "redirect('https://example.com/' + uid)", + "redirect(f'https://example.com/login?next={uid}')", + ], +) +def test_a_target_on_a_fixed_host_is_a_hotspot(call: str) -> None: + assert redirects(FLASK.format(call=call)) == [(8, "hotspot")] + + +@pytest.mark.parametrize( + "call", + [ + # ``next='/evil.com'`` makes ``//evil.com``, another host. + "redirect('/' + request.args['next'])", + "redirect('/\\\\' + uid)", + "redirect('/\\t' + uid)", + "redirect('https://example.com' + uid)", + "redirect('a:b/' + uid)", + "redirect('java' + uid)", + "redirect(request.args['next'])", + ], +) +def test_a_target_the_input_may_send_elsewhere_stays_a_vulnerability(call: str) -> None: + assert redirects(FLASK.format(call=call)) == [(8, "vulnerability")] + + +def test_an_unknown_base_stays_a_vulnerability() -> None: + text = FLASK.format(call="redirect(HOME + uid)").replace("HOME = '/account/'\n", "from settings import HOME\n") + + assert redirects(text) == [(8, "vulnerability")] + + +def test_a_target_built_in_a_helper_function_stays_a_vulnerability() -> None: + text = FLASK.format(call="profile(uid)") + "\ndef profile(name):\n return redirect('/profile/' + name)\n" + + assert redirects(text) == [(8, "vulnerability")] + + +def test_the_hotspot_evidence_names_the_fixed_host_and_the_further_redirect() -> None: + (finding,) = [f for f in check(FLASK.format(call="redirect('https://example.com/' + uid)")) if f.rule_id == "open-redirect"] + + assert "'https://example.com/'" in finding.metadata["evidence"] and "redirect" in finding.metadata["evidence"] + + +@pytest.mark.parametrize( + "target, verdicts", + [("'/items/' + pk", []), ("request.GET['next']", [(5, "vulnerability")])], +) +def test_django_redirects_follow_the_same_criteria(target: str, verdicts: list[tuple[int, str]]) -> None: + text = ( + "from django.shortcuts import redirect\nfrom django.urls import path\n\n" + "def go(request, pk):\n" + f" return redirect({target})\n\n" + "urlpatterns = [path('go//', go)]\n" + ) + + assert redirects(text) == verdicts diff --git a/tests/test_url_proof.py b/tests/test_url_proof.py new file mode 100644 index 0000000..88b4292 --- /dev/null +++ b/tests/test_url_proof.py @@ -0,0 +1,80 @@ +"""What the constant text a URL starts with proves, and nothing more: one proof that the +rules reading URLs share, each drawing its own verdict from it. + +- ``origin``: constant ``scheme://authority`` and the ``/``, ``?`` or ``#`` ending it, so + the input cannot choose the host. A backslash, which some parsers take for a slash, + does not end the authority. +- ``path_fixed``: the text fixes the path too; what follows is query or fragment. +- ``relative``: the text starts a reference a browser resolves on the current site: + ``/`` then a character that is neither a slash, a backslash nor a space or control + character (a browser takes ``//`` and ``/\\`` for another host, and drops tabs and + newlines), ``?`` or ``#``, or a relative path whose first segment, ended in the text, + holds no ``:`` that would make it a scheme. +""" + +from __future__ import annotations + +import pytest + +from coretrace_python.interprocedural import Arguments +from coretrace_python.taint.urls import UrlProof, redirects_disabled, url_proof + + +@pytest.mark.parametrize( + "text, proof", + [ + ("https://api.example.com/users/", UrlProof("https://api.example.com/users/", "https://api.example.com/")), + ("https://api.example.com/search?q=", UrlProof("https://api.example.com/search?q=", "https://api.example.com/", True)), + ("https://api.example.com/#", UrlProof("https://api.example.com/#", "https://api.example.com/", True)), + ("https://api.example.com?q=", UrlProof("https://api.example.com?q=", "https://api.example.com?", True)), + ("https://api.example.com", UrlProof("https://api.example.com")), + ("https://api.example.com:", UrlProof("https://api.example.com:")), + ("https:/", UrlProof("https:/")), + ("https://", UrlProof("https://")), + ("https://api.example.com\\", UrlProof("https://api.example.com\\")), + ("//api.example.com/", UrlProof("//api.example.com/")), + ("", UrlProof("")), + ], +) +def test_constant_text_fixes_an_origin_only_with_a_complete_authority(text: str, proof: UrlProof) -> None: + assert url_proof(text) == proof + + +@pytest.mark.parametrize( + "text, relative", + [ + ("/profile/", True), + ("/p", True), + ("?page=", True), + ("#top", True), + ("profile/", True), + ("search?q=", True), + ("/", False), + ("//evil.example/", False), + ("/\\", False), + ("/\t", False), + ("/ ", False), + ("profile", False), + ("javascript:", False), + ("a:b/", False), + (" /profile/", False), + ], +) +def test_constant_text_starts_a_reference_on_the_current_site_as_a_browser_resolves_it(text: str, relative: bool) -> None: + assert url_proof(text).relative is relative + + +@pytest.mark.parametrize( + "arguments, disabled", + [ + (Arguments((), (("allow_redirects", "False"),)), True), + (Arguments((), (("follow_redirects", "False"),)), True), + (Arguments((), (("allow_redirects", "True"),)), False), + (Arguments((), (("allow_redirects", None),)), False), + (Arguments((), (), unpacked=True), False), + (Arguments(), False), + (None, False), + ], +) +def test_only_an_explicit_keyword_disables_redirects(arguments: Arguments | None, disabled: bool) -> None: + assert redirects_disabled(arguments) is disabled