fix(taint): a filter returns its value argument; a source is the access, not a call around it

The apply_filters class from the second PMPro adjudication (2 of 3
remaining false positives): `apply_filters('pmpro_random_code', $code,
$this)` returned whatever $this carried, and a report's `$type` reached SQL
only as an extra filter argument. Two causes, both fixed generally:

- The dispatch fact now says what an apply call RETURNS: `returns` names
  the calls whose result is the dispatched value and `value_arg` its
  position (WordPress: apply_filters/apply_filters_ref_array return
  argument 2; do_action returns nothing). A path through such a call must
  ENTER by that argument -- the first of the run of the call's arguments
  before it, because the engine hops between sibling arguments
  ($_GET["c"] -> md5("x") -> apply_filters). Loaded and validated as data;
  the query knows no framework.
- A call was a source when the pattern appeared anywhere in its text, so
  `apply_filters(..., $_GET["c"])` and `sanitize_text_field($_POST[..])`
  were sources themselves -- the "a token matches the thing it names"
  invariant. A call is now a source when the pattern names its callee,
  receiver or the access it is; the inner access stays a source.

Controls: dispatch facts load/validate; a real-Joern case with request
data as filter context (quiet) and as the filtered value (found). 1,453
host and 18 query tests pass.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01EmM1j46qgKMh3zqD1QMwEo

src/openultrasast/cpg/backend.py

@@ -181,7 +181,7 @@ FRONTEND_BUILD_ATTEMPTS = 4
 # across 2,500 requests made a 42.5MB request file of which 41.7MB was the same string over and over. Joern
 # parsed that with ujson inside a 2GB heap and the whole batch died in ForkJoinPool, which the driver
 # correctly reported as `query_failed` for all 2,500 regions -- a whole-repository scan that decided nothing.
-_SHARED_REQUEST_FIELDS = ("hookCallbacks", "dispatchApply", "contextFilter", "contextPaths")
+_SHARED_REQUEST_FIELDS = ("hookCallbacks", "dispatchApply", "dispatchValue", "contextFilter", "contextPaths")
 
 Runner = Callable[..., subprocess.CompletedProcess[str]]
 

src/openultrasast/cpg/queries/taint.sc

@@ -52,6 +52,7 @@
     evidenceOnly: String = "false",
     hookCallbacks: String = "",
     dispatchApply: String = "",
+    dispatchValue: String = "",
     contextFilter: String = "",
     contextPaths: String = "",
     requests: String = "",
@@ -91,6 +92,7 @@
       sanitizersS: String,
       hookCallbacksS: String,
       dispatchApplyS: String,
+      dispatchValueS: String,
       functionS: String,
       paramSrc: String,
       fieldParamSrc: String,
@@ -137,7 +139,17 @@
   // Framework sources: a call whose code contains one of the patterns. Matching on `code` rather than a
   // method name keeps `request.args["x"]` (an indexAccess over a fieldAccess) and `req.body.name` both
   // reachable without a per-framework rule for each.
-  def frameworkSources = cpg.call.filter(c => sourcePatterns.exists(p => c.code.contains(p)))
+  // A call IS a source when the pattern names the call itself -- its callee or receiver, or the access it is
+  // (`$_GET["c"]`, `req.body.x`) -- not when the pattern merely appears inside an argument. Matched on the
+  // 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.
+  def isSourceCall(c: io.shiftleft.codepropertygraph.generated.nodes.Call): Boolean = {
+    val named = if (c.name.startsWith("<operator")) c.code else c.code.takeWhile(_ != '(')
+    sourcePatterns.exists(p => named.contains(p))
+  }
+
+  def frameworkSources = cpg.call.filter(isSourceCall)
 
   // The labeled methods, and the region they lexically own. A closure defined inside the labeled function --
   // the callback passed to `fs.stat`, say -- has its own synthetic method (`<lambda>0`) whose astParentFullName
@@ -341,6 +353,34 @@
   // path's calls afresh cost one outlier request 101 s -> 160 s.
   val familySinkMemo = scala.collection.mutable.Map.empty[Long, Boolean]
 
+  // A registry read returns ONE of its arguments -- the dispatched value -- or nothing at all, per the facts'
+  // `returns`/`value_arg`. A path that enters such a call through any other argument has used the engine's
+  // default "every argument reaches the return", which is how `$this`, passed as filter context, turned a
+  // random md5 fragment into an SQL injection.
+  val dispatchValues: Map[String, Int] = split(dispatchValueS).flatMap { entry =>
+    entry.split(":") match {
+      case Array(name, position) => scala.util.Try(name.trim -> position.trim.toInt).toOption
+      case _                     => None
+    }
+  }.toMap
+
+  def offValueDispatch(elements: List[io.shiftleft.codepropertygraph.generated.nodes.AstNode]): Boolean =
+    dispatchValues.nonEmpty && elements.indices.exists { k =>
+      elements(k) match {
+        case call: io.shiftleft.codepropertygraph.generated.nodes.Call if k > 0 && dispatchValues.contains(call.name) =>
+          val position = dispatchValues(call.name)
+          // The argument the path ENTERED by, which is the first of the run of this call's arguments right
+          // before it: the engine hops between sibling arguments (`$_GET["c"]` -> `md5("x")` -> the call),
+          // so the one immediately before the call can be the value argument when the data came in as context.
+          val arguments = call.argument.l.map(a => a.id -> a).toMap
+          var j = k - 1
+          while (j >= 0 && arguments.contains(elements(j).id)) j -= 1
+          if (j == k - 1) false
+          else position == 0 || arguments(elements(j + 1).id).argumentIndex != position
+        case _ => false
+      }
+    }
+
   def plausible(flow: io.joern.dataflowengineoss.language.Path): Boolean = {
     val elements = flow.elements.l
     val throughSink = elements.dropRight(1).exists {
@@ -348,7 +388,7 @@
         familySinkMemo.getOrElseUpdate(c.id, familySinkCall(c))
       case _ => false
     }
-    !throughSink && !(elements.exists {
+    !throughSink && !offValueDispatch(elements) && !(elements.exists {
       case c: io.shiftleft.codepropertygraph.generated.nodes.Call => c.name == "<operator>.fieldAccess"
       case _                                                      => false
     } && absorbedByObject(elements))
@@ -362,7 +402,7 @@
       (sourcesS, fieldParamSrc, 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)
+          methods.flatMap(_.ast.isCall.filter(isSourceCall).l)
         val params: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] =
           if (fieldParamSrc == "true") methods.flatMap(_.parameter.l) else Nil
         framework ++ params
@@ -466,7 +506,7 @@
   // and only the sink is elsewhere. Task 5.13 has the numbers.
   def hookSeedsOf(callback: io.shiftleft.codepropertygraph.generated.nodes.Method) = {
     val framework: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] =
-      callback.ast.isCall.filter(c => sourcePatterns.exists(p => c.code.contains(p))).l
+      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))
     framework ++ fields
@@ -1039,6 +1079,7 @@
           // -region path.
           if (field("hookCallbacks").nonEmpty) field("hookCallbacks") else hookCallbacks,
           if (field("dispatchApply").nonEmpty) field("dispatchApply") else dispatchApply,
+          if (field("dispatchValue").nonEmpty) field("dispatchValue") else dispatchValue,
           field("function"),
           paramSrc,
           fieldParamSrc,
@@ -1063,7 +1104,7 @@
   } else {
     println("---OUSAST-CPG-BEGIN---")
     println(
-      ujson.write(ujson.Arr(rowsFor(sources, sinks, sanitizers, hookCallbacks, dispatchApply, function, parameterSources, parameterSources, fieldSources, evidenceOnly, file, callDepth, boundedSinks): _*))
+      ujson.write(ujson.Arr(rowsFor(sources, sinks, sanitizers, hookCallbacks, dispatchApply, dispatchValue, function, parameterSources, parameterSources, fieldSources, evidenceOnly, file, callDepth, boundedSinks): _*))
     )
   }
   println("---OUSAST-CPG-END---")

src/openultrasast/model/specs.py

@@ -115,6 +115,9 @@ class TaintSpec:
     # The calls that read a value back out of a string-keyed registry (`apply_filters`, `do_action`). Facts,
     # not constants, so a second framework is a row in a TOML rather than an edit to the query.
     dispatch_apply: tuple[str, ...] = ()
+    # Which argument of each apply call its RESULT comes from, as `call:position`; position 0 means the call
+    # returns nothing a flow can carry. Only calls a fact describes appear here.
+    dispatch_value: tuple[str, ...] = ()
 
 
 @dataclass(frozen=True)
@@ -243,6 +246,11 @@ def taint_specs(
         return bool(getattr(fact, "parameterized", False)) or getattr(fact, "literal_format_arg", None) is not None
 
     dispatch_apply = tuple(sorted({call for fact in scoped.dispatches for call in fact.apply}))
+    dispatch_value = tuple(
+        sorted(
+            {f"{call}:{fact.value_arg if call in fact.returns else 0}" for fact in scoped.dispatches if fact.returns for call in fact.apply}
+        )
+    )
     sanitizers = tuple(sorted({call for fact in scoped.sanitizers if not _is_shape(fact) for call in fact.calls}))
     safe_shapes = tuple(sorted({call for fact in scoped.sanitizers if _is_shape(fact) for call in fact.calls}))
     by_family: dict[str, set[str]] = {}
@@ -266,6 +274,7 @@ def taint_specs(
             bounded_sinks=tuple(sorted(bounded.get(family_id, ()))),
             safe_shape_sinks=safe_shapes,
             dispatch_apply=dispatch_apply,
+            dispatch_value=dispatch_value,
         )
         for family_id, calls in sorted(by_family.items())
     }

src/openultrasast/model/taint.py

@@ -59,6 +59,7 @@ def request_params(
         # The registry's own vocabulary, from the fact tables. Without it the query would have to know what
         # `apply_filters` is, which makes a general mechanism into a WordPress feature.
         "dispatchApply": spec.dispatch_apply,
+        "dispatchValue": spec.dispatch_value,
     }
 
 

src/openultrasast/ruleset/semantic/php.toml

@@ -208,6 +208,11 @@ parameterized = true
 id = "wordpress_hooks"
 register = ["add_action", "add_filter", "add_shortcode"]
 apply = ["apply_filters", "apply_filters_ref_array", "do_action", "do_action_ref_array"]
+# A filter returns its VALUE argument, filtered; its further arguments are context and never the result, and
+# an action returns nothing. Without this, `apply_filters('pmpro_random_code', $code, $this)` returned
+# whatever `$this` carried, and a random md5 fragment became an SQL injection.
+returns = ["apply_filters", "apply_filters_ref_array"]
+value_arg = 2
 
 # What a PHP repository's paths say about scope. `vendored` trees are out of the scan and out of the graph
 # (a bundled library is a separate unit); `tests` paths stay in scope but are not product, so their

src/openultrasast/semantic/facts.py

@@ -72,6 +72,14 @@ class DispatchFact:
     register: tuple[str, ...]
     apply: tuple[str, ...]
     language: str
+    # What an apply call RETURNS. `returns` names the apply calls whose result is the dispatched value, and
+    # `value_arg` is that value's argument position (1-based, receiver excluded): WordPress's
+    # `apply_filters('hook', $value, ...$context)` returns the filtered `$value`, and its context arguments
+    # never become the result. An apply call not named in `returns` returns nothing a flow can carry
+    # (`do_action`). Without this the engine let EVERY argument reach the return: PMPro's random order code
+    # came out "tainted" because `$this` was passed as filter context. Empty keeps the engine's default.
+    returns: tuple[str, ...] = ()
+    value_arg: int = 0
 
 
 @dataclass(frozen=True)
@@ -164,11 +172,30 @@ def _dispatches(value: object, language: str, path: Path) -> list[DispatchFact]:
                 register=_strings(item.get("register"), "register", path),
                 apply=_strings(item.get("apply"), "apply", path),
                 language=language,
+                returns=_returning(item, path),
+                value_arg=_value_arg(item, path),
             )
         )
     return facts
 
 
+def _returning(item: dict[str, object], path: Path) -> tuple[str, ...]:
+    returns = _strings(item.get("returns", []), "returns", path)
+    unknown = set(returns) - set(_strings(item.get("apply"), "apply", path))
+    if unknown:
+        raise FactLoadError(f"dispatch {item.get('id')!r} in {path}: returns names calls that are not apply calls: {sorted(unknown)}")
+    return returns
+
+
+def _value_arg(item: dict[str, object], path: Path) -> int:
+    value = item.get("value_arg", 0)
+    if not isinstance(value, int) or isinstance(value, bool) or value < 0:
+        raise FactLoadError(f"dispatch {item.get('id')!r} in {path}: value_arg must be a non-negative integer")
+    if item.get("returns") and value < 1:
+        raise FactLoadError(f"dispatch {item.get('id')!r} in {path}: returns needs the value's argument position")
+    return value
+
+
 def _layouts(value: object, language: str, path: Path) -> list[LayoutFact]:
     facts: list[LayoutFact] = []
     for item in _items(value):