diff --git a/.kiro/specs/contributor-scan/tasks.md b/.kiro/specs/contributor-scan/tasks.md index da87363..a09180e 100644 --- a/.kiro/specs/contributor-scan/tasks.md +++ b/.kiro/specs/contributor-scan/tasks.md @@ -408,7 +408,7 @@ - _Requirements: 9.2, 9.4, 9.5, 9.6_ - _Depends: 4.1_ -- [ ] 5.4 WordPress entry points, and the one-verdict-per-region limit +- [x] 5.4 WordPress entry points, and the one-verdict-per-region limit - Two things stand between the PHP facts and a WordPress result, and the second was found by running the first. - **Entry points.** WordPress registers handlers with `add_action`, `add_filter`, admin-ajax and REST @@ -425,6 +425,23 @@ - _Requirements: 2.1, 8.1, 9.1_ - _Depends: 5.1_ +- [ ] 5.5 php2cpg emits a CPG containing one file + - **Found by 5.4 and it blocks the WordPress target.** A CPG built over two PHP files contains only one, + and the survivor is the last alphabetically. Reproduced twice on independent samples; both files parse + cleanly under `php -l` and under PHP-Parser directly, and `joern-parse` reports success either way. + - php2cpg logs `Found warning in PHP-Parser JSON output` naming the surviving file, and that warning is + PHP-Parser's own `====> File X:` banner appearing in what php2cpg expects to be pure JSON. That is the + thread to pull. + - Not yet isolated: this machine has no PHP interpreter, so php2cpg was driven through a container shim. + The banner comes from PHP-Parser rather than from the shim, which makes a shim-only cause unlikely but + not excluded. **Rerun with the native php-cli in the shipped image first.** + - A plugin is many files. A scan that silently covers one of them and reports success is exactly the quiet + failure Req 5.6 was approved for. + - Observable: a CPG over N PHP files contains N files, or a recorded reason it cannot with the version and + the reproduction. + - _Requirements: 4.3, 5.6, 9.1_ + - _Depends: 5.4_ + ## Group 6 — Regression baselines - [ ] 6.1 Repository baseline and delta diff --git a/benchmarks/measurements/2026-09-09-php-regions.json b/benchmarks/measurements/2026-09-09-php-regions.json new file mode 100644 index 0000000..5ed4b5a --- /dev/null +++ b/benchmarks/measurements/2026-09-09-php-regions.json @@ -0,0 +1,23 @@ +{ + "caveat": "Every PHP number here comes from probe files of a dozen lines. The recogniser is validated on a WordPress-shaped sample, not on WordPress.", + "generated": "2026-09-09", + "one_verdict_per_region_fixed": { + "libpng": "1 -> 2 entailed; the second (contrib/libtests/makepng.c:1580:strstash) was hidden by the collapse. Neither is validated and both are in contrib/.", + "php_probe": "2 of 3 -> 3 of 3; the SQL injection at vuln.php:5 had been losing to `system` on flow length", + "vampi": "6 entailed, unchanged -- and now with line numbers on access-control findings from task 2.7", + "what": "taint.verdicts() returns one verdict per distinct SINK SITE, strongest first, and its first element is exactly what verdict() returns -- so the pair path and every committed measurement resting on it are unchanged, and the gates are byte-identical." + }, + "reading": "Both halves of 5.4 are implemented and one is blocked by the frontend. Per-site verdicts take the PHP probe from 2 of 3 to 3 of 3 and surface a second libpng finding that the collapse was hiding. The WordPress recogniser reads declared access correctly and turns one file-level region into ten function-scoped ones. But php2cpg emits a CPG containing only ONE PHP file in this configuration, which blocks the WordPress target until it is understood.", + "spec": "contributor-scan 5.4 (Req 2.1, 8.1, 9.1)", + "the_blocker": { + "detail": "A CPG built over two PHP files contains only one of them, and the survivor is the last alphabetically. Reproduced twice: {plugin.php, vuln.php} kept vuln.php; {only_fn.php, stmt_then_fn.php} kept stmt_then_fn.php. Both files parse cleanly under `php -l` and under PHP-Parser directly, and joern-parse reports success. php2cpg logs 'Found warning in PHP-Parser JSON output' naming the surviving file, which is PHP-Parser's own `====> File X:` banner appearing in what php2cpg expects to be pure JSON.", + "id": "php2cpg-emits-one-file", + "not_yet_isolated": "This machine has no PHP interpreter, so php2cpg was driven through a container shim. The banner comes from PHP-Parser rather than from the shim, which makes a shim-only cause unlikely but not excluded. Rerunning with a native php-cli -- which the shipped image has -- is the first step.", + "why_it_blocks_wordpress": "A plugin is many files. A scan that silently covers one of them and reports success is the quiet failure Req 5.6 was approved for, and it would read as a clean bill of health." + }, + "wordpress_recogniser": { + "access_is_declared_not_guessed": "The hook name IS the declaration: wp_ajax_nopriv_* is WordPress saying logged-out callers reach this, so it is public; wp_ajax_* fires only for logged-in ones. permission_callback => __return_true declares a REST route open. That is the declared-provenance signal task 2.8 needs, arriving from a second framework.", + "measured": "1 file-level region -> 10 function-scoped; mp_fetch_report public (rank 1.0), mp_save_report authenticated (0.8), mp_render_box public, mp_helper review-required", + "what": "add_action / add_filter / add_shortcode with a string callback, and register_rest_route's callback with its permission_callback. Plus every top-level PHP function as its own region, without which a file is one region however many functions it holds." + } +} diff --git a/src/openultrasast/mapping.py b/src/openultrasast/mapping.py index d6de3b4..8c3164a 100644 --- a/src/openultrasast/mapping.py +++ b/src/openultrasast/mapping.py @@ -187,9 +187,99 @@ def _language_entry_points(target: FileTarget, text: str) -> list[EntryPointReco return _c_entry_points(target, text) if target.language == "solidity": return _solidity_entry_points(target, text) + if target.language == "php": + return _php_entry_points(target, text) return [] +# WordPress registers handlers rather than decorating them, and the hook name states the access level: +# `wp_ajax_nopriv_*` is admin-ajax for logged-OUT users and is therefore declared public, while `wp_ajax_*` +# fires only for logged-in ones. That is a DECLARED access level, not a guess from an absent decorator, which +# is the distinction task 2.8 turns on. +_PHP_REGISTRATION = re.compile( + r"""\b(?Padd_action|add_filter|add_shortcode)\s*\(\s*['"](?P[^'"]+)['"]\s*,\s*['"](?P[A-Za-z_][A-Za-z0-9_]*)['"]""", + re.VERBOSE, +) +_PHP_REST_ROUTE = re.compile(r"\bregister_rest_route\s*\(") +_PHP_CALLBACK = re.compile(r"""['"]callback['"]\s*=>\s*['"](?P[A-Za-z_][A-Za-z0-9_]*)['"]""") +_PHP_PERMISSION = re.compile(r"""['"]permission_callback['"]\s*=>\s*(?P[^,\)]+)""") +_PHP_FUNCTION = re.compile(r"^\s*function\s+(?P[A-Za-z_][A-Za-z0-9_]*)\s*\(") + + +def _php_entry_points(target: FileTarget, text: str) -> list[EntryPointRecord]: + """WordPress's registration model, plus every top-level function as a region of its own. + + The functions matter as much as the hooks. Without them a PHP file is ONE file-level region, and a region + that spans a file attributes every finding to the file rather than to the function that holds it. + """ + records: list[EntryPointRecord] = [] + lines = text.splitlines() + bounds = _php_function_bounds(lines) + + for number, line in enumerate(lines, start=1): + for match in _PHP_REGISTRATION.finditer(line): + hook, handler = match.group("hook"), match.group("handler") + 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, [])) + if _PHP_REST_ROUTE.search(line): + window = "\n".join(lines[number - 1 : number + 12]) + callback = _PHP_CALLBACK.search(window) + if callback is not None: + handler = callback.group("handler") + permission = _PHP_PERMISSION.search(window) + access, evidence = _rest_route_access(permission.group("value").strip() if permission else None) + start, end = bounds.get(handler, (number, number)) + records.append(_entry(target, start, end, handler, "wp:rest_route", "route", access, "http_request", evidence, [])) + + registered = {record.function_name for record in records} + for name, (start, end) in sorted(bounds.items()): + if name not in registered: + records.append( + _entry(target, start, end, name, f"php:{name}", "function", "review-required", "call", ["php function declaration"], []) + ) + return records + + +def _wordpress_hook_access(call: str, hook: str) -> tuple[AccessLevel, list[str]]: + """The hook name is the declaration. `nopriv` is WordPress saying "logged-out callers reach this".""" + if hook.startswith("wp_ajax_nopriv_"): + return "public", [f"{call}('{hook}') is admin-ajax for logged-out callers"] + if hook.startswith("wp_ajax_"): + return "authenticated", [f"{call}('{hook}') fires only for logged-in callers"] + if call == "add_shortcode": + return "public", [f"add_shortcode('{hook}') renders in page content"] + return "review-required", [f"{call}('{hook}')"] + + +def _rest_route_access(permission: str | None) -> tuple[AccessLevel, list[str]]: + """`permission_callback` is the REST contract. `__return_true` declares the route open.""" + if permission is None: + return "review-required", ["register_rest_route with no permission_callback"] + if "__return_true" in permission: + return "public", ["permission_callback => __return_true declares the route public"] + return "authenticated", [f"permission_callback => {permission[:60]}"] + + +def _php_function_bounds(lines: Sequence[str]) -> dict[str, tuple[int, int]]: + """Line ranges of top-level PHP functions, by brace depth. Good enough to scope a region.""" + bounds: dict[str, tuple[int, int]] = {} + name: str | None = None + start = 0 + depth = 0 + for number, line in enumerate(lines, start=1): + if name is None: + match = _PHP_FUNCTION.match(line) + if match is not None: + name, start, depth = match.group("name"), number, 0 + if name is not None: + depth += line.count("{") - line.count("}") + if depth <= 0 and "{" in "".join(lines[start - 1 : number]): + bounds[name] = (start, number) + name = None + return bounds + + def _python_entry_points(target: FileTarget, text: str) -> list[EntryPointRecord]: records: list[EntryPointRecord] = [] try: diff --git a/src/openultrasast/model/pipeline.py b/src/openultrasast/model/pipeline.py index 22a061a..9894160 100644 --- a/src/openultrasast/model/pipeline.py +++ b/src/openultrasast/model/pipeline.py @@ -34,6 +34,7 @@ from .dominance import verdict as dominance_verdict from .ladder import Rung, Verdict from .specs import ConfigSpec, DominanceSpec, TaintSpec from .taint import verdict as taint_verdict +from .taint import verdicts as taint_verdicts logger = logging.getLogger(__name__) @@ -65,6 +66,30 @@ class ModelFinding: contradiction: str = "" +def _arbitrate_all( + cpg: CpgResult, + spec: TaintSpec | DominanceSpec | ConfigSpec, + *, + path: str, + function: str, + parameter_sources: bool, + call_depth: int, +) -> list[Verdict]: + """Every verdict this region admits, strongest first. + + One verdict per region was the right answer for a pair -- one labelled function, one bug -- and the wrong + one for a region that spans a file. A PHP file with an SQL injection on line 6 and a command injection on + line 13 produced ONE injection verdict, and `system` won it on flow length, so the SQL injection went + unreported by construction. Only taint can report several sites today; dominance and config still answer + once per region, which is correct for dominance (its claim is about the region's consistency) and a + known gap for config. + """ + if isinstance(spec, (DominanceSpec, ConfigSpec)): + answer = _arbitrate(cpg, spec, path=path, function=function, parameter_sources=parameter_sources, call_depth=call_depth) + return [answer] if answer is not None else [] + return taint_verdicts(cpg, spec, function=function, file=path, parameter_sources=parameter_sources, call_depth=call_depth) + + def _arbitrate( cpg: CpgResult, spec: TaintSpec | DominanceSpec | ConfigSpec, @@ -101,12 +126,18 @@ def scan_region( call_depth: int = 0, ) -> list[ModelFinding]: """Findings for one region and family, ordered strongest first.""" - answer = _arbitrate(cpg, spec, path=path, function=function, parameter_sources=parameter_sources, call_depth=call_depth) + answers = _arbitrate_all(cpg, spec, path=path, function=function, parameter_sources=parameter_sources, call_depth=call_depth) - if answer is not None and answer.rung is Rung.ENTAILED: - # The graph decided. No model call, and no candidate needs asking about: this region has its finding. - return [ModelFinding(site=_site(path, function, answer), family=spec.family, rung=Rung.ENTAILED, witness=answer.witness)] + entailed = [answer for answer in answers if answer.rung is Rung.ENTAILED] + if entailed: + # The graph decided. No model call, and no candidate needs asking about: this region has its findings, + # one per site rather than one per region. + return [ + ModelFinding(site=_site(path, function, answer), family=spec.family, rung=Rung.ENTAILED, witness=answer.witness) + for answer in entailed + ] + answer = answers[0] if answers else None if answer is not None: # A real flow through a sanitizer. Whether it suffices is the residual, and the model HAS coverage # here — so a "no" from the judge is a genuine contradiction and the claim is dropped. diff --git a/src/openultrasast/model/taint.py b/src/openultrasast/model/taint.py index f6e7c44..523cc55 100644 --- a/src/openultrasast/model/taint.py +++ b/src/openultrasast/model/taint.py @@ -59,24 +59,9 @@ def verdict( function-local question; above zero is what lets a handler's parameter reach a sink in another module, which is the shape VAmPI's SQL injection has and the shape a function-scoped question cannot see. """ - rows = cpg.run( - "taint", - request_params(spec, function=function, file=file, parameter_sources=parameter_sources, call_depth=call_depth), - ) - flows = _flows(rows, function=function) - # A sink whose *shape* is safe is the fix, not the bug. `execute(sql, params)` binds rather than - # interpolates and `printf("literal", x)` has a constant format string, so the flow that reaches them is - # not a vulnerability -- even though the taint path is identical to the vulnerable twin's. Dropping these - # is what lets the model distinguish a pair at all: measured on the injection slice, taint reachability - # alone entailed both sides of the canonical SQL pair and therefore arbitrated nothing. - flows = [flow for flow in flows if not _has_safe_shape(flow, spec)] - if not flows: + considered = _usable_flows(cpg, spec, function=function, file=file, parameter_sources=parameter_sources, call_depth=call_depth) + if not considered: return None - # Parameter sources are a FALLBACK, never an override. When a modelled source reaches the sink, those - # flows are the evidence: a captured `res` parameter also reaching it unsanitized must not report a fix as - # still vulnerable on the strength of a flow that names no attacker input. - modelled = [flow for flow in flows if _from_modelled_source(flow, spec)] - considered = modelled or flows # A flow the graph cannot see a discharge for is the finding; a sanitized or BOUNDED one is a question. # libpng bounds `strcpy(outname+len, ".png")` with `(len = strlen(inname)) > 250` against a char[256], # and entailing it is a claim the code contradicts. But the bound reaches the sink through an `error` @@ -87,9 +72,79 @@ def verdict( open_flows = [flow for flow in considered if not flow["sanitized"] and not flow["bounded"]] # Deterministic by construction: a total order, then take the first. Nothing depends on engine ordering. chosen = min(open_flows or considered, key=lambda flow: _rank(flow, spec)) - settled = bool(chosen["sanitized"]) or bool(chosen["bounded"]) + return _verdict_for(chosen, spec) + + +def _usable_flows( + cpg: CpgResult, + spec: TaintSpec, + *, + function: str, + file: str, + parameter_sources: bool, + call_depth: int, +) -> list[dict[str, object]]: + """The flows both entry points reason over, so neither can drift from the other. + + A sink whose *shape* is safe is the fix, not the bug: `execute(sql, params)` binds rather than + interpolates and `printf("literal", x)` has a constant format string, so a flow reaching them is not a + vulnerability even though its taint path is identical to the vulnerable twin's. Dropping those is what + lets the model distinguish a pair at all -- measured on the injection slice, taint reachability alone + entailed both sides of the canonical SQL pair and arbitrated nothing. + + Parameter sources are then a FALLBACK, never an override. When a modelled source reaches the sink those + flows are the evidence, so a captured `res` parameter also reaching it unsanitized cannot report a fix as + still vulnerable on the strength of a flow that names no attacker input. + """ + rows = cpg.run( + "taint", + request_params(spec, function=function, file=file, parameter_sources=parameter_sources, call_depth=call_depth), + ) + flows = [flow for flow in _flows(rows, function=function) if not _has_safe_shape(flow, spec)] + modelled = [flow for flow in flows if _from_modelled_source(flow, spec)] + return modelled or flows + + +def verdicts( + cpg: CpgResult, + spec: TaintSpec, + *, + function: str = "", + file: str = "", + parameter_sources: bool = False, + call_depth: int = 0, +) -> list[Verdict]: + """One verdict per distinct SINK SITE, strongest first. + + ``verdict`` answers "what is the strongest thing to say about this region", which is the right question + for a pair: one labelled function, one bug, one answer. It is the wrong question for a region that spans + a file. A PHP file with an SQL injection on line 6 and a command injection on line 13 got ONE injection + verdict, and `system` won it on flow length -- so the SQL injection went unreported, by construction + rather than by any failure of the analysis. + + The first element is exactly what ``verdict`` returns, so the pair path and the committed measurements + that rest on it are unchanged. + """ + flows = _usable_flows(cpg, spec, function=function, file=file, parameter_sources=parameter_sources, call_depth=call_depth) + if not flows: + return [] + by_site: dict[str, list[Mapping[str, object]]] = {} + for flow in flows: + by_site.setdefault(_location(flow) or str(flow.get("sink", "")), []).append(flow) + + chosen: list[Mapping[str, object]] = [] + for site_flows in by_site.values(): + open_flows = [flow for flow in site_flows if not flow["sanitized"] and not flow["bounded"]] + chosen.append(min(open_flows or site_flows, key=lambda flow: _rank(flow, spec))) + # Same total order the single-verdict path uses, so `verdicts(...)[0] == verdict(...)`. + chosen.sort(key=lambda flow: (bool(flow["sanitized"]) or bool(flow["bounded"]), _rank(flow, spec))) + return [_verdict_for(flow, spec) for flow in chosen] + + +def _verdict_for(flow: Mapping[str, object], spec: TaintSpec) -> Verdict: + settled = bool(flow["sanitized"]) or bool(flow["bounded"]) rung = Rung.CORROBORATED if settled else Rung.ENTAILED - return Verdict(rung=rung, family=spec.family, witness=_witness(chosen), location=_location(chosen)) + return Verdict(rung=rung, family=spec.family, witness=_witness(flow), location=_location(flow)) def _flows(rows: object, *, function: str) -> list[dict[str, object]]: diff --git a/tests/test_mapping.py b/tests/test_mapping.py index ce42cd0..5f930dc 100644 --- a/tests/test_mapping.py +++ b/tests/test_mapping.py @@ -261,3 +261,44 @@ def test_entry_points_name_handlers_registered_by_call_or_convention(tmp_path: P assert routes[("api_views/books.py", "delete")].access_level == "public" body = routes[("api_views/books.py", "get_by_title")] assert body.line == 4 and body.end_line == 5 # the record spans the handler, not the registration line + + +def test_wordpress_hooks_declare_their_access(tmp_path: Path) -> None: + """contributor-scan 5.4. WordPress registers handlers rather than decorating them, and the hook name is + the declaration: `wp_ajax_nopriv_*` is WordPress saying logged-out callers reach this.""" + from openultrasast.mapping import analyze_entry_points + from openultrasast.preprocess import preprocess_repository + + (tmp_path / "plugin.php").write_text( + " None: + """Without this a PHP file is ONE region however many functions it holds, and a region that spans a file + attributes every finding to the file rather than to the function that holds it.""" + from openultrasast.mapping import analyze_entry_points + from openultrasast.model.regions import regions_for + from openultrasast.preprocess import preprocess_repository + + (tmp_path / "lib.php").write_text("= {"a", "b"} diff --git a/tests/test_model_taint.py b/tests/test_model_taint.py index 4a4a9d7..861e97f 100644 --- a/tests/test_model_taint.py +++ b/tests/test_model_taint.py @@ -463,3 +463,39 @@ def test_only_a_length_sink_can_be_discharged_by_a_bound() -> None: assert not taint_specs(language="python")["injection"].bounded_sinks assert not taint_specs(language="c")["injection"].bounded_sinks assert taint_specs(language="c")["memory"].bounded_sinks + + +def test_a_region_reports_every_sink_site_not_just_the_strongest() -> None: + """contributor-scan 5.4. One verdict per region was right for a pair and wrong for a region spanning a + file: a PHP file with SQL injection on one line and command injection on another got ONE injection + verdict, and `system` won it on flow length, so the SQL injection went unreported by construction.""" + from openultrasast.cpg.backend import CpgResult + from openultrasast.model.specs import taint_specs + from openultrasast.model.taint import verdict, verdicts + + def row(sink, line, length): # type: ignore[no-untyped-def] + return { + "sink": sink, + "sinkLine": str(line), + "sinkMethod": "handler", + "sinkFile": "app.php", + "source": '$_GET["x"]', + "sanitized": False, + "length": length, + "sinkArity": 2, + "sinkArg0Literal": False, + "inLabeledScope": True, + "bounded": False, + "bound": "", + } + + rows = [row("mysqli_query($c, $sql)", 6, 5), row("system($cmd)", 13, 1)] + cpg = CpgResult(cpg_path=Path("c.bin"), run=lambda q, p: rows) + spec = taint_specs(language="php")["injection"] + + every = verdicts(cpg, spec, function="", file="app.php") + + assert len(every) == 2, "two sink sites are two findings" + assert {v.location for v in every} == {"app.php:6:handler", "app.php:13:handler"} + # The pair path is unchanged: the strongest is still what `verdict` returns. + assert every[0].location == verdict(cpg, spec, function="", file="app.php").location