mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-16 03:49:26 +00:00
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) <[email protected]>
Claude-Session: https://claude.ai/code/session_01EmM1j46qgKMh3zqD1QMwEo
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
03f2ce7162
commit
69d4a8626f
@@ -773,7 +773,19 @@
|
|||||||
case _ => false
|
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("<operator>.assignment")
|
||||||
|
case _ => false
|
||||||
|
}
|
||||||
|
case _ => false
|
||||||
|
}
|
||||||
|
ternary || (end :: elements.filter(stored)).distinctBy(_.id).exists {
|
||||||
case cfg: io.shiftleft.codepropertygraph.generated.nodes.CfgNode =>
|
case cfg: io.shiftleft.codepropertygraph.generated.nodes.CfgNode =>
|
||||||
val method = cfg.method
|
val method = cfg.method
|
||||||
method.ast.isControlStructure.controlStructureType("IF").l.exists { cs =>
|
method.ast.isControlStructure.controlStructureType("IF").l.exists { cs =>
|
||||||
@@ -781,7 +793,7 @@
|
|||||||
val checks = guardsIn(cond, names)
|
val checks = guardsIn(cond, names)
|
||||||
checks.nonEmpty && {
|
checks.nonEmpty && {
|
||||||
val (whenTrue, _) = branches(cs)
|
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 =>
|
inside || (checks.exists(g => polarity(g, cond).contains(false)) && whenTrue.exists { b =>
|
||||||
val bails = b.ast.exists {
|
val bails = b.ast.exists {
|
||||||
case _: io.shiftleft.codepropertygraph.generated.nodes.Return => true
|
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 i: io.shiftleft.codepropertygraph.generated.nodes.Identifier => names(i.name) || names(i.code.trim)
|
||||||
case _ => false
|
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 c: io.shiftleft.codepropertygraph.generated.nodes.CfgNode => cfg.dominatedBy.exists(_.id == c.id)
|
||||||
case _ => false
|
case _ => false
|
||||||
})
|
})
|
||||||
@@ -801,7 +813,7 @@
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
case _ => false
|
case _ => false
|
||||||
})
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
def cleansed(elements: List[io.shiftleft.codepropertygraph.generated.nodes.AstNode]): Boolean =
|
def cleansed(elements: List[io.shiftleft.codepropertygraph.generated.nodes.AstNode]): Boolean =
|
||||||
|
|||||||
@@ -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"
|
"function else_branch() { global $wpdb; $s = $_POST['s']; $core = array( 'login' );\n"
|
||||||
" if ( in_array( $s, $core, true ) ) { return null; }\n"
|
" if ( in_array( $s, $core, true ) ) { return null; }\n"
|
||||||
' else { return $wpdb->query( "SELECT 1 FROM t ORDER BY u.{$s}" ); } }\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 = {
|
common = {
|
||||||
"sources": ",".join(spec.sources),
|
"sources": ",".join(spec.sources),
|
||||||
"sinks": ",".join(spec.sinks),
|
"sinks": ",".join(spec.sinks),
|
||||||
"sanitizers": ",".join(spec.sanitizers),
|
"sanitizers": ",".join(spec.sanitizers),
|
||||||
"guards": ",".join(spec.guards),
|
"guards": ",".join(spec.guards),
|
||||||
|
"dispatchApply": ",".join(spec.dispatch_apply),
|
||||||
|
"dispatchValue": ",".join(spec.dispatch_value),
|
||||||
"file": "app.php",
|
"file": "app.php",
|
||||||
}
|
}
|
||||||
payload = _taint_rows(tmp_path, "php2cpg", {"app.php": app}, {n: {**common, "function": n} for n in names})
|
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)}
|
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}
|
||||||
|
|||||||
Reference in New Issue
Block a user