mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-16 11:49:28 +00:00
Revert "feat(taint): project-local sink wrappers are sinks where they are called"
This reverts commit 54c9dfc. The recall-pin check (p16) showed a large precision
and completion regression, although every recall target survived:
- MW WP Form: 3 -> 60 findings. Form-field renderers became sinks.
- WP Statistics: 16 -> 40.
- PMPro: 16 -> 65, 20 of them inside bundled Braintree/Stripe code. Completion fell
to 390 of 1189 questions, because wrappers made many more regions look
sink-bearing.
- YesWiki: completion fell from 2798/2807 to 216/2807.
The structural summary was too loose. Any identifier in a sink argument counted as
fed by a parameter if any assignment anywhere in the method mentioned it, with no
order or dominance. It also ignored vendored library code and cascaded three
levels by name. A redesign needs dataflow-confirmed, level-limited summaries that
leave the question count alone.
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01EmM1j46qgKMh3zqD1QMwEo
This commit is contained in:
Marius Ciepluch
co-authored by
Claude Opus 5.5
Claude Opus 5.5
parent
54c9dfcd44
commit
c7e3a05ba0
@@ -72,7 +72,6 @@
|
||||
// Per-family answers that do not depend on the region, kept across the whole batch.
|
||||
val familyInRepoMemo = scala.collection.mutable.Map.empty[String, Boolean]
|
||||
val sinkCandidatesMemo = scala.collection.mutable.Map.empty[String, List[io.shiftleft.codepropertygraph.generated.nodes.Call]]
|
||||
val wrapperMemo = scala.collection.mutable.Map.empty[String, Map[String, Set[Int]]]
|
||||
val reachableMemo = scala.collection.mutable.Map.empty[(String, String, Int), Set[String]]
|
||||
val methodSourceMemo = scala.collection.mutable.Map.empty[String, Boolean]
|
||||
val fedFieldsMemo = scala.collection.mutable.Map.empty[String, Map[String, String]]
|
||||
@@ -1105,99 +1104,9 @@
|
||||
// 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
|
||||
// kept for the batch; the per-request work is the scope filter over that list.
|
||||
// ---- project-local sink WRAPPERS ------------------------------------------------------------------------
|
||||
//
|
||||
// Real code rarely calls the driver where it builds the query. YesWiki builds its INSERT in
|
||||
// `FormManager::create($data)` and runs it through `$this->dbService->query($query)`, whose body is the
|
||||
// `mysqli_query` the facts name -- so the one sink the facts knew sat in a helper no request ever reached
|
||||
// directly, and the injectable value was never asked about where it entered. A method whose parameter
|
||||
// feeds a family sink is itself a sink for that argument, reported where it is CALLED.
|
||||
//
|
||||
// Summarised STRUCTURALLY, not by dataflow: the parameter reaches the sink call's argument directly or
|
||||
// through at most two local assignments, and at least one occurrence is outside every cleansing call. A
|
||||
// flow query per parameter per method, repeated at every engine start of a portioned scan, is what this
|
||||
// project's scale walls were made of. Up to three levels (a wrapper of a wrapper). Calls resolve by NAME,
|
||||
// because an injected receiver (`$this->dbService`) has no type the frontend can see -- so a name counts
|
||||
// only if every project method so named is a wrapper; one harmless `query()` elsewhere disqualifies it.
|
||||
def underCleansing(node: io.shiftleft.codepropertygraph.generated.nodes.AstNode, root: io.shiftleft.codepropertygraph.generated.nodes.AstNode): Boolean = {
|
||||
var current: Option[io.shiftleft.codepropertygraph.generated.nodes.AstNode] = Some(node)
|
||||
var found = false
|
||||
while (current.isDefined && !found && current.get.id != root.id) {
|
||||
if (current.get.id != node.id && sanitizesHere(current.get, strictNames)) found = true
|
||||
current = current.get._astIn.nextOption() match {
|
||||
case Some(parent: io.shiftleft.codepropertygraph.generated.nodes.AstNode) => Some(parent)
|
||||
case _ => None
|
||||
}
|
||||
}
|
||||
found
|
||||
}
|
||||
|
||||
def feeds(
|
||||
method: io.shiftleft.codepropertygraph.generated.nodes.Method,
|
||||
expression: io.shiftleft.codepropertygraph.generated.nodes.AstNode,
|
||||
name: String,
|
||||
depth: Int
|
||||
): Boolean =
|
||||
depth <= 2 && {
|
||||
val identifiers = expression.ast.isIdentifier.l
|
||||
identifiers.exists(i => i.name == name && !underCleansing(i, expression)) ||
|
||||
identifiers.map(_.name).distinct.filter(_ != name).exists { local =>
|
||||
method.ast.isCall.nameExact(ASSIGNMENT).l.exists { assignment =>
|
||||
val args = assignment.argument.l
|
||||
args.find(_.argumentIndex == 1).exists {
|
||||
case target: io.shiftleft.codepropertygraph.generated.nodes.Identifier => target.name == local
|
||||
case _ => false
|
||||
} && args.find(_.argumentIndex == 2).exists(value => feeds(method, value, name, depth + 1))
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
lazy val wrappers: Map[String, Set[Int]] =
|
||||
wrapperMemo.getOrElseUpdate(
|
||||
sinksS + "|" + sanitizersS + "|" + quotedSanitizersS, {
|
||||
var known = Map.empty[String, Set[Int]]
|
||||
var wrapperMethods = Set.empty[String]
|
||||
var frontier = sinkCandidatesMemo.getOrElseUpdate(sinksS, cpg.call.filter(c => sinkNames.exists(n => sinkMatches(c, n))).l)
|
||||
var level = 0
|
||||
var growing = true
|
||||
while (level < 3 && growing && frontier.nonEmpty) {
|
||||
val found = frontier.groupBy(_.method).toList.flatMap { case (method, calls) =>
|
||||
if (method.isExternal || method.name.startsWith("<")) Nil
|
||||
else
|
||||
method.parameter.l
|
||||
.filter(p => p.index >= 1 && !objectBases.contains(p.name) && p.name != "self" && p.name != "this")
|
||||
.filter(p => calls.exists(c => c.argument.argumentIndexGt(0).l.exists(a => feeds(method, a, p.name, 0))))
|
||||
.map(p => (method, p.index))
|
||||
}
|
||||
val added = found.filterNot { case (m, index) => known.get(m.name).exists(_.contains(index)) }
|
||||
growing = added.nonEmpty
|
||||
added.foreach { case (m, index) =>
|
||||
known = known.updated(m.name, known.getOrElse(m.name, Set.empty[Int]) + index)
|
||||
wrapperMethods += m.fullName
|
||||
}
|
||||
val names = added.map(_._1.name).toSet
|
||||
frontier = if (names.isEmpty) Nil else cpg.call.filter(c => !c.name.startsWith("<operator") && names.contains(c.name)).l
|
||||
level += 1
|
||||
}
|
||||
known.filter { case (name, _) => cpg.method.nameExact(name).filterNot(_.isExternal).l.forall(m => wrapperMethods.contains(m.fullName)) }
|
||||
}
|
||||
)
|
||||
|
||||
def wrapperCall(c: io.shiftleft.codepropertygraph.generated.nodes.Call): Boolean =
|
||||
!c.name.startsWith("<operator") && wrappers.contains(c.name) && !sinkNames.exists(n => sinkMatches(c, n))
|
||||
|
||||
// The arguments a flow must arrive through: every real argument of a fact sink, only the summarised ones of
|
||||
// a wrapper.
|
||||
def sinkTargets(c: io.shiftleft.codepropertygraph.generated.nodes.Call) =
|
||||
if (wrapperCall(c)) c.argument.l.filter(a => wrappers(c.name).contains(a.argumentIndex))
|
||||
else c.argument.argumentIndexGt(0).l
|
||||
|
||||
def sinkCalls =
|
||||
sinkCandidatesMemo
|
||||
.getOrElseUpdate("wrapped|" + sinksS + "|" + sanitizersS + "|" + quotedSanitizersS, {
|
||||
val facts = sinkCandidatesMemo.getOrElseUpdate(sinksS, cpg.call.filter(c => sinkNames.exists(n => sinkMatches(c, n))).l)
|
||||
if (wrappers.isEmpty) facts else facts ++ cpg.call.filter(wrapperCall).l
|
||||
})
|
||||
.getOrElseUpdate(sinksS, cpg.call.filter(c => sinkNames.exists(n => sinkMatches(c, n))).l)
|
||||
.filter(c => inScope(c.method))
|
||||
|
||||
// ---- stage one of the two-stage join WITHOUT dataflow, for evidence mode (flow-aware-ranking, phase 2) --
|
||||
@@ -1448,7 +1357,7 @@
|
||||
// no data at all, yet `res` is reachable from the request in any handler that captures it, so the flow
|
||||
// query answered yes about a call that receives nothing. Measured on NodeGoat, where `res.write(body)` is a
|
||||
// real finding and `res.end()` two lines below it was reported identically.
|
||||
val flows = if (hasSources) sinkTargets(sink).reachableByFlows(sourceNodes).l.filter(plausible) else Nil
|
||||
val flows = if (hasSources) sink.argument.argumentIndexGt(0).reachableByFlows(sourceNodes).l.filter(plausible) else Nil
|
||||
|
||||
// The SHAPE of the sink call, which is what distinguishes a fix from a bug when the fix is a safe form
|
||||
// rather than a sanitizing call: `execute(sql, params)` binds where `execute(sql + x)` interpolates, and
|
||||
|
||||
@@ -1417,56 +1417,3 @@ def test_an_allowlist_check_guards_its_branch_and_its_ternary_arm(tmp_path: Path
|
||||
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", "stored_in_else", "haystack"}, {n: payload[n] for n in names}
|
||||
|
||||
|
||||
_WRAPPED_PHP = (
|
||||
"<?php\n"
|
||||
"class DbService { public $link;\n"
|
||||
" function query( $query ) { return mysqli_query( $this->link, $query ); }\n"
|
||||
" function number( $n ) { return mysqli_query( $this->link, 'SELECT ' . intval( $n ) ); } }\n"
|
||||
"class FormManager { public $dbService;\n"
|
||||
" function create( $data ) { $query = 'INSERT INTO t VALUES (' . $data['id'] . ')';\n"
|
||||
" return $this->dbService->query( $query ); } }\n"
|
||||
"class FormController { public $formManager; public $dbService;\n"
|
||||
" function displayAll() { $value = $_POST['form']; return $this->formManager->create( $value ); }\n"
|
||||
" function direct() { return $this->dbService->query( 'SELECT ' . $_POST['q'] ); }\n"
|
||||
" function literal() { return $this->dbService->query( 'SELECT 1' ); }\n"
|
||||
" function cast() { return $this->dbService->number( $_POST['n'] ); } }\n"
|
||||
)
|
||||
|
||||
|
||||
def _wrapped_requests(spec, names):
|
||||
common = {
|
||||
"sources": ",".join(spec.sources),
|
||||
"sinks": ",".join(spec.sinks),
|
||||
"sanitizers": ",".join(spec.sanitizers),
|
||||
"quotedSanitizers": ",".join(spec.quoted_sanitizers),
|
||||
"file": "app.php",
|
||||
}
|
||||
return {n: {**common, "function": n} for n in names}
|
||||
|
||||
|
||||
@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine")
|
||||
def test_a_project_sink_wrapper_is_a_sink_where_it_is_called(tmp_path: Path) -> None:
|
||||
"""YesWiki runs its INSERT through `$this->dbService->query($query)`, whose body is the `mysqli_query` the facts
|
||||
name, and builds it in `FormManager::create($data)`. Neither receiver has a type the frontend can see. The
|
||||
request value is reported where it enters a wrapper -- two levels up -- and not where it cannot."""
|
||||
from openultrasast.model.specs import taint_specs
|
||||
|
||||
spec = taint_specs(language="php")["injection"]
|
||||
names = ("displayAll", "direct", "literal", "cast")
|
||||
payload = _taint_rows(tmp_path, "php2cpg", {"app.php": _WRAPPED_PHP}, _wrapped_requests(spec, names))
|
||||
reported = {n: [r for r in _unsanitized(payload, n) if r.get("sinkMethod") == n] for n in names}
|
||||
assert {n for n in names if reported[n]} == {"displayAll", "direct"}, {n: payload[n] for n in names}
|
||||
assert "create" in str(reported["displayAll"][0]["sink"]), reported["displayAll"]
|
||||
|
||||
|
||||
@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine")
|
||||
def test_a_wrapper_name_shared_with_a_harmless_method_is_not_a_sink(tmp_path: Path) -> None:
|
||||
"""Calls resolve by name, so a name is a sink only if every project method so named is a wrapper."""
|
||||
from openultrasast.model.specs import taint_specs
|
||||
|
||||
spec = taint_specs(language="php")["injection"]
|
||||
code = _WRAPPED_PHP + "class Cache { function query( $key ) { return strlen( $key ); } }\n"
|
||||
payload = _taint_rows(tmp_path, "php2cpg", {"app.php": code}, _wrapped_requests(spec, ("direct",)))
|
||||
assert not [r for r in _unsanitized(payload, "direct") if r.get("sinkMethod") == "direct"], payload["direct"]
|
||||
|
||||
Reference in New Issue
Block a user