mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-17 12:09:27 +00:00
fix(cpg): null-guard method access in the query scripts -- a class-level call aborted the whole taint batch
On real PHP source (CPS-IT/mailqueue) a call outside any method has a null .method; inScope/nestedInLabeled dereferenced it (NullPointerException) and the script exited 1, losing every taint answer for the repository. 16 sites in taint/dominance/config/vocabulary.sc now treat a null method as out of scope; non-null behaviour is unchanged (same outputs on the repro graph for the other scripts). Static regression test added. Implemented with Codex. Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
41f56170bb
commit
e492ebd16e
@@ -53,7 +53,8 @@
|
|||||||
}
|
}
|
||||||
|
|
||||||
val calls = {
|
val calls = {
|
||||||
val all = cpg.call.filter(c => matches(c.name, c.code)).l
|
// CPS-IT/mailqueue Extension::KEY has a null .method; exclude methodless calls before attribution.
|
||||||
|
val all = cpg.call.filter(c => Option(c.method).nonEmpty && matches(c.name, c.code)).l
|
||||||
val inFile = if (fileS.isEmpty) all else all.filter(_.method.filename.endsWith(fileS))
|
val inFile = if (fileS.isEmpty) all else all.filter(_.method.filename.endsWith(fileS))
|
||||||
if (function.isEmpty) inFile else inFile.filter(_.method.name == function)
|
if (function.isEmpty) inFile else inFile.filter(_.method.name == function)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -172,7 +172,8 @@
|
|||||||
// repository this was measured on, and unscoped rows attributed the model's unguarded query to the
|
// repository this was measured on, and unscoped rows attributed the model's unguarded query to the
|
||||||
// handler. The sibling comparison is a statement about a FILE contradicting itself, which is what the
|
// handler. The sibling comparison is a statement about a FILE contradicting itself, which is what the
|
||||||
// rung's justification says, so the file is also the right scope for it.
|
// rung's justification says, so the file is also the right scope for it.
|
||||||
val allOperations = cpg.call.filter(isOperation).l
|
// CPS-IT/mailqueue Extension::KEY has a null .method; exclude methodless calls before attribution.
|
||||||
|
val allOperations = cpg.call.filter(c => Option(c.method).nonEmpty).filter(isOperation).l
|
||||||
val opCalls = if (fileS.isEmpty) allOperations else allOperations.filter(_.method.filename.endsWith(fileS))
|
val opCalls = if (fileS.isEmpty) allOperations else allOperations.filter(_.method.filename.endsWith(fileS))
|
||||||
|
|
||||||
// ---- The module boundary an application puts between its route and its query --------------------------
|
// ---- The module boundary an application puts between its route and its query --------------------------
|
||||||
@@ -236,7 +237,7 @@
|
|||||||
if (fileS.isEmpty) Nil
|
if (fileS.isEmpty) Nil
|
||||||
else
|
else
|
||||||
fileMethods(fileS).flatMap { m =>
|
fileMethods(fileS).flatMap { m =>
|
||||||
m.call.l.flatMap { via =>
|
m.call.l.filter(c => Option(c.method).nonEmpty).flatMap { via =>
|
||||||
via.callee.l
|
via.callee.l
|
||||||
.filter(_.isExternal)
|
.filter(_.isExternal)
|
||||||
.flatMap(stub => stubTarget(stub.fullName))
|
.flatMap(stub => stubTarget(stub.fullName))
|
||||||
|
|||||||
@@ -226,6 +226,7 @@
|
|||||||
// module region exists to cover the code that is in NO function, so its membership is direct.
|
// module region exists to cover the code that is in NO function, so its membership is direct.
|
||||||
val moduleScope = function == "<module>"
|
val moduleScope = function == "<module>"
|
||||||
|
|
||||||
|
// CPS-IT/mailqueue Extension::KEY is a class-level PHP call with a null .method; exclude it from scope.
|
||||||
def nestedInLabeled(m: io.shiftleft.codepropertygraph.generated.nodes.Method): Boolean =
|
def nestedInLabeled(m: io.shiftleft.codepropertygraph.generated.nodes.Method): Boolean =
|
||||||
if (moduleScope) m.name == function
|
if (moduleScope) m.name == function
|
||||||
else
|
else
|
||||||
@@ -496,7 +497,7 @@
|
|||||||
case c: io.shiftleft.codepropertygraph.generated.nodes.Call if originAnchors.contains(c.name) => true
|
case c: io.shiftleft.codepropertygraph.generated.nodes.Call if originAnchors.contains(c.name) => true
|
||||||
case c: io.shiftleft.codepropertygraph.generated.nodes.Call if concatenations.contains(c.name) =>
|
case c: io.shiftleft.codepropertygraph.generated.nodes.Call if concatenations.contains(c.name) =>
|
||||||
c.argument.l.sortBy(_.argumentIndex).headOption.exists(a => fixesOrigin(a, depth + 1))
|
c.argument.l.sortBy(_.argumentIndex).headOption.exists(a => fixesOrigin(a, depth + 1))
|
||||||
case i: io.shiftleft.codepropertygraph.generated.nodes.Identifier =>
|
case i: io.shiftleft.codepropertygraph.generated.nodes.Identifier if i.method != null =>
|
||||||
// A variable fixes the origin when EVERY assignment to it in the method does.
|
// A variable fixes the origin when EVERY assignment to it in the method does.
|
||||||
val writes = i.method.ast.isCall.nameExact(ASSIGNMENT).l.filter(_.argument.l.sortBy(_.argumentIndex).headOption.exists {
|
val writes = i.method.ast.isCall.nameExact(ASSIGNMENT).l.filter(_.argument.l.sortBy(_.argumentIndex).headOption.exists {
|
||||||
case t: io.shiftleft.codepropertygraph.generated.nodes.Identifier => t.name == i.name
|
case t: io.shiftleft.codepropertygraph.generated.nodes.Identifier => t.name == i.name
|
||||||
@@ -790,7 +791,7 @@
|
|||||||
case _ => false
|
case _ => false
|
||||||
}
|
}
|
||||||
ternary || (end :: elements.filter(stored)).distinctBy(_.id).exists {
|
ternary || (end :: elements.filter(stored)).distinctBy(_.id).exists {
|
||||||
case cfg: io.shiftleft.codepropertygraph.generated.nodes.CfgNode =>
|
case cfg: io.shiftleft.codepropertygraph.generated.nodes.CfgNode if cfg.method != null =>
|
||||||
val method = cfg.method
|
val method = cfg.method
|
||||||
method.ast.isControlStructure.controlStructureType("IF").l.exists { cs =>
|
method.ast.isControlStructure.controlStructureType("IF").l.exists { cs =>
|
||||||
cs.condition.l.headOption.exists { cond =>
|
cs.condition.l.headOption.exists { cond =>
|
||||||
@@ -898,7 +899,7 @@
|
|||||||
def fieldSourceNodes =
|
def fieldSourceNodes =
|
||||||
if (fieldSrc != "true") Iterator.empty
|
if (fieldSrc != "true") Iterator.empty
|
||||||
else {
|
else {
|
||||||
val reads = cpg.call.nameExact(FIELD_ACCESS).filter(c => inScope(c.method)).l.filterNot(writeTarget)
|
val reads = cpg.call.nameExact(FIELD_ACCESS).filter(c => Option(c.method).exists(inScope)).l.filterNot(writeTarget)
|
||||||
if (reads.isEmpty) Iterator.empty
|
if (reads.isEmpty) Iterator.empty
|
||||||
else reads.filter(r => taintedPrefixes(r.code.trim, fileS, r.method) && !overwrittenBefore(r)).iterator
|
else reads.filter(r => taintedPrefixes(r.code.trim, fileS, r.method) && !overwrittenBefore(r)).iterator
|
||||||
}
|
}
|
||||||
@@ -958,7 +959,7 @@
|
|||||||
val framework: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] =
|
val framework: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] =
|
||||||
callback.ast.isCall.filter(isSourceCall).l
|
callback.ast.isCall.filter(isSourceCall).l
|
||||||
val fields: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] =
|
val fields: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] =
|
||||||
callback.ast.isCall.nameExact(FIELD_ACCESS).l.filter(r => taintedPrefixes(r.code.trim, callback.filename, r.method))
|
callback.ast.isCall.nameExact(FIELD_ACCESS).l.filter(r => Option(r.method).exists(m => taintedPrefixes(r.code.trim, callback.filename, m)))
|
||||||
framework ++ fields
|
framework ++ fields
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1013,7 +1014,7 @@
|
|||||||
def hookSourceNodes =
|
def hookSourceNodes =
|
||||||
if (hookTable.isEmpty || hookApply.isEmpty) Iterator.empty
|
if (hookTable.isEmpty || hookApply.isEmpty) Iterator.empty
|
||||||
else {
|
else {
|
||||||
val applies = cpg.call.filter(c => hookApply.contains(c.name)).filter(c => inScope(c.method)).l
|
val applies = cpg.call.filter(c => hookApply.contains(c.name)).filter(c => Option(c.method).exists(inScope)).l
|
||||||
if (applies.isEmpty) Iterator.empty
|
if (applies.isEmpty) Iterator.empty
|
||||||
else {
|
else {
|
||||||
val keys = applies.flatMap { call =>
|
val keys = applies.flatMap { call =>
|
||||||
@@ -1024,7 +1025,7 @@
|
|||||||
else
|
else
|
||||||
cpg.call
|
cpg.call
|
||||||
.nameExact(INDEX_ACCESS)
|
.nameExact(INDEX_ACCESS)
|
||||||
.filter(c => inScope(c.method))
|
.filter(c => Option(c.method).exists(inScope))
|
||||||
.l
|
.l
|
||||||
.filter(access => keys.contains(keyOf(access)))
|
.filter(access => keys.contains(keyOf(access)))
|
||||||
// ...and actually derived from the filtered value, not merely sharing a key name with it.
|
// ...and actually derived from the filtered value, not merely sharing a key name with it.
|
||||||
@@ -1108,7 +1109,7 @@
|
|||||||
def sinkCalls =
|
def sinkCalls =
|
||||||
sinkCandidatesMemo
|
sinkCandidatesMemo
|
||||||
.getOrElseUpdate(sinksS, cpg.call.filter(c => sinkNames.exists(n => sinkMatches(c, n))).l)
|
.getOrElseUpdate(sinksS, cpg.call.filter(c => sinkNames.exists(n => sinkMatches(c, n))).l)
|
||||||
.filter(c => inScope(c.method))
|
.filter(c => Option(c.method).exists(inScope))
|
||||||
|
|
||||||
// ---- stage one of the two-stage join WITHOUT dataflow, for evidence mode (flow-aware-ranking, phase 2) --
|
// ---- stage one of the two-stage join WITHOUT dataflow, for evidence mode (flow-aware-ranking, phase 2) --
|
||||||
//
|
//
|
||||||
@@ -1170,7 +1171,7 @@
|
|||||||
)
|
)
|
||||||
|
|
||||||
def fieldCarriedKind: String =
|
def fieldCarriedKind: String =
|
||||||
strongest(cpg.call.nameExact(FIELD_ACCESS).filter(c => inScope(c.method)).l.map(r => prefixKind(r.code.trim, fedFields(r.method.filename))))
|
strongest(cpg.call.nameExact(FIELD_ACCESS).filter(c => Option(c.method).exists(inScope)).l.map(r => prefixKind(r.code.trim, fedFields(r.method.filename))))
|
||||||
|
|
||||||
def callbackFedKind(callback: String): String =
|
def callbackFedKind(callback: String): String =
|
||||||
callbackFedMemo.getOrElseUpdate(
|
callbackFedMemo.getOrElseUpdate(
|
||||||
@@ -1187,7 +1188,7 @@
|
|||||||
def hookCarriedKind: String =
|
def hookCarriedKind: String =
|
||||||
if (hookTable.isEmpty || hookApply.isEmpty) ""
|
if (hookTable.isEmpty || hookApply.isEmpty) ""
|
||||||
else
|
else
|
||||||
strongest(cpg.call.filter(c => hookApply.contains(c.name)).filter(c => inScope(c.method)).l.flatMap { call =>
|
strongest(cpg.call.filter(c => hookApply.contains(c.name)).filter(c => Option(c.method).exists(inScope)).l.flatMap { call =>
|
||||||
val hook = call.argument.l.headOption.map(a => unquote(a.code)).getOrElse("")
|
val hook = call.argument.l.headOption.map(a => unquote(a.code)).getOrElse("")
|
||||||
hookTable.getOrElse(hook, Nil).map(callbackFedKind)
|
hookTable.getOrElse(hook, Nil).map(callbackFedKind)
|
||||||
})
|
})
|
||||||
@@ -1233,7 +1234,7 @@
|
|||||||
else {
|
else {
|
||||||
val confirmed = cpg.call
|
val confirmed = cpg.call
|
||||||
.nameExact(FIELD_ACCESS)
|
.nameExact(FIELD_ACCESS)
|
||||||
.filter(c => inScope(c.method))
|
.filter(c => Option(c.method).exists(inScope))
|
||||||
.l
|
.l
|
||||||
.exists(r => prefixKind(r.code.trim, fedFields(r.method.filename)) == "parameter" && taintedPrefixes(r.code.trim, r.method.filename, r.method))
|
.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"
|
if (confirmed) "source" else "parameter"
|
||||||
@@ -1277,9 +1278,9 @@
|
|||||||
// Field carrying is deliberately same-file in the existing abstraction. Retain
|
// Field carrying is deliberately same-file in the existing abstraction. Retain
|
||||||
// its file context conservatively, never claim every method carries the value.
|
// its file context conservatively, never claim every method carries the value.
|
||||||
val fieldFiles = if (fieldCarriedKind.isEmpty) Set.empty[String] else
|
val fieldFiles = if (fieldCarriedKind.isEmpty) Set.empty[String] else
|
||||||
cpg.call.nameExact(FIELD_ACCESS).filter(c => inScope(c.method)).l.map(_.method.filename).toSet
|
cpg.call.nameExact(FIELD_ACCESS).filter(c => Option(c.method).exists(inScope)).l.map(_.method.filename).toSet
|
||||||
val callbacks = if (hookApply.isEmpty) Nil else cpg.call.filter(c => hookApply.contains(c.name))
|
val callbacks = if (hookApply.isEmpty) Nil else cpg.call.filter(c => hookApply.contains(c.name))
|
||||||
.filter(c => inScope(c.method)).l.flatMap { call =>
|
.filter(c => Option(c.method).exists(inScope)).l.flatMap { call =>
|
||||||
val hook = call.argument.l.headOption.map(a => unquote(a.code)).getOrElse("")
|
val hook = call.argument.l.headOption.map(a => unquote(a.code)).getOrElse("")
|
||||||
hookTable.getOrElse(hook, Nil).flatMap(name => cpg.method.nameExact(name).filterNot(_.isExternal).l)
|
hookTable.getOrElse(hook, Nil).flatMap(name => cpg.method.nameExact(name).filterNot(_.isExternal).l)
|
||||||
}.distinct
|
}.distinct
|
||||||
@@ -1401,7 +1402,7 @@
|
|||||||
def traced(flow: io.joern.dataflowengineoss.language.Path): ujson.Arr =
|
def traced(flow: io.joern.dataflowengineoss.language.Path): ujson.Arr =
|
||||||
ujson.Arr.from(spliced(flow.elements.l).map { node =>
|
ujson.Arr.from(spliced(flow.elements.l).map { node =>
|
||||||
val where = node match {
|
val where = node match {
|
||||||
case c: io.shiftleft.codepropertygraph.generated.nodes.CfgNode => c.method.filename.split("/").last + ":" + c.method.name
|
case c: io.shiftleft.codepropertygraph.generated.nodes.CfgNode => Option(c.method).map(m => m.filename.split("/").last + ":" + m.name).getOrElse("?")
|
||||||
case _ => "?"
|
case _ => "?"
|
||||||
}
|
}
|
||||||
s"$where:${node.lineNumber.getOrElse(-1)} [${node.label}] ${node.code.take(120)}"
|
s"$where:${node.lineNumber.getOrElse(-1)} [${node.label}] ${node.code.take(120)}"
|
||||||
@@ -1436,7 +1437,7 @@
|
|||||||
"sinkArg0Literal" -> arg0Literal,
|
"sinkArg0Literal" -> arg0Literal,
|
||||||
"bounded" -> bounds.nonEmpty,
|
"bounded" -> bounds.nonEmpty,
|
||||||
"bound" -> bounds.headOption.getOrElse("").take(120),
|
"bound" -> bounds.headOption.getOrElse("").take(120),
|
||||||
"inLabeledScope" -> (function.isEmpty || nestedInLabeled(sink.method) || reachableMethods.contains(sink.method.fullName))
|
"inLabeledScope" -> (function.isEmpty || Option(sink.method).exists(nestedInLabeled) || reachableMethods.contains(sink.method.fullName))
|
||||||
)
|
)
|
||||||
if (traceS == "true") row("trace") = traced(shortest)
|
if (traceS == "true") row("trace") = traced(shortest)
|
||||||
row
|
row
|
||||||
|
|||||||
@@ -51,11 +51,13 @@
|
|||||||
val sourceMemo = scala.collection.mutable.Map.empty[String, Boolean]
|
val sourceMemo = scala.collection.mutable.Map.empty[String, Boolean]
|
||||||
def nearSource(c: io.shiftleft.codepropertygraph.generated.nodes.Call): Boolean =
|
def nearSource(c: io.shiftleft.codepropertygraph.generated.nodes.Call): Boolean =
|
||||||
if (sourcePatterns.isEmpty) false
|
if (sourcePatterns.isEmpty) false
|
||||||
else
|
// CPS-IT/mailqueue Extension::KEY has a null .method; it cannot be near a method source.
|
||||||
|
else Option(c.method).exists { m =>
|
||||||
sourceMemo.getOrElseUpdate(
|
sourceMemo.getOrElseUpdate(
|
||||||
c.method.fullName,
|
m.fullName,
|
||||||
c.method.ast.isCall.exists(x => sourcePatterns.exists(p => boundedPattern(p).matcher(x.code).find()))
|
m.ast.isCall.exists(x => sourcePatterns.exists(p => boundedPattern(p).matcher(x.code).find()))
|
||||||
)
|
)
|
||||||
|
}
|
||||||
|
|
||||||
def isModelled(c: io.shiftleft.codepropertygraph.generated.nodes.Call): Boolean = {
|
def isModelled(c: io.shiftleft.codepropertygraph.generated.nodes.Call): Boolean = {
|
||||||
val callee = calleeText(c.code)
|
val callee = calleeText(c.code)
|
||||||
|
|||||||
@@ -20,6 +20,7 @@ host's pytest mounted in, which needs no network and installs nothing:
|
|||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
import json
|
import json
|
||||||
|
import re
|
||||||
import subprocess
|
import subprocess
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
||||||
@@ -28,6 +29,18 @@ import pytest
|
|||||||
QUERIES = Path("src/openultrasast/cpg/queries")
|
QUERIES = Path("src/openultrasast/cpg/queries")
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.parametrize("name", ["taint", "dominance", "config", "census", "overlay", "vocabulary"])
|
||||||
|
def test_query_scope_checks_guard_nullable_methods(name: str) -> None:
|
||||||
|
"""PHP class constants can have no method; one unsafe scope check aborts the batch."""
|
||||||
|
script = QUERIES / f"{name}.sc"
|
||||||
|
source = script.read_text()
|
||||||
|
assert source, f"{script} is empty"
|
||||||
|
# Require Option(node.method).exists(predicate), keeping the null guard at the call site.
|
||||||
|
unsafe = re.compile(r"\b(?:inScope|nestedInLabeled)\s*\(\s*[\w.]+\s*\.\s*method\s*\)")
|
||||||
|
sites = [source.count("\n", 0, match.start()) + 1 for match in unsafe.finditer(source)]
|
||||||
|
assert not sites, f"{script}: unguarded method scope checks at lines {sites}"
|
||||||
|
|
||||||
|
|
||||||
def _joern() -> str | None:
|
def _joern() -> str | None:
|
||||||
import shutil
|
import shutil
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user