From d5277c60d0e3a737378745dbe7650f161c5dc8e2 Mon Sep 17 00:00:00 2001 From: Marius Ciepluch <11855163+norandom@users.noreply.github.com> Date: Thu, 24 Sep 2026 15:53:57 +0200 Subject: [PATCH] fix(taint): a field the method itself just overwrote is not object state The largest remaining class in the third PMPro sample (2 of 4 false positives): every MemberOrder method builds its statement in $this->sqlQuery and runs it at once. One method writes request data there, the field join keyed the field file-wide, and deleteMe() -- which had just set it from the internal $this->id -- and updateTimestamp() (no caller in the plugin) were reported as SQL injections. A field read is now a field-join source only when it reads state another method left: not the target of a write, and not a read that the method's own write to the same field dominates. If that own write carries request data, the ordinary flow from the source still finds it. A real-Joern control covers all three: the method whose write carries $_POST (found), the method that overwrote it with an internal id (quiet), and a method reading what another left (found). It fails on the old query. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01EmM1j46qgKMh3zqD1QMwEo --- src/openultrasast/cpg/queries/taint.sc | 24 +++++++++++- tests/test_cpg_queries_compile.py | 52 ++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 2 deletions(-) diff --git a/src/openultrasast/cpg/queries/taint.sc b/src/openultrasast/cpg/queries/taint.sc index e474ca1..66d67d5 100644 --- a/src/openultrasast/cpg/queries/taint.sc +++ b/src/openultrasast/cpg/queries/taint.sc @@ -479,11 +479,31 @@ // Off by request only, and defaulting to ON when the field is absent, so nothing about the shipped // behaviour depends on a caller remembering to set it. It exists so the join can be measured against // itself: a cost you cannot switch off is a cost you cannot attribute. + // A field read is a source only where it reads OBJECT STATE -- a value some other method left there. Two + // reads are not that: + // * the target of a write (`$this->sqlQuery = ...`), which is a field-access node like any read; + // * a read the method's own write to the same field dominates. Every PMPro `MemberOrder` method builds its + // statement in `$this->sqlQuery` and runs it at once; because one method writes request data there, the + // join marked the field tainted everywhere, and `deleteMe()` -- which had just set it from the internal + // `$this->id` -- became an SQL injection. After a dominating write the value is the method's own, and if + // that write carries request data the ordinary flow from the source already finds it. + def writeTarget(c: io.shiftleft.codepropertygraph.generated.nodes.Call): Boolean = + c.argumentIndex == 1 && (c.astParent match { + case parent: io.shiftleft.codepropertygraph.generated.nodes.Call => parent.name == ASSIGNMENT + case _ => false + }) + + def overwrittenBefore(read: io.shiftleft.codepropertygraph.generated.nodes.Call): Boolean = + read.dominatedBy.isCall + .nameExact(ASSIGNMENT) + .exists(a => a.argument.l.find(_.argumentIndex == 1).exists(_.code.trim == read.code.trim)) + def fieldSourceNodes = if (fieldSrc != "true") Iterator.empty else { - val reads = cpg.call.nameExact(FIELD_ACCESS).filter(c => inScope(c.method)).l - if (reads.isEmpty) Iterator.empty else reads.filter(r => taintedPrefixes(r.code.trim, fileS, r.method)).iterator + val reads = cpg.call.nameExact(FIELD_ACCESS).filter(c => inScope(c.method)).l.filterNot(writeTarget) + if (reads.isEmpty) Iterator.empty + else reads.filter(r => taintedPrefixes(r.code.trim, fileS, r.method) && !overwrittenBefore(r)).iterator } // ---- the hook half of the two-stage join (task 5.11) ---------------------------------------------- diff --git a/tests/test_cpg_queries_compile.py b/tests/test_cpg_queries_compile.py index 9767041..bac74bb 100644 --- a/tests/test_cpg_queries_compile.py +++ b/tests/test_cpg_queries_compile.py @@ -911,3 +911,55 @@ def test_a_sanitizer_the_value_passes_through_inside_an_expression_counts(tmp_pa } assert not unsanitized["cast"], f"a value cast inside a conditional was reported unsanitized: {unsanitized['cast']}" assert unsanitized["other"], "a sanitizer in the other branch of the conditional cleaned the tainted branch" + + +@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine") +def test_a_field_the_method_just_overwrote_is_not_object_state(tmp_path: Path) -> None: + """Every PMPro MemberOrder method builds its statement in `$this->sqlQuery` and runs it at once. One writes + request data there; `deleteMe()` had just set it from the internal id and was reported. A method that READS + state another method left must still be found, and so must the method whose own write carries the input.""" + 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( + "sqlQuery = \"SELECT 1 WHERE c = '\" . $_POST['c'] . \"'\"; return $wpdb->query($this->sqlQuery); }\n" + " function deleteMe() { global $wpdb;\n" + ' $this->sqlQuery = "DELETE FROM t WHERE id = \'" . $this->id . "\'"; return $wpdb->query($this->sqlQuery); }\n' + " function rerun() { global $wpdb; return $wpdb->query($this->sqlQuery); }\n}\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": "$_POST", "sinks": "$wpdb->query", "file": "order.php", "fieldParameterSources": "true"} + requests = tmp_path / "requests.json" + requests.write_text(json.dumps({name: {**common, "function": name} for name in ("find", "deleteMe", "rerun")})) + 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 ("find", "deleteMe", "rerun")} + assert flows["find"], "the method whose own write carries request data must be found" + assert not flows["deleteMe"], f"a field the method had just overwritten was read as object state: {flows['deleteMe']}" + assert flows["rerun"], "a method reading the state another method left must still be found"