diff --git a/src/openultrasast/mapping.py b/src/openultrasast/mapping.py index d9bd4e9..3219013 100644 --- a/src/openultrasast/mapping.py +++ b/src/openultrasast/mapping.py @@ -91,6 +91,11 @@ class EntryPointRecord: # "public" only because nothing was found -- which is precisely the bug the access-control family exists # to report, so treating it as a declaration would silence the detector on its primary case. access_declared: bool = False + # The registry call that made this function an entry, when a string-keyed registry did (`add_action`, + # `add_filter`, `add_shortcode`). Such a callback is handed its arguments BY THE REGISTRY -- whatever the + # matching `do_action`/`apply_filters` passes -- not by the request, which is what the scan's parameter + # rule needs to know. Empty for every entry that did not come from a registration. + registered_by: str = "" def ingest_sarif(path: Path) -> list[StaticHint]: @@ -302,7 +307,20 @@ def _php_entry_points(target: FileTarget, text: str) -> list[EntryPointRecord]: access, evidence = _wordpress_hook_access(match.group("call"), hook) start, end = bounds.get(handler, (number, number)) records.append( - _entry(target, start, end, handler, f"wp:{hook}", "route", access, "http_request", evidence, [], access_declared=True) + _entry( + target, + start, + end, + handler, + f"wp:{hook}", + "route", + access, + "http_request", + evidence, + [], + access_declared=True, + registered_by=match.group("call"), + ) ) if _PHP_REST_ROUTE.search(line): window = "\n".join(lines[number - 1 : number + 12]) @@ -774,6 +792,7 @@ def _entry( conditions: list[str], access_declared: bool = False, defines_handler: bool = True, + registered_by: str = "", ) -> EntryPointRecord: return EntryPointRecord( path=target.path, @@ -790,6 +809,7 @@ def _entry( provenance=f"entrypoint:{kind}:{access_level}", rationale=f"{kind} surface classified as {access_level} at {trust_boundary}", access_declared=access_declared, + registered_by=registered_by, ) diff --git a/src/openultrasast/model/regions.py b/src/openultrasast/model/regions.py index 6e68c48..56a698c 100644 --- a/src/openultrasast/model/regions.py +++ b/src/openultrasast/model/regions.py @@ -70,6 +70,9 @@ class ScanRegion: # its catch-all for a named function with no evidence of receiving a request. Only the scan's parameter # rule reads it, because only it depends on whether a parameter carries request data. entry_kind: str = "" + # The string-keyed registry call that registered this entry (`add_action`), or empty. Read by the scan's + # parameter rule alongside the dispatch facts; see `EntryPointRecord.registered_by`. + entry_registry: str = "" def regions_for(entries: Sequence[object], targets: Sequence[object], *, shipped: frozenset[str] | None = None) -> tuple[ScanRegion, ...]: @@ -131,6 +134,7 @@ def regions_for(entries: Sequence[object], targets: Sequence[object], *, shipped rank=rank, source="module_scope" if module_body else "entry_point", entry_kind=str(getattr(entry, "kind", "") or ""), + entry_registry=str(getattr(entry, "registered_by", "") or ""), ) # A nameless entry point IS the file. Where a named region already covers that file it adds nothing but diff --git a/src/openultrasast/model/scan.py b/src/openultrasast/model/scan.py index c5dbc1e..9bcc3fb 100644 --- a/src/openultrasast/model/scan.py +++ b/src/openultrasast/model/scan.py @@ -28,6 +28,7 @@ from pathlib import Path from typing import Any from ..cpg.backend import TIMING, CpgResult +from ..semantic.facts import load_facts from .candidates import enumerate_candidates from .config_value import request_params as config_params from .contracts import ( @@ -1514,7 +1515,23 @@ def _parameters_are_input(region: ScanRegion) -> bool: keep the rule; JavaScript and Python emit no `function` entries at all. The region is still followed into its callees -- call depth stays with `_is_entry_point` -- only its parameters stop being sources. """ - return _is_entry_point(region) and region.entry_kind != "function" + return _is_entry_point(region) and region.entry_kind != "function" and not _registry_supplied(region) + + +def _registry_supplied(region: ScanRegion) -> bool: + """Was this entry registered in a string-keyed registry whose callbacks get the REGISTRY's arguments? + + A WordPress hook callback is called by `do_action`/`apply_filters` with whatever the call site passes: + `pmpro_membership_history_profile_fields($user)` on `edit_user_profile` gets the `WP_User` WordPress + loaded, and an adjudicated SQL injection on PMPro was that object's `ID`. The dispatch facts say which + registrations do hand a callback request-borne arguments (a shortcode's `$atts`); every other one does not. + """ + if not region.entry_registry: + return False + for fact in load_facts().for_language(region.language).dispatches: + if region.entry_registry in fact.register: + return region.entry_registry not in fact.request_arguments + return False def _spec_for(family: str, language: str) -> ArbiterSpec | None: diff --git a/src/openultrasast/ruleset/semantic/php.toml b/src/openultrasast/ruleset/semantic/php.toml index 8bd1345..94e6f26 100644 --- a/src/openultrasast/ruleset/semantic/php.toml +++ b/src/openultrasast/ruleset/semantic/php.toml @@ -213,6 +213,10 @@ apply = ["apply_filters", "apply_filters_ref_array", "do_action", "do_action_ref # whatever `$this` carried, and a random md5 fragment became an SQL injection. returns = ["apply_filters", "apply_filters_ref_array"] value_arg = 2 +# A hook callback is handed its arguments by `do_action`/`apply_filters` -- a WP_User, a post id, a filtered +# value -- not by the request, so its parameters are not attacker input. A shortcode's `$atts` come from post +# content a contributor writes, which is input, so `add_shortcode` keeps the parameter rule. +request_arguments = ["add_shortcode"] # What a PHP repository's paths say about scope. `vendored` trees are out of the scan and out of the graph # (a bundled library is a separate unit); `tests` paths stay in scope but are not product, so their diff --git a/src/openultrasast/semantic/facts.py b/src/openultrasast/semantic/facts.py index 079afcc..241cb9b 100644 --- a/src/openultrasast/semantic/facts.py +++ b/src/openultrasast/semantic/facts.py @@ -80,6 +80,11 @@ class DispatchFact: # came out "tainted" because `$this` was passed as filter context. Empty keeps the engine's default. returns: tuple[str, ...] = () value_arg: int = 0 + # The `register` calls whose callbacks are handed REQUEST-BORNE arguments. Every other registration's + # callback receives what the matching apply call passes -- framework or plugin values such as the + # `WP_User` a profile hook gets -- so its parameters are not attacker input; where request data does + # reach an apply call's arguments, the hook half of the taint query follows it from there. + request_arguments: tuple[str, ...] = () @dataclass(frozen=True) @@ -174,6 +179,7 @@ def _dispatches(value: object, language: str, path: Path) -> list[DispatchFact]: language=language, returns=_returning(item, path), value_arg=_value_arg(item, path), + request_arguments=_request_arguments(item, path), ) ) return facts @@ -187,6 +193,16 @@ def _returning(item: dict[str, object], path: Path) -> tuple[str, ...]: return returns +def _request_arguments(item: dict[str, object], path: Path) -> tuple[str, ...]: + calls = _strings(item.get("request_arguments", []), "request_arguments", path) + unknown = set(calls) - set(_strings(item.get("register"), "register", path)) + if unknown: + raise FactLoadError( + f"dispatch {item.get('id')!r} in {path}: request_arguments names calls that are not register calls: {sorted(unknown)}" + ) + return calls + + def _value_arg(item: dict[str, object], path: Path) -> int: value = item.get("value_arg", 0) if not isinstance(value, int) or isinstance(value, bool) or value < 0: diff --git a/tests/test_dispatch_value.py b/tests/test_dispatch_value.py index 8bd676f..7f0ab54 100644 --- a/tests/test_dispatch_value.py +++ b/tests/test_dispatch_value.py @@ -1,5 +1,7 @@ """A registry read returns its VALUE argument, not its context: a fact, loaded and checked.""" +import dataclasses + import pytest from openultrasast.model.specs import taint_specs @@ -34,3 +36,40 @@ def test_a_malformed_dispatch_row_is_refused(tmp_path, row: str) -> None: _write(tmp_path, '[[dispatch]]\nid = "hooks"\nregister = ["add_filter"]\napply = ["apply_filters"]\n' + row) with pytest.raises(FactLoadError): load_facts(tmp_path) + + +def test_request_arguments_must_name_register_calls(tmp_path) -> None: + row = '[[dispatch]]\nid = "hooks"\nregister = ["add_action"]\napply = ["do_action"]\n' + _write(tmp_path, row + 'request_arguments = ["register_rest_route"]\n') + with pytest.raises(FactLoadError): + load_facts(tmp_path) + + +def test_a_hook_callback_gets_the_registrys_arguments_not_the_requests(tmp_path) -> None: + """`edit_user_profile` hands its callback the WP_User WordPress loaded; a shortcode's $atts are content.""" + from openultrasast.mapping import analyze_entry_points + from openultrasast.model import scan + from openultrasast.model.regions import regions_for + from openultrasast.preprocess import build_file_target, enumerate_source_files + + (tmp_path / "plugin.php").write_text( + "get_var("SELECT 1 WHERE id = \'" . $user->ID . "\'"); }\n' + "add_shortcode('box', 'render_box');\n" + "function render_box($atts) { global $wpdb; return $wpdb->get_var(\"SELECT 1 WHERE n = '\" . $atts['n'] . \"'\"); }\n" + ) + files = enumerate_source_files(tmp_path) + targets = [build_file_target(tmp_path, p) for p in files] + entries = analyze_entry_points(tmp_path, targets) + assert {e.function_name: e.registered_by for e in entries if e.registered_by} == { + "show_history": "add_action", + "render_box": "add_shortcode", + } + regions = {r.function: r for r in regions_for(entries, targets)} + assert regions["show_history"].entry_registry == "add_action" + assert not scan._parameters_are_input(regions["show_history"]) + assert scan._parameters_are_input(regions["render_box"]) + # A route that did not come from a registry keeps its parameters as input. + route = dataclasses.replace(regions["show_history"], entry_registry="", entry_kind="route") + assert scan._parameters_are_input(route)