mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-16 19:59:26 +00:00
feat(access-control): carry the obligation across the route-and-DAO split
The census measured this family completing 35 questions on NodeGoat with no obligation raised anywhere: every modelled data operation lives in app/data/*-dao.js, none in a route handler file, and operations were scoped to the region's own file. The family had nothing to be right or wrong about. The obligation now follows the one named call the handler writes, out of the region's file and no further. How it is followed is the point. jssrc2cpg does not resolve allocationsDAO.getByUserIdAndThreshold(...) to the DAO body: the callee is an external stub with no code. That stub's fullName records where the member was written, so the implementation is reached by the graph's own provenance rather than by a repository-wide name guess, which is the false positive the file scope was closed against. A control asserts it: a third module defining a member of the same name that nothing imports is not attributed. A row is attributed to the region's DECLARED function, which is what lets a question about displayAllocations match a graph that holds a <lambda>. A carried row is also checked for a guard at the CALL, because that is where a handler establishes who is asking. The method holding a carried operation joins the question's context scope, so the module holding the query cannot change under a claim that depends on it without the change being attributed. The witness names both sites and the finding is located at the call, which is the line a contributor changed. A carried obligation corroborates, it does not entail, and that was measured rather than assumed. Entailing it reported handleSignup, which looks a user up by name to check the name is free during registration and is public by design, because two authenticated siblings in the same file pass req.session.userId into the same module. The entailed rung's justification is that one FILE contradicts itself, same file and same trust context; carried, the compared operations live in a data-access module that handlers of different trust contexts share. The corroborated band already asks the right residual: does this operation require a check, or is the route legitimately public? Measured on NodeGoat: 15 handlers asked, obligations raised at zero before and six now, three reaching a verdict, all corroborated, none reported without a judge. VAmPI unchanged at two entailed findings, because it keeps handler and query in one file and carries nothing. What this does not reach is recorded, not implied. NodeGoat's documented insecure direct object reference still raises nothing, because its module reads with allocationsCol.find(...) and find is not a modelled operation: Array.prototype.find and jQuery's .find are everywhere, so it needs an argument-shape discriminator and its own measurement (task 14.8). Two of the three corroborated claims are correct code whose identity read a destructuring spelled away, and the cross-file contradiction that would entail the real defect needs a sibling-scope decision (task 14.7). 1,415 tests pass, fifteen skipped; eleven Joern-gated query tests pass in the image. Co-Authored-By: Claude Opus 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
Claude Opus 5
parent
f31ef862a7
commit
102a21fc4e
@@ -135,11 +135,87 @@
|
||||
// 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
|
||||
// rung's justification says, so the file is also the right scope for it.
|
||||
val opCalls = {
|
||||
val all = cpg.call.filter(c => matchesOperation(c.name, c.code)).l
|
||||
if (fileS.isEmpty) all else all.filter(_.method.filename.endsWith(fileS))
|
||||
val allOperations = cpg.call.filter(c => matchesOperation(c.name, c.code)).l
|
||||
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 --------------------------
|
||||
//
|
||||
// Measured on NodeGoat: every modelled data operation lives in `app/data/*-dao.js`, none in a route handler
|
||||
// file, and the family answered 35 questions there with nothing to say -- the obligation was never raised
|
||||
// anywhere the question was asked. Widening the file scope is not the fix, because an unscoped row is
|
||||
// exactly what attributed a model's unguarded query to a handler of the same name. The obligation is
|
||||
// carried along the call the handler actually writes.
|
||||
//
|
||||
// How it is followed matters. jssrc2cpg does not resolve `allocationsDAO.getByUserIdAndThreshold(...)` to
|
||||
// the DAO's body: the callee is an EXTERNAL stub with no code and no filename. But the stub's fullName
|
||||
// records where the member was written -- `app/data/allocations-dao.js::program:AllocationsDAO:getByUser...`
|
||||
// -- so the implementation is reached by the graph's own provenance rather than by guessing a name across
|
||||
// the repository, which is the false positive this scope was closed to avoid.
|
||||
def stubTarget(fullName: String): Option[(String, String)] = {
|
||||
val idx = fullName.indexOf("::program:")
|
||||
val member = fullName.substring(fullName.lastIndexOf(':') + 1)
|
||||
if (idx <= 0 || member.isEmpty) None else Some((fullName.substring(0, idx), member))
|
||||
}
|
||||
|
||||
def spans(outer: io.shiftleft.codepropertygraph.generated.nodes.Method, inner: io.shiftleft.codepropertygraph.generated.nodes.Method): Boolean =
|
||||
outer.filename == inner.filename &&
|
||||
inner.lineNumber.getOrElse(-1) >= outer.lineNumber.getOrElse(0) &&
|
||||
inner.lineNumberEnd.getOrElse(-1) <= outer.lineNumberEnd.getOrElse(Int.MaxValue)
|
||||
|
||||
// The method a MEMBER NAME is written as, in one file. A handler or a DAO method written
|
||||
// `this.getUserById = (userId, callback) => {}` is a `<lambda>` in the graph, and the assignment puts the
|
||||
// lambda's first line on itself -- which is how the written name and the graph's method are joined without
|
||||
// a repository-wide name lookup. A language that gives its functions real names (every Python handler this
|
||||
// family already answers) matches on the first branch and never needs the second.
|
||||
val methodsByFile = scala.collection.mutable.Map.empty[String, List[io.shiftleft.codepropertygraph.generated.nodes.Method]]
|
||||
def fileMethods(path: String): List[io.shiftleft.codepropertygraph.generated.nodes.Method] =
|
||||
methodsByFile.getOrElseUpdate(path, cpg.method.filterNot(_.isExternal).filter(_.filename.endsWith(path)).l)
|
||||
|
||||
def writtenAs(path: String, member: String): List[io.shiftleft.codepropertygraph.generated.nodes.Method] = {
|
||||
val inFile = fileMethods(path)
|
||||
val named = inFile.filter(_.name == member)
|
||||
if (named.nonEmpty) named
|
||||
else {
|
||||
val prefixes = List("this." + member, member, "const " + member, "let " + member, "var " + member)
|
||||
val lines = inFile
|
||||
.flatMap(_.ast.isCall.nameExact("<operator>.assignment").l)
|
||||
.filter(a => prefixes.exists(p => a.code.startsWith(p + " =") || a.code.startsWith(p + "=")))
|
||||
.flatMap(_.lineNumber)
|
||||
.toSet
|
||||
inFile.filter(m => m.lineNumber.exists(lines.contains))
|
||||
}
|
||||
}
|
||||
|
||||
def operationsWithin(path: String, member: String): List[io.shiftleft.codepropertygraph.generated.nodes.Call] = {
|
||||
val holders = writtenAs(path, member)
|
||||
if (holders.isEmpty) Nil
|
||||
else allOperations.filter(op => op.method.filename.endsWith(path) && holders.exists(h => h.id == op.method.id || spans(h, op.method)))
|
||||
}
|
||||
|
||||
// One named call out of the scoped file, not a transitive walk. The claim is about the call this handler
|
||||
// WRITES: a deeper walk would attribute a helper's helper's query to a handler that never mentions it, and
|
||||
// the witness could no longer name the two sites a reader has to open.
|
||||
val carried: List[(io.shiftleft.codepropertygraph.generated.nodes.Call, io.shiftleft.codepropertygraph.generated.nodes.Call)] =
|
||||
if (fileS.isEmpty) Nil
|
||||
else
|
||||
fileMethods(fileS).flatMap { m =>
|
||||
m.call.l.flatMap { via =>
|
||||
via.callee.l
|
||||
.filter(_.isExternal)
|
||||
.flatMap(stub => stubTarget(stub.fullName))
|
||||
.distinct
|
||||
.filterNot { case (path, _) => path.endsWith(fileS) }
|
||||
.flatMap { case (path, member) => operationsWithin(path, member).map(op => (op, via)) }
|
||||
}
|
||||
}.distinctBy { case (op, via) => (op.id, via.id) }
|
||||
|
||||
// The region asks about a declared function; the graph may hold it as a lambda. Attributing a row to the
|
||||
// declared name is what lets the arbiter match at all -- the same query-to-operation identity problem task
|
||||
// 12.3 fixed at the scope end, appearing here at the attribution end.
|
||||
val labelled = if (function.isEmpty || fileS.isEmpty) Nil else writtenAs(fileS, function)
|
||||
def attributedName(m: io.shiftleft.codepropertygraph.generated.nodes.Method): String =
|
||||
if (labelled.exists(l => l.id == m.id || spans(l, m))) function else m.name
|
||||
|
||||
// Change-attribution context for THIS family, describing the scope this query actually used.
|
||||
// It is deliberately not taint's projection: dominance compares sibling operations across a
|
||||
// file, so the file's own methods are the scope, the methods holding a matched operation are
|
||||
@@ -154,21 +230,26 @@
|
||||
val scoped =
|
||||
if (fileS.isEmpty) cpg.method.filterNot(_.isExternal).l
|
||||
else cpg.method.filterNot(_.isExternal).filter(_.filename.endsWith(fileS)).l
|
||||
val operationIds = opCalls.map(_.method.id).toSet
|
||||
val operationIds = opCalls.map(_.method.id).toSet ++ carried.map { case (_, via) => via.method.id }.toSet
|
||||
val guardIds = scoped.filter(m => m.ast.isCall.code.l.exists(code => mentionsGuard(code))).map(_.id).toSet -- operationIds
|
||||
val rows = scoped.map { m =>
|
||||
val kind = if (operationIds.contains(m.id)) "operation" else if (guardIds.contains(m.id)) "guard" else "sibling"
|
||||
location(m, kind)
|
||||
}
|
||||
// A carried obligation makes the claim span two files, so the method holding the operation is part of this
|
||||
// question's scope even though it is not in the region's file. Leaving it out would let the module that
|
||||
// holds the query change under a claim that depends on it without the change being attributed.
|
||||
val carriedRows = carried.map { case (op, _) => location(op.method, "carried_operation") }.distinctBy(row => row.value.toString)
|
||||
// An empty scope is reported, never implied: a file the frontend produced no methods for
|
||||
// cannot support a statement about that file contradicting itself.
|
||||
val boundaries =
|
||||
if (scoped.isEmpty) List(ujson.Obj("kind" -> "context_boundary", "reason" -> "context_scope_empty:contributor-scan"))
|
||||
else Nil
|
||||
ujson.Obj("kind" -> "context_summary") :: (rows ++ boundaries)
|
||||
ujson.Obj("kind" -> "context_summary") :: (rows ++ carriedRows ++ boundaries)
|
||||
}
|
||||
|
||||
val rows = opCalls.map { op =>
|
||||
// `via` is the call that carried the obligation here, absent when the operation is in the region's own file.
|
||||
def rowFor(op: io.shiftleft.codepropertygraph.generated.nodes.Call, via: Option[io.shiftleft.codepropertygraph.generated.nodes.Call]): ujson.Obj = {
|
||||
val method = op.method
|
||||
|
||||
val applicable = guardsFor(op.name, op.code)
|
||||
@@ -183,19 +264,36 @@
|
||||
// 3. dominating guard: a call naming a discharger that actually dominates the operation.
|
||||
val dominating = op.dominatedBy.isCall.code.l.filter(c => mentions(c, applicable))
|
||||
|
||||
val guards = (inArguments ++ inCondition ++ dominating).distinct.map(_.take(120))
|
||||
// 4. a guard at the CARRIER. When the obligation crossed a module boundary the check that discharges it is
|
||||
// overwhelmingly on this side of it: the handler establishes who is asking and passes the result down.
|
||||
// Reading only the operation's own site would report every correctly guarded handler in the repository.
|
||||
val atCarrier = via.toList.flatMap { call =>
|
||||
call.argument.code.l.filter(c => mentions(c, applicable)) ++
|
||||
call.method.controlStructure.condition.code.l.filter(c => mentions(c, applicable)) ++
|
||||
call.dominatedBy.isCall.code.l.filter(c => mentions(c, applicable))
|
||||
}
|
||||
|
||||
val guards = (inArguments ++ inCondition ++ dominating ++ atCarrier).distinct.map(_.take(120))
|
||||
val attributed = via.map(call => attributedName(call.method)).getOrElse(attributedName(method))
|
||||
|
||||
ujson.Obj(
|
||||
"operation" -> op.code.take(200),
|
||||
"opLine" -> op.lineNumber.getOrElse(-1).toString,
|
||||
"opMethod" -> method.name,
|
||||
"opMethod" -> attributed,
|
||||
// Where the operation is. Without it an access-control finding reaches a contributor as
|
||||
// `api_views/users.py:?:update_password` -- a location no editor can open.
|
||||
"opFile" -> method.filename,
|
||||
// The two sites of a carried obligation: the call a reader would open first, and the operation it
|
||||
// reaches. Empty for an operation the region's own file holds, which is the committed shape.
|
||||
"viaCall" -> via.map(_.code.take(120).replace("\n", " ")).getOrElse(""),
|
||||
"viaLine" -> via.flatMap(_.lineNumber).map(_.toString).getOrElse(""),
|
||||
"viaFile" -> via.map(_.method.filename).getOrElse(""),
|
||||
"dominatingGuards" -> ujson.Arr(guards.map(ujson.Str(_)): _*)
|
||||
)
|
||||
}
|
||||
|
||||
val rows = opCalls.map(op => rowFor(op, None)) ++ carried.map { case (op, via) => rowFor(op, Some(via)) }
|
||||
|
||||
if (contextS == "true") rows ++ contextRowsFor() else rows
|
||||
}
|
||||
|
||||
|
||||
@@ -71,18 +71,54 @@ def verdict(cpg: CpgResult, spec: DominanceSpec, *, function: str = "", file: st
|
||||
guarded_methods = sorted({str(row["opMethod"]) for row in siblings if row["guards"]})
|
||||
target = min(unguarded, key=lambda row: (row["opMethod"], row["opLine"]))
|
||||
|
||||
if guarded_methods:
|
||||
where = _where(target)
|
||||
# A carried obligation does not ENTAIL, however clean the asymmetry looks.
|
||||
#
|
||||
# The rung's justification is that one FILE contradicts itself: same file, same trust context, so an
|
||||
# unguarded operation beside a guarded one is evidence. Once the obligation is carried across a module
|
||||
# boundary, the operations being compared live in a data-access module that handlers of DIFFERENT trust
|
||||
# contexts share, and a route file mixing public with authenticated handlers is the normal shape -- so the
|
||||
# asymmetry stops implying inconsistency.
|
||||
#
|
||||
# Measured on NodeGoat, where this was not hypothetical: `handleSignup` looks a user up by name to check
|
||||
# the name is free, which is public by design, and two authenticated siblings in the same file pass
|
||||
# `req.session.userId` into the same module. The asymmetry is real and the defect is not. That question --
|
||||
# does this operation require a check, or is the route legitimately public? -- is exactly what the
|
||||
# corroborated band's residual asks, so a carried row is capped there and the judge decides it.
|
||||
if guarded_methods and not target.get("viaLine"):
|
||||
witness = (
|
||||
f"{target['opMethod']} line {target['opLine']}: no discharging guard, "
|
||||
f"while {len(guarded_methods)} sibling handler(s) are guarded "
|
||||
f"({', '.join(guarded_methods[:3])})"
|
||||
f"{where}: no discharging guard, while {len(guarded_methods)} sibling handler(s) are guarded ({', '.join(guarded_methods[:3])})"
|
||||
)
|
||||
return Verdict(rung=Rung.ENTAILED, family=spec.family, witness=witness, location=_location(target))
|
||||
|
||||
witness = f"{target['opMethod']} line {target['opLine']}: obligated operation with no discharging guard, and no guarded sibling"
|
||||
if guarded_methods:
|
||||
witness = (
|
||||
f"{where}: no discharging guard, while {len(guarded_methods)} sibling handler(s) reaching the same "
|
||||
f"module are guarded ({', '.join(guarded_methods[:3])}); the obligation was carried across a module "
|
||||
f"boundary, so those siblings need not share this route's trust context"
|
||||
)
|
||||
return Verdict(rung=Rung.CORROBORATED, family=spec.family, witness=witness, location=_location(target))
|
||||
|
||||
witness = f"{where}: obligated operation with no discharging guard, and no guarded sibling"
|
||||
return Verdict(rung=Rung.CORROBORATED, family=spec.family, witness=witness, location=_location(target))
|
||||
|
||||
|
||||
def _where(row: Mapping[str, object]) -> str:
|
||||
"""The sites a reader has to open. Two of them when the obligation crossed a module boundary.
|
||||
|
||||
An application that puts its route in one file and its query in another raised no obligation at all until
|
||||
the query followed the call, and a witness that named only the operation would send a contributor to a
|
||||
data-access module that is not where the decision was made. Naming only the handler would hide what the
|
||||
claim is about. So a carried row names both, in the order a reader needs them.
|
||||
"""
|
||||
method = str(row["opMethod"])
|
||||
via_line = str(row.get("viaLine") or "")
|
||||
if not via_line:
|
||||
return f"{method} line {row['opLine']}"
|
||||
call = str(row.get("viaCall") or "").strip()
|
||||
return f"{method} line {via_line} calls {call}, whose operation at {row.get('opFile')}:{row['opLine']}"
|
||||
|
||||
|
||||
def _location(row: Mapping[str, object]) -> str:
|
||||
"""``path:line:function`` of the obligated operation, when the engine reported a file.
|
||||
|
||||
@@ -90,6 +126,12 @@ def _location(row: Mapping[str, object]) -> str:
|
||||
reached a contributor as `api_views/users.py:?:update_password` -- the right file by accident, because
|
||||
the region was asked about that file, and no line at all.
|
||||
"""
|
||||
# A carried obligation is located at the CALL, not at the operation: that is the line the contributor
|
||||
# changed and the line an admission decision has to be attributable to. The operation's own site travels
|
||||
# in the witness.
|
||||
carried_line = str(row.get("viaLine") or "")
|
||||
if carried_line.lstrip("-").isdigit() and int(carried_line) >= 0 and row.get("viaFile"):
|
||||
return f"{row['viaFile']}:{carried_line}:{row.get('opMethod') or '?'}"
|
||||
where = str(row.get("opFile") or "")
|
||||
line = str(row.get("opLine") or "")
|
||||
if not where or not line.lstrip("-").isdigit() or int(line) < 0:
|
||||
@@ -112,6 +154,9 @@ def _operations(rows: object) -> list[dict[str, object]]:
|
||||
"opLine": str(row.get("opLine", "")),
|
||||
"opMethod": str(row.get("opMethod", "")),
|
||||
"opFile": str(row.get("opFile", "")),
|
||||
"viaCall": str(row.get("viaCall", "")),
|
||||
"viaLine": str(row.get("viaLine", "")),
|
||||
"viaFile": str(row.get("viaFile", "")),
|
||||
"guards": [str(g) for g in guards] if isinstance(guards, Sequence) and not isinstance(guards, str) else [],
|
||||
}
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user