From 69d4a8626f7003c25b735531fa8863ecff582a45 Mon Sep 17 00:00:00 2001 From: Marius Ciepluch <11855163+norandom@users.noreply.github.com> Date: Fri, 25 Sep 2026 17:24:49 +0200 Subject: [PATCH] fix(taint): a value stored while its guard held stays guarded Ultimate Member's fixed pin kept one alert. The fix writes `$this->sql_order = " ORDER BY u.{$sortby} "` inside `elseif (in_array($sortby, $allowed, true))`, and after the if/elseif chain it passes the field through `apply_filters(...)` into a second assignment. The guard was judged only at the flow's final node, which is outside the branch. A guard is now also judged at every point where the path stores the value (an assignment target). A value stored while the check held carries the check forward, because the rest of the path derives from that store. A mere read inside a branch is not a candidate: the engine lists uses in branches the flow does not depend on. The Joern-gated test gains the shape and its control: the same store in the unguarded else branch, through the same filter, is still reported. The test fails when only the end is judged. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01EmM1j46qgKMh3zqD1QMwEo --- src/openultrasast/cpg/queries/taint.sc | 20 ++++++++++++++++---- tests/test_cpg_queries_compile.py | 15 +++++++++++++-- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/src/openultrasast/cpg/queries/taint.sc b/src/openultrasast/cpg/queries/taint.sc index 842736a..5524f84 100644 --- a/src/openultrasast/cpg/queries/taint.sc +++ b/src/openultrasast/cpg/queries/taint.sc @@ -773,7 +773,19 @@ case _ => false } } - ternary || (end match { + // Where the check is judged: the end of the flow, and every point where the path STORES the value. A + // store made while the check held carries it forward -- Ultimate Member writes `$this->sql_order` inside + // `elseif (in_array($sortby, ...))` and passes it through a filter after the chain. A mere READ inside a + // branch is not a candidate: the engine lists uses in branches the flow does not depend on. + def stored(node: io.shiftleft.codepropertygraph.generated.nodes.AstNode) = node match { + case e: io.shiftleft.codepropertygraph.generated.nodes.Expression if e.argumentIndex == 1 => + e.astParent match { + case a: io.shiftleft.codepropertygraph.generated.nodes.Call => a.name.startsWith(".assignment") + case _ => false + } + case _ => false + } + ternary || (end :: elements.filter(stored)).distinctBy(_.id).exists { case cfg: io.shiftleft.codepropertygraph.generated.nodes.CfgNode => val method = cfg.method method.ast.isControlStructure.controlStructureType("IF").l.exists { cs => @@ -781,7 +793,7 @@ val checks = guardsIn(cond, names) checks.nonEmpty && { val (whenTrue, _) = branches(cs) - val inside = whenTrue.exists(b => contains(b, end)) && checks.exists(g => polarity(g, cond).contains(true)) + val inside = whenTrue.exists(b => contains(b, cfg)) && checks.exists(g => polarity(g, cond).contains(true)) inside || (checks.exists(g => polarity(g, cond).contains(false)) && whenTrue.exists { b => val bails = b.ast.exists { case _: io.shiftleft.codepropertygraph.generated.nodes.Return => true @@ -792,7 +804,7 @@ case i: io.shiftleft.codepropertygraph.generated.nodes.Identifier => names(i.name) || names(i.code.trim) case _ => false }) - (bails || overwrites) && !contains(cs, end) && (cond match { + (bails || overwrites) && !contains(cs, cfg) && (cond match { case c: io.shiftleft.codepropertygraph.generated.nodes.CfgNode => cfg.dominatedBy.exists(_.id == c.id) case _ => false }) @@ -801,7 +813,7 @@ } } case _ => false - }) + } } def cleansed(elements: List[io.shiftleft.codepropertygraph.generated.nodes.AstNode]): Boolean = diff --git a/tests/test_cpg_queries_compile.py b/tests/test_cpg_queries_compile.py index ba9052a..a3142ea 100644 --- a/tests/test_cpg_queries_compile.py +++ b/tests/test_cpg_queries_compile.py @@ -1391,15 +1391,26 @@ def test_an_allowlist_check_guards_its_branch_and_its_ternary_arm(tmp_path: Path "function else_branch() { global $wpdb; $s = $_POST['s']; $core = array( 'login' );\n" " if ( in_array( $s, $core, true ) ) { return null; }\n" ' else { return $wpdb->query( "SELECT 1 FROM t ORDER BY u.{$s}" ); } }\n' + "function stored_in_branch() { global $wpdb; $s = $_POST['s']; $core = array( 'login' ); $o = '';\n" + ' if ( in_array( $s, $core, true ) ) { $o = " ORDER BY u.{$s} "; }\n' + " $o = apply_filters( 'sort', $o, $s );\n" + ' return $wpdb->query( "SELECT 1 FROM t {$o}" ); }\n' + "function stored_in_else() { global $wpdb; $s = $_POST['s']; $core = array( 'login' ); $o = '';\n" + " if ( in_array( $s, $core, true ) ) { $o = ' ORDER BY u.login '; }\n" + ' else { $o = " ORDER BY u.{$s} "; }\n' + " $o = apply_filters( 'sort', $o, $s );\n" + ' return $wpdb->query( "SELECT 1 FROM t {$o}" ); }\n' ) - names = ("ternary", "wrong_arm", "branch", "else_branch") + names = ("ternary", "wrong_arm", "branch", "else_branch", "stored_in_branch", "stored_in_else") common = { "sources": ",".join(spec.sources), "sinks": ",".join(spec.sinks), "sanitizers": ",".join(spec.sanitizers), "guards": ",".join(spec.guards), + "dispatchApply": ",".join(spec.dispatch_apply), + "dispatchValue": ",".join(spec.dispatch_value), "file": "app.php", } payload = _taint_rows(tmp_path, "php2cpg", {"app.php": app}, {n: {**common, "function": n} for n in names}) reported = {n for n in names if _unsanitized(payload, n)} - assert reported == {"wrong_arm", "else_branch"}, {n: payload[n] for n in names} + assert reported == {"wrong_arm", "else_branch", "stored_in_else"}, {n: payload[n] for n in names}