mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-18 20:49:28 +00:00
fix(model): a guard discharges the obligation it is a guard of
contributor-scan 2.6. Access control entailed 7 of 9 VAmPI handlers,
including three that do authorize and three the OpenAPI spec declares
public. Five separate defects, each invisible to a corpus of one-function
excerpts and each found by asking the query what it actually matched.
**A trailing segment is not a match.** `query.get` was matched by its last
segment, so every `.get(` in the repository became a protected database
operation -- `request.headers.get('Authorization')` and
`request_data.get('username')` included. Four of the seven false positives
came from this, and so did the apparent detection of the known BOLA: it was
entailed because fetching the Authorization header had no dominating guard,
not because of the defect. A qualified token now requires its qualifier.
**Configuration is not authorization.** dominance_specs took every
obligation fact. `security_setting` operations are discharged by a VALUE,
which config_specs already claims them for; pooled in here they made
`app.run(...)` an operation requiring an authorization guard and put
thirteen tokens in BOTH lists -- an obligation that is its own discharge,
the same shape as the taint sink that used to sanitize itself.
**Input validation is not authorization.** `jsonschema.validate` was
counted as a guard, so handlers with no authorization check at all read as
guarded. No operation in the table requires validated_input.
**A guard discharges the obligation it is a guard of.** The largest one.
`orm-read` requires an identity_constraint or an ownership_check;
`token_validator` is a path_guard. Flattened into one list, authenticating
the caller discharged an object-level obligation -- a handler that
authenticates and then looks the record up by a PATH parameter read as
fully guarded. The facts state that relation and the spec was discarding
it; it is now carried and the engine asks per operation.
**A word boundary needs a word.** Found while fixing the above. Guard
matching wrapped every token in `(?<![A-Za-z0-9_])...(?![A-Za-z0-9_])`.
The identity token `['sub']` begins with `[`, and the character before it
in `resp['sub']` is `p` -- so the assertion could never hold. Every
identity token starting with punctuation was unmatchable, and the identity
constraint that separates a correct lookup from an IDOR was invisible.
Also: dominance rows were repository-wide, so the model's unguarded
`delete_user` was attributed to the API handler of the same name. Rows are
scoped to the region's file, which is what the rung's own justification
says -- the evidence is a file contradicting itself.
Measured against VAmPI's OpenAPI `security` blocks, which declare per
operation whether auth is required, and agree with the code:
vampi precision 28.6% (2/7) -> 50.0% (2/4)
known found 3 of 3, all model_entailed
And on the labelled access-control pairs, which nobody tuned this against:
all 9 pairs pair_correct 3 -> 3, leak 44.4% -> 11.1%
7 non-VAmPI pair_correct 2 -> 2, leak 42.9% -> 0.0%
Every detection lost was on a pair whose FIXED side was flagged too, which
scores nothing. No correct detection was traded for the precision.
get_by_title was flagged before it was in the recipe: its code carries the
comment "# Broken Object Level Authorization" and the README lists it, so a
real finding was being scored as a false positive. It is now the recipe's
third known vulnerability.
Both remaining false positives are endpoints the spec declares public.
Recorded as 2.8, with the reason the mapper's access_level cannot simply be
trusted: for decorator frameworks "public" means "no decorator found",
which is the bug itself.
Gates byte-identical.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01T7XD7x5KonMdmGRkp63EQG
This commit is contained in:
co-authored by
Claude Opus 5
parent
0fcd5feade
commit
d15985f695
@@ -18,7 +18,15 @@
|
||||
// so it never dominates the operation, but it does control whether the value escapes.
|
||||
//
|
||||
// Parameters: cpgFile, operations, dischargers, function (optional; reported, never used to filter, because
|
||||
// the sibling comparison needs every operation in the CPG).
|
||||
// the sibling comparison needs the file's other operations), file (optional: the source file the region
|
||||
// belongs to), operationRequires and dischargersByKind (optional JSON, see below).
|
||||
//
|
||||
// A discharger only discharges the obligation it is a discharger OF. The facts say `orm-read` requires an
|
||||
// identity_constraint or an ownership_check, and that `token_validator` is a path_guard; pooling them made
|
||||
// authenticating the caller discharge an object-level obligation, so a handler that authenticates and then
|
||||
// looks a record up by a PATH parameter read as fully guarded. `operationRequires` maps an operation call to
|
||||
// the kinds that would discharge it and `dischargersByKind` maps a kind to its tokens; when both are absent
|
||||
// the flat list is used, which is the committed single-request behaviour.
|
||||
//
|
||||
// Output: a fenced JSON array of {operation, opLine, opMethod, dominatingGuards}.
|
||||
|
||||
@@ -31,13 +39,27 @@
|
||||
operations: String = "",
|
||||
dischargers: String = "",
|
||||
function: String = "",
|
||||
file: String = "",
|
||||
operationRequires: String = "",
|
||||
dischargersByKind: String = "",
|
||||
requests: String = ""
|
||||
) = {
|
||||
importCpg(cpgFile)
|
||||
|
||||
def split(raw: String): List[String] = raw.split(",").map(_.trim).filter(_.nonEmpty).toList
|
||||
|
||||
def rowsFor(operationsS: String, dischargersS: String, functionS: String): List[ujson.Obj] = {
|
||||
def parseMap(raw: String): Map[String, List[String]] =
|
||||
if (raw.isEmpty) Map.empty
|
||||
else ujson.read(raw).obj.map { case (k, v) => k -> v.arr.map(_.str).toList }.toMap
|
||||
|
||||
def rowsFor(
|
||||
operationsS: String,
|
||||
dischargersS: String,
|
||||
functionS: String,
|
||||
fileS: String,
|
||||
requiresS: String,
|
||||
byKindS: String
|
||||
): List[ujson.Obj] = {
|
||||
|
||||
val opNames = split(operationsS)
|
||||
val guardTokens = split(dischargersS)
|
||||
@@ -46,31 +68,82 @@
|
||||
// Word-boundary matching, never substring. A discharger token like `user` matched as a substring hits
|
||||
// `users` inside "SELECT * FROM users WHERE id = ?", marking a textbook IDOR as guarded -- a silent false
|
||||
// negative on the one family this arbiter exists for.
|
||||
def mentionsGuard(code: String): Boolean =
|
||||
guardTokens.exists(g => java.util.regex.Pattern.compile("(?<![A-Za-z0-9_])" + java.util.regex.Pattern.quote(g) + "(?![A-Za-z0-9_])").matcher(code).find())
|
||||
val requiresByOp = parseMap(requiresS)
|
||||
val tokensByKind = parseMap(byKindS)
|
||||
|
||||
// Word-boundary matching, never substring: a token like `user` matched as a substring hits `users` inside
|
||||
// "SELECT * FROM users WHERE id = ?", marking a textbook IDOR as guarded.
|
||||
//
|
||||
// But the boundary only applies where there IS a word character to bound. `['sub']` begins with `[`, and
|
||||
// the character before it in `resp['sub']` is `p` -- so an unconditional lookbehind made every identity
|
||||
// token that starts with punctuation unmatchable, and the identity constraint that distinguishes a
|
||||
// correct lookup from an IDOR could never be seen.
|
||||
def boundedPattern(token: String): java.util.regex.Pattern = {
|
||||
val before = if (token.headOption.exists(c => c.isLetterOrDigit || c == '_')) "(?<![A-Za-z0-9_])" else ""
|
||||
val after = if (token.lastOption.exists(c => c.isLetterOrDigit || c == '_')) "(?![A-Za-z0-9_])" else ""
|
||||
java.util.regex.Pattern.compile(before + java.util.regex.Pattern.quote(token) + after)
|
||||
}
|
||||
|
||||
def mentions(code: String, tokens: List[String]): Boolean =
|
||||
tokens.exists(g => boundedPattern(g).matcher(code).find())
|
||||
|
||||
def mentionsGuard(code: String): Boolean = mentions(code, guardTokens)
|
||||
|
||||
// The tokens that can discharge THIS operation, rather than every token that can discharge anything.
|
||||
def guardsFor(name: String, code: String): List[String] =
|
||||
if (requiresByOp.isEmpty || tokensByKind.isEmpty) guardTokens
|
||||
else {
|
||||
val kinds = opNames
|
||||
.filter(n => name == n || (n.split("\\.").length > 1 && name == n.split("\\.").last && n.split("\\.").init.forall(code.contains)))
|
||||
.flatMap(n => requiresByOp.getOrElse(n, Nil))
|
||||
.distinct
|
||||
if (kinds.isEmpty) guardTokens else kinds.flatMap(k => tokensByKind.getOrElse(k, Nil)).distinct
|
||||
}
|
||||
|
||||
// Match by CALL NAME, never by code containment: the frontend desugars `Note.query.filter_by(x).first()`
|
||||
// into a chain of temporaries, and matching on code yields four or five rows for one operation -- some of
|
||||
// them the bare `tmp1.filter_by` receiver, which carries none of the guards the real call does. A `leaky`
|
||||
// verdict computed over those extra rows would flag a guarded operation. A dotted spec (`query.get`)
|
||||
// matches on its trailing segment, which is how the call is named in the graph.
|
||||
def matchesOperation(name: String): Boolean =
|
||||
opNames.exists(n => name == n || name == n.split("\\.").last)
|
||||
//
|
||||
// But the trailing segment ALONE is not enough, and on a repository it is catastrophic: `query.get` ends
|
||||
// in `get`, so it matched `request.headers.get('Authorization')` and `request_data.get('username')` --
|
||||
// ordinary dictionary access read as a protected database operation. Four of seven false positives on the
|
||||
// first repository measured came from exactly that, including three handlers that do call an
|
||||
// authorization guard. A qualified token means the qualification matters, so the receiver must appear
|
||||
// too; the desugared chain still carries it.
|
||||
def matchesOperation(name: String, code: String): Boolean =
|
||||
opNames.exists { n =>
|
||||
if (name == n) true
|
||||
else {
|
||||
val parts = n.split("\\.")
|
||||
parts.length > 1 && name == parts.last && parts.init.forall(q => code.contains(q))
|
||||
}
|
||||
}
|
||||
|
||||
val opCalls = cpg.call.filter(c => matchesOperation(c.name)).l
|
||||
// Scoped to the region's own file. `delete_user` exists in both the API handler and the model of 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
|
||||
// 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 rows = opCalls.map { op =>
|
||||
val method = op.method
|
||||
|
||||
val applicable = guardsFor(op.name, op.code)
|
||||
|
||||
// 1. identity constraint: a discharger inside the operation's own arguments.
|
||||
val inArguments = op.argument.code.l.filter(mentionsGuard)
|
||||
val inArguments = op.argument.code.l.filter(c => mentions(c, applicable))
|
||||
|
||||
// 2. identity branch: a control structure in this method whose condition names a discharger. The denial
|
||||
// it guards need not dominate the operation -- it controls whether the fetched value escapes.
|
||||
val inCondition = method.controlStructure.condition.code.l.filter(mentionsGuard)
|
||||
val inCondition = method.controlStructure.condition.code.l.filter(c => mentions(c, applicable))
|
||||
|
||||
// 3. dominating guard: a call naming a discharger that actually dominates the operation.
|
||||
val dominating = op.dominatedBy.isCall.code.l.filter(mentionsGuard)
|
||||
val dominating = op.dominatedBy.isCall.code.l.filter(c => mentions(c, applicable))
|
||||
|
||||
val guards = (inArguments ++ inCondition ++ dominating).distinct.map(_.take(120))
|
||||
|
||||
@@ -90,11 +163,20 @@
|
||||
val parsed = ujson.read(requests).obj
|
||||
val answers = parsed.map { case (id, req) =>
|
||||
def field(name: String): String = req.obj.get(name).map(_.str).getOrElse("")
|
||||
id -> ujson.Arr(rowsFor(field("operations"), field("dischargers"), field("function")): _*)
|
||||
id -> ujson.Arr(
|
||||
rowsFor(
|
||||
field("operations"),
|
||||
field("dischargers"),
|
||||
field("function"),
|
||||
field("file"),
|
||||
field("operationRequires"),
|
||||
field("dischargersByKind")
|
||||
): _*
|
||||
)
|
||||
}
|
||||
println(ujson.write(ujson.Obj.from(answers)))
|
||||
} else {
|
||||
println(ujson.write(ujson.Arr(rowsFor(operations, dischargers, function): _*)))
|
||||
println(ujson.write(ujson.Arr(rowsFor(operations, dischargers, function, file, operationRequires, dischargersByKind): _*)))
|
||||
}
|
||||
println("---OUSAST-CPG-END---")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user