From ece4473c2b7133dd591fc51f4dff38c88c6aabb8 Mon Sep 17 00:00:00 2001 From: Marius Ciepluch <11855163+norandom@users.noreply.github.com> Date: Thu, 24 Sep 2026 09:04:42 +0200 Subject: [PATCH] fix(scan): a hook callback's parameters are the registry's arguments, not the request's The core-hook-argument class from the second PMPro adjudication: pmpro_membership_history_profile_fields($user) on `edit_user_profile` was a route entry, so its parameters were attacker input -- and $user is the WP_User WordPress loaded; its ID became an SQL injection. A callback registered in a string-keyed registry is handed its arguments by the matching apply call, not by the request. The mapper now records the registration call on the entry (`registered_by`), regions carry it, and the dispatch fact says which registrations DO hand a callback request-borne arguments (`request_arguments`; WordPress: add_shortcode, whose $atts are post content). Every other registered callback keeps its call depth but its parameters stop being sources; request data that reaches an apply call's arguments is still followed by the hook half of the taint query. REST routes are not registry entries and keep the parameter rule. Controls: the fact field is validated against `register`; a hook callback is not input, a shortcode and a plain route are. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01EmM1j46qgKMh3zqD1QMwEo --- src/openultrasast/mapping.py | 22 +++++++++++- src/openultrasast/model/regions.py | 4 +++ src/openultrasast/model/scan.py | 19 +++++++++- src/openultrasast/ruleset/semantic/php.toml | 4 +++ src/openultrasast/semantic/facts.py | 16 +++++++++ tests/test_dispatch_value.py | 39 +++++++++++++++++++++ 6 files changed, 102 insertions(+), 2 deletions(-) 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)