mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-16 03:49:26 +00:00
fix(taint): the field join links methods only through the object itself
profile.php:424 survived the hook-argument fix through the field join: in another function of the same file, `$user = new stdClass; $user->ID = $_POST['user_id']`, and the join keys a field by its TEXT across the whole file, so the WP_User parameter's `$user->ID` became a field source. The join exists to carry object state between methods ($this->attachments written in the constructor, read in _delete_files). That reading of a field's text holds only when the base IS the object ($this, this, self). Any other base is a variable, matched within its own method only, where the engine's dataflow already sees it. A real-Joern control covers both (a local variable's field in another function stays quiet; $this state across methods is traced) and fails on the old query. 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
53d20691c3
commit
d57966a6b6
@@ -409,13 +409,29 @@
|
||||
}
|
||||
)
|
||||
|
||||
def fieldIsTainted(fileName: String, fieldCode: String): Boolean =
|
||||
// The join carries OBJECT STATE between methods: `$this->attachments` written in the constructor, read in
|
||||
// `_delete_files()`. That is what a field keyed by its text means only when the base IS the object. Any other
|
||||
// base is a variable, and the same text in two functions names two variables: PMPro's
|
||||
// `$user = new stdClass; $user->ID = $_POST['user_id']` in one function made the `WP_User` parameter's
|
||||
// `$user->ID` in another an SQL injection source. So a non-object base is joined within its own method only,
|
||||
// where it is one variable -- and where the engine's own dataflow already sees it.
|
||||
val objectBases = Set("$this", "this", "self")
|
||||
|
||||
def fieldScope(fieldCode: String, reader: io.shiftleft.codepropertygraph.generated.nodes.Method): String = {
|
||||
val base = fieldCode.split("->|\\.").headOption.map(_.trim).getOrElse("")
|
||||
if (objectBases.contains(base)) "" else reader.fullName
|
||||
}
|
||||
|
||||
def fieldIsTainted(fileName: String, fieldCode: String, scope: String = ""): Boolean =
|
||||
fieldTaintMemo.getOrElseUpdate(
|
||||
(sourcesS, fieldParamSrc, sanitizersS, fileName, fieldCode), {
|
||||
(sourcesS, fieldParamSrc, sanitizersS, fileName, scope + "|" + fieldCode), {
|
||||
val seeds = seedsIn(fileName)
|
||||
if (seeds.isEmpty) false
|
||||
else
|
||||
methodsIn(fileName).flatMap(_.ast.isCall.nameExact(ASSIGNMENT).l).exists { assignment =>
|
||||
methodsIn(fileName)
|
||||
.filter(m => scope.isEmpty || m.fullName == scope)
|
||||
.flatMap(_.ast.isCall.nameExact(ASSIGNMENT).l)
|
||||
.exists { assignment =>
|
||||
val args = assignment.argument.l
|
||||
args.size >= 2 && (args.head match {
|
||||
case target: io.shiftleft.codepropertygraph.generated.nodes.Call
|
||||
@@ -437,10 +453,14 @@
|
||||
// `$this->rest_hits` in one method and reads `$this->rest_hits->current_page_id` in another, so an exact
|
||||
// code match sees two unrelated strings where the source text says one is inside the other. Every prefix
|
||||
// that ends on a `->` boundary is checked, left to right.
|
||||
def taintedPrefixes(code: String, fileName: String): Boolean = {
|
||||
def taintedPrefixes(code: String, fileName: String, reader: io.shiftleft.codepropertygraph.generated.nodes.Method): Boolean = {
|
||||
val parts = code.split("->").map(_.trim).filter(_.nonEmpty)
|
||||
if (parts.length < 2) false
|
||||
else (2 to parts.length).exists(n => fieldIsTainted(fileName, parts.take(n).mkString("->")))
|
||||
else
|
||||
(2 to parts.length).exists { n =>
|
||||
val prefix = parts.take(n).mkString("->")
|
||||
fieldIsTainted(fileName, prefix, fieldScope(prefix, reader))
|
||||
}
|
||||
}
|
||||
|
||||
// Off by request only, and defaulting to ON when the field is absent, so nothing about the shipped
|
||||
@@ -450,7 +470,7 @@
|
||||
if (fieldSrc != "true") Iterator.empty
|
||||
else {
|
||||
val reads = cpg.call.nameExact(FIELD_ACCESS).filter(c => inScope(c.method)).l
|
||||
if (reads.isEmpty) Iterator.empty else reads.filter(r => taintedPrefixes(r.code.trim, fileS)).iterator
|
||||
if (reads.isEmpty) Iterator.empty else reads.filter(r => taintedPrefixes(r.code.trim, fileS, r.method)).iterator
|
||||
}
|
||||
|
||||
// ---- the hook half of the two-stage join (task 5.11) ----------------------------------------------
|
||||
@@ -508,7 +528,7 @@
|
||||
val framework: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] =
|
||||
callback.ast.isCall.filter(isSourceCall).l
|
||||
val fields: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] =
|
||||
callback.ast.isCall.nameExact(FIELD_ACCESS).l.filter(r => taintedPrefixes(r.code.trim, callback.filename))
|
||||
callback.ast.isCall.nameExact(FIELD_ACCESS).l.filter(r => taintedPrefixes(r.code.trim, callback.filename, r.method))
|
||||
framework ++ fields
|
||||
}
|
||||
|
||||
@@ -811,7 +831,7 @@
|
||||
.nameExact(FIELD_ACCESS)
|
||||
.filter(c => inScope(c.method))
|
||||
.l
|
||||
.exists(r => prefixKind(r.code.trim, fedFields(r.method.filename)) == "parameter" && taintedPrefixes(r.code.trim, r.method.filename))
|
||||
.exists(r => prefixKind(r.code.trim, fedFields(r.method.filename)) == "parameter" && taintedPrefixes(r.code.trim, r.method.filename, r.method))
|
||||
if (confirmed) "source" else "parameter"
|
||||
}
|
||||
val carried = carriedKind.nonEmpty
|
||||
|
||||
@@ -802,3 +802,59 @@ def test_a_filter_returns_its_value_argument_not_its_context(tmp_path: Path) ->
|
||||
flows = {rid: [r for r in payload[rid] if isinstance(r, dict) and r.get("sourceKind")] for rid in ("context", "value")}
|
||||
assert not flows["context"], f"filter context became its result: {flows['context']}"
|
||||
assert flows["value"], "the filtered value no longer carries request data"
|
||||
|
||||
|
||||
@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine")
|
||||
def test_the_field_join_links_methods_only_through_the_object_itself(tmp_path: Path) -> None:
|
||||
"""`$this->x` written in one method and read in another is object state; `$user->ID` in two functions is
|
||||
two variables. PMPro's `$user = new stdClass; $user->ID = $_POST['user_id']` in one function made the
|
||||
WP_User parameter's `$user->ID` in another an SQL injection."""
|
||||
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 / "app.php").write_text(
|
||||
"<?php\n"
|
||||
"function fill() { $user = new stdClass; $user->ID = $_POST['user_id']; return $user; }\n"
|
||||
'function history($user) { global $wpdb; return $wpdb->get_var("SELECT 1 WHERE u = \'" . $user->ID . "\'"); }\n'
|
||||
"class Mailer {\n public $attachments;\n function collect() { $this->attachments = $_POST['files']; }\n"
|
||||
' function send() { global $wpdb; return $wpdb->query("DELETE FROM t WHERE f = \'" . $this->attachments . "\'"); }\n}\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": "$_POST",
|
||||
"sinks": "$wpdb->get_var,$wpdb->query",
|
||||
"file": "app.php",
|
||||
"parameterSources": "false",
|
||||
"fieldParameterSources": "true",
|
||||
}
|
||||
requests = tmp_path / "requests.json"
|
||||
requests.write_text(json.dumps({"local": {**common, "function": "history"}, "object": {**common, "function": "send"}}))
|
||||
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 ("local", "object")}
|
||||
assert not flows["local"], f"another function's local variable tainted this parameter's field: {flows['local']}"
|
||||
assert flows["object"], "object state written in one method and read in another is no longer traced"
|
||||
|
||||
Reference in New Issue
Block a user