From 949977c7a62084aa439ec8697e46bd9999fc8f50 Mon Sep 17 00:00:00 2001 From: Marius Ciepluch <11855163+norandom@users.noreply.github.com> Date: Fri, 25 Sep 2026 13:35:45 +0200 Subject: [PATCH] fix(taint): an SQL escape protects only a quoted value; splice the engine's object detours `esc_sql` and the mysql/pg `*_escape_string` functions only backslash quotes. A value they escape is safe inside a quoted literal and nowhere else. Ultimate Member CVE-2024-1071 is `$sortby = esc_sql(...)` built into `" ORDER BY u.{$sortby} "`, and it read as sanitized. Sanitizer facts can now say `quoted_only`. The escape keeps its credit unless the first string the value is built into (concat, interpolation, addition, f-string) places it after literal text that opens no quote. With no string built downstream, the credit stays as before. pg_escape_literal adds its own quotes and stays a full cleanser. Two further defects hid the same CVE: - The engine is field-insensitive on the object. Its one path from `$this->sql_order` went `$this` -> `$this->having` -> `esc_sql($this->having)` -> ... -> the query. absorbedByObject cut it, and it credited an unrelated field's escape. When a path leaves a field through the object for another field and reads the first field again, the detour is spliced out. The later read may be on the path or inside the sink argument where the path ends. - The field join judged a field clean if ANY sanitizer name was on its path. It now uses the same quote-aware check as a row. This matters because the engine's own path to the query changed between runs: 2 of 4 identical runs chose a path via `$wpdb->prepare`. The field join is the deterministic route. Exploratory (retune after protocol-v1): the Ultimate Member vulnerable pin is detected at class-member-directory-meta.php:936. The fixed pin still alerts at :945. Its fix is an allowlist (`in_array(...) ? $v : 'ASC'`, and an `in_array` guard), which is not modelled yet. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01EmM1j46qgKMh3zqD1QMwEo --- src/openultrasast/cpg/queries/taint.sc | 266 ++++++++++++++------ src/openultrasast/model/specs.py | 4 + src/openultrasast/model/taint.py | 1 + src/openultrasast/ruleset/semantic/php.toml | 11 +- src/openultrasast/semantic/facts.py | 6 +- tests/test_cpg_queries_compile.py | 136 ++++++++++ 6 files changed, 351 insertions(+), 73 deletions(-) diff --git a/src/openultrasast/cpg/queries/taint.sc b/src/openultrasast/cpg/queries/taint.sc index f6e1b3c..ceb82c0 100644 --- a/src/openultrasast/cpg/queries/taint.sc +++ b/src/openultrasast/cpg/queries/taint.sc @@ -106,7 +106,8 @@ contextPathsS: String = "", traceS: String = "", fixedOriginS: String = "", - originAnchorsS: String = "" + originAnchorsS: String = "", + quotedSanitizersS: String = "" ): List[ujson.Obj] = { // Word-boundary matching, never substring. `resolveUrl` contains `resolve`, so a bare-substring sanitizer @@ -132,6 +133,12 @@ val sourcePatterns = split(sourcesS) val sinkNames = split(sinksS) val sanitizerNames = split(sanitizersS) + // Escapes (`esc_sql`) among the sanitizers, and the calls that build a string a value can land in; see + // `cleansed`. Declared here because Scala forbids a forward reference across a value definition. + val quotedOnly = split(quotedSanitizersS).toSet + val strictNames = sanitizerNames.filterNot(quotedOnly.contains) + val quotedNames = sanitizerNames.filter(quotedOnly.contains) + val stringBuilders = Set(".concat", "encaps", ".addition", ".formatString") val boundedNames = split(boundedS) // A relational comparison against an integer literal: `len > 250`, `n <= sizeof(buf)`. Equality and null @@ -370,6 +377,52 @@ } } + // The engine is field-INSENSITIVE on the object: a write to `$this->sql_order` taints `$this`, and the path + // it reports can leave through the NEXT use of `$this` -- another field entirely -- and come back to + // `$this->sql_order` later. Ultimate Member's CVE-2024-1071 is exactly that: the ORDER BY built in + // `$this->sql_order` reaches the query, but the one path the engine gave went `$this` -> `$this->having` -> + // `esc_sql($this->having)` -> ... -> `$this->sql_order`, which `absorbedByObject` rightly cut and which + // credited an unrelated field's escape. Where a path leaves a field for ANOTHER field through the object and + // later reads the first field again -- on the path, or inside the sink's argument the path ends at -- the + // segment between is that detour, and it is spliced out. Only such a detour: an arbitrary loop may hold the + // field's own re-sanitisation. + def spliced(elements: List[io.shiftleft.codepropertygraph.generated.nodes.AstNode]): List[io.shiftleft.codepropertygraph.generated.nodes.AstNode] = { + def isField(node: io.shiftleft.codepropertygraph.generated.nodes.AstNode) = node match { + case c: io.shiftleft.codepropertygraph.generated.nodes.Call => c.name == ".fieldAccess" + case _ => false + } + val detour = elements.indices.iterator.flatMap { k => + elements(k) match { + case read: io.shiftleft.codepropertygraph.generated.nodes.Call + if k > 0 && read.name == ".fieldAccess" && objectHop(elements(k - 1)) => + var j = k - 1 + while (j >= 0 && objectHop(elements(j))) j -= 1 + if (j < 0 || !isField(elements(j))) None + else { + val left = elements(j).asInstanceOf[io.shiftleft.codepropertygraph.generated.nodes.Call] + if (fieldOf(left) == fieldOf(read)) None + else + elements.indices.drop(k + 1).find(m => isField(elements(m)) && elements(m).code.trim == left.code.trim) match { + case Some(m) => Some(elements.take(j) ++ elements.drop(m)) + case None => + // The path may never read the field again and reach the sink's argument by sibling hops + // instead -- the query string that READS `$this->sql_order` is where it ends. A read of the + // same field inside that last element is the rejoin. + elements.lastOption.flatMap(last => last.ast.isCall.nameExact(".fieldAccess").find(_.code.trim == left.code.trim).map(r => (last, r))) match { + case Some((last, r)) if r.id != last.id => Some(elements.take(j) ++ List(r, last)) + case _ => None + } + } + } + case _ => None + } + }.nextOption() + detour match { + case Some(shorter) if shorter.size < elements.size => spliced(shorter) + case _ => elements + } + } + // Per element, not per path: the same nodes recur across thousands of paths of one sink, and walking each // path's calls afresh cost one outlier request 101 s -> 160 s. val familySinkMemo = scala.collection.mutable.Map.empty[Long, Boolean] @@ -415,14 +468,19 @@ val concatenations = Set(".addition", ".assignmentPlus", ".formatString") val fixesOriginMemo = scala.collection.mutable.Map.empty[Long, Boolean] - def literalFixesOrigin(code: String): Boolean = { - // The literal's TEXT, without its quotes. With the closing quote left on, `"/"` read as `/"` and passed - // the path test -- and `"/" + x` is the protocol-relative `//evil.example`. + // A literal's TEXT: its code without the quotes, and without a Python string prefix. + def literalText(code: String): String = { val quoted = "(?s)^(?:[rRbBuUfF]{1,2})?(['\"`])(.*)\\1$".r - val text = code.trim match { + code.trim match { case quoted(_, inner) => inner case other => other } + } + + def literalFixesOrigin(code: String): Boolean = { + // The literal's TEXT, without its quotes. With the closing quote left on, `"/"` read as `/"` and passed + // the path test -- and `"/" + x` is the protocol-relative `//evil.example`. + val text = literalText(code) text.startsWith("?") || text.startsWith("#") || text.matches("(?s)^/[^/\\\\].*") || schemeHostThenPath.matcher(text).find() } @@ -479,7 +537,7 @@ } def plausible(flow: io.joern.dataflowengineoss.language.Path): Boolean = { - val elements = flow.elements.l + val elements = spliced(flow.elements.l) val throughSink = elements.dropRight(1).exists { case c: io.shiftleft.codepropertygraph.generated.nodes.Call if !c.name.startsWith(" familySinkMemo.getOrElseUpdate(c.id, familySinkCall(c)) @@ -519,9 +577,124 @@ if (objectBases.contains(base)) "" else reader.fullName } + // 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, names: List[String] = sanitizerNames): Boolean = { + val code = node.code + val byName = node match { + case call: io.shiftleft.codepropertygraph.generated.nodes.Call => names.exists(n => call.name == n) + case _ => false + } + byName || names.exists(n => code.startsWith(n + "(") || code.contains("'" + n + "'") || code.contains("\"" + n + "\"")) + } + + // A sanitizer the value passes THROUGH between two path elements. The engine can step from a sanitizer's + // argument straight to the expression around the call -- `$params["id"]` inside + // `isset($params["id"]) ? intval($params["id"]) : null` went to the conditional, and `intval` itself was + // never an element -- so checking elements alone reported PMPro's integer-cast REST level id as an SQL + // injection. Where the next element CONTAINS the previous one, the syntax between them is what the value + // was wrapped in; a sanitizer there sanitized it. A sanitizer elsewhere in the expression does not count. + def sanitizedBetween(elements: List[io.shiftleft.codepropertygraph.generated.nodes.AstNode], names: List[String] = sanitizerNames): Boolean = + elements.zip(elements.drop(1)).exists { case (inner, outer) => wrappedBetween(inner, outer, names) } + + def wrappedBetween( + inner: io.shiftleft.codepropertygraph.generated.nodes.AstNode, + outer: io.shiftleft.codepropertygraph.generated.nodes.AstNode, + names: List[String] + ): Boolean = { + var node: Option[io.shiftleft.codepropertygraph.generated.nodes.AstNode] = inner._astIn.nextOption() match { + case Some(parent: io.shiftleft.codepropertygraph.generated.nodes.AstNode) => Some(parent) + case _ => None + } + var wrapped = false + var reached = false + var depth = 0 + while (node.isDefined && !reached && depth < 8) { + val current = node.get + if (current.id == outer.id) reached = true + else { + if (sanitizesHere(current, names)) wrapped = true + node = current._astIn.nextOption() match { + case Some(parent: io.shiftleft.codepropertygraph.generated.nodes.AstNode) => Some(parent) + case _ => None + } + depth += 1 + } + } + reached && wrapped + } + + // An ESCAPE protects a value only inside a quoted literal. The credit is withdrawn where the first string the + // escaped value is built into leaves it OUTSIDE quotes: the literal text before it opens no quote. + // Ultimate Member's `$sortby = esc_sql(...); " ORDER BY u.{$sortby} "` read as sanitized, and is + // CVE-2024-1071. With no string built downstream the credit stays, as it always has. + + // The literal text of `root` that precedes `holder`, walking nested string builders in argument order. + def textBefore( + root: io.shiftleft.codepropertygraph.generated.nodes.Call, + holder: io.shiftleft.codepropertygraph.generated.nodes.AstNode + ): String = { + val text = new StringBuilder + def walk(node: io.shiftleft.codepropertygraph.generated.nodes.AstNode): Boolean = + if (node.id == holder.id) true + else + node match { + case l: io.shiftleft.codepropertygraph.generated.nodes.Literal => text.append(literalText(l.code)); false + case c: io.shiftleft.codepropertygraph.generated.nodes.Call if stringBuilders.contains(c.name) => + c.argument.l.sortBy(_.argumentIndex).exists(walk) + case _ => false + } + walk(root) + text.toString + } + + def opensQuote(text: String): Boolean = { + val singles = text.count(_ == '\'') + val doubles = "\\\"".r.findAllMatchIn(text).size + singles % 2 == 1 || doubles % 2 == 1 + } + + def landsQuoted(elements: List[io.shiftleft.codepropertygraph.generated.nodes.AstNode], from: Int): Boolean = + elements.indices.drop(from).find { m => + elements(m) match { + case c: io.shiftleft.codepropertygraph.generated.nodes.Call => m > 0 && stringBuilders.contains(c.name) + case _ => false + } + } match { + case None => true + case Some(m) => + val builder = elements(m).asInstanceOf[io.shiftleft.codepropertygraph.generated.nodes.Call] + argumentHolding(builder, elements(m - 1)) match { + case None => true + case Some(holder) => opensQuote(textBefore(builder, holder)) + } + } + + def cleansed(elements: List[io.shiftleft.codepropertygraph.generated.nodes.AstNode]): Boolean = + elements.exists(node => sanitizesHere(node, strictNames)) || sanitizedBetween(elements, strictNames) || + (quotedNames.nonEmpty && elements.indices.exists { k => + (sanitizesHere(elements(k), quotedNames) || + (k + 1 < elements.size && wrappedBetween(elements(k), elements(k + 1), quotedNames))) && landsQuoted(elements, k + 1) + }) + def fieldIsTainted(fileName: String, fieldCode: String, scope: String = ""): Boolean = fieldTaintMemo.getOrElseUpdate( - (sourcesS, fieldParamSrc, sanitizersS, fileName, scope + "|" + fieldCode), { + (sourcesS, fieldParamSrc, sanitizersS + "|" + quotedSanitizersS, fileName, scope + "|" + fieldCode), { val seeds = seedsIn(fileName) if (seeds.isEmpty) false else @@ -539,7 +712,12 @@ // 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.filter(plausible) - flows.exists(f => !f.elements.map(_.code).l.exists(code => mentionsToken(code, sanitizerNames))) + // + // Judged exactly as a flow's row is: an ESCAPE counts only where the value lands quoted. Ultimate + // Member builds `$this->sql_order = " ORDER BY u.{$sortby} "` from an `esc_sql`'d value, and a + // test for any sanitizer NAME on the path marked the field clean -- the one deterministic route + // to CVE-2024-1071, while the engine's own path to the query was a coin toss between runs. + flows.exists(f => !cleansed(spliced(f.elements.l))) case _ => false }) } @@ -687,7 +865,7 @@ 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.filter(plausible).exists(f => !f.elements.l.exists(sanitizesHere) && !sanitizedBetween(f.elements.l))) + else if (args(1).start.reachableByFlows(seeds.iterator).l.filter(plausible).exists(f => !cleansed(spliced(f.elements.l)))) Some(key) else None case _ => None @@ -788,62 +966,6 @@ c.name == n || (!operator && (mentionsToken(calleeText(c.code), List(n)) || fullNameMentions(c.methodFullName, n))) } - // 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 + "\"")) - } - - // A sanitizer the value passes THROUGH between two path elements. The engine can step from a sanitizer's - // argument straight to the expression around the call -- `$params["id"]` inside - // `isset($params["id"]) ? intval($params["id"]) : null` went to the conditional, and `intval` itself was - // never an element -- so checking elements alone reported PMPro's integer-cast REST level id as an SQL - // injection. Where the next element CONTAINS the previous one, the syntax between them is what the value - // was wrapped in; a sanitizer there sanitized it. A sanitizer elsewhere in the expression does not count. - def sanitizedBetween(elements: List[io.shiftleft.codepropertygraph.generated.nodes.AstNode]): Boolean = - elements.zip(elements.drop(1)).exists { case (inner, outer) => - var node: Option[io.shiftleft.codepropertygraph.generated.nodes.AstNode] = inner._astIn.nextOption() match { - case Some(parent: io.shiftleft.codepropertygraph.generated.nodes.AstNode) => Some(parent) - case _ => None - } - var wrapped = false - var reached = false - var depth = 0 - while (node.isDefined && !reached && depth < 8) { - val current = node.get - if (current.id == outer.id) reached = true - else { - if (sanitizesHere(current)) wrapped = true - node = current._astIn.nextOption() match { - case Some(parent: io.shiftleft.codepropertygraph.generated.nodes.AstNode) => Some(parent) - case _ => None - } - depth += 1 - } - } - reached && wrapped - } - // The candidate sink calls for a FAMILY do not depend on the region, and scanning every call in the graph // for each of 2,487 requests was a large share of what made a request cost seconds -- in evidence mode, // which asks for no dataflow at all, it alone took a query past an hour. Scanned once per sink list and @@ -1032,7 +1154,7 @@ scoped.map(m => m -> (if (labeledMethods.exists(_.id == m.id)) "entry" else "callee")) ++ fieldFiles.toList.flatMap(f => methodsIn(f).filterNot(_.isExternal).map(_ -> "field")) ++ callbacks.map(_ -> "hook") ++ - scoped.filter(_.ast.l.exists(sanitizesHere)).map(_ -> "guard") + scoped.filter(_.ast.l.exists(node => sanitizesHere(node))).map(_ -> "guard") // ONLY THE CHANGED FILES are itemised when the driver says which they are. The consumer attaches a // location to a change only when the location's file is one the change touched, and discards every // other row after checking it names a real file with a sane extent. Itemised anyway, a request whose @@ -1142,7 +1264,7 @@ // for adjudication, where a witness naming only its two ends hid that PMPro's object-field findings // start at one field and end at another. def traced(flow: io.joern.dataflowengineoss.language.Path): ujson.Arr = - ujson.Arr.from(flow.elements.l.map { node => + ujson.Arr.from(spliced(flow.elements.l).map { node => val where = node match { case c: io.shiftleft.codepropertygraph.generated.nodes.CfgNode => c.method.filename.split("/").last + ":" + c.method.name case _ => "?" @@ -1151,9 +1273,10 @@ }) flows .map { flow => - val elements = flow.elements.map(_.code).l - val sanitized = sanitizerNames.nonEmpty && (flow.elements.l.exists(node => sanitizesHere(node)) || sanitizedBetween(flow.elements.l)) - val sourceKind = sourceKindOf(flow.elements.l.headOption) + val path = spliced(flow.elements.l) + val elements = path.map(_.code) + val sanitized = sanitizerNames.nonEmpty && cleansed(path) + val sourceKind = sourceKindOf(path.headOption) ((sourceKind, elements.headOption.getOrElse("").take(200), sanitized), (elements.size, flow)) } .groupBy(_._1) @@ -1260,7 +1383,8 @@ if (field("contextPaths").nonEmpty) field("contextPaths") else contextPaths, field("trace"), field("fixedOrigin"), - field("originAnchors") + field("originAnchors"), + field("quotedSanitizers") ): _* ) println(ujson.write(ujson.Obj("id" -> id, "rows" -> rows, "ms" -> ((System.nanoTime() - started) / 1000000L).toDouble))) diff --git a/src/openultrasast/model/specs.py b/src/openultrasast/model/specs.py index c5060f4..52d7fdb 100644 --- a/src/openultrasast/model/specs.py +++ b/src/openultrasast/model/specs.py @@ -122,6 +122,8 @@ class TaintSpec: # prefix (`reverse`). From the sink facts; only destination families set them. fixed_origin: bool = False origin_anchors: tuple[str, ...] = () + # The sanitizers that protect only a value inside a quoted literal (`esc_sql`); a subset of `sanitizers`. + quoted_sanitizers: tuple[str, ...] = () @dataclass(frozen=True) @@ -257,6 +259,7 @@ def taint_specs( ) sanitizers = tuple(sorted({call for fact in scoped.sanitizers if not _is_shape(fact) for call in fact.calls})) safe_shapes = tuple(sorted({call for fact in scoped.sanitizers if _is_shape(fact) for call in fact.calls})) + quoted = tuple(sorted({call for fact in scoped.sanitizers if not _is_shape(fact) and fact.quoted_only for call in fact.calls})) by_family: dict[str, set[str]] = {} bounded: dict[str, set[str]] = {} fixed_origin: set[str] = set() @@ -286,6 +289,7 @@ def taint_specs( dispatch_value=dispatch_value, fixed_origin=family_id in fixed_origin, origin_anchors=tuple(sorted(anchors.get(family_id, ()))), + quoted_sanitizers=quoted, ) for family_id, calls in sorted(by_family.items()) } diff --git a/src/openultrasast/model/taint.py b/src/openultrasast/model/taint.py index a45d1e7..20fa0b9 100644 --- a/src/openultrasast/model/taint.py +++ b/src/openultrasast/model/taint.py @@ -62,6 +62,7 @@ def request_params( "dispatchValue": spec.dispatch_value, "fixedOrigin": "true" if spec.fixed_origin else "false", "originAnchors": spec.origin_anchors, + "quotedSanitizers": spec.quoted_sanitizers, } diff --git a/src/openultrasast/ruleset/semantic/php.toml b/src/openultrasast/ruleset/semantic/php.toml index 94e6f26..66c98c4 100644 --- a/src/openultrasast/ruleset/semantic/php.toml +++ b/src/openultrasast/ruleset/semantic/php.toml @@ -161,9 +161,18 @@ rule_ids = [] id = "html_escape" calls = ["htmlspecialchars", "htmlentities", "html_entity_decode_safe"] +# Escapes, not cleansers: each backslashes quotes, so each protects a value that lands INSIDE a quoted +# literal and nothing else. `ORDER BY u.{$sortby}` and `WHERE id = " . $n` are injectable escaped -- Ultimate +# Member CVE-2024-1071 is the first, and it read as sanitized. `pg_escape_literal` adds its own quotes and is +# a cleanser. [[sanitizer]] id = "sql_escape" -calls = ["mysqli_real_escape_string", "mysql_real_escape_string", "pg_escape_string", "pg_escape_literal", "esc_sql"] +calls = ["mysqli_real_escape_string", "mysql_real_escape_string", "pg_escape_string", "esc_sql"] +quoted_only = true + +[[sanitizer]] +id = "sql_literal" +calls = ["pg_escape_literal"] # `$wpdb->prepare` is WordPress's parameterized form and appears ON the flow path, so it reads as a cleansing # call rather than as a shape qualifier. diff --git a/src/openultrasast/semantic/facts.py b/src/openultrasast/semantic/facts.py index c55a7eb..436bb9b 100644 --- a/src/openultrasast/semantic/facts.py +++ b/src/openultrasast/semantic/facts.py @@ -57,6 +57,9 @@ class SanitizerFact: language: str parameterized: bool = False literal_format_arg: int | None = None + # An ESCAPE, not a cleansing: `esc_sql` backslashes quotes, which protects a value inside a quoted SQL + # literal and nothing else. `ORDER BY u.{$sortby}` is Ultimate Member's CVE-2024-1071, escaped and injectable. + quoted_only: bool = False @dataclass(frozen=True) @@ -273,7 +276,7 @@ def _flag(value: object, field: str, path: Path, ident: str) -> bool: if value is None: return False if not isinstance(value, bool): - raise FactLoadError(f"{path} sink {ident} has a non-boolean {field}") + raise FactLoadError(f"{path} fact {ident} has a non-boolean {field}") return value @@ -327,6 +330,7 @@ def _sanitizers(value: object, language: str, path: Path) -> list[SanitizerFact] language=language, parameterized=bool(item.get("parameterized", False)), literal_format_arg=literal if isinstance(literal, int) else None, + quoted_only=_flag(item.get("quoted_only"), "quoted_only", path, ident), ) ) return rows diff --git a/tests/test_cpg_queries_compile.py b/tests/test_cpg_queries_compile.py index c8f5d85..fda52ad 100644 --- a/tests/test_cpg_queries_compile.py +++ b/tests/test_cpg_queries_compile.py @@ -1092,3 +1092,139 @@ def test_a_destination_whose_origin_is_fixed_is_not_untrusted(tmp_path: Path) -> assert isinstance(payload, dict) reported = {name for name in names if any(isinstance(r, dict) and r.get("sink") and r.get("sourceKind") for r in payload[name])} assert reported == {"host", "bare", "slash"}, {name: payload[name] for name in names} + + +@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine") +def test_an_sql_escape_protects_only_a_quoted_value(tmp_path: Path) -> None: + """Ultimate Member CVE-2024-1071 is `$sortby = esc_sql(...)` built into `" ORDER BY u.{$sortby} "`: escaped, + unquoted, injectable, and read as sanitized. An escape inside quotes -- concatenated, interpolated, inline, + inside a LIKE pattern -- still cleanses; an unquoted identifier or number does not.""" + from openultrasast.cpg.backend import extract_payload + from openultrasast.model.specs import taint_specs + + frontend = __import__("shutil").which("php2cpg") + if frontend is None: + pytest.skip("php2cpg is not installed") + spec = taint_specs(language="php")["injection"] + assert "esc_sql" in spec.quoted_sanitizers, "the shipped esc_sql fact no longer says it is an escape" + src = tmp_path / "src" + src.mkdir() + (src / "app.php").write_text( + "get_col( "SELECT ID FROM t {$order}" ); }\n' + "function numeric() { global $wpdb; $v = esc_sql( $_POST['v'] );\n" + ' return $wpdb->query( "DELETE FROM t WHERE id = " . $v ); }\n' + "function quoted() { global $wpdb; $v = esc_sql( $_POST['v'] );\n" + ' return $wpdb->query( "SELECT 1 FROM t WHERE a = \'" . $v . "\'" ); }\n' + "function quoted_inline() { global $wpdb;\n" + " return $wpdb->query( \"SELECT 1 FROM t WHERE a = '\" . esc_sql( $_POST['v'] ) . \"' AND b = 1\" ); }\n" + "function interpolated() { global $wpdb; $v = esc_sql( $_POST['v'] );\n" + " return $wpdb->query( \"SELECT 1 FROM t WHERE a = '{$v}'\" ); }\n" + "function like() { global $wpdb; $v = esc_sql( $_POST['v'] );\n" + ' return $wpdb->query( "SELECT 1 FROM t WHERE a LIKE \'%" . $v . "%\'" ); }\n' + ) + cpg = tmp_path / "cpg.bin" + built = subprocess.run([frontend, str(src), "-o", str(cpg)], capture_output=True, text=True, timeout=600, check=False) + if not cpg.is_file(): + pytest.skip(f"could not build a sample cpg: {(built.stderr or '')[-200:]}") + common = { + "sources": "$_POST", + "sinks": "$wpdb->query,$wpdb->get_col", + "sanitizers": ",".join(spec.sanitizers), + "quotedSanitizers": ",".join(spec.quoted_sanitizers), + "file": "app.php", + } + names = ("unquoted", "numeric", "quoted", "quoted_inline", "interpolated", "like") + requests = tmp_path / "requests.json" + requests.write_text(json.dumps({name: {**common, "function": name} for name in names})) + done = subprocess.run( + [ + _joern() or "joern", + "--script", + str((QUERIES / "taint.sc").resolve()), + "--param", + f"cpgFile={cpg}", + "--param", + f"requestsFile={requests}", + ], + capture_output=True, + text=True, + timeout=900, + check=False, + cwd=str(tmp_path), + ) + assert done.returncode == 0, (done.stderr or done.stdout or "")[-500:] + payload = extract_payload(done.stdout or "") + assert isinstance(payload, dict) + flows = {name: [r for r in payload[name] if isinstance(r, dict) and r.get("sourceKind")] for name in names} + assert all(flows.values()), f"a flow was lost altogether: {flows}" + unsanitized = {name for name in names if any(not r.get("sanitized") for r in flows[name])} + assert unsanitized == {"unquoted", "numeric"}, flows + + +@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine") +def test_an_escaped_but_unquoted_field_is_object_state_that_carries_input(tmp_path: Path) -> None: + """The field join decided whether `$this->sql_order` carries request data by looking for ANY sanitizer name + on the path, so an `esc_sql` inside `" ORDER BY u.{$s} "` marked the field clean. On Ultimate Member that + was the only deterministic route to CVE-2024-1071: the engine's own path to the query changed run to run. + A field built from a QUOTED escape stays clean.""" + from openultrasast.cpg.backend import extract_payload + from openultrasast.model.specs import taint_specs + + frontend = __import__("shutil").which("php2cpg") + if frontend is None: + pytest.skip("php2cpg is not installed") + spec = taint_specs(language="php")["injection"] + src = tmp_path / "src" + src.mkdir() + (src / "dir.php").write_text( + "order = " ORDER BY u.{$s} "; }\n' + " function prepare_where() { $s = esc_sql( $_POST['name'] );\n" + " $this->where = \" WHERE u.name = '{$s}' \"; }\n" + ' function members() { global $wpdb; return $wpdb->get_col( "SELECT ID FROM t u {$this->order}" ); }\n' + ' function named() { global $wpdb; return $wpdb->get_col( "SELECT ID FROM t u {$this->where}" ); }\n' + "}\n" + ) + cpg = tmp_path / "cpg.bin" + built = subprocess.run([frontend, str(src), "-o", str(cpg)], capture_output=True, text=True, timeout=600, check=False) + if not cpg.is_file(): + pytest.skip(f"could not build a sample cpg: {(built.stderr or '')[-200:]}") + common = { + "sources": ",".join(spec.sources), + "sinks": ",".join(spec.sinks), + "sanitizers": ",".join(spec.sanitizers), + "quotedSanitizers": ",".join(spec.quoted_sanitizers), + "file": "dir.php", + } + requests = tmp_path / "requests.json" + requests.write_text(json.dumps({name: {**common, "function": name} for name in ("members", "named")})) + done = subprocess.run( + [ + _joern() or "joern", + "--script", + str((QUERIES / "taint.sc").resolve()), + "--param", + f"cpgFile={cpg}", + "--param", + f"requestsFile={requests}", + ], + capture_output=True, + text=True, + timeout=900, + check=False, + cwd=str(tmp_path), + ) + assert done.returncode == 0, (done.stderr or done.stdout or "")[-500:] + payload = extract_payload(done.stdout or "") + assert isinstance(payload, dict) + unsanitized = { + name: [r for r in payload[name] if isinstance(r, dict) and r.get("sourceKind") and not r.get("sanitized")] + for name in ("members", "named") + } + assert any("$this->order" in str(r.get("source")) for r in unsanitized["members"]), payload["members"] + assert not unsanitized["named"], f"a field built from a quoted escape was reported: {unsanitized['named']}"