From 6ebbc89d64601dd9e289d44946b7d1d087d76b6e Mon Sep 17 00:00:00 2001 From: Marius Ciepluch <11855163+norandom@users.noreply.github.com> Date: Wed, 23 Sep 2026 21:47:41 +0200 Subject: [PATCH] fix(taint): a field is attacker input only when a caller feeds it request data The field join (task 5.11) seeded every parameter of every method in the file, so every setter made its field a source. Adjudicated on PMPro, six of eight false SQL injections in a 1-in-5 sample were object fields holding database or internal values ($this->membership_id, $morder->user_id). A parameter now seeds the join only where some call site passes it request data without a sanitizer on the way (memoised per method and index); a request source read in the file still seeds as before. A real-Joern control covers both directions -- a setter only given a constant stays quiet, a constructor handed $_GET (the CVE-2023-6559 shape) is still found -- and fails on the old query with exactly the PMPro false positive. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01EmM1j46qgKMh3zqD1QMwEo --- src/openultrasast/cpg/queries/taint.sc | 40 +++++++++++++++-- tests/test_cpg_queries_compile.py | 61 ++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 3 deletions(-) diff --git a/src/openultrasast/cpg/queries/taint.sc b/src/openultrasast/cpg/queries/taint.sc index d1a728f..8461d0e 100644 --- a/src/openultrasast/cpg/queries/taint.sc +++ b/src/openultrasast/cpg/queries/taint.sc @@ -81,7 +81,11 @@ // field join's flow queries. Keyed by the parameters the answer depends on, so one family's seeds never // answer another's. val methodsByFile = scala.collection.mutable.Map.empty[String, List[io.shiftleft.codepropertygraph.generated.nodes.Method]] - val seedsByFile = scala.collection.mutable.Map.empty[(String, String, String), List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode]] + val seedsByFile = scala.collection.mutable.Map.empty[(String, String, String, String), List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode]] + // Whether any caller hands a method's parameter request data, per (sources, sanitizers, method, index); and + // the request-source calls themselves, which every such question starts from. + val callerFedMemo = scala.collection.mutable.Map.empty[(String, String, String, Int), Boolean] + val sourceCallMemo = scala.collection.mutable.Map.empty[String, List[io.shiftleft.codepropertygraph.generated.nodes.Call]] val fieldTaintMemo = scala.collection.mutable.Map.empty[(String, String, String, String, String), Boolean] val hookKeysMemo = scala.collection.mutable.Map.empty[(String, String, String), Set[String]] @@ -268,14 +272,44 @@ // Framework sources are untrusted wherever they appear. Parameters are untrusted only where the caller has // said this region is an entry point -- the same contract `parameterNodes` carries, and for the same // reason: an arbitrary helper's parameters carry whatever its caller happened to have. + // + // A parameter seeds the FIELD join only where some CALLER hands it request data. Counting every parameter of + // every method in the file made each setter a source: `$morder->user_id = $user_id` in a gateway's hook + // callback, `$this->membership_id = ...` loaded from the order's own row. Adjudicated on PMPro 2026-09-23, + // six of eight false SQL injections in a 1-in-5 sample were exactly that -- object fields holding database + // or internal values, read as attacker input. MW WP Form's CVE-2023-6559, which the join exists for, is the + // case the rule keeps: its constructor's `$attachments` is passed the uploaded files by the controller. + def requestSourceCalls: List[io.shiftleft.codepropertygraph.generated.nodes.Call] = + sourceCallMemo.getOrElseUpdate(sourcesS, cpg.call.filter(c => sourcePatterns.exists(p => c.code.contains(p))).l) + + def fedByCaller( + m: io.shiftleft.codepropertygraph.generated.nodes.Method, + param: io.shiftleft.codepropertygraph.generated.nodes.MethodParameterIn + ): Boolean = + callerFedMemo.getOrElseUpdate( + (sourcesS, sanitizersS, m.fullName, param.index), { + val sources = requestSourceCalls + sources.nonEmpty && m.callIn.l.exists { call => + call.argument.l.filter(_.argumentIndex == param.index).exists { arg => + arg.ast.isCall.exists(c => sourcePatterns.exists(p => c.code.contains(p))) || + arg.start.reachableByFlows(sources.iterator).l.exists(f => + !f.elements.map(_.code).l.exists(code => mentionsToken(code, sanitizerNames)) + ) + } + } + } + ) + def seedsIn(fileName: String) = seedsByFile.getOrElseUpdate( - (sourcesS, paramSrc, fileName), { + (sourcesS, sanitizersS, paramSrc, fileName), { val methods = methodsIn(fileName) val framework: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] = methods.flatMap(_.ast.isCall.filter(c => sourcePatterns.exists(p => c.code.contains(p))).l) val params: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] = - if (parameterSources == "true") methods.flatMap(_.parameter.l) else Nil + if (parameterSources == "true") + methods.flatMap(m => m.parameter.l.filter(p => p.name != "this" && fedByCaller(m, p))) + else Nil framework ++ params } ) diff --git a/tests/test_cpg_queries_compile.py b/tests/test_cpg_queries_compile.py index 08c6c7a..bfea1d0 100644 --- a/tests/test_cpg_queries_compile.py +++ b/tests/test_cpg_queries_compile.py @@ -679,3 +679,64 @@ def test_context_locations_outside_the_changed_files_are_summarised(tmp_path: Pa assert kept == [r for r in whole if r["path"] == "b.js"], "the changed file's locations must be unchanged" assert len(summary) == 1 and summary[0]["paths"] == sorted({r["path"] for r in whole if r["path"] != "b.js"}) assert summary[0]["locations"] == len(whole) - len(kept) + + +@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine") +def test_a_field_is_attacker_input_only_when_a_caller_feeds_it_request_data(tmp_path: Path) -> None: + """The field join's parameter seeds are the parameters some caller passes request data. + + Counting every parameter of every method made each setter a source: on PMPro six of eight false SQL + injections in an adjudicated sample were object fields holding database or internal values. The shape the + join exists for -- MW WP Form's CVE-2023-6559, a constructor handed the uploads by the controller -- must + still be found. + """ + from openultrasast.cpg.backend import extract_payload + + frontend = __import__("shutil").which("php2cpg") + if frontend is None: + pytest.skip("php2cpg is not installed") + src = tmp_path / "src" + src.mkdir() + (src / "order.php").write_text( + "user_id = $user_id; }\n" + ' function load() { global $wpdb; return $wpdb->get_var("SELECT x FROM t WHERE id = \'" . $this->user_id . "\'"); }\n}\n' + "function internal_caller() { $o = new Order(); $o->set_user(42); return $o->load(); }\n" + ) + (src / "mailer.php").write_text( + "attachments = $attachments; }\n" + ' function send() { global $wpdb; return $wpdb->query("DELETE FROM t WHERE f = \'" . $this->attachments . "\'"); }\n}\n' + "function controller() { $m = new Mailer($_GET['files']); return $m->send(); }\n" + ) + cpg = tmp_path / "cpg.bin" + built = subprocess.run([frontend, str(src), "-o", str(cpg)], capture_output=True, text=True, timeout=600, check=False) + if not cpg.is_file(): + pytest.skip(f"could not build a sample cpg: {(built.stderr or '')[-200:]}") + common = {"sources": "$_GET", "sinks": "$wpdb->get_var,$wpdb->query", "parameterSources": "true"} + requests = tmp_path / "requests.json" + requests.write_text( + json.dumps( + {"internal": {**common, "function": "load", "file": "order.php"}, "fed": {**common, "function": "send", "file": "mailer.php"}} + ) + ) + done = subprocess.run( + [ + _joern() or "joern", + "--script", + str((QUERIES / "taint.sc").resolve()), + "--param", + f"cpgFile={cpg}", + "--param", + f"requestsFile={requests}", + ], + capture_output=True, + text=True, + timeout=900, + check=False, + cwd=str(tmp_path), + ) + assert done.returncode == 0, (done.stderr or done.stdout or "")[-500:] + payload = extract_payload(done.stdout or "") + assert isinstance(payload, dict) + flows = {rid: [r for r in payload[rid] if isinstance(r, dict) and r.get("sourceKind")] for rid in ("internal", "fed")} + assert not flows["internal"], f"a setter only ever given a constant made its field attacker input: {flows['internal']}" + assert flows["fed"], "a constructor handed $_GET data no longer carries it through the field to the sink"