From d6908f18ba84fea049d1e9bf07dcb9d66bca9dde Mon Sep 17 00:00:00 2001 From: Marius Ciepluch <11855163+norandom@users.noreply.github.com> Date: Wed, 9 Sep 2026 20:42:35 +0200 Subject: [PATCH] =?UTF-8?q?feat(cpg):=20the=20hook=20half=20=E2=80=94=20CV?= =?UTF-8?q?E-2022-25148=20found=20across=20an=20add=5Ffilter/apply=5Ffilte?= =?UTF-8?q?rs=20edge?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last of the three WordPress flaw classes 5.8 measured as missed. The value crosses a registry inside WordPress core: add_filter('wp_statistics_current_page', array($this, 'set_current_page')); return apply_filters('wp_statistics_current_page', $current_page); php2cpg drops the callback argument entirely -- the CPG holds `add_filter("wp_statistics_current_page", )` -- so the edge cannot come from the graph at any price. It comes from the source text instead, where both ends are literals, via `mapping.php_hook_callbacks`. vulnerable fixed wpstatistics 3 entailed, incl. pages.php:225:record 2, that one absent mwwpform 1 ($this->attachments) 0 pmpro 18, incl. CVE at :936 1 VAmPI 4 unchanged The correctly-prepared sibling query one line below the CVE is not flagged on either side. Four things this cost, each measured rather than guessed: * Half one must be asked per ARRAY KEY, not per callback. "Does an unsanitized value reach this callback's return" is TRUE on both sides: the fix escapes `type` and `id` and leaves `search_query` alone. A callback-level answer flags the fixed side as readily as the vulnerable one, which is a leak, not a detection. The key is a literal at both ends, so it joins the way the hook name and the field name do -- the same idea for the third time. * The sanitizer test had to stop crediting a sibling's cleansing. The vulnerable query is ONE concatenation holding both `esc_sql($page_uri)` and the injectable `{$current_page['id']}`, so asking whether a sanitizer's name appears in a path element's text marked the whole flow sanitized. An element must now BE the cleansing -- a call to it, or its name as a string literal, which keeps `array_map('esc_sql', ...)` working. * Hook seeds are repository-wide where field seeds are file-scoped. A hook registry is global by construction: the callback is in one file and its `apply_filters` in another. The query returned nothing until those two scopes were separated. * `sourceKind` had to be REPORTED, not inferred. By the time a row reaches Python a hook source reads as `$current_page["id"]` and a parameter as `$args`, and `_usable_flows` discards parameter flows whenever a modelled one is present -- so seven sanitized `$_SERVER["REQUEST_URI"]` flows displaced the CVE between the query and the verdict. That preference then had to move PER SINK SITE: applied across a region it decides one site's question with another site's evidence, and PMPro's CVE at :936 vanished from a region still reporting fifteen other findings. `verdict` now delegates to `verdicts` rather than running its own selection over the union, because two copies of "which flow decides" is exactly the duplication this module has been bitten by before -- and had been again: the per-site preference did not reach the single-verdict path, so the two entry points disagreed about the same rows. 841 tests pass; gates byte-identical; VAmPI unchanged. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01T7XD7x5KonMdmGRkp63EQG --- .kiro/specs/contributor-scan/tasks.md | 38 +++- benchmarks/repos/README.md | 14 +- src/openultrasast/cpg/queries/taint.sc | 229 +++++++++++++++++++++++-- src/openultrasast/mapping.py | 30 ++++ src/openultrasast/model/scan.py | 32 +++- src/openultrasast/model/taint.py | 59 +++++-- 6 files changed, 362 insertions(+), 40 deletions(-) diff --git a/.kiro/specs/contributor-scan/tasks.md b/.kiro/specs/contributor-scan/tasks.md index aa779e6..826f131 100644 --- a/.kiro/specs/contributor-scan/tasks.md +++ b/.kiro/specs/contributor-scan/tasks.md @@ -590,7 +590,7 @@ - _Requirements: 4.2, 4.3, 9.1_ - _Depends: 5.7_ -- [~] 5.11 Two-stage taint with a link table — the detection strategy for both structural misses +- [x] 5.11 Two-stage taint with a link table — the detection strategy for both structural misses - "Structurally impossible" is a verdict, not a plan, and both PHP structural misses turn out to be the SAME shape: two halves of a path, joined by a key that is a literal in the source text. @@ -654,8 +654,40 @@ analysis twice costs more than skipping path reconstruction saves. - Memoizing per **field** rather than per file took it to 97s, because only fields some region actually reads are worth the flow query. Roughly 1.6x the pre-field cost for that class. - - Still open: the hook half, and the one finding that survives on PMPro's fixed side - (`saveOrder:1469 <- $this->timestamp`). + - **Hook half done 2026-09-09, and CVE-2022-25148 is found.** + + | | vulnerable | fixed | + |---|---|---| + | `wpstatistics` | 3 entailed, incl. `class-wp-statistics-pages.php:225:record` | 2 entailed, that one absent | + | `mwwpform` | 1 (`$this->attachments`) | 0 | + | `pmpro` | 18, incl. CVE at `:936` | 1 | + | VAmPI | 4 | unchanged | + + The link table is `mapping.php_hook_callbacks`, read from the source text -- php2cpg drops a + registration's callback argument, so the CPG holds `add_filter("wp_statistics_current_page", )` and the + edge cannot come from the graph at any price. + - Four things this cost, each a measurement rather than a guess: + - **Half one must be asked per ARRAY KEY, not per callback.** "Does an unsanitized value reach this + callback's return" is TRUE on both sides: the fix escapes `type` and `id` and leaves `search_query` + alone. A callback-level answer flags the fixed side as readily as the vulnerable one, which is a leak. + The key is a literal at both ends -- `"id"` in the callback, `['id']` at the sink -- so it joins the + same way the hook name and the field name do. Third use of the one idea. + - **The sanitizer test had to stop crediting a sibling's cleansing.** WP Statistics' query is one + concatenation holding `esc_sql($page_uri)` AND the injectable `{$current_page['id']}`; asking whether a + sanitizer's name appears in a path element's text marked the whole flow sanitized. An element must now + BE the cleansing -- a call to it, or its name as a string literal, which keeps `array_map('esc_sql', ...)` + working. + - **Hook seeds are repository-wide where field seeds are file-scoped.** A hook registry is global by + construction: `set_current_page` is in one file and its `apply_filters` in another. The query returned + nothing at all until those two scopes were separated. + - **`sourceKind` had to be reported, not inferred.** By the time a row reaches Python a hook source reads + as `$current_page["id"]` and a parameter as `$args`. `_usable_flows` discards parameter flows whenever + a modelled one is present, so seven sanitized `$_SERVER["REQUEST_URI"]` flows displaced the CVE. And + that preference had to move PER SINK SITE: applied across a region it decided one site with another + site's evidence, and PMPro's CVE at :936 vanished from a region still reporting fifteen findings. + - Still open: array-key sensitivity is only modelled where the callback builds its result by key + assignment; the one finding surviving on PMPro's fixed side (`saveOrder:1469 <- $this->timestamp`); and + `verdict` now delegates to `verdicts`, so the two can no longer disagree. - _Requirements: 4.4, 6.1, 8.1, 9.1_ - _Depends: 5.8_ diff --git a/benchmarks/repos/README.md b/benchmarks/repos/README.md index 226cf2e..2d70c4c 100644 --- a/benchmarks/repos/README.md +++ b/benchmarks/repos/README.md @@ -70,7 +70,7 @@ text, and each measured against its own fixed commit. |---|---|---|---|---| | Unauthenticated injection, sink in the function that receives the value | `pmpro`, CVE-2023-23488 | entailed at `class.memberorder.php:936:getMemberOrderByCode`, sourced from `$code` | absent | **found**, and the pair separates | | Arbitrary file operation | `mwwpform`, CVE-2023-6559 | entailed at `class.mail.php:259:_delete_files` | absent | **found**, and the pair separates | -| Unauthenticated injection, value crosses a WordPress hook | `wpstatistics`, CVE-2022-25148 | not reported | not reported | **missed**, structurally | +| Unauthenticated injection, value crosses a WordPress hook | `wpstatistics`, CVE-2022-25148 | entailed at `class-wp-statistics-pages.php:225:record` | absent | **found**, and the pair separates | | Missing capability check | — | — | — | **cannot be asked**: PHP has no obligation facts | | Unescaped output | — | — | — | **cannot be trusted**: the sanitizer list is flat | @@ -135,7 +135,17 @@ not merely its reachability. Asking only "does a source reach this assignment" m fixed side went from 1 finding to 13 before that clause existed, which is the pair no longer separating at all. It costs about 1.6× on the taint query for that class (62s → 97s), memoised per field. -The hook half is the same machinery against a different key, and is not written yet. +**The hook half is implemented too, and CVE-2022-25148 is found**: `class-wp-statistics-pages.php:225:record` +on the vulnerable side, absent on the fixed side, with the correctly-`prepare`d sibling query one line below +it not flagged on either. The link table is read from the source text, because php2cpg drops a registration's +callback argument entirely -- the CPG holds `add_filter("wp_statistics_current_page", )`, so the edge cannot +come from the graph at any price. + +The half that mattered was asking per ARRAY KEY rather than per callback. "Does an unsanitized value reach +this callback's return" is true on *both* sides, because the fix escapes `type` and `id` and leaves +`search_query` alone; a callback-level answer flags the fix as readily as the bug. The key is a literal at +both ends — `"id"` where the callback writes it, `['id']` where the sink reads it — so it joins the same way +the hook name and the field name do. That is the same idea for the third time. What the engine reports on that plugin instead is two other sites — `getTop:406` and `TotalCount:443` — the same weakness class at the wrong lines, identical on both sides, so the pair does not separate. They are not diff --git a/src/openultrasast/cpg/queries/taint.sc b/src/openultrasast/cpg/queries/taint.sc index 23a0a44..84e451e 100644 --- a/src/openultrasast/cpg/queries/taint.sc +++ b/src/openultrasast/cpg/queries/taint.sc @@ -48,6 +48,7 @@ callDepth: String = "0", boundedSinks: String = "", parameterSources: String = "false", + hookCallbacks: String = "", requests: String = "", requestsFile: String = "" ) = { @@ -66,6 +67,7 @@ sourcesS: String, sinksS: String, sanitizersS: String, + hookCallbacksS: String, functionS: String, paramSrc: String, fileS: String, @@ -183,6 +185,21 @@ else if (function.isEmpty) cpg.method.parameter.filterNot(_.name == "this").iterator else cpg.method.nameExact(function).parameter.filterNot(_.name == "this").iterator + // The region this request is about, as a predicate on the method. Named rather than inlined because the + // field sources have to be scoped to exactly the same place as the sinks, and a second copy of these + // three branches is precisely the drift this file has been bitten by before. + def inScope(m: io.shiftleft.codepropertygraph.generated.nodes.Method): Boolean = + if (function.isEmpty) { + // A region with no enclosing function: the file IS the scope. + fileS.isEmpty || m.filename.endsWith(fileS) + } else if (depth > 0) { + // Following the call graph means leaving the region's file on purpose, so the file filter would + // contradict the point. The reachable set is the scope, and the row reports where the sink landed. + nestedInLabeled(m) || reachableMethods.contains(m.fullName) + } else { + (fileS.isEmpty || m.filename.endsWith(fileS)) && nestedInLabeled(m) + } + // ---- the field half of the two-stage join (task 5.11) --------------------------------------------- // // `_delete_files()` takes no parameters at all. Its attacker-controlled path arrives as a FIELD, written @@ -260,12 +277,172 @@ } ) - def fieldSourceNodes = { - val reads = cpg.call.nameExact(FIELD_ACCESS).filter(c => inScope(c.method)).l - if (reads.isEmpty) Iterator.empty else reads.filter(r => fieldIsTainted(fileS, r.code.trim)).iterator + // A read of a MEMBER of a tainted object is tainted too. WP Statistics assigns the whole request to + // `$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 = { + 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("->"))) } - def sourceNodes = frameworkSources.l.iterator ++ parameterNodes ++ fieldSourceNodes + def fieldSourceNodes = { + 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 + } + + // ---- the hook half of the two-stage join (task 5.11) ---------------------------------------------- + // + // WP Statistics' CVE-2022-25148 has a modelled source, a modelled sink and `esc_sql` as its fix, and is + // invisible on both sides, because the only thing joining the two files is + // + // add_filter('wp_statistics_current_page', array($this, 'set_current_page')); // hits.php:43 + // return apply_filters('wp_statistics_current_page', $current_page); // pages.php:105 + // + // Both ends name the callback with a STRING, and php2cpg drops the callback argument entirely -- the CPG + // literally holds `add_filter("wp_statistics_current_page", )`. So the link CANNOT come from the graph at + // any price, and it arrives instead as `hookCallbacks`, read out of the source text by + // `mapping.php_hook_callbacks` where both ends are literals. + // + // The rule: an `apply_filters('H', ...)` call is a SOURCE when some callback registered for H returns + // data that an unsanitized flow reached. That is the same question as the field half, against a different + // key. + val HOOK_APPLY = Set("apply_filters", "apply_filters_ref_array", "do_action", "do_action_ref_array") + + val hookTable: Map[String, List[String]] = + hookCallbacksS + .split(";") + .map(_.trim) + .filter(_.nonEmpty) + .flatMap { entry => + val at = entry.indexOf(':') + if (at <= 0 || at == entry.length - 1) None else Some(entry.substring(0, at) -> entry.substring(at + 1)) + } + .groupBy(_._1) + .map { case (hook, pairs) => hook -> pairs.map(_._2).toList } + + def unquote(raw: String): String = { + val t = raw.trim + if (t.length >= 2 && ((t.head == '"' && t.last == '"') || (t.head == '\'' && t.last == '\''))) t.substring(1, t.length - 1) else t + } + + // Half one's seeds are STRICTER than the field half's, and deliberately. A filter callback's own parameter + // is the value being filtered, not request data, and the receiver is the object -- counting either makes + // every registered callback "return tainted" and the join degenerates into the complete graph this design + // exists to refuse. Only a real framework source, or a field already shown to hold one, counts. + // + // They are also NOT scoped to the requesting file, which the field half is. A hook registry is global by + // construction: the whole point of the edge is that `set_current_page` lives in one file and the + // `apply_filters` that calls it lives in another. Scoping these seeds the way the field half scopes its + // own defeats exactly the join being built, and the query returned nothing until this was separated. + val hookSeedsByFile = scala.collection.mutable.Map.empty[String, List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode]] + + def hookSeedsIn(fileName: String) = + hookSeedsByFile.getOrElseUpdate( + 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) + val fields: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] = + methods.flatMap(_.ast.isCall.nameExact(FIELD_ACCESS).l).filter(r => taintedPrefixes(r.code.trim, fileName)) + framework ++ fields + } + ) + + // Half one is asked PER ARRAY KEY, not per callback, and that distinction is the whole result. + // + // "Does an unsanitized value reach this callback's return" is true on BOTH sides of the WP Statistics + // pair, because the fix escapes two of the three members and leaves the third alone: + // + // vulnerable $tmp["id"] = $this->rest_hits->current_page_id + // fixed $tmp["id"] = esc_sql($this->rest_hits->current_page_id) + // both $tmp["search_query"] = ... unescaped, and harmless where it is used + // + // A callback-level answer flags the fixed side exactly as readily as the vulnerable one, which is a leak + // and not a detection. The key is a literal at both ends -- written `"id"` in the callback and `['id']` + // at the sink -- so it joins the same way the hook name and the field name do. Third use of one idea. + val INDEX_ACCESS = ".indexAccess" + val hookKeysMemo = scala.collection.mutable.Map.empty[(String, String), Set[String]] + + def keyOf(access: io.shiftleft.codepropertygraph.generated.nodes.Call): String = + access.argument.l.lift(1).map(a => unquote(a.code)).getOrElse("") + + def taintedKeysOf(fileName: String, callback: String): Set[String] = + hookKeysMemo.getOrElseUpdate( + (fileName, callback), { + val seeds = hookSeedsIn(fileName) + if (seeds.isEmpty) Set.empty[String] + else + cpg.method + .nameExact(callback) + .ast + .isCall + .nameExact(ASSIGNMENT) + .l + .flatMap { assignment => + val args = assignment.argument.l + if (args.size < 2) None + else + args.head match { + 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.exists(f => !f.elements.l.exists(sanitizesHere))) + Some(key) + else None + case _ => None + } + } + .toSet + } + ) + + def hookSourceNodes = + if (hookTable.isEmpty) Iterator.empty + else { + val applies = cpg.call.filter(c => HOOK_APPLY.contains(c.name)).filter(c => inScope(c.method)).l + if (applies.isEmpty) Iterator.empty + else { + val keys = applies.flatMap { call => + val hook = call.argument.l.headOption.map(a => unquote(a.code)).getOrElse("") + hookTable.getOrElse(hook, Nil).flatMap(cb => taintedKeysOf("", cb)) + }.toSet + if (keys.isEmpty) Iterator.empty + else + cpg.call + .nameExact(INDEX_ACCESS) + .filter(c => inScope(c.method)) + .l + .filter(access => keys.contains(keyOf(access))) + // ...and actually derived from the filtered value, not merely sharing a key name with it. + .filter(access => access.start.reachableBy(applies.iterator).nonEmpty) + .iterator + } + } + + def sourceNodes = frameworkSources.l.iterator ++ parameterNodes ++ fieldSourceNodes ++ hookSourceNodes + + // WHICH KIND of source a flow started from, reported per row so the driver can tell them apart. + // + // It has to be reported rather than guessed, because by the time a row reaches Python a hook source reads + // as `$current_page["id"]` and a parameter reads as `$args` -- two identifiers, indistinguishable. The + // driver treats parameter-sourced flows as a FALLBACK it discards whenever a real modelled source is also + // present, which is right for parameters and wrong for these: on WP Statistics, seven sanitized + // `$_SERVER["REQUEST_URI"]` flows displaced the unsanitized hook flow carrying CVE-2022-25148, and the + // finding disappeared between the query and the verdict. + lazy val frameworkIds = frameworkSources.l.map(_.id).toSet + lazy val fieldIds = fieldSourceNodes.map(_.id).toSet + lazy val hookIds = hookSourceNodes.map(_.id).toSet + + def sourceKindOf(head: Option[io.shiftleft.codepropertygraph.generated.nodes.AstNode]): String = + head match { + case Some(node) if frameworkIds.contains(node.id) => "framework" + case Some(node) if hookIds.contains(node.id) => "hook" + case Some(node) if fieldIds.contains(node.id) => "field" + case Some(_) => "parameter" + case None => "" + } // Whether there is anything to trace from at all. Asking `reachableByFlows` with an empty source list is // not merely wasted work: Joern answers it by logging "Attempting to determine flows from empty list of @@ -287,20 +464,31 @@ def sinkMatches(c: io.shiftleft.codepropertygraph.generated.nodes.Call, n: String): Boolean = c.name == n || c.code.startsWith(n + "(") || mentionsToken(c.code, List(n)) || mentionsToken(c.methodFullName, List(n)) - // The region this request is about, as a predicate on the method. Named rather than inlined because the - // field sources have to be scoped to exactly the same place as the sinks, and a second copy of these - // three branches is precisely the drift this file has been bitten by before. - def inScope(m: io.shiftleft.codepropertygraph.generated.nodes.Method): Boolean = - if (function.isEmpty) { - // A region with no enclosing function: the file IS the scope. - fileS.isEmpty || m.filename.endsWith(fileS) - } else if (depth > 0) { - // Following the call graph means leaving the region's file on purpose, so the file filter would - // contradict the point. The reachable set is the scope, and the row reports where the sink landed. - nestedInLabeled(m) || reachableMethods.contains(m.fullName) - } else { - (fileS.isEmpty || m.filename.endsWith(fileS)) && nestedInLabeled(m) + // Does THIS path element cleanse the value flowing through it? + // + // The old test asked whether a sanitizer's name appeared anywhere in the element's source text, and that + // is wrong on exactly the shape WordPress writes. WP Statistics' vulnerable query is one concatenation: + // + // "... " . (array_key_exists(...) ? "AND `uri` = '" . esc_sql($page_uri) . "'" : "") + // . "AND `id` = {$current_page['id']}" + // + // `esc_sql` cleanses `$page_uri`. It does nothing whatsoever for `$current_page['id']`, which is the + // injectable value and CVE-2022-25148 -- but both live in one expression, so the concatenation node's + // code mentions `esc_sql` and the whole flow read as sanitized. A sibling's cleansing was being credited + // to its neighbour. + // + // So the element must BE the cleansing, not merely contain a mention of one: + // * a call to the sanitizer -- by node name, or by its code opening with `name(`; + // * the sanitizer named as a string literal, which is how `array_map('esc_sql', $status)` applies it and + // is a form PMPro actually writes. + def sanitizesHere(node: io.shiftleft.codepropertygraph.generated.nodes.AstNode): Boolean = { + val code = node.code + val byName = node match { + case call: io.shiftleft.codepropertygraph.generated.nodes.Call => sanitizerNames.exists(n => call.name == n) + case _ => false } + byName || sanitizerNames.exists(n => code.startsWith(n + "(") || code.contains("'" + n + "'") || code.contains("\"" + n + "\"")) + } def sinkCalls = cpg.call.filter(c => sinkNames.exists(n => sinkMatches(c, n))).filter(c => inScope(c.method)) @@ -344,9 +532,11 @@ flows.map { flow => val elements = flow.elements.map(_.code).l - val sanitized = sanitizerNames.nonEmpty && elements.exists(code => mentionsToken(code, sanitizerNames)) + val sanitized = sanitizerNames.nonEmpty && flow.elements.l.exists(node => sanitizesHere(node)) + val sourceKind = sourceKindOf(flow.elements.l.headOption) ujson.Obj( "sink" -> sink.code.take(200), + "sourceKind" -> sourceKind, "sinkLine" -> sink.lineNumber.getOrElse(-1).toString, "sinkMethod" -> sink.method.name, // Where the sink actually is. With callDepth > 0 that need not be the region's own file, and a @@ -378,6 +568,7 @@ field("sources"), field("sinks"), field("sanitizers"), + field("hookCallbacks"), field("function"), paramSrc, field("file"), @@ -395,7 +586,7 @@ println(ujson.write(ujson.Obj.from(answers.toSeq :+ ("__census__" -> census)))) } else { println( - ujson.write(ujson.Arr(rowsFor(sources, sinks, sanitizers, function, parameterSources, file, callDepth, boundedSinks): _*)) + ujson.write(ujson.Arr(rowsFor(sources, sinks, sanitizers, hookCallbacks, function, parameterSources, file, callDepth, boundedSinks): _*)) ) } println("---OUSAST-CPG-END---") diff --git a/src/openultrasast/mapping.py b/src/openultrasast/mapping.py index ff03cdc..e9a9019 100644 --- a/src/openultrasast/mapping.py +++ b/src/openultrasast/mapping.py @@ -117,6 +117,36 @@ def analyze_entry_points(root: Path, targets: list[FileTarget]) -> list[EntryPoi return sorted(records, key=lambda item: (item.path, item.line is None, item.line or 0, item.name)) +def php_hook_callbacks(root: Path, targets: list[FileTarget]) -> dict[str, tuple[str, ...]]: + """WordPress's hook registry, read out of the source text because the graph does not carry it. + + `php2cpg` 4.0.623 drops the callback argument of a registration entirely. What the CPG holds is + + add_filter("wp_statistics_current_page", ) + + with an empty second argument, so the link between a hook and its handler cannot be recovered from the + graph at any price. It can be recovered from the source, where both ends are string literals -- and that + is the same place, and the same regex, the entry-point mapper already reads them from. + + This is the link table for the hook half of the two-stage join (task 5.11). It is deliberately a plain + name-to-names mapping with no inference in it: a hook whose name is computed (`"save_post_" . $type`) + simply does not appear, rather than being guessed at. + """ + table: dict[str, set[str]] = {} + for target in targets: + if target.language != "php": + continue + try: + text = (root / target.path).read_text(errors="ignore") + except OSError: + continue + for match in _PHP_REGISTRATION.finditer(text): + handler = match.group("handler") or match.group("method") + if handler: + table.setdefault(match.group("hook"), set()).add(handler) + return {hook: tuple(sorted(names)) for hook, names in sorted(table.items())} + + def attach_reachability_hints(targets: list[FileTarget], entry_points: list[EntryPointRecord]) -> list[FileTarget]: hints_by_path: dict[str, list[dict[str, object]]] = {} for entry_point in entry_points: diff --git a/src/openultrasast/model/scan.py b/src/openultrasast/model/scan.py index 8e00c32..547040f 100644 --- a/src/openultrasast/model/scan.py +++ b/src/openultrasast/model/scan.py @@ -146,6 +146,12 @@ def scan_repository( # spent in belongs to whoever holds the budget. ordered_regions = sorted(regions, key=lambda r: (not r.shipped, -r.rank, r.path, r.function or "")) + # The hook link table, built once for the whole scan. It is a property of the repository rather than of + # any region, and it is read from the source text because the graph does not carry it: php2cpg drops a + # registration's callback argument, so `add_filter("hook", array($this, "m"))` reaches the CPG as + # `add_filter("hook", )`. See `mapping.php_hook_callbacks`. + hooks = _hook_callbacks(root, regions) + # Phase 1: collect the whole scan's questions, for at most `max_regions` regions. Nothing is asked yet. work: list[tuple[str, ScanRegion, ArbiterSpec]] = [] for region in ordered_regions[: limits.max_regions]: @@ -162,7 +168,7 @@ def scan_repository( per_kind: dict[str, float] = {} census_reported = False query_started = time.monotonic() - for kind, requests in _grouped(work).items(): + for kind, requests in _grouped(work, hook_callbacks=hooks).items(): kind_started = time.monotonic() batch = getattr(cpg, "run_batch", None) if callable(batch): @@ -276,7 +282,9 @@ def _prefetched(rows: list[object]): # type: ignore[no-untyped-def] return run -def _grouped(work: Sequence[tuple[str, ScanRegion, ArbiterSpec]]) -> dict[str, dict[str, Mapping[str, object]]]: +def _grouped( + work: Sequence[tuple[str, ScanRegion, ArbiterSpec]], *, hook_callbacks: str = "" +) -> dict[str, dict[str, Mapping[str, object]]]: """The work, keyed by query kind, with each request built by the arbiter's OWN parameter function. Building the parameters here instead would be a second copy that drifts from the arbiter's -- the exact @@ -299,12 +307,32 @@ def _grouped(work: Sequence[tuple[str, ScanRegion, ArbiterSpec]]) -> dict[str, d file=region.path, parameter_sources=entry, call_depth=ENTRY_POINT_CALL_DEPTH if entry else 0, + hook_callbacks=hook_callbacks, ), ) grouped.setdefault(kind, {})[rid] = params return grouped +def _hook_callbacks(root: Path, regions: Sequence[ScanRegion]) -> str: + """``hook:callback;hook:callback`` for the PHP files this scan is about, or ``""``. + + Rendered here rather than in the query because a CPGQL parameter is a string, and rendered from the + regions rather than by walking the tree because the regions already name every file in scope. + """ + paths = sorted({region.path for region in regions if region.language == "php" and region.path}) + if not paths: + return "" + from ..mapping import php_hook_callbacks + from ..preprocess import FileTarget + + targets = [ + FileTarget(path=path, absolute_path=str(root / path), language="php", loc=0, tags=[], has_fuzz_entry_point=False) for path in paths + ] + table = php_hook_callbacks(root, targets) + return ";".join(f"{hook}:{name}" for hook, names in table.items() for name in names) + + def _is_entry_point(region: ScanRegion) -> bool: """Is this region a place untrusted input actually enters? diff --git a/src/openultrasast/model/taint.py b/src/openultrasast/model/taint.py index 523cc55..ea269d7 100644 --- a/src/openultrasast/model/taint.py +++ b/src/openultrasast/model/taint.py @@ -20,7 +20,13 @@ from .specs import TaintSpec def request_params( - spec: TaintSpec, *, function: str = "", file: str = "", parameter_sources: bool = False, call_depth: int = 0 + spec: TaintSpec, + *, + function: str = "", + file: str = "", + parameter_sources: bool = False, + call_depth: int = 0, + hook_callbacks: str = "", ) -> dict[str, object]: """The query parameters this arbiter sends. Shared with the batcher so there is one definition, not two. @@ -41,6 +47,10 @@ def request_params( "parameterSources": "true" if parameter_sources else "false", "callDepth": str(call_depth), "boundedSinks": spec.bounded_sinks, + # The hook link table, `hook:callback;hook:callback`. Read out of the source text rather than the + # graph, because php2cpg drops a registration's callback argument entirely -- see + # `mapping.php_hook_callbacks`. Empty for every language that does not register handlers by string. + "hookCallbacks": hook_callbacks, } @@ -59,9 +69,11 @@ def verdict( function-local question; above zero is what lets a handler's parameter reach a sink in another module, which is the shape VAmPI's SQL injection has and the shape a function-scoped question cannot see. """ - considered = _usable_flows(cpg, spec, function=function, file=file, parameter_sources=parameter_sources, call_depth=call_depth) - if not considered: - return None + # The strongest of the per-site verdicts, which is what this always meant. It used to run its own + # selection over the union of flows, and two copies of "which flow decides" is the duplication this + # module has been bitten by before: the per-site preference added to `verdicts` did not reach here, so + # the two entry points disagreed about the same rows. + # # A flow the graph cannot see a discharge for is the finding; a sanitized or BOUNDED one is a question. # libpng bounds `strcpy(outname+len, ".png")` with `(len = strlen(inname)) > 250` against a char[256], # and entailing it is a claim the code contradicts. But the bound reaches the sink through an `error` @@ -69,10 +81,8 @@ def verdict( # bound is no proof of safety in any case, off-by-one being the classic way they fail. Corroborated is # the honest rung: the model cannot assert this, so the judge is asked instead of the site being # silenced. - open_flows = [flow for flow in considered if not flow["sanitized"] and not flow["bounded"]] - # Deterministic by construction: a total order, then take the first. Nothing depends on engine ordering. - chosen = min(open_flows or considered, key=lambda flow: _rank(flow, spec)) - return _verdict_for(chosen, spec) + answers = verdicts(cpg, spec, function=function, file=file, parameter_sources=parameter_sources, call_depth=call_depth) + return answers[0] if answers else None def _usable_flows( @@ -100,9 +110,7 @@ def _usable_flows( "taint", request_params(spec, function=function, file=file, parameter_sources=parameter_sources, call_depth=call_depth), ) - flows = [flow for flow in _flows(rows, function=function) if not _has_safe_shape(flow, spec)] - modelled = [flow for flow in flows if _from_modelled_source(flow, spec)] - return modelled or flows + return [flow for flow in _flows(rows, function=function) if not _has_safe_shape(flow, spec)] def verdicts( @@ -134,8 +142,13 @@ def verdicts( chosen: list[Mapping[str, object]] = [] for site_flows in by_site.values(): - open_flows = [flow for flow in site_flows if not flow["sanitized"] and not flow["bounded"]] - chosen.append(min(open_flows or site_flows, key=lambda flow: _rank(flow, spec))) + # Parameter sources are a FALLBACK, never an override, and that preference is PER SINK SITE. Applied + # across the whole region it decides one site's question with another site's evidence: on Paid + # Memberships Pro, field-sourced flows at lines 671 and 705 displaced the parameter-sourced flow at + # 936, and CVE-2023-23488 vanished from a region that still reported fifteen other findings. + preferred = [flow for flow in site_flows if _from_modelled_source(flow, spec)] or list(site_flows) + open_flows = [flow for flow in preferred if not flow["sanitized"] and not flow["bounded"]] + chosen.append(min(open_flows or preferred, key=lambda flow: _rank(flow, spec))) # Same total order the single-verdict path uses, so `verdicts(...)[0] == verdict(...)`. chosen.sort(key=lambda flow: (bool(flow["sanitized"]) or bool(flow["bounded"]), _rank(flow, spec))) return [_verdict_for(flow, spec) for flow in chosen] @@ -170,6 +183,7 @@ def _flows(rows: object, *, function: str) -> list[dict[str, object]]: "bound": str(row.get("bound", "")), "source": str(row.get("source", "")), "sanitized": bool(row.get("sanitized", False)), + "sourceKind": str(row.get("sourceKind", "")), "length": _as_int(row.get("length")), } ) @@ -215,8 +229,25 @@ def _in_scope(row: Mapping[str, object], function: str) -> bool: return str(row.get("sinkMethod", "")) == function +# A source the model VOUCHES for, as opposed to a bare captured parameter. The two-stage joins produce the +# first kind without producing a superglobal: a hook source reads as `$current_page["id"]` and a field source +# as `$this->attachments`, neither of which matches a source pattern, yet both were established by a flow the +# query resolved from a real one. +_MODELLED_KINDS = frozenset({"framework", "field", "hook"}) + + def _from_modelled_source(flow: Mapping[str, object], spec: TaintSpec) -> bool: - """Does this flow start at something the source model names, rather than a bare captured parameter?""" + """Does this flow start at something the source model names, rather than a bare captured parameter? + + `sourceKind` is the engine's own answer and wins when present. Before it existed this was decided by + matching the source's TEXT against the source patterns, which cannot see the difference between a hook + source and a parameter -- both are just identifiers by then. On WP Statistics that cost the CVE: seven + sanitized `$_SERVER["REQUEST_URI"]` flows counted as modelled, the unsanitized hook flow did not, and + `modelled or flows` in `_usable_flows` discarded the finding between the query and the verdict. + """ + kind = str(flow.get("sourceKind", "")) + if kind: + return kind in _MODELLED_KINDS source = str(flow["source"]) return any(pattern in source for pattern in spec.sources)