diff --git a/src/openultrasast/cpg/queries/taint.sc b/src/openultrasast/cpg/queries/taint.sc index ba800e2..6941f6b 100644 --- a/src/openultrasast/cpg/queries/taint.sc +++ b/src/openultrasast/cpg/queries/taint.sc @@ -409,13 +409,29 @@ } ) - def fieldIsTainted(fileName: String, fieldCode: String): Boolean = + // The join carries OBJECT STATE between methods: `$this->attachments` written in the constructor, read in + // `_delete_files()`. That is what a field keyed by its text means only when the base IS the object. Any other + // base is a variable, and the same text in two functions names two variables: PMPro's + // `$user = new stdClass; $user->ID = $_POST['user_id']` in one function made the `WP_User` parameter's + // `$user->ID` in another an SQL injection source. So a non-object base is joined within its own method only, + // where it is one variable -- and where the engine's own dataflow already sees it. + val objectBases = Set("$this", "this", "self") + + def fieldScope(fieldCode: String, reader: io.shiftleft.codepropertygraph.generated.nodes.Method): String = { + val base = fieldCode.split("->|\\.").headOption.map(_.trim).getOrElse("") + if (objectBases.contains(base)) "" else reader.fullName + } + + def fieldIsTainted(fileName: String, fieldCode: String, scope: String = ""): Boolean = fieldTaintMemo.getOrElseUpdate( - (sourcesS, fieldParamSrc, sanitizersS, fileName, fieldCode), { + (sourcesS, fieldParamSrc, sanitizersS, fileName, scope + "|" + fieldCode), { val seeds = seedsIn(fileName) if (seeds.isEmpty) false else - methodsIn(fileName).flatMap(_.ast.isCall.nameExact(ASSIGNMENT).l).exists { assignment => + methodsIn(fileName) + .filter(m => scope.isEmpty || m.fullName == scope) + .flatMap(_.ast.isCall.nameExact(ASSIGNMENT).l) + .exists { assignment => val args = assignment.argument.l args.size >= 2 && (args.head match { case target: io.shiftleft.codepropertygraph.generated.nodes.Call @@ -437,10 +453,14 @@ // `$this->rest_hits` in one method and reads `$this->rest_hits->current_page_id` in another, so an exact // code match sees two unrelated strings where the source text says one is inside the other. Every prefix // that ends on a `->` boundary is checked, left to right. - def taintedPrefixes(code: String, fileName: String): Boolean = { + def taintedPrefixes(code: String, fileName: String, reader: io.shiftleft.codepropertygraph.generated.nodes.Method): Boolean = { val parts = code.split("->").map(_.trim).filter(_.nonEmpty) if (parts.length < 2) false - else (2 to parts.length).exists(n => fieldIsTainted(fileName, parts.take(n).mkString("->"))) + else + (2 to parts.length).exists { n => + val prefix = parts.take(n).mkString("->") + fieldIsTainted(fileName, prefix, fieldScope(prefix, reader)) + } } // Off by request only, and defaulting to ON when the field is absent, so nothing about the shipped @@ -450,7 +470,7 @@ 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)).iterator + if (reads.isEmpty) Iterator.empty else reads.filter(r => taintedPrefixes(r.code.trim, fileS, r.method)).iterator } // ---- the hook half of the two-stage join (task 5.11) ---------------------------------------------- @@ -508,7 +528,7 @@ val framework: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] = callback.ast.isCall.filter(isSourceCall).l val fields: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] = - callback.ast.isCall.nameExact(FIELD_ACCESS).l.filter(r => taintedPrefixes(r.code.trim, callback.filename)) + callback.ast.isCall.nameExact(FIELD_ACCESS).l.filter(r => taintedPrefixes(r.code.trim, callback.filename, r.method)) framework ++ fields } @@ -811,7 +831,7 @@ .nameExact(FIELD_ACCESS) .filter(c => inScope(c.method)) .l - .exists(r => prefixKind(r.code.trim, fedFields(r.method.filename)) == "parameter" && taintedPrefixes(r.code.trim, r.method.filename)) + .exists(r => prefixKind(r.code.trim, fedFields(r.method.filename)) == "parameter" && taintedPrefixes(r.code.trim, r.method.filename, r.method)) if (confirmed) "source" else "parameter" } val carried = carriedKind.nonEmpty diff --git a/tests/test_cpg_queries_compile.py b/tests/test_cpg_queries_compile.py index 4354527..8460800 100644 --- a/tests/test_cpg_queries_compile.py +++ b/tests/test_cpg_queries_compile.py @@ -802,3 +802,59 @@ def test_a_filter_returns_its_value_argument_not_its_context(tmp_path: Path) -> flows = {rid: [r for r in payload[rid] if isinstance(r, dict) and r.get("sourceKind")] for rid in ("context", "value")} assert not flows["context"], f"filter context became its result: {flows['context']}" assert flows["value"], "the filtered value no longer carries request data" + + +@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine") +def test_the_field_join_links_methods_only_through_the_object_itself(tmp_path: Path) -> None: + """`$this->x` written in one method and read in another is object state; `$user->ID` in two functions is + two variables. PMPro's `$user = new stdClass; $user->ID = $_POST['user_id']` in one function made the + WP_User parameter's `$user->ID` in another an SQL injection.""" + 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 / "app.php").write_text( + "ID = $_POST['user_id']; return $user; }\n" + 'function history($user) { global $wpdb; return $wpdb->get_var("SELECT 1 WHERE u = \'" . $user->ID . "\'"); }\n' + "class Mailer {\n public $attachments;\n function collect() { $this->attachments = $_POST['files']; }\n" + ' function send() { global $wpdb; return $wpdb->query("DELETE FROM t WHERE f = \'" . $this->attachments . "\'"); }\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->get_var,$wpdb->query", + "file": "app.php", + "parameterSources": "false", + "fieldParameterSources": "true", + } + requests = tmp_path / "requests.json" + requests.write_text(json.dumps({"local": {**common, "function": "history"}, "object": {**common, "function": "send"}})) + 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 ("local", "object")} + assert not flows["local"], f"another function's local variable tainted this parameter's field: {flows['local']}" + assert flows["object"], "object state written in one method and read in another is no longer traced"