From cc885f025c18a21faae8c4e1a6ba08d121c7ab21 Mon Sep 17 00:00:00 2001 From: Marius Ciepluch <11855163+norandom@users.noreply.github.com> Date: Sat, 26 Sep 2026 13:05:12 +0200 Subject: [PATCH] feat(taint): project sink wrappers, one level and confirmed by dataflow This is the second attempt at the YesWiki shape: SQL run through `$this->dbService->query($query)`, whose body is the `mysqli_query` the facts name, through a receiver the frontend cannot type. The first attempt was reverted in c7e3a05. A method is now a wrapper for a parameter only when the engine's own dataflow inside the method carries that parameter to a fact sink's argument, on a plausible path that no cleansing or guard discharges. The guard rails: - one level only; - nothing under a bundled library tree (lib/, vendor/, node_modules/, ...); - a name counts only if every project method with that name is a wrapper; - wrappers are asked in the flow pass only, never in the evidence pass, so no region's selection or pruning changes; - at a wrapper call, only the confirmed argument is asked. Three Joern-gated tests cover the direct shape (reported), a literal, an intval-casting helper and a level-2 feeder (not reported), a harmless same-named method, and a wrapper inside lib/. Each fails when its rule is removed. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01EmM1j46qgKMh3zqD1QMwEo --- src/openultrasast/cpg/queries/taint.sc | 61 ++++++++++++++++++++++- tests/test_cpg_queries_compile.py | 67 ++++++++++++++++++++++++++ 2 files changed, 126 insertions(+), 2 deletions(-) diff --git a/src/openultrasast/cpg/queries/taint.sc b/src/openultrasast/cpg/queries/taint.sc index 5c13769..82b622c 100644 --- a/src/openultrasast/cpg/queries/taint.sc +++ b/src/openultrasast/cpg/queries/taint.sc @@ -72,6 +72,7 @@ // Per-family answers that do not depend on the region, kept across the whole batch. val familyInRepoMemo = scala.collection.mutable.Map.empty[String, Boolean] val sinkCandidatesMemo = scala.collection.mutable.Map.empty[String, List[io.shiftleft.codepropertygraph.generated.nodes.Call]] + val wrapperMemo = scala.collection.mutable.Map.empty[String, Map[String, Set[Int]]] val reachableMemo = scala.collection.mutable.Map.empty[(String, String, Int), Set[String]] val methodSourceMemo = scala.collection.mutable.Map.empty[String, Boolean] val fedFieldsMemo = scala.collection.mutable.Map.empty[String, Map[String, String]] @@ -1346,7 +1347,63 @@ return summary :: (perSink ++ contextRows) } - val rows = sinkCalls.l.flatMap { sink => + // ---- project-local sink WRAPPERS, one level, confirmed by dataflow --------------------------------------- + // + // Real code rarely calls the driver where it builds the query: YesWiki runs its INSERT through + // `$this->dbService->query($query)`, whose body is the `mysqli_query` the facts name. A method whose + // parameter REACHES a fact sink's argument -- by the engine's own dataflow inside the method, on a plausible + // path no cleansing or guard discharges -- is a sink for that argument where it is called. + // + // The first version (reverted, c7e3a05) summarised structurally, cascaded three levels by name and fed the + // structural pass: MW WP Form went from 3 findings to 60, PMPro to 65 with 20 inside bundled Braintree and + // Stripe, and the question count tripled until completion collapsed. So: one level only; nothing under a + // bundled library tree; a name only if every project method so named is a wrapper (an injected receiver has + // no type to resolve by); and wrappers are asked in this flow pass alone, never in the evidence pass, so they + // change no region's selection. + val bundledTrees = Set("lib", "libs", "vendor", "vendors", "third_party", "third-party", "node_modules", "bower_components") + + lazy val wrappers: Map[String, Set[Int]] = + wrapperMemo.getOrElseUpdate( + List(sinksS, sanitizersS, quotedSanitizersS, guardsS, dispatchValueS).mkString("|"), { + val factSinks = sinkCandidatesMemo.getOrElseUpdate(sinksS, cpg.call.filter(c => sinkNames.exists(n => sinkMatches(c, n))).l) + val confirmed = factSinks.groupBy(_.method).toList.flatMap { case (method, calls) => + val bundled = method.filename.split("/").exists(bundledTrees.contains) + if (method.isExternal || bundled || method.name.startsWith("<")) Nil + else + method.parameter.l + .filter(p => p.index >= 1 && !objectBases.contains(p.name) && p.name != "self" && p.name != "this") + .filter { p => + calls.exists { c => + c.argument.argumentIndexGt(0).reachableByFlows(Iterator.single(p)).l.filter(plausible).exists(f => !cleansed(spliced(f.elements.l))) + } + } + .map(p => (method, p.index)) + } + val byName = confirmed.groupBy(_._1.name).map { case (name, found) => name -> found.map(_._2).toSet } + val wrapperMethods = confirmed.map(_._1.fullName).toSet + byName.filter { case (name, _) => + cpg.method.nameExact(name).filterNot(_.isExternal).l.forall(m => wrapperMethods.contains(m.fullName)) + } + } + ) + + def wrapperCall(c: io.shiftleft.codepropertygraph.generated.nodes.Call): Boolean = + !c.name.startsWith(" sinkMatches(c, n)) + + // The arguments a flow must arrive through: every real argument of a fact sink, only the confirmed ones of a + // wrapper. + def sinkTargets(c: io.shiftleft.codepropertygraph.generated.nodes.Call) = + if (wrapperCall(c)) c.argument.l.filter(a => wrappers(c.name).contains(a.argumentIndex)) + else c.argument.argumentIndexGt(0).l + + def wrapperCalls: List[io.shiftleft.codepropertygraph.generated.nodes.Call] = + if (!hasSources || wrappers.isEmpty) Nil + else + sinkCandidatesMemo + .getOrElseUpdate("wrapped|" + List(sinksS, sanitizersS, quotedSanitizersS, guardsS, dispatchValueS).mkString("|"), cpg.call.filter(wrapperCall).l) + .filter(c => inScope(c.method)) + + val rows = (sinkCalls.l ++ wrapperCalls).flatMap { sink => // Data flows into the arguments; asking the call node itself finds nothing. // `call.argument` includes the RECEIVER at argumentIndex 0 (`db` in `db.execute(...)`), so counting it // makes a one-argument interpolated call look like a two-argument bound one -- the exact inversion of the @@ -1357,7 +1414,7 @@ // no data at all, yet `res` is reachable from the request in any handler that captures it, so the flow // query answered yes about a call that receives nothing. Measured on NodeGoat, where `res.write(body)` is a // real finding and `res.end()` two lines below it was reported identically. - val flows = if (hasSources) sink.argument.argumentIndexGt(0).reachableByFlows(sourceNodes).l.filter(plausible) else Nil + val flows = if (hasSources) sinkTargets(sink).reachableByFlows(sourceNodes).l.filter(plausible) else Nil // The SHAPE of the sink call, which is what distinguishes a fix from a bug when the fix is a safe form // rather than a sanitizing call: `execute(sql, params)` binds where `execute(sql + x)` interpolates, and diff --git a/tests/test_cpg_queries_compile.py b/tests/test_cpg_queries_compile.py index 7aa9120..82640cc 100644 --- a/tests/test_cpg_queries_compile.py +++ b/tests/test_cpg_queries_compile.py @@ -1292,6 +1292,7 @@ def _taint_rows(tmp_path: Path, frontend_name: str, files: dict[str, str], reque src = tmp_path / "src" src.mkdir() for name, text in files.items(): + (src / name).parent.mkdir(parents=True, exist_ok=True) (src / name).write_text(text) cpg = tmp_path / "cpg.bin" built = subprocess.run([frontend, str(src), "-o", str(cpg)], capture_output=True, text=True, timeout=600, check=False) @@ -1417,3 +1418,69 @@ def test_an_allowlist_check_guards_its_branch_and_its_ternary_arm(tmp_path: Path payload = _taint_rows(tmp_path, "php2cpg", {"app.php": app}, {n: {**common, "function": n} for n in names}) reported = {n for n in names if _unsanitized(payload, n)} assert reported == {"wrong_arm", "else_branch", "stored_in_else", "haystack"}, {n: payload[n] for n in names} + + +_WRAPPED_PHP = ( + "link, $query ); }\n" + " function number( $n ) { return mysqli_query( $this->link, 'SELECT ' . intval( $n ) ); } }\n" + "class FormManager { public $dbService;\n" + " function create( $data ) { $query = 'INSERT INTO t VALUES (' . $data['id'] . ')';\n" + " return $this->dbService->query( $query ); } }\n" + "class FormController { public $formManager; public $dbService;\n" + " function displayAll() { $value = $_POST['form']; return $this->formManager->create( $value ); }\n" + " function direct() { return $this->dbService->query( 'SELECT ' . $_POST['q'] ); }\n" + " function literal() { return $this->dbService->query( 'SELECT 1' ); }\n" + " function cast() { return $this->dbService->number( $_POST['n'] ); } }\n" +) + + +def _wrapped_requests(spec, names, file="app.php"): + common = { + "sources": ",".join(spec.sources), + "sinks": ",".join(spec.sinks), + "sanitizers": ",".join(spec.sanitizers), + "quotedSanitizers": ",".join(spec.quoted_sanitizers), + "file": file, + } + return {n: {**common, "function": n} for n in names} + + +@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine") +def test_a_project_sink_wrapper_is_a_sink_where_it_is_called(tmp_path: Path) -> None: + """YesWiki runs its SQL through `$this->dbService->query($query)`, whose body is the `mysqli_query` the facts + name, through a receiver the frontend cannot type. One level only: `create`, which feeds the wrapper rather + than the driver, is not itself a sink -- the first version cascaded and flooded real projects.""" + from openultrasast.model.specs import taint_specs + + spec = taint_specs(language="php")["injection"] + names = ("displayAll", "direct", "literal", "cast") + payload = _taint_rows(tmp_path, "php2cpg", {"app.php": _WRAPPED_PHP}, _wrapped_requests(spec, names)) + reported = {n for n in names if [r for r in _unsanitized(payload, n) if r.get("sinkMethod") == n]} + assert reported == {"direct"}, {n: payload[n] for n in names} + + +@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine") +def test_a_wrapper_name_shared_with_a_harmless_method_is_not_a_sink(tmp_path: Path) -> None: + """Calls resolve by name, so a name is a sink only if every project method so named is a wrapper.""" + from openultrasast.model.specs import taint_specs + + spec = taint_specs(language="php")["injection"] + code = _WRAPPED_PHP + "class Cache { function query( $key ) { return strlen( $key ); } }\n" + payload = _taint_rows(tmp_path, "php2cpg", {"app.php": code}, _wrapped_requests(spec, ("direct",))) + assert not [r for r in _unsanitized(payload, "direct") if r.get("sinkMethod") == "direct"], payload["direct"] + + +@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine") +def test_a_bundled_library_method_is_not_a_wrapper(tmp_path: Path) -> None: + """PMPro bundles Braintree and Stripe under `includes/lib/`; the first version made their HTTP helpers sinks.""" + from openultrasast.model.specs import taint_specs + + spec = taint_specs(language="php")["injection"] + files = { + "lib/Db.php": "link, $q ); } }\n", + "app.php": "db->run( 'SELECT ' . $_POST['q'] ); } }\n", + } + payload = _taint_rows(tmp_path, "php2cpg", files, _wrapped_requests(spec, ("go",))) + assert not [r for r in _unsanitized(payload, "go") if r.get("sinkMethod") == "go"], payload["go"]