mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-16 03:49:26 +00:00
feat(cpg): the hook half — CVE-2022-25148 found across an add_filter/apply_filters edge
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) <[email protected]>
Claude-Session: https://claude.ai/code/session_01T7XD7x5KonMdmGRkp63EQG
This commit is contained in:
co-authored by
Claude Opus 5
parent
dcdbe322a1
commit
d6908f18ba
@@ -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_
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 = "<operator>.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,19 +464,30 @@
|
||||
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---")
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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?
|
||||
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user