From dcdbe322a1e1498639636b478bd38229e120844d Mon Sep 17 00:00:00 2001 From: Marius Ciepluch <11855163+norandom@users.noreply.github.com> Date: Wed, 9 Sep 2026 19:42:20 +0200 Subject: [PATCH] =?UTF-8?q?feat(cpg):=20taint=20a=20field=20where=20it=20i?= =?UTF-8?q?s=20assigned=20=E2=80=94=20the=20field=20half=20of=205.11?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_delete_files()` takes no parameters. CVE-2023-6559 arrives as a FIELD, written by one method and read by another: public function __construct( ..., array $attachments = array() ) $this->attachments = $attachments; // half one ends here protected function _delete_files() foreach ( $this->attachments as $file ) // half two starts here unlink( $file ); Both halves were already in the graph. Only the join was missing, and the key it joins on is the literal `$this->attachments`, written identically at both ends -- so this is a second question asked with the query that already exists, not an edge a frontend has to invent. A read of `$this->F` is a source when some assignment to the same literal `$this->F` in the same file is fed by an unsanitized flow from a seed. Scoped three ways so a field name common to every class ever written cannot become a wormhole: only fields the sink scope reads, only assignments in the same file, and the right-hand side must itself be reachable from a source. The receiver could then leave `parameterNodes` -- the precision fix refused two commits ago, now sound, because the path it was standing in for is carried by the field it actually travels through: before after mwwpform vulnerable 1, from `this` 1, from $this->attachments mwwpform fixed 0 0 pmpro vulnerable 15 -- 4 named, 11 receiver 18 -- all named or field pmpro fixed 1, receiver-sourced 1 VAmPI 4 entailed 4 entailed, unchanged CVE-2023-23488 is still at class.memberorder.php:936 and CVE-2023-6559 at class.mail.php:259. Every finding now names the value it followed instead of naming the object. Three things cost a measurement each, recorded in the task: * A summary must carry the SANITIZATION status of the half it summarises, not merely its reachability. Without that clause, `$this->sqlQuery = "..." . esc_sql($x) . "..."` reads as tainted -- and PMPro builds most of its queries that way, so the fixed side went from 1 finding to 13. That is the pair no longer separating, which is the only number that matters. * Two-phase `reachableBy` then `reachableByFlows` is SLOWER, not faster: 183s -> 256s. Running the analysis twice costs more than skipping path reconstruction saves. Reverted. * Memoizing per field rather than per file took it to 97s, about 1.6x the pre-field cost for that class, because only fields some region reads are worth the flow query. `inScope` is factored out of `sinkCalls` so the field sources are scoped to exactly the same place as the sinks, rather than a second copy of those three branches drifting from the first. 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 | 24 ++++- benchmarks/repos/README.md | 33 +++++-- src/openultrasast/cpg/queries/taint.sc | 128 ++++++++++++++++++++----- 3 files changed, 151 insertions(+), 34 deletions(-) diff --git a/.kiro/specs/contributor-scan/tasks.md b/.kiro/specs/contributor-scan/tasks.md index a390f87..aa779e6 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 +- [~] 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. @@ -634,6 +634,28 @@ count down from 11 to 0 with its CVE still found. - Order: the field half first. It is the smaller link table, it has a pinned pair of its own, and it is what unlocks the receiver change; the hook half then reuses the same two-stage machinery. + - **Field half done 2026-09-09.** `taint.sc` gains `fieldSourceNodes`: a read of `$this->F` is a source + when some assignment to the same literal `$this->F` in the same file is fed by an UNSANITIZED flow from + a seed. The receiver then left `parameterNodes`, which is only sound because of it. + + | | before | after | + |---|---|---| + | `mwwpform` vulnerable | 1, sourced from `this` | 1, sourced from `$this->attachments` | + | `mwwpform` fixed | 0 | 0 | + | `pmpro` vulnerable | 15 — 4 named, 11 receiver | 18 — all named or field, CVE still at :936 | + | `pmpro` fixed | 1, receiver-sourced | 1 | + | VAmPI | 4 entailed | 4 entailed, unchanged | + + Three things learned, each of which cost a measurement: + - **A summary must carry the sanitization status of its half, not just reachability.** Without that, + `$this->sqlQuery = "..." . esc_sql($x)` reads as tainted and PMPro's fixed side went 1 -> 13. That is + the pair not separating, which is the only number that matters. + - **Two-phase `reachableBy` then `reachableByFlows` is SLOWER**, not faster: 183s -> 256s. Running the + 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`). - _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 5ba0c91..226cf2e 100644 --- a/benchmarks/repos/README.md +++ b/benchmarks/repos/README.md @@ -81,17 +81,19 @@ side because the fix is a call the fact tables already carry: `$wpdb->prepare` f other. That is the distinction worth teaching. The engine is not recognising a bug; it is failing to find a cleansing call on a path it resolved, and then finding one. -The precision is not clean, and saying so is the point. On PMPro's order class the vulnerable side reports -**15 entailed findings**, of which **4** are sourced from a named parameter and **11** from the receiver, -`$this`. One receiver-sourced finding survives into 2.9.8 at a query whose every fragment is wrapped in -`esc_sql` — a false positive, and a legible one: the flow reaches the sink through `$this->sqlQuery`, so the -sanitizer is on the fragments and not on the path. +Every finding now names the value it followed. On PMPro's order class the vulnerable side reports **18 +entailed findings** and the fixed side **1**, and each one is sourced either from a named parameter (`$id`, +`$code`, `$token`) or from a named field (`$this->sqlQuery`, `$this->Email`). None is sourced from `$this`. -Excluding the receiver was tried and reverted. It cuts that file to 6 findings, all named-parameter sourced, -and makes the PMPro pair separate cleanly — and it **loses CVE-2023-6559**, because `_delete_files()` takes -no parameters at all and its attacker-controlled path arrives as `$this->attachments`, assigned by another -method. Trading a verified CVE for findings merely suspected of being false is the wrong trade. The real -defect is field-sensitivity: a field should be tainted where it is assigned. +That took two changes, and the order between them is the whole point. Excluding the receiver from parameter +sources was tried **first** and reverted: it cuts the file to 6 findings and makes the pair separate cleanly, +and it **loses CVE-2023-6559**, because `_delete_files()` takes no parameters at all and its +attacker-controlled path arrives as `$this->attachments`, assigned by another method entirely. Trading a +verified CVE for findings merely suspected of being false is the wrong trade. + +With the field half of the join in place (below), that path is carried by the field it actually travels +through, and the receiver no longer has to stand in for it — so it can go, and the eleven receiver-sourced +findings go with it. The precision fix became available once the mechanism underneath it was right. ### The one it misses, and why @@ -124,6 +126,17 @@ synthetic endpoint, build the table by reading literals rather than inferring an carries the join's uncertainty — one registered callback and a literal key entails, several callbacks or a computed hook name corroborates. Task 5.11 states it, with both pinned pairs as its observable. +**The field half is implemented and measured.** `$this->attachments` is tainted at the assignment that fills +it, joined to the read by the literal field code, so CVE-2023-6559 is found as +`class.mail.php:259:_delete_files ← $this->attachments` and is absent on the fixed side. One thing it needed +that is easy to get wrong: the summary has to carry the **sanitization** status of the half it summarises, +not merely its reachability. Asking only "does a source reach this assignment" marks +`$this->sqlQuery = "..." . esc_sql($x) . "..."` tainted, and PMPro builds most of its queries that way — the +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. + 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 noise: upstream now annotates both with `// phpcs:ignore WordPress.DB.PreparedSQL...`, and later rewrote diff --git a/src/openultrasast/cpg/queries/taint.sc b/src/openultrasast/cpg/queries/taint.sc index 017b3f8..23a0a44 100644 --- a/src/openultrasast/cpg/queries/taint.sc +++ b/src/openultrasast/cpg/queries/taint.sc @@ -165,27 +165,107 @@ // attacker input, and counting them produced flows like `res -> readFileSync` that name no real source. // Off by default: outside a labeled function, treating every parameter as untrusted is not sound. // - // The RECEIVER is INCLUDED, and it was measured rather than assumed. `$this` is a parameter in the CPG -- - // index 0 of every PHP method -- and excluding it is tempting: on Paid Memberships Pro's order class, 12 of - // 16 entailed findings are `this -> $wpdb->...`, and one of them survives into 2.9.8 at a query whose every - // fragment is `esc_sql`-wrapped, reaching the sink through `$this->sqlQuery`. Excluding it cuts that file - // from 16 findings to 6, all sourced from a named parameter, and makes the 2.9.7/2.9.8 pair separate - // cleanly. + // The RECEIVER is excluded, and only because the field half below makes that safe. `$this` is a parameter + // in the CPG -- index 0 of every PHP method -- and counting it means every method that reads any field + // reaches every sink: 11 of 15 entailed findings on one PMPro class were `this -> $wpdb->...`, one of them + // surviving into 2.9.8 at a query whose every fragment is `esc_sql`-wrapped. // - // It also loses CVE-2023-6559. MW WP Form's `_delete_files()` takes NO parameters: the attacker-controlled - // path arrives as `$this->attachments`, assigned by another method entirely. With the receiver excluded - // that finding goes to zero on the vulnerable side. + // Excluding it was TRIED FIRST and reverted, because on its own it loses CVE-2023-6559: MW WP Form's + // `_delete_files()` takes no parameters at all and its attacker-controlled path arrives as + // `$this->attachments`. Trading a verified CVE for findings merely suspected of being false is the wrong + // trade, and deleting the receiver removes the symptom by removing the evidence. // - // So the trade is a verified CVE against findings merely SUSPECTED of being false, and it is refused. The - // real defect is field-sensitivity -- `$this->attachments` should be tainted where it is assigned, not by - // the receiver being tainted everywhere -- and deleting the receiver removes the symptom by removing the - // evidence. + // With `fieldSourceNodes` below, that path is carried by the field it actually travels through, so the + // receiver no longer has to stand in for it. The order matters: this line is only correct because of the + // one after it. def parameterNodes = if (parameterSources != "true") Iterator.empty - else if (function.isEmpty) cpg.method.parameter.iterator - else cpg.method.nameExact(function).parameter.iterator + else if (function.isEmpty) cpg.method.parameter.filterNot(_.name == "this").iterator + else cpg.method.nameExact(function).parameter.filterNot(_.name == "this").iterator - def sourceNodes = frameworkSources.l.iterator ++ parameterNodes + // ---- 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 + // by one method and read back by another: + // + // public function __construct( ..., array $attachments = array() ) // a parameter + // $this->attachments = $attachments; // half one ends here + // protected function _delete_files() + // foreach ( $this->attachments as $file ) // half two starts here + // unlink( $file ); // CVE-2023-6559 + // + // Both halves are already in the graph: parameter to assignment inside one method, field read to sink + // inside another. Only the JOIN between them is missing, and the key it joins on is the literal + // `$this->attachments`, written identically at both ends. So this is a second question asked with the + // query that already exists, not an edge a frontend has to invent. + // + // The alternative is making `$this` itself a source. That finds this CVE and eleven false positives with + // it on one PMPro class, because every method that reads any field then reaches every sink. A field name + // is the precision, and it costs one string comparison. + // + // Scoped three ways, so a field name common to every class ever written cannot become a wormhole: + // * only fields the sink scope actually READS are considered; + // * only assignments in the SAME FILE count; + // * the assignment's right-hand side must itself be reachable from a source -- which is the whole claim. + val FIELD_ACCESS = ".fieldAccess" + val ASSIGNMENT = ".assignment" + + // Memoized per FIELD, not per file, because the expensive part is one flow query per candidate assignment + // and only the fields some region actually reads are worth asking about. A batch asks about one file many + // times over -- 205 requests across 41 regions of one class -- so without a memo this runs hundreds of + // times over the same assignments. + val methodsByFile = scala.collection.mutable.Map.empty[String, List[io.shiftleft.codepropertygraph.generated.nodes.Method]] + val seedsByFile = scala.collection.mutable.Map.empty[String, List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode]] + val fieldTaintMemo = scala.collection.mutable.Map.empty[(String, String), Boolean] + + def methodsIn(fileName: String) = + methodsByFile.getOrElseUpdate(fileName, cpg.method.filter(m => fileName.isEmpty || m.filename.endsWith(fileName)).l) + + // Framework sources are untrusted wherever they appear. Parameters are untrusted only where the caller has + // said this region is an entry point -- the same contract `parameterNodes` carries, and for the same + // reason: an arbitrary helper's parameters carry whatever its caller happened to have. + def seedsIn(fileName: String) = + seedsByFile.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 params: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] = + if (parameterSources == "true") methods.flatMap(_.parameter.l) else Nil + framework ++ params + } + ) + + def fieldIsTainted(fileName: String, fieldCode: String): Boolean = + fieldTaintMemo.getOrElseUpdate( + (fileName, fieldCode), { + val seeds = seedsIn(fileName) + if (seeds.isEmpty) false + else + methodsIn(fileName).flatMap(_.ast.isCall.nameExact(ASSIGNMENT).l).exists { assignment => + val args = assignment.argument.l + args.size >= 2 && (args.head match { + case target: io.shiftleft.codepropertygraph.generated.nodes.Call + if target.name == FIELD_ACCESS && target.code.trim == fieldCode => + // The summary must carry the SANITIZATION status of the half it summarises, not merely its + // reachability. Asking only "does a source reach this assignment" marks + // `$this->sqlQuery = "..." . esc_sql($x) . "..."` tainted, and PMPro builds most of its + // queries that way: the fixed side of its pair went from 1 finding to 13 without this, + // which is the pair no longer separating at all. + val flows = args(1).start.reachableByFlows(seeds.iterator).l + flows.exists(f => !f.elements.map(_.code).l.exists(code => mentionsToken(code, sanitizerNames))) + case _ => false + }) + } + } + ) + + 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 + } + + def sourceNodes = frameworkSources.l.iterator ++ parameterNodes ++ fieldSourceNodes // 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 @@ -207,20 +287,22 @@ 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)) - def sinkCalls = { - val all = cpg.call.filter(c => sinkNames.exists(n => sinkMatches(c, 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. - if (fileS.isEmpty) all else all.filter(_.method.filename.endsWith(fileS)) + 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. - all.filter(c => nestedInLabeled(c.method) || reachableMethods.contains(c.method.fullName)) + nestedInLabeled(m) || reachableMethods.contains(m.fullName) } else { - val inFile = if (fileS.isEmpty) all else all.filter(_.method.filename.endsWith(fileS)) - inFile.filter(c => nestedInLabeled(c.method)) + (fileS.isEmpty || m.filename.endsWith(fileS)) && nestedInLabeled(m) } - } + + def sinkCalls = cpg.call.filter(c => sinkNames.exists(n => sinkMatches(c, n))).filter(c => inScope(c.method)) val rows = sinkCalls.l.flatMap { sink => // Data flows into the arguments; asking the call node itself finds nothing.