diff --git a/src/openultrasast/cpg/queries/taint.sc b/src/openultrasast/cpg/queries/taint.sc index 1af59a3..5c13769 100644 --- a/src/openultrasast/cpg/queries/taint.sc +++ b/src/openultrasast/cpg/queries/taint.sc @@ -72,7 +72,6 @@ // Per-family answers that do not depend on the region, kept across the whole batch. val familyInRepoMemo = scala.collection.mutable.Map.empty[String, Boolean] val sinkCandidatesMemo = scala.collection.mutable.Map.empty[String, List[io.shiftleft.codepropertygraph.generated.nodes.Call]] - val wrapperMemo = scala.collection.mutable.Map.empty[String, Map[String, Set[Int]]] val reachableMemo = scala.collection.mutable.Map.empty[(String, String, Int), Set[String]] val methodSourceMemo = scala.collection.mutable.Map.empty[String, Boolean] val fedFieldsMemo = scala.collection.mutable.Map.empty[String, Map[String, String]] @@ -1105,99 +1104,9 @@ // 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 // kept for the batch; the per-request work is the scope filter over that list. - // ---- project-local sink WRAPPERS ------------------------------------------------------------------------ - // - // Real code rarely calls the driver where it builds the query. YesWiki builds its INSERT in - // `FormManager::create($data)` and runs it through `$this->dbService->query($query)`, whose body is the - // `mysqli_query` the facts name -- so the one sink the facts knew sat in a helper no request ever reached - // directly, and the injectable value was never asked about where it entered. A method whose parameter - // feeds a family sink is itself a sink for that argument, reported where it is CALLED. - // - // Summarised STRUCTURALLY, not by dataflow: the parameter reaches the sink call's argument directly or - // through at most two local assignments, and at least one occurrence is outside every cleansing call. A - // flow query per parameter per method, repeated at every engine start of a portioned scan, is what this - // project's scale walls were made of. Up to three levels (a wrapper of a wrapper). Calls resolve by NAME, - // because an injected receiver (`$this->dbService`) has no type the frontend can see -- so a name counts - // only if every project method so named is a wrapper; one harmless `query()` elsewhere disqualifies it. - def underCleansing(node: io.shiftleft.codepropertygraph.generated.nodes.AstNode, root: io.shiftleft.codepropertygraph.generated.nodes.AstNode): Boolean = { - var current: Option[io.shiftleft.codepropertygraph.generated.nodes.AstNode] = Some(node) - var found = false - while (current.isDefined && !found && current.get.id != root.id) { - if (current.get.id != node.id && sanitizesHere(current.get, strictNames)) found = true - current = current.get._astIn.nextOption() match { - case Some(parent: io.shiftleft.codepropertygraph.generated.nodes.AstNode) => Some(parent) - case _ => None - } - } - found - } - - def feeds( - method: io.shiftleft.codepropertygraph.generated.nodes.Method, - expression: io.shiftleft.codepropertygraph.generated.nodes.AstNode, - name: String, - depth: Int - ): Boolean = - depth <= 2 && { - val identifiers = expression.ast.isIdentifier.l - identifiers.exists(i => i.name == name && !underCleansing(i, expression)) || - identifiers.map(_.name).distinct.filter(_ != name).exists { local => - method.ast.isCall.nameExact(ASSIGNMENT).l.exists { assignment => - val args = assignment.argument.l - args.find(_.argumentIndex == 1).exists { - case target: io.shiftleft.codepropertygraph.generated.nodes.Identifier => target.name == local - case _ => false - } && args.find(_.argumentIndex == 2).exists(value => feeds(method, value, name, depth + 1)) - } - } - } - - lazy val wrappers: Map[String, Set[Int]] = - wrapperMemo.getOrElseUpdate( - sinksS + "|" + sanitizersS + "|" + quotedSanitizersS, { - var known = Map.empty[String, Set[Int]] - var wrapperMethods = Set.empty[String] - var frontier = sinkCandidatesMemo.getOrElseUpdate(sinksS, cpg.call.filter(c => sinkNames.exists(n => sinkMatches(c, n))).l) - var level = 0 - var growing = true - while (level < 3 && growing && frontier.nonEmpty) { - val found = frontier.groupBy(_.method).toList.flatMap { case (method, calls) => - if (method.isExternal || method.name.startsWith("<")) Nil - else - method.parameter.l - .filter(p => p.index >= 1 && !objectBases.contains(p.name) && p.name != "self" && p.name != "this") - .filter(p => calls.exists(c => c.argument.argumentIndexGt(0).l.exists(a => feeds(method, a, p.name, 0)))) - .map(p => (method, p.index)) - } - val added = found.filterNot { case (m, index) => known.get(m.name).exists(_.contains(index)) } - growing = added.nonEmpty - added.foreach { case (m, index) => - known = known.updated(m.name, known.getOrElse(m.name, Set.empty[Int]) + index) - wrapperMethods += m.fullName - } - val names = added.map(_._1.name).toSet - frontier = if (names.isEmpty) Nil else cpg.call.filter(c => !c.name.startsWith(" cpg.method.nameExact(name).filterNot(_.isExternal).l.forall(m => wrapperMethods.contains(m.fullName)) } - } - ) - - def wrapperCall(c: io.shiftleft.codepropertygraph.generated.nodes.Call): Boolean = - !c.name.startsWith(" sinkMatches(c, n)) - - // The arguments a flow must arrive through: every real argument of a fact sink, only the summarised ones of - // a wrapper. - def sinkTargets(c: io.shiftleft.codepropertygraph.generated.nodes.Call) = - if (wrapperCall(c)) c.argument.l.filter(a => wrappers(c.name).contains(a.argumentIndex)) - else c.argument.argumentIndexGt(0).l - def sinkCalls = sinkCandidatesMemo - .getOrElseUpdate("wrapped|" + sinksS + "|" + sanitizersS + "|" + quotedSanitizersS, { - val facts = sinkCandidatesMemo.getOrElseUpdate(sinksS, cpg.call.filter(c => sinkNames.exists(n => sinkMatches(c, n))).l) - if (wrappers.isEmpty) facts else facts ++ cpg.call.filter(wrapperCall).l - }) + .getOrElseUpdate(sinksS, cpg.call.filter(c => sinkNames.exists(n => sinkMatches(c, n))).l) .filter(c => inScope(c.method)) // ---- stage one of the two-stage join WITHOUT dataflow, for evidence mode (flow-aware-ranking, phase 2) -- @@ -1448,7 +1357,7 @@ // no data at all, yet `res` is reachable from the request in any handler that captures it, so the flow // query answered yes about a call that receives nothing. Measured on NodeGoat, where `res.write(body)` is a // real finding and `res.end()` two lines below it was reported identically. - val flows = if (hasSources) sinkTargets(sink).reachableByFlows(sourceNodes).l.filter(plausible) else Nil + val flows = if (hasSources) sink.argument.argumentIndexGt(0).reachableByFlows(sourceNodes).l.filter(plausible) else Nil // The SHAPE of the sink call, which is what distinguishes a fix from a bug when the fix is a safe form // rather than a sanitizing call: `execute(sql, params)` binds where `execute(sql + x)` interpolates, and diff --git a/tests/test_cpg_queries_compile.py b/tests/test_cpg_queries_compile.py index 34eefc5..7aa9120 100644 --- a/tests/test_cpg_queries_compile.py +++ b/tests/test_cpg_queries_compile.py @@ -1417,56 +1417,3 @@ def test_an_allowlist_check_guards_its_branch_and_its_ternary_arm(tmp_path: Path payload = _taint_rows(tmp_path, "php2cpg", {"app.php": app}, {n: {**common, "function": n} for n in names}) reported = {n for n in names if _unsanitized(payload, n)} assert reported == {"wrong_arm", "else_branch", "stored_in_else", "haystack"}, {n: payload[n] for n in names} - - -_WRAPPED_PHP = ( - "link, $query ); }\n" - " function number( $n ) { return mysqli_query( $this->link, 'SELECT ' . intval( $n ) ); } }\n" - "class FormManager { public $dbService;\n" - " function create( $data ) { $query = 'INSERT INTO t VALUES (' . $data['id'] . ')';\n" - " return $this->dbService->query( $query ); } }\n" - "class FormController { public $formManager; public $dbService;\n" - " function displayAll() { $value = $_POST['form']; return $this->formManager->create( $value ); }\n" - " function direct() { return $this->dbService->query( 'SELECT ' . $_POST['q'] ); }\n" - " function literal() { return $this->dbService->query( 'SELECT 1' ); }\n" - " function cast() { return $this->dbService->number( $_POST['n'] ); } }\n" -) - - -def _wrapped_requests(spec, names): - common = { - "sources": ",".join(spec.sources), - "sinks": ",".join(spec.sinks), - "sanitizers": ",".join(spec.sanitizers), - "quotedSanitizers": ",".join(spec.quoted_sanitizers), - "file": "app.php", - } - return {n: {**common, "function": n} for n in names} - - -@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine") -def test_a_project_sink_wrapper_is_a_sink_where_it_is_called(tmp_path: Path) -> None: - """YesWiki runs its INSERT through `$this->dbService->query($query)`, whose body is the `mysqli_query` the facts - name, and builds it in `FormManager::create($data)`. Neither receiver has a type the frontend can see. The - request value is reported where it enters a wrapper -- two levels up -- and not where it cannot.""" - from openultrasast.model.specs import taint_specs - - spec = taint_specs(language="php")["injection"] - names = ("displayAll", "direct", "literal", "cast") - payload = _taint_rows(tmp_path, "php2cpg", {"app.php": _WRAPPED_PHP}, _wrapped_requests(spec, names)) - reported = {n: [r for r in _unsanitized(payload, n) if r.get("sinkMethod") == n] for n in names} - assert {n for n in names if reported[n]} == {"displayAll", "direct"}, {n: payload[n] for n in names} - assert "create" in str(reported["displayAll"][0]["sink"]), reported["displayAll"] - - -@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine") -def test_a_wrapper_name_shared_with_a_harmless_method_is_not_a_sink(tmp_path: Path) -> None: - """Calls resolve by name, so a name is a sink only if every project method so named is a wrapper.""" - from openultrasast.model.specs import taint_specs - - spec = taint_specs(language="php")["injection"] - code = _WRAPPED_PHP + "class Cache { function query( $key ) { return strlen( $key ); } }\n" - payload = _taint_rows(tmp_path, "php2cpg", {"app.php": code}, _wrapped_requests(spec, ("direct",))) - assert not [r for r in _unsanitized(payload, "direct") if r.get("sinkMethod") == "direct"], payload["direct"]