diff --git a/CHANGELOG.md b/CHANGELOG.md index daaf788..fffae03 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,15 @@ # 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). + +### Advisories + +- An advisory entry point may declare a `host` condition: the attacker must choose the host of the URL passed as the named argument. It is decided from the URL's constant text, the proof `ssrf` and `open-redirect` read. A fixed host with redirects disabled in the call leaves the call reachable, not exploitable. A fixed host with redirects followed keeps it exploitable, because a redirect may lead to a host the attacker controls, and the condition pending review says so. Input in the query does not choose the host. Anything else stays pending review, and so do the environment conditions. A `host` condition must name its argument and carry nothing else (#170). + ## 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..7292c72 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. | @@ -386,6 +386,23 @@ a call passing a variable, or unpacking `*args` or `**kwargs`, leaves the condit pending review. A `semantic` condition — the document's tags, the platform, the template's origin — cannot be decided and is always pending review, never dropped. +A `host` condition says the attacker must choose the host of the URL passed as +`argument` (or at `position`), as credentials leaking to the host of a crafted URL +need: `{"kind": "host", "text": "…", "argument": "url", "position": 0}`. It is +decided from the URL's constant text, as `ssrf` and `open-redirect` read it, when the +attacker's input is passed in that argument. The outcome has three cases: + +- Constant text fixes the host (`"https://api.example.com/" + path`) and the call + disables redirects (`allow_redirects=False`, `follow_redirects=False`): the call is + reachable, not exploitable. +- Constant text fixes the host but redirects may be followed: the call stays + exploitable. The fixed initial host does not clear the chain, since a redirect may + lead to a host the attacker controls, and the condition pending review says so. + Input in the query does not choose the host. +- Otherwise the condition stays pending review. + +The other conditions, the environment included, stay pending as before. + Every dependency finding records the highest level of evidence established in its `level` metadata: `declared` (the requirement allows a vulnerable version), `imported` (a module of the package is imported somewhere), `reachable` (an entry point or affected 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/dependency/advisories.py b/src/coretrace_python/dependency/advisories.py index 9853add..0a92bd9 100644 --- a/src/coretrace_python/dependency/advisories.py +++ b/src/coretrace_python/dependency/advisories.py @@ -251,7 +251,7 @@ def _condition(entry: Mapping[str, Any]) -> Condition: default = entry.get("default", False) if not isinstance(default, bool): raise TypeError(f"condition default must be true or false, got {default!r}") - return Condition( + condition = Condition( str(entry["kind"]), str(entry["text"]), None if entry.get("argument") is None else str(entry["argument"]), @@ -259,3 +259,8 @@ def _condition(entry: Mapping[str, Any]) -> Condition: position, default, ) + if condition.kind == "host" and ( + (condition.argument is None and condition.position is None) or condition.values or condition.default + ): + raise ValueError("a host condition names the URL argument, by keyword or position, and nothing else") + return condition diff --git a/src/coretrace_python/dependency/correlation.py b/src/coretrace_python/dependency/correlation.py index 001f3da..7ce0ba4 100644 --- a/src/coretrace_python/dependency/correlation.py +++ b/src/coretrace_python/dependency/correlation.py @@ -10,7 +10,7 @@ from __future__ import annotations from collections.abc import Iterable, Mapping -from dataclasses import dataclass +from dataclasses import dataclass, replace from coretrace_python.dependency.graph import ( DIRECT, @@ -24,6 +24,7 @@ from coretrace_python.interprocedural import Arguments, CallSite, ExternalSymbol from coretrace_python.semantic.symbols import SymbolId from coretrace_python.taint import Sink, TaintFlow, TaintKind +from coretrace_python.taint.urls import UrlProof, redirects_disabled Affected = Mapping[SymbolId, tuple[Advisory, ...]] @@ -61,14 +62,33 @@ class ConditionCheck: passed: str | None = None -def check_conditions(entry: AdvisoryEntryPoint | None, arguments: Arguments | None) -> ConditionCheck: - """Decide ``entry``'s conditions against what the call's ``arguments`` denote.""" +def check_conditions( + entry: AdvisoryEntryPoint | None, + arguments: Arguments | None, + url: UrlProof | None = None, + passed_as: frozenset[tuple[int | None, str | None]] = frozenset(), +) -> ConditionCheck: + """Decide ``entry``'s conditions against what the call's ``arguments`` denote and, for + a ``host`` condition, what ``url`` proves of the value the attacker's input is passed + in (``passed_as``). A host the URL's constant text fixes rules the call out when the + call disables redirects; otherwise a redirect may still lead to a host the attacker + controls, and the condition stays pending with that uncertainty.""" if entry is None: return ConditionCheck() met: list[Condition] = [] pending: list[Condition] = [] for condition in entry.conditions: + if condition.kind == "host": + origin = url.origin if url is not None and _passed_in(condition, passed_as) else None + if origin is None: + pending.append(condition) + elif redirects_disabled(arguments): + return ConditionCheck(tuple(met), tuple(pending), condition, repr(origin)) + else: + text = f"the host is fixed by {origin!r}, but a redirect may lead to a host the attacker controls" + pending.append(replace(condition, text=text)) + continue given = arguments.given(condition.argument, condition.position) if condition.checkable and arguments is not None else None if given is None: pending.append(condition) @@ -84,6 +104,16 @@ def check_conditions(entry: AdvisoryEntryPoint | None, arguments: Arguments | No return ConditionCheck(tuple(met), tuple(pending)) +def _passed_in(condition: Condition, passed_as: frozenset[tuple[int | None, str | None]]) -> bool: + """Whether a value passed as ``passed_as`` is the argument ``condition`` names.""" + + return any( + (keyword is not None and keyword == condition.argument) + or (keyword is None and position is not None and position == condition.position) + for position, keyword in passed_as + ) + + def ruled_out(module: str, site: CallSite, check: ConditionCheck) -> str: """Which call a contradicted condition rules out, and why: ``app:12 python.yaml.load(Loader=python.yaml.SafeLoader)``.""" @@ -120,9 +150,10 @@ def correlate( flows: Iterable[TaintFlow], verdicts: Verdicts | None, affected: Affected, + urls: Mapping[TaintFlow, UrlProof] | None = None, ) -> tuple[Finding, ...]: """Exploitable-vulnerability findings for the non-refuted ADVISORY flows of a function, - one per advisory the sink is affected by.""" + one per advisory the sink is affected by; ``urls`` holds what each flow's URL proves.""" findings: list[Finding] = [] for flow in flows: @@ -136,7 +167,7 @@ def correlate( entry = advisory.entry_point(flow.sink.symbol) if entry is not None and not any(entry.exploitable_through(p, k) for p, k in flow.passed_as): continue - check = check_conditions(entry, flow.sink_arguments) + check = check_conditions(entry, flow.sink_arguments, (urls or {}).get(flow), flow.passed_as) if check.contradicted is None: findings.append(_exploitable(function, flow, advisory, verdict, hotspot, check)) return tuple(findings) diff --git a/src/coretrace_python/dependency/graph.py b/src/coretrace_python/dependency/graph.py index 853dfec..5aafb77 100644 --- a/src/coretrace_python/dependency/graph.py +++ b/src/coretrace_python/dependency/graph.py @@ -151,8 +151,10 @@ class Condition: ``argument`` condition names the argument by keyword, and by ``position`` when it may be passed positionally, and lists the ``values`` that satisfy it — symbols (``python.yaml.FullLoader``) or constants as Python writes them (``True``); - ``default`` says an absent argument means a vulnerable value. A ``semantic`` - condition cannot be checked and is reported as pending review.""" + ``default`` says an absent argument means a vulnerable value. A ``host`` condition + says the attacker must choose the host of the URL passed as ``argument`` (or at + ``position``); the engine decides it from what the URL's constant text proves. A + ``semantic`` condition cannot be checked and is reported as pending review.""" kind: str text: str diff --git a/src/coretrace_python/engine.py b/src/coretrace_python/engine.py index bcc3148..af5e9e2 100644 --- a/src/coretrace_python/engine.py +++ b/src/coretrace_python/engine.py @@ -126,6 +126,7 @@ escaped_templates, registered_routes, ) +from coretrace_python.taint.urls import flow_url TOOL_NAME = "coretrace-python-analyzer" @@ -770,14 +771,14 @@ def _analyse_module( graph = manager.get(CallGraphAnalysis) correlated: list[Finding] = [] if affected: + strings = manager.get(ModuleStringsAnalysis) for function in supported: + flows = manager.get(TaintAnalysis, function).flows + ssa = manager.get(SSAAnalysis, function) + defs = {i.result: i for block in ssa.blocks for i in block.instructions if i.result is not None} + urls = {flow: flow_url(flow, defs, strings) for flow in flows if flow.kinds & TaintKind.ADVISORY} correlated.extend( - correlate( - graph.name_of(function), - manager.get(TaintAnalysis, function).flows, - manager.get(RefutationAnalysis, function), - affected, - ) + correlate(graph.name_of(function), flows, manager.get(RefutationAnalysis, function), affected, urls) ) return CachedModule( manager.get(EntryPointAnalysis), 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_advisory_url_host.py b/tests/test_advisory_url_host.py new file mode 100644 index 0000000..c659d7a --- /dev/null +++ b/tests/test_advisory_url_host.py @@ -0,0 +1,145 @@ +"""An advisory entry point may declare that exploiting it needs the attacker to choose the +host of a URL: a ``host`` condition naming the argument. The engine decides it from what +the URL's constant text proves (``taint.urls``), the proof ``ssrf`` and ``open-redirect`` +read, with the advisory's own criteria: + +- constant text fixing the host, and the call disabling redirects: the attacker cannot + choose the host, so the call is reachable, not exploitable; +- constant text fixing the host, redirects followed: the initial host does not clear the + chain, since a redirect may lead to a host the attacker controls. The call stays + exploitable, and the condition pending review says so. Input in the query does not + choose the host; +- anything else: the condition stays pending review, undecided. + +The other conditions, those on the environment included, stay pending as before. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from coretrace_python import engine +from coretrace_python.dependency import ( + Advisory, + AdvisoryEntryPoint, + AdvisoryFileError, + AttackerArgument, + Condition, + dump_advisories, + load_advisories, +) +from coretrace_python.findings import Finding, Severity +from coretrace_python.semantic.symbols import SymbolId + +HOST = Condition("host", "the attacker chooses the host of the URL", argument="url", position=0) +NETRC = Condition("semantic", "a .netrc file holds credentials for the host") +FETCH = Advisory( + "CVE-2099-0901", + "vulnlib", + "<1.1", + "fetch sends the credentials of one host to another", + Severity.HIGH, + entry_points=( + AdvisoryEntryPoint( + SymbolId("python.vulnlib.fetch"), + "commit abc1234", + (HOST, NETRC), + attacker_arguments=(AttackerArgument("url", 0),), + ), + ), + modules=("vulnlib",), +) +FIXED = "the host is fixed by 'https://api.example.com/', but a redirect may lead to a host the attacker controls" + + +def analyse(root: Path, call: str) -> tuple[Finding, ...]: + (root / "requirements.txt").write_text("vulnlib==1.0\n", encoding="utf-8") + (root / "advisories.json").write_text(dump_advisories((FETCH,)), encoding="utf-8") + (root / "app.py").write_text( + f"import vulnlib\n\ndef run():\n data = input()\n return {call}\n", encoding="utf-8" + ) + return engine.analyze_project(root, [engine.BUNDLED_PLUGINS]).findings + + +def exploitable(root: Path, call: str) -> list[str]: + """The conditions pending review of each exploitable finding.""" + + return [ + f.metadata.get("conditions_pending_review", "") + for f in analyse(root, call) + if f.rule_id == "exploitable-vulnerability" + ] + + +@pytest.mark.parametrize( + "call", + [ + "vulnlib.fetch(data)", + # The host is left open: ``data='@evil.example'`` chooses it. + "vulnlib.fetch('https://api.example.com' + data, allow_redirects=False)", + "vulnlib.fetch(BASE + data)", + ], +) +def test_a_host_not_proven_fixed_leaves_the_condition_pending(tmp_path: Path, call: str) -> None: + assert exploitable(tmp_path, call) == [f"{HOST.text}; {NETRC.text}"] + + +@pytest.mark.parametrize( + "call", + [ + "vulnlib.fetch('https://api.example.com/users/' + data)", + # Input in the query does not choose the host either. + "vulnlib.fetch(f'https://api.example.com/search?q={data}')", + ], +) +def test_a_fixed_host_with_redirects_followed_keeps_the_chain_visible(tmp_path: Path, call: str) -> None: + assert exploitable(tmp_path, call) == [f"{FIXED}; {NETRC.text}"] + + +@pytest.mark.parametrize( + "call", + [ + "vulnlib.fetch('https://api.example.com/users/' + data, allow_redirects=False)", + "vulnlib.fetch(url='https://api.example.com/users/' + data, follow_redirects=False)", + ], +) +def test_a_fixed_host_without_redirects_leaves_the_call_reachable_only(tmp_path: Path, call: str) -> None: + findings = analyse(tmp_path, call) + + assert [f for f in findings if f.rule_id == "exploitable-vulnerability"] == [] + assert [f.metadata["advisory"] for f in findings if f.rule_id == "reachable-vulnerability"] == [FETCH.id] + + +def test_host_conditions_round_trip_through_advisory_files(tmp_path: Path) -> None: + path = tmp_path / "advisories.json" + path.write_text(dump_advisories((FETCH,)), encoding="utf-8") + + assert load_advisories(path) == (FETCH,) + (entry,) = json.loads(path.read_text())["advisories"][0]["entry_points"] + assert entry["conditions"][0] == { + "kind": "host", + "text": "the attacker chooses the host of the URL", + "argument": "url", + "position": 0, + } + + +@pytest.mark.parametrize( + "condition", + [ + {"kind": "host", "text": "the attacker chooses the host"}, + {"kind": "host", "text": "the attacker chooses the host", "argument": "url", "values": ["x"]}, + {"kind": "host", "text": "the attacker chooses the host", "argument": "url", "default": True}, + ], +) +def test_a_host_condition_names_its_argument_and_nothing_else(tmp_path: Path, condition: dict[str, object]) -> None: + document = json.loads(dump_advisories((FETCH,))) + document["advisories"][0]["entry_points"][0]["conditions"] = [condition] + path = tmp_path / "advisories.json" + path.write_text(json.dumps(document), encoding="utf-8") + + with pytest.raises(AdvisoryFileError): + load_advisories(path) 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