mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-16 03:49:26 +00:00
Revert "feat(taint): project sink wrappers, one level and confirmed by dataflow"
This reverts commit cc885f0. It failed the acceptance bar set before the run (each
recall pin within +2 findings, completion unchanged):
- WP Statistics: 16 -> 31.
- PMPro: 16 -> 26, and completion 378/382 -> 103/382. Six prior findings were lost
to the budget; CVE line 936 survived.
- YesWiki: completion 2798 -> 1968, still not detected.
Two causes:
- Cost: wrapper confirmation runs a flow query per parameter of every sink-bearing
method, repeated at each engine start of a portioned scan.
- Precision: a helper whose parameter reaches a sink under its own escaping (for
example `getMemberOrderByPayPalToken(sanitize_text_field(...))`) is not a sink
for its caller.
Doing this properly needs summaries that carry the wrapper's internal
sanitisation, computed once per scan, not per request batch.
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
cc885f025c
commit
d14fd5e960
@@ -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]]
|
||||
@@ -1347,63 +1346,7 @@
|
||||
return summary :: (perSink ++ contextRows)
|
||||
}
|
||||
|
||||
// ---- project-local sink WRAPPERS, one level, confirmed by dataflow ---------------------------------------
|
||||
//
|
||||
// Real code rarely calls the driver where it builds the query: YesWiki runs its INSERT through
|
||||
// `$this->dbService->query($query)`, whose body is the `mysqli_query` the facts name. A method whose
|
||||
// parameter REACHES a fact sink's argument -- by the engine's own dataflow inside the method, on a plausible
|
||||
// path no cleansing or guard discharges -- is a sink for that argument where it is called.
|
||||
//
|
||||
// The first version (reverted, c7e3a05) summarised structurally, cascaded three levels by name and fed the
|
||||
// structural pass: MW WP Form went from 3 findings to 60, PMPro to 65 with 20 inside bundled Braintree and
|
||||
// Stripe, and the question count tripled until completion collapsed. So: one level only; nothing under a
|
||||
// bundled library tree; a name only if every project method so named is a wrapper (an injected receiver has
|
||||
// no type to resolve by); and wrappers are asked in this flow pass alone, never in the evidence pass, so they
|
||||
// change no region's selection.
|
||||
val bundledTrees = Set("lib", "libs", "vendor", "vendors", "third_party", "third-party", "node_modules", "bower_components")
|
||||
|
||||
lazy val wrappers: Map[String, Set[Int]] =
|
||||
wrapperMemo.getOrElseUpdate(
|
||||
List(sinksS, sanitizersS, quotedSanitizersS, guardsS, dispatchValueS).mkString("|"), {
|
||||
val factSinks = sinkCandidatesMemo.getOrElseUpdate(sinksS, cpg.call.filter(c => sinkNames.exists(n => sinkMatches(c, n))).l)
|
||||
val confirmed = factSinks.groupBy(_.method).toList.flatMap { case (method, calls) =>
|
||||
val bundled = method.filename.split("/").exists(bundledTrees.contains)
|
||||
if (method.isExternal || bundled || 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).reachableByFlows(Iterator.single(p)).l.filter(plausible).exists(f => !cleansed(spliced(f.elements.l)))
|
||||
}
|
||||
}
|
||||
.map(p => (method, p.index))
|
||||
}
|
||||
val byName = confirmed.groupBy(_._1.name).map { case (name, found) => name -> found.map(_._2).toSet }
|
||||
val wrapperMethods = confirmed.map(_._1.fullName).toSet
|
||||
byName.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 confirmed 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 wrapperCalls: List[io.shiftleft.codepropertygraph.generated.nodes.Call] =
|
||||
if (!hasSources || wrappers.isEmpty) Nil
|
||||
else
|
||||
sinkCandidatesMemo
|
||||
.getOrElseUpdate("wrapped|" + List(sinksS, sanitizersS, quotedSanitizersS, guardsS, dispatchValueS).mkString("|"), cpg.call.filter(wrapperCall).l)
|
||||
.filter(c => inScope(c.method))
|
||||
|
||||
val rows = (sinkCalls.l ++ wrapperCalls).flatMap { sink =>
|
||||
val rows = sinkCalls.l.flatMap { sink =>
|
||||
// Data flows into the arguments; asking the call node itself finds nothing.
|
||||
// `call.argument` includes the RECEIVER at argumentIndex 0 (`db` in `db.execute(...)`), so counting it
|
||||
// makes a one-argument interpolated call look like a two-argument bound one -- the exact inversion of the
|
||||
@@ -1414,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
|
||||
|
||||
@@ -1292,7 +1292,6 @@ def _taint_rows(tmp_path: Path, frontend_name: str, files: dict[str, str], reque
|
||||
src = tmp_path / "src"
|
||||
src.mkdir()
|
||||
for name, text in files.items():
|
||||
(src / name).parent.mkdir(parents=True, exist_ok=True)
|
||||
(src / name).write_text(text)
|
||||
cpg = tmp_path / "cpg.bin"
|
||||
built = subprocess.run([frontend, str(src), "-o", str(cpg)], capture_output=True, text=True, timeout=600, check=False)
|
||||
@@ -1418,69 +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, file="app.php"):
|
||||
common = {
|
||||
"sources": ",".join(spec.sources),
|
||||
"sinks": ",".join(spec.sinks),
|
||||
"sanitizers": ",".join(spec.sanitizers),
|
||||
"quotedSanitizers": ",".join(spec.quoted_sanitizers),
|
||||
"file": file,
|
||||
}
|
||||
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 SQL through `$this->dbService->query($query)`, whose body is the `mysqli_query` the facts
|
||||
name, through a receiver the frontend cannot type. One level only: `create`, which feeds the wrapper rather
|
||||
than the driver, is not itself a sink -- the first version cascaded and flooded real projects."""
|
||||
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 for n in names if [r for r in _unsanitized(payload, n) if r.get("sinkMethod") == n]}
|
||||
assert reported == {"direct"}, {n: payload[n] for n in names}
|
||||
|
||||
|
||||
@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"]
|
||||
|
||||
|
||||
@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine")
|
||||
def test_a_bundled_library_method_is_not_a_wrapper(tmp_path: Path) -> None:
|
||||
"""PMPro bundles Braintree and Stripe under `includes/lib/`; the first version made their HTTP helpers sinks."""
|
||||
from openultrasast.model.specs import taint_specs
|
||||
|
||||
spec = taint_specs(language="php")["injection"]
|
||||
files = {
|
||||
"lib/Db.php": "<?php\nclass Db { public $link; function run( $q ) { return mysqli_query( $this->link, $q ); } }\n",
|
||||
"app.php": "<?php\nclass C { public $db; function go() { return $this->db->run( 'SELECT ' . $_POST['q'] ); } }\n",
|
||||
}
|
||||
payload = _taint_rows(tmp_path, "php2cpg", files, _wrapped_requests(spec, ("go",)))
|
||||
assert not [r for r in _unsanitized(payload, "go") if r.get("sinkMethod") == "go"], payload["go"]
|
||||
|
||||
Reference in New Issue
Block a user