mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-16 11:49:28 +00:00
revert(taint): the caller-fed field rule removed a true positive and missed the false ones
Reverts6ebbc89ande0c260c(keeping finding_dump's completed-regions record). Measured on PMPro, comparing only questions both runs answered (157): 37 -> 31 findings, but the six removed included the adjudicated TRUE positive discount-codes.php:154 (REST-reachable), and only one of the six sampled object-field false positives went away; five are still reported. It also cut completed questions 295 -> 158. MW WP Form and WP Statistics were unchanged either way. The diagnosis was wrong: the object-field false positives do not come from setter parameters. The witnesses name one field (e.g. $update_order->BillingFrequency) and the query interpolates another ($this->membership_id), which points at field-insensitive propagation through the object instead. To be confirmed on a flow path before any fix. 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
e0c260ca5f
commit
bdb5a866d6
@@ -81,11 +81,7 @@
|
||||
// field join's flow queries. Keyed by the parameters the answer depends on, so one family's seeds never
|
||||
// answer another's.
|
||||
val methodsByFile = scala.collection.mutable.Map.empty[String, List[io.shiftleft.codepropertygraph.generated.nodes.Method]]
|
||||
val seedsByFile = scala.collection.mutable.Map.empty[(String, String, String, String), List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode]]
|
||||
// Whether any caller hands a method's parameter request data, per (sources, sanitizers, method, index); and
|
||||
// the request-source calls themselves, which every such question starts from.
|
||||
val callerFedMemo = scala.collection.mutable.Map.empty[(String, String, String, Int), Boolean]
|
||||
val sourceCallMemo = scala.collection.mutable.Map.empty[String, List[io.shiftleft.codepropertygraph.generated.nodes.Call]]
|
||||
val seedsByFile = scala.collection.mutable.Map.empty[(String, String, String), List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode]]
|
||||
val fieldTaintMemo = scala.collection.mutable.Map.empty[(String, String, String, String, String), Boolean]
|
||||
val hookKeysMemo = scala.collection.mutable.Map.empty[(String, String, String), Set[String]]
|
||||
|
||||
@@ -272,46 +268,14 @@
|
||||
// Framework sources are untrusted wherever they appear. Parameters are untrusted only where the caller has
|
||||
// said this region is an entry point -- the same contract `parameterNodes` carries, and for the same
|
||||
// reason: an arbitrary helper's parameters carry whatever its caller happened to have.
|
||||
//
|
||||
// A parameter seeds the FIELD join only where some CALLER hands it request data. Counting every parameter of
|
||||
// every method in the file made each setter a source: `$morder->user_id = $user_id` in a gateway's hook
|
||||
// callback, `$this->membership_id = ...` loaded from the order's own row. Adjudicated on PMPro 2026-09-23,
|
||||
// six of eight false SQL injections in a 1-in-5 sample were exactly that -- object fields holding database
|
||||
// or internal values, read as attacker input. MW WP Form's CVE-2023-6559, which the join exists for, is the
|
||||
// case the rule keeps: its constructor's `$attachments` is passed the uploaded files by the controller.
|
||||
def requestSourceCalls: List[io.shiftleft.codepropertygraph.generated.nodes.Call] =
|
||||
sourceCallMemo.getOrElseUpdate(sourcesS, cpg.call.filter(c => sourcePatterns.exists(p => c.code.contains(p))).l)
|
||||
|
||||
def fedByCaller(
|
||||
m: io.shiftleft.codepropertygraph.generated.nodes.Method,
|
||||
param: io.shiftleft.codepropertygraph.generated.nodes.MethodParameterIn
|
||||
): Boolean =
|
||||
callerFedMemo.getOrElseUpdate(
|
||||
(sourcesS, sanitizersS, m.fullName, param.index), {
|
||||
val sources = requestSourceCalls
|
||||
sources.nonEmpty && m.callIn.l.exists { call =>
|
||||
call.argument.l.filter(_.argumentIndex == param.index).exists { arg =>
|
||||
arg.ast.isCall.exists(c => sourcePatterns.exists(p => c.code.contains(p))) ||
|
||||
arg.start.reachableByFlows(sources.iterator).l.exists(f =>
|
||||
!f.elements.map(_.code).l.exists(code => mentionsToken(code, sanitizerNames))
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
)
|
||||
|
||||
def seedsIn(fileName: String) =
|
||||
seedsByFile.getOrElseUpdate(
|
||||
(sourcesS, sanitizersS, paramSrc, fileName), {
|
||||
(sourcesS, paramSrc, fileName), {
|
||||
val methods = methodsIn(fileName)
|
||||
val framework: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] =
|
||||
methods.flatMap(_.ast.isCall.filter(c => sourcePatterns.exists(p => c.code.contains(p))).l)
|
||||
// Every parameter is a CANDIDATE start; whether its callers feed it is asked only of a parameter that
|
||||
// actually begins an unsanitized flow into the field being read (`fieldIsTainted`). Asked of every
|
||||
// parameter up front, the caller check ran a repository-wide flow query per parameter of every
|
||||
// method in the file and cut PMPro's completed questions from 272 to 67 in the same budget.
|
||||
val params: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] =
|
||||
if (parameterSources == "true") methods.flatMap(_.parameter.l.filter(_.name != "this")) else Nil
|
||||
if (parameterSources == "true") methods.flatMap(_.parameter.l) else Nil
|
||||
framework ++ params
|
||||
}
|
||||
)
|
||||
@@ -333,13 +297,7 @@
|
||||
// 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
|
||||
flows.exists { f =>
|
||||
!f.elements.map(_.code).l.exists(code => mentionsToken(code, sanitizerNames)) && (f.elements.headOption match {
|
||||
case Some(param: io.shiftleft.codepropertygraph.generated.nodes.MethodParameterIn) =>
|
||||
fedByCaller(param.method, param)
|
||||
case _ => true
|
||||
})
|
||||
}
|
||||
flows.exists(f => !f.elements.map(_.code).l.exists(code => mentionsToken(code, sanitizerNames)))
|
||||
case _ => false
|
||||
})
|
||||
}
|
||||
|
||||
@@ -679,64 +679,3 @@ def test_context_locations_outside_the_changed_files_are_summarised(tmp_path: Pa
|
||||
assert kept == [r for r in whole if r["path"] == "b.js"], "the changed file's locations must be unchanged"
|
||||
assert len(summary) == 1 and summary[0]["paths"] == sorted({r["path"] for r in whole if r["path"] != "b.js"})
|
||||
assert summary[0]["locations"] == len(whole) - len(kept)
|
||||
|
||||
|
||||
@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine")
|
||||
def test_a_field_is_attacker_input_only_when_a_caller_feeds_it_request_data(tmp_path: Path) -> None:
|
||||
"""The field join's parameter seeds are the parameters some caller passes request data.
|
||||
|
||||
Counting every parameter of every method made each setter a source: on PMPro six of eight false SQL
|
||||
injections in an adjudicated sample were object fields holding database or internal values. The shape the
|
||||
join exists for -- MW WP Form's CVE-2023-6559, a constructor handed the uploads by the controller -- must
|
||||
still be found.
|
||||
"""
|
||||
from openultrasast.cpg.backend import extract_payload
|
||||
|
||||
frontend = __import__("shutil").which("php2cpg")
|
||||
if frontend is None:
|
||||
pytest.skip("php2cpg is not installed")
|
||||
src = tmp_path / "src"
|
||||
src.mkdir()
|
||||
(src / "order.php").write_text(
|
||||
"<?php\nclass Order {\n public $user_id;\n function set_user($user_id) { $this->user_id = $user_id; }\n"
|
||||
' function load() { global $wpdb; return $wpdb->get_var("SELECT x FROM t WHERE id = \'" . $this->user_id . "\'"); }\n}\n'
|
||||
"function internal_caller() { $o = new Order(); $o->set_user(42); return $o->load(); }\n"
|
||||
)
|
||||
(src / "mailer.php").write_text(
|
||||
"<?php\nclass Mailer {\n private $attachments;\n function __construct($attachments) { $this->attachments = $attachments; }\n"
|
||||
' function send() { global $wpdb; return $wpdb->query("DELETE FROM t WHERE f = \'" . $this->attachments . "\'"); }\n}\n'
|
||||
"function controller() { $m = new Mailer($_GET['files']); return $m->send(); }\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": "$_GET", "sinks": "$wpdb->get_var,$wpdb->query", "parameterSources": "true"}
|
||||
requests = tmp_path / "requests.json"
|
||||
requests.write_text(
|
||||
json.dumps(
|
||||
{"internal": {**common, "function": "load", "file": "order.php"}, "fed": {**common, "function": "send", "file": "mailer.php"}}
|
||||
)
|
||||
)
|
||||
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 = {rid: [r for r in payload[rid] if isinstance(r, dict) and r.get("sourceKind")] for rid in ("internal", "fed")}
|
||||
assert not flows["internal"], f"a setter only ever given a constant made its field attacker input: {flows['internal']}"
|
||||
assert flows["fed"], "a constructor handed $_GET data no longer carries it through the field to the sink"
|
||||
|
||||
Reference in New Issue
Block a user