From 21fdb554aab3b3bb15b6bde64e7101a0b72d0d1d Mon Sep 17 00:00:00 2001 From: Marius Ciepluch <11855163+norandom@users.noreply.github.com> Date: Wed, 9 Sep 2026 17:15:51 +0200 Subject: [PATCH] fix(cpg): a query that could not be asked is not an empty answer contributor-scan 5.7, and it was the wrong question. **The 117k-line pmpro CPG does not load.** Importing it throws in a ForkJoinParallelCpgPassWithAccumulator during overlay application, so every query against it returns nothing. It is not a call-graph question at all. Isolated by running the ARBITER against two prebuilt CPGs rather than writing a sixth ad-hoc Scala probe, four of which had already failed on my own mistakes: small wpdb CPG get_order_by_code -> 1 model_entailed pmpro CPG getMemberOrderByCode -> 0 Both have their source and sink in ONE function, both were asked with parameter sources and depth 3, same arbiter and same spec. The CPG is the only difference. That clears the facts, which is worth stating: `$wpdb` sinks work, the parameter-source rule works, and a PHP parameter reaching `$wpdb->get_var` through a concat is entailed. Whether php2cpg resolves `new X()` and `$this->method()` is still unmeasured -- it cannot be measured on a CPG that will not load -- and is recorded as 5.10 along with finding the size boundary. **And the defect underneath it, which is the fourth quiet failure found on this one checkout.** `query_batch` returned `{}` for a failure and `{}` for a genuine empty answer, so a caller could not tell "could not ask" from "nothing found". Every query on this scan failed at load and it reported 500 regions examined, no findings, and no degradation: a vulnerable plugin, clean. A failed batch now returns None, unparseable output counts as a failure rather than an empty answer, and the driver records a `query_failed` degradation naming the kind and the request count. This is the failure mode the rung ladder structurally cannot catch. Every other honesty mechanism here labels a VERDICT -- suspicion, regions_unjudged, regions_unasked, stated family limits -- and when no verdict is produced there is nothing to label. Gates byte-identical. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01T7XD7x5KonMdmGRkp63EQG --- .kiro/specs/contributor-scan/tasks.md | 23 ++++++++++++++- .kiro/specs/ir-consolidation/brief.md | 29 +++++++++++++++++-- .kiro/specs/ir-consolidation/spec.json | 23 +++++++++++---- .../measurements/2026-09-09-repo-pmpro.json | 16 +++++++++- src/openultrasast/cpg/backend.py | 22 +++++++++----- src/openultrasast/model/scan.py | 9 +++++- tests/test_cpg_backend.py | 5 +++- tests/test_model_scan.py | 26 +++++++++++++++++ 8 files changed, 133 insertions(+), 20 deletions(-) diff --git a/.kiro/specs/contributor-scan/tasks.md b/.kiro/specs/contributor-scan/tasks.md index 4708f98..ab04cee 100644 --- a/.kiro/specs/contributor-scan/tasks.md +++ b/.kiro/specs/contributor-scan/tasks.md @@ -476,7 +476,7 @@ - _Requirements: 8.1, 9.1_ - _Depends: 5.1_ -- [ ] 5.7 Does the PHP call graph carry a flow through an object? +- [x] 5.7 Does the PHP call graph carry a flow through an object? — NOT a call-graph question - **The WordPress checkout scans and does not find its CVE.** Both ends are inside the budget -- `pmpro_rest_api_get_order` at position 290 as a recognised entry point, `getMemberOrderByCode` at 472 -- and the taint query runs for 23 seconds. So the question is the middle. @@ -490,6 +490,15 @@ something else, which is a finding worth having either way. - Observable: a recorded answer naming which edges exist, and either the CVE found or a stated reason it cannot be. + - **Answered 2026-09-09, and it was the wrong question.** The 117k-line pmpro CPG does not LOAD -- import + throws in a ForkJoinParallelCpgPassWithAccumulator during overlay application -- so every query against + it returns nothing. Isolated by running the ARBITER against two prebuilt CPGs rather than writing more + ad-hoc Scala: `get_order_by_code` on the small probe returns 1 `model_entailed` with parameter sources + and depth 3, while `getMemberOrderByCode` on pmpro, the identical shape, returns 0 under identical + settings. Same arbiter, same spec, same question; the CPG is the difference. + - That clears the facts: `$wpdb` sinks work, the parameter-source rule works, and a PHP parameter reaching + `$wpdb->get_var` through a concat is entailed. Whether php2cpg resolves `new X()` and `$this->method()` + remains **unmeasured**, because it cannot be measured on a CPG that will not load. Recorded as 5.10. - _Requirements: 4.4, 8.1, 9.1_ - _Depends: 5.1_ @@ -523,6 +532,18 @@ - _Requirements: 8.1, 9.1_ - _Depends: 5.6_ +- [ ] 5.10 A PHP CPG large enough to matter that actually loads + - `php2cpg` builds a 4.3MB CPG for a 637-file plugin with no errors, and importing it throws during + overlay application. Every query then returns nothing. + - Find the boundary: does it load at 100 files, 300, 600? Is one file responsible, or is it size? Joern's + own message suggests the frontend for large codebases, which is what the fallback already does, so the + remaining lever is whether overlays can be applied separately or the tree scoped. + - Until this is answered, PHP detection is demonstrated only on files small enough to build cleanly, and + no claim about WordPress can be made either way. + - Observable: the largest PHP tree that loads and queries, measured, with the failure boundary recorded. + - _Requirements: 4.2, 4.3, 9.1_ + - _Depends: 5.7_ + ## Group 6 — Regression baselines - [ ] 6.1 Repository baseline and delta diff --git a/.kiro/specs/ir-consolidation/brief.md b/.kiro/specs/ir-consolidation/brief.md index c2f652e..b2ad9fa 100644 --- a/.kiro/specs/ir-consolidation/brief.md +++ b/.kiro/specs/ir-consolidation/brief.md @@ -43,6 +43,30 @@ definite 1, or unknown, with abstract transformers for `INT_ADD`, `INT_MULT`, `I validated against concrete execution in its own tests. That is precisely the shape of analysis the C family lacks, and it exists. +### The candidates, and why they are not equivalent + +| | language | IR | analyses | licence | state | +| --- | --- | --- | --- | --- | --- | +| **angr** | **Python 3** (+ Rust) | VEX, lifting | CFG recovery, data-dependency, **value-set analysis**, symbolic execution | BSD-2-Clause | actively maintained, ~14.8k commits | +| **GhidraPAL** | Java | Ghidra P-Code | **three-valued-logic abstract interpreter**, transformers validated against concrete runs | see repo | research code, small | +| **Vine** | OCaml/C++ | own IL | BitBlaze static analysis | GPL-2.0 | dormant since 2009 | + +**angr is the strongest candidate and Vine is not one.** Three things decide it: + +* **It is Python.** This codebase is Python with `dependencies = []` in the core, and every heavyweight + capability so far has been reached across a subprocess boundary rather than imported. angr fits that + pattern as an extra, the way `harnessx` and the tree-sitter grammars already do. +* **Value-set analysis is the abstraction we are missing**, by name. "Can this index exceed this allocation" + is a VSA question, and VSA is abstract interpretation rather than SMT -- which is the choice + `model-grounded-detection` already made and gave its reasons for. +* **BSD-2-Clause**, against Vine's GPL-2.0. This project has a standing rule about never vendoring code + without a stated licence, and a GPL dependency is a decision about the whole tool rather than about one + analysis. + +GhidraPAL stays interesting for a different reason: it is a worked example of the *domain*, not a platform. +Its TVL bit-vector and its validated transformers for `INT_ADD`, `INT_MULT` and the comparisons are the +thing to read before writing any abstract domain here, whether the platform underneath is angr or Ghidra. + It is also the honest answer to a decision this project already took. `model-grounded-detection` chose abstract interpretation over SMT, and chose to build the model from code because execution cannot be assumed. A bit-level abstract interpreter over P-Code is *still abstract interpretation* and still static — @@ -53,8 +77,9 @@ what changes is the artifact: it needs a **built** library rather than its sourc 1. Whether the source-IR subtraction is safe, measured rather than assumed. 2. Whether the C arithmetic class is worth a second pipeline at all, or whether `c/memory` stays scoped to string misuse and says so — which is where task 2.16 left it, deliberately. -3. If it is worth it: P-Code with a TVL domain, adapting GhidraPAL's transformers, versus writing one. Vine - is dormant since 2009 and is not the candidate; Ghidra is maintained and P-Code has tooling. +3. If it is worth it: **angr** first, on language, licence and the fact that value-set analysis is the + named abstraction -- with GhidraPAL read as the worked example of a bit-precise domain rather than as a + platform. Vine is dormant since 2009 and is not a candidate. 4. What a second pipeline costs in the one currency this project has been strict about — **not lines, but the number of things that can silently return nothing.** This session found three separate quiet failures in one PHP scan. A binary pipeline adds a build step, a lifter, and a domain, each of which can produce diff --git a/.kiro/specs/ir-consolidation/spec.json b/.kiro/specs/ir-consolidation/spec.json index b95efac..279eae4 100644 --- a/.kiro/specs/ir-consolidation/spec.json +++ b/.kiro/specs/ir-consolidation/spec.json @@ -1,15 +1,26 @@ { "feature_name": "ir-consolidation", "created_at": "2026-09-09T17:00:00Z", - "updated_at": "2026-09-09T17:00:00Z", + "updated_at": "2026-09-09T17:30:00Z", "language": "en", "phase": "brief", "approvals": { - "requirements": { "generated": false, "approved": false }, - "design": { "generated": false, "approved": false }, - "tasks": { "generated": false, "approved": false } + "requirements": { + "generated": false, + "approved": false + }, + "design": { + "generated": false, + "approved": false + }, + "tasks": { + "generated": false, + "approved": false + } }, "ready_for_implementation": false, - "depends_on": ["contributor-scan"], - "approval_note": "2026-09-09 brief drafted after the maintainer raised codebase growth and suggested adopting an existing IR (Vine, or Rolf Rolles' work, or Ghidra's). The brief separates two questions with opposite answers. The SOURCE IR is already carried rather than about to be added: semantic/ is 3218 lines, 16% of the codebase, holding a hand-written IR and flat taint engine measured at 2.2% entailment against the CPG's 41.3%, still running as a proposer -- and Vine and P-Code are binary IRs that do not help, so the work there is subtraction not substitution. The MISSING abstraction is the C arithmetic-overflow class, where GhidraPAL's three-valued-logic abstract interpreter over Ghidra P-Code is exactly the right shape and exists; Vine is dormant since 2009 and is not the candidate. Constraint recorded: question 1 lands before question 2 starts, or the brief makes the growth problem worse. Not approved." + "depends_on": [ + "contributor-scan" + ], + "approval_note": "2026-09-09 brief drafted after the maintainer raised codebase growth and suggested adopting an existing IR (Vine, or Rolf Rolles' work, or Ghidra's). The brief separates two questions with opposite answers. The SOURCE IR is already carried rather than about to be added: semantic/ is 3218 lines, 16% of the codebase, holding a hand-written IR and flat taint engine measured at 2.2% entailment against the CPG's 41.3%, still running as a proposer -- and Vine and P-Code are binary IRs that do not help, so the work there is subtraction not substitution. The MISSING abstraction is the C arithmetic-overflow class, where GhidraPAL's three-valued-logic abstract interpreter over Ghidra P-Code is exactly the right shape and exists; Vine is dormant since 2009 and is not the candidate. Constraint recorded: question 1 lands before question 2 starts, or the brief makes the growth problem worse. Not approved. ; 2026-09-09 angr added as the strongest candidate for the binary-side question after the maintainer raised it: Python 3 so it fits the extras pattern this codebase already uses for heavyweight capabilities, BSD-2-Clause against Vine's GPL-2.0 which would be a decision about the whole tool, actively maintained, and it names VALUE-SET ANALYSIS as a capability -- which is precisely the abstraction c/memory lacks and is abstract interpretation rather than SMT, matching the architecture decision already taken. GhidraPAL stays as the worked example of a bit-precise TVL domain to read before writing one, not as a platform." } diff --git a/benchmarks/measurements/2026-09-09-repo-pmpro.json b/benchmarks/measurements/2026-09-09-repo-pmpro.json index aa03857..bf7c9b8 100644 --- a/benchmarks/measurements/2026-09-09-repo-pmpro.json +++ b/benchmarks/measurements/2026-09-09-repo-pmpro.json @@ -1,10 +1,18 @@ { + "call_graph_question_answered": { + "answer": "It was never a call-graph question. The 117k-line pmpro CPG does not LOAD: importing it throws in a ForkJoinParallelCpgPassWithAccumulator during overlay application, so every query against it returns nothing.", + "how_it_was_isolated": "The arbiter itself was run against two prebuilt CPGs instead of writing more ad-hoc Scala. On the small wpdb probe, `get_order_by_code` -- source and sink in ONE function -- returns 1 model_entailed with parameter_sources and depth 3. On the pmpro CPG, `getMemberOrderByCode`, which has exactly the same shape, returns 0 under identical settings. Same arbiter, same spec, same question: the difference is the CPG.", + "question": "does the PHP call graph carry a flow through `new MemberOrder()` and `$this->method()`?", + "task": "5.7", + "what_remains_unknown": "Whether php2cpg resolves `new X()` and `$this->method()` into callee edges is STILL unmeasured, because it cannot be measured on a CPG that will not load. It needs a plugin small enough to build cleanly, or the load failure fixed first.", + "what_this_clears": "The `$wpdb` sinks work, the parameter-source rule works, and a PHP flow from a parameter through a concat into `$wpdb->get_var` is entailed. None of those was the problem." + }, "detection": { "both_in_budget": true, "cve": "CVE-2023-23488", "entry_point": "includes/rest-api.php:pmpro_rest_api_get_order -- recognised, rank 0.80, position 290, entry_point", "found": false, - "open_question": "The flow crosses `new MemberOrder( $code )` into a constructor and then `$this->getMemberOrderByCode( $code )`. Whether Joern's PHP frontend resolves either into a call edge is unverified -- two attempts to query the call graph failed on script errors. If it does not, task 2.4's interprocedural region cannot bridge this and the PHP story needs something else. Recorded as 5.7.", + "open_question": "ANSWERED 2026-09-09 -- see call_graph_question_answered", "sink": "classes/class.memberorder.php:936 getMemberOrderByCode -- $wpdb->get_var with concatenated $code, position 472" }, "envelope": { @@ -27,6 +35,12 @@ "reading": "The checkout is real, the CVE is verified in the code, and the scan runs cleanly on 117k lines of PHP. **The CVE is not detected.** That is the result, and three separate engine defects had to be fixed before the scan could even produce it -- each of which had previously reported the plugin clean for a different reason.", "repo": "Paid Memberships Pro 2.9.7 at 1a900be3f09f243876150fc3c04a97bcb24ef84b, CVE-2023-23488", "spec": "contributor-scan 5.1 -- the WordPress detection checkout", + "the_fourth_quiet_failure": { + "cause": "`query_batch` returned `{}` for a failure and `{}` for a genuine empty answer, so a caller could not tell 'could not ask' from 'nothing found'. Every query on this checkout failed at CPG load and the scan recorded none of it.", + "fix": "A failed batch returns None and the driver records a `query_failed` degradation naming the kind and the request count. Unparseable output is a failure too, not an empty answer.", + "symptom": "500 regions scanned, 0 findings, no degradations -- a vulnerable plugin reported clean", + "why_it_matters_most": "Every other honesty mechanism here labels a VERDICT -- suspicion, regions_unjudged, regions_unasked, stated family limits. When no verdict is produced there is nothing to label, and the report is indistinguishable from a clean one. That is the fourth such failure found on this single checkout." + }, "three_defects_found_getting_here": [ { "cause": "joern-parse threw after applying overlays; php2cpg on the identical tree produced a 4.3MB CPG with no errors", diff --git a/src/openultrasast/cpg/backend.py b/src/openultrasast/cpg/backend.py index 6a9f153..00e629e 100644 --- a/src/openultrasast/cpg/backend.py +++ b/src/openultrasast/cpg/backend.py @@ -93,7 +93,7 @@ class CpgResult: cpg_path: Path run: Callable[[str, Mapping[str, object]], object | None] - run_batch: Callable[[str, Mapping[str, Mapping[str, object]]], dict[str, list[object]]] | None = None + run_batch: Callable[[str, Mapping[str, Mapping[str, object]]], dict[str, list[object]] | None] | None = None class CpgBackend(Protocol): @@ -172,7 +172,7 @@ class JoernBackend: logger.info("built the cpg for %s with %s after joern-parse failed", root, frontend) return True - def query_batch(self, cpg_path: Path, query: str, requests: Mapping[str, Mapping[str, object]]) -> dict[str, list[object]]: + def query_batch(self, cpg_path: Path, query: str, requests: Mapping[str, Mapping[str, object]]) -> dict[str, list[object]] | None: """Run ONE script invocation carrying many requests, keyed back to their ids. JVM startup, not CPG construction, is what dominates a repository scan: every ``joern --script`` call @@ -181,13 +181,15 @@ class JoernBackend: by then; the work itself is milliseconds. So the batch carries the whole scan's questions in one parameter and the script loops over them. - Fails closed like every other path here: an engine that could not answer returns ``{}``, never an - empty result per request, because "no rows" and "could not ask" must stay distinguishable. + Fails closed like every other path here, and says which kind of nothing it is: an engine that could + not answer returns ``None`` while a genuine empty answer is ``{}``. That distinction is the whole + point -- "no rows" and "could not ask" look identical to a caller that only sees a dict, and a + 117k-line PHP CPG that threw while loading turned a failed scan into a clean bill of health. """ script = self.queries_dir / f"{query}.sc" if not script.is_file(): logger.warning("no such cpg query: %s", script) - return {} + return None payload = json.dumps({rid: {k: _render(v) for k, v in req.items()} for rid, req in requests.items()}) # Through a FILE, never the command line. Linux caps a single argument at MAX_ARG_STRLEN (128KB) # whatever ARG_MAX says, and a repository blows through that: 2500 taint requests over a 637-file @@ -200,7 +202,7 @@ class JoernBackend: requests_file.write_text(payload) except OSError as exc: logger.warning("could not stage cpg batch %s: %s", query, exc) - return {} + return None command = [ shutil.which("joern") or "joern", self._heap_flag(), @@ -215,10 +217,14 @@ class JoernBackend: if completed is None or completed.returncode != 0: detail = (completed.stderr or "")[-400:] if completed is not None else "timeout" logger.warning("cpg batch %s failed: %s", query, detail) - return {} + return None parsed = extract_payload(completed.stdout or "") if not isinstance(parsed, Mapping): - return {} + # Unparseable output is a failure, not an empty answer. A 117k-line PHP CPG that throws while + # loading printed a stack trace and no payload, and returning {} made the whole scan read as a + # clean repository -- 500 regions examined, nothing found, no degradation recorded. + logger.warning("cpg batch %s returned no parseable payload", query) + return None # A request the engine did not answer is absent, not empty: the caller must be able to tell them apart. return {str(rid): list(rows) for rid, rows in parsed.items() if isinstance(rows, list)} diff --git a/src/openultrasast/model/scan.py b/src/openultrasast/model/scan.py index c787d34..2d2885d 100644 --- a/src/openultrasast/model/scan.py +++ b/src/openultrasast/model/scan.py @@ -155,7 +155,14 @@ def scan_repository( kind_started = time.monotonic() batch = getattr(cpg, "run_batch", None) if callable(batch): - rows_by_id.update(batch(kind, requests) or {}) + answered_batch = batch(kind, requests) + if answered_batch is None: + # The engine could not answer. That is NOT an empty result, and recording it is what keeps a + # failed scan from reading as a clean repository: on a 117k-line PHP checkout every query + # failed at CPG load and the scan reported 500 regions examined with nothing found. + degradations.append({"stage": "model", "reason": "query_failed", "kind": kind, "requests": len(requests)}) + else: + rows_by_id.update(answered_batch) else: # a backend without batching still works, one call at a time for rid, params in requests.items(): answered = cpg.run(kind, params) diff --git a/tests/test_cpg_backend.py b/tests/test_cpg_backend.py index 04ba800..6f94720 100644 --- a/tests/test_cpg_backend.py +++ b/tests/test_cpg_backend.py @@ -147,7 +147,10 @@ def test_a_failed_batch_yields_no_rows_for_any_request(monkeypatch: pytest.Monke stderr = "boom" backend = JoernBackend(runner=lambda c, **k: _Bad()) - assert backend.query_batch(Path("/tmp/c.bin"), "taint", {"r1": {}}) == {} + assert backend.query_batch(Path("/tmp/c.bin"), "taint", {"r1": {}}) is None, ( + "None means COULD NOT ASK; {} would mean the engine answered and found nothing, and a caller that " + "cannot tell them apart reports a failed scan as a clean repository" + ) def test_a_batch_result_that_omits_a_request_returns_nothing_for_it() -> None: diff --git a/tests/test_model_scan.py b/tests/test_model_scan.py index 4e24ae1..84b9299 100644 --- a/tests/test_model_scan.py +++ b/tests/test_model_scan.py @@ -364,3 +364,29 @@ def test_two_families_at_one_site_stay_two_findings() -> None: ] assert len(_deduplicated(both)) == 2 + + +def test_a_query_the_engine_could_not_answer_is_recorded_not_ignored() -> None: + """contributor-scan 5.7. A 117k-line PHP CPG threw while loading, so every query returned nothing -- and + the scan reported 500 regions examined, no findings, and no degradation. A failed query and a clean + repository looked identical. + + This is the failure mode the rung ladder cannot catch: every other honesty counter labels a VERDICT, and + here none was produced to label. + """ + from openultrasast.cpg.backend import CpgResult + from openultrasast.model.scan import scan_repository + + class _Backend: + def available(self) -> bool: + return True + + def build(self, root, **kwargs): # type: ignore[no-untyped-def] + return CpgResult(cpg_path=Path("cpg.bin"), run=lambda q, p: [], run_batch=lambda q, r: None) + + result = scan_repository(Path("/repo"), (_region(),), backend=_Backend(), client=None, model="") + + failures = [d for d in result.degradations if d.get("reason") == "query_failed"] + assert failures, "a query that could not be asked must be reported" + assert failures[0]["kind"] == "taint" + assert result.findings == ()