diff --git a/src/openultrasast/cpg/queries/taint.sc b/src/openultrasast/cpg/queries/taint.sc index ea83845..d1a728f 100644 --- a/src/openultrasast/cpg/queries/taint.sc +++ b/src/openultrasast/cpg/queries/taint.sc @@ -81,11 +81,7 @@ // 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, 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 seedsByFile = scala.collection.mutable.Map.empty[(String, String, String), List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode]] 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]] @@ -272,46 +268,14 @@ // 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, sanitizersS, paramSrc, fileName), { + (sourcesS, 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) - // Every parameter is a CANDIDATE start; whether its callers feed it is asked only of a parameter that - // actually begins an unsanitized flow into the field being read (`fieldIsTainted`). Asked of every - // parameter up front, the caller check ran a repository-wide flow query per parameter of every - // method in the file and cut PMPro's completed questions from 272 to 67 in the same budget. val params: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] = - if (parameterSources == "true") methods.flatMap(_.parameter.l.filter(_.name != "this")) else Nil + if (parameterSources == "true") methods.flatMap(_.parameter.l) else Nil framework ++ params } ) @@ -333,13 +297,7 @@ // queries that way: the fixed side of its pair went from 1 finding to 13 without this, // which is the pair no longer separating at all. val flows = args(1).start.reachableByFlows(seeds.iterator).l - flows.exists { f => - !f.elements.map(_.code).l.exists(code => mentionsToken(code, sanitizerNames)) && (f.elements.headOption match { - case Some(param: io.shiftleft.codepropertygraph.generated.nodes.MethodParameterIn) => - fedByCaller(param.method, param) - case _ => true - }) - } + flows.exists(f => !f.elements.map(_.code).l.exists(code => mentionsToken(code, sanitizerNames))) case _ => false }) } diff --git a/tests/test_cpg_queries_compile.py b/tests/test_cpg_queries_compile.py index bfea1d0..08c6c7a 100644 --- a/tests/test_cpg_queries_compile.py +++ b/tests/test_cpg_queries_compile.py @@ -679,64 +679,3 @@ 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"