diff --git a/src/openultrasast/cpg/queries/taint.sc b/src/openultrasast/cpg/queries/taint.sc index 6941f6b..e474ca1 100644 --- a/src/openultrasast/cpg/queries/taint.sc +++ b/src/openultrasast/cpg/queries/taint.sc @@ -144,9 +144,22 @@ // whole text, `apply_filters("h", md5("x"), $_GET["c"])` and `sanitize_text_field($_POST["x"])` were // sources themselves, so a filter's return was request data whatever it returned. The access inside the // argument is a source in its own right; nothing that reaches a sink through it is lost. + // Of the operators, only an ACCESS names its own value: `$_GET["c"]`, `req.body.x`. A conditional or a + // concatenation whose text contains `$_GET` is an expression built from the access, not the access -- matched + // on its text, `isset($_GET["id"]) ? intval($_GET["id"]) : null` was itself a source, downstream of the cast. + val accessOperators = Set( + ".fieldAccess", + ".indexAccess", + ".indirectFieldAccess", + ".indirectIndexAccess" + ) + def isSourceCall(c: io.shiftleft.codepropertygraph.generated.nodes.Call): Boolean = { - val named = if (c.name.startsWith(" named.contains(p)) + val named = + if (accessOperators.contains(c.name)) c.code + else if (c.name.startsWith(" named.contains(p)) } def frameworkSources = cpg.call.filter(isSourceCall) @@ -570,7 +583,7 @@ case target: io.shiftleft.codepropertygraph.generated.nodes.Call if target.name == INDEX_ACCESS => val key = keyOf(target) if (key.isEmpty) None - else if (args(1).start.reachableByFlows(seeds.iterator).l.filter(plausible).exists(f => !f.elements.l.exists(sanitizesHere))) + else if (args(1).start.reachableByFlows(seeds.iterator).l.filter(plausible).exists(f => !f.elements.l.exists(sanitizesHere) && !sanitizedBetween(f.elements.l))) Some(key) else None case _ => None @@ -697,6 +710,36 @@ byName || sanitizerNames.exists(n => code.startsWith(n + "(") || code.contains("'" + n + "'") || code.contains("\"" + n + "\"")) } + // A sanitizer the value passes THROUGH between two path elements. The engine can step from a sanitizer's + // argument straight to the expression around the call -- `$params["id"]` inside + // `isset($params["id"]) ? intval($params["id"]) : null` went to the conditional, and `intval` itself was + // never an element -- so checking elements alone reported PMPro's integer-cast REST level id as an SQL + // injection. Where the next element CONTAINS the previous one, the syntax between them is what the value + // was wrapped in; a sanitizer there sanitized it. A sanitizer elsewhere in the expression does not count. + def sanitizedBetween(elements: List[io.shiftleft.codepropertygraph.generated.nodes.AstNode]): Boolean = + elements.zip(elements.drop(1)).exists { case (inner, outer) => + var node: Option[io.shiftleft.codepropertygraph.generated.nodes.AstNode] = inner._astIn.nextOption() match { + case Some(parent: io.shiftleft.codepropertygraph.generated.nodes.AstNode) => Some(parent) + case _ => None + } + var wrapped = false + var reached = false + var depth = 0 + while (node.isDefined && !reached && depth < 8) { + val current = node.get + if (current.id == outer.id) reached = true + else { + if (sanitizesHere(current)) wrapped = true + node = current._astIn.nextOption() match { + case Some(parent: io.shiftleft.codepropertygraph.generated.nodes.AstNode) => Some(parent) + case _ => None + } + depth += 1 + } + } + reached && wrapped + } + // The candidate sink calls for a FAMILY do not depend on the region, and scanning every call in the graph // for each of 2,487 requests was a large share of what made a request cost seconds -- in evidence mode, // which asks for no dataflow at all, it alone took a query past an hour. Scanned once per sink list and @@ -1005,7 +1048,7 @@ flows .map { flow => val elements = flow.elements.map(_.code).l - val sanitized = sanitizerNames.nonEmpty && flow.elements.l.exists(node => sanitizesHere(node)) + val sanitized = sanitizerNames.nonEmpty && (flow.elements.l.exists(node => sanitizesHere(node)) || sanitizedBetween(flow.elements.l)) val sourceKind = sourceKindOf(flow.elements.l.headOption) ((sourceKind, elements.headOption.getOrElse("").take(200), sanitized), (elements.size, flow)) } diff --git a/tests/test_cpg_queries_compile.py b/tests/test_cpg_queries_compile.py index 8460800..9767041 100644 --- a/tests/test_cpg_queries_compile.py +++ b/tests/test_cpg_queries_compile.py @@ -858,3 +858,56 @@ def test_the_field_join_links_methods_only_through_the_object_itself(tmp_path: P 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" + + +@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine") +def test_a_sanitizer_the_value_passes_through_inside_an_expression_counts(tmp_path: Path) -> None: + """`isset($p["id"]) ? intval($p["id"]) : null`: the engine steps from intval's argument to the conditional, + so `intval` is never a path element. PMPro's REST level id, cast exactly so, was reported as an injection. + A sanitizer in the OTHER branch must not count.""" + 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( + "query("DELETE FROM t WHERE id = " . $id); }\n' + "function other_branch($x) { global $wpdb; $id = isset($_GET['id']) ? $_GET['id'] : intval($x);\n" + ' return $wpdb->query("DELETE FROM t WHERE id = " . $id); }\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->query", "sanitizers": "intval", "file": "app.php"} + requests = tmp_path / "requests.json" + requests.write_text(json.dumps({"cast": {**common, "function": "cast"}, "other": {**common, "function": "other_branch"}})) + 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) + unsanitized = { + rid: [r for r in payload[rid] if isinstance(r, dict) and r.get("sourceKind") and not r.get("sanitized")] + for rid in ("cast", "other") + } + 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"