marius/Open_Ultra_SAST
1
0
Fork 0
mirror of https://github.com/norandom/OpenUltraSAST.git synced 2026-10-19 13:10:03 +00:00
Code Issues Packages Projects Releases Wiki Activity

feat(model): a bound on the data lowers the rung, it does not clear it

Browse Source 
contributor-scan 2.12. c/injection and c/memory carried zero sanitizers,
so no C flow could ever be discharged: every `argv -> strcpy` entailed
however carefully it was bounded.

Which sinks can be discharged this way is read from the sink fact's own
CWE rather than declared: CWE-121 is a stack buffer overflow, so
gets/sprintf/strcat/strcpy qualify. It is the only overflow CWE in the
fact tables and it is C-only, so no Python or JavaScript sink is touched
and the pair corpus is unaffected -- gates byte-identical.

Not a sanitizer list, because `strlen` appears in the guarded and the
unguarded case alike; the discharge is a guard, not a call.

**The first implementation required the bound to DOMINATE the sink, and it
found nothing.** libpng guards `strcpy(outname+len, ".png")` with
`(len = strlen(inname)) > 250`, but that check sits on an else-if chain
whose sibling branch skips it entirely, and the protection reaches the
sink through an `error` flag: `++error` in the guarded branch, `if
(!error)` around the copy. Joern's dominator set for that sink has 89
calls and correctly excludes the bound; `controlledBy` returns none
either. Verified against the CPG before the rule was changed rather than
after it failed.

So the bound is searched in the method, and the consequence is
corroborated rather than silence. That is the honest rung: a bound on this
variable means the graph has NOT shown the copy is unguarded, and it has
not shown it is guarded either -- off-by-one being how bounds fail. The
judge is asked. Proving the flag-mediated case needs path-sensitive
reasoning this arbiter does not do.

  libpng entailed   3 -> 1

The survivor is also a false positive: `strcpy(output + len, ".icc")` in
iccfrompng.c, where `output = malloc(len + 5)` makes it fit exactly. That
is a different discharge shape -- allocated to size, not bounds-checked --
and it is recorded as 2.13 rather than chased. Both shapes were found on
one repository, and extending a rule until that repository is clean is the
overfitting Req 8 exists to prevent; 2.13 requires a second C checkout
with a genuine CWE-121 so the change can be measured for what it silences
as well as what it clears.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01T7XD7x5KonMdmGRkp63EQG
This commit is contained in:
Marius Ciepluch
2026-09-09 13:01:30 +02:00
co-authored by Claude Opus 5
parent 6dc25d77e4
commit 9221ba8e40
6 changed files with 181 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/openultrasast/cpg/queries/taint.sc
+59 -4
View File
@@ -19,7 +19,8 @@
// closure lexically nested inside it -- see `nestedInLabeled`),
// parameterSources ("true" to treat the labeled function's parameters as untrusted -- see below),
// file (optional: restrict sinks to this source file),
// callDepth (optional: how many call-graph levels below the labeled method may hold a sink)
// callDepth (optional: how many call-graph levels below the labeled method may hold a sink),
// boundedSinks (optional: sinks whose danger is a LENGTH, which a dominating bound discharges)
//
// `file` is what makes a region with no enclosing function askable at all. Without it such a region sends
// function="" and matches every sink in the repository, and the driver attributes that whole answer to
@@ -45,6 +46,7 @@
function: String = "",
file: String = "",
callDepth: String = "0",
boundedSinks: String = "",
parameterSources: String = "false",
requests: String = ""
) = {
@@ -52,7 +54,16 @@
def split(raw: String): List[String] = raw.split(",").map(_.trim).filter(_.nonEmpty).toList
def rowsFor(sourcesS: String, sinksS: String, sanitizersS: String, functionS: String, paramSrc: String, fileS: String, depthS: String): List[ujson.Obj] = {
def rowsFor(
sourcesS: String,
sinksS: String,
sanitizersS: String,
functionS: String,
paramSrc: String,
fileS: String,
depthS: String,
boundedS: String
): List[ujson.Obj] = {
// Word-boundary matching, never substring. `resolveUrl` contains `resolve`, so a bare-substring sanitizer
// test marked the VULNERABLE flow sanitized and the verdict fell back to a captured `res` that names no
@@ -71,6 +82,12 @@
val sourcePatterns = split(sourcesS)
val sinkNames = split(sinksS)
val sanitizerNames = split(sanitizersS)
val boundedNames = split(boundedS)
// A relational comparison against an integer literal: `len > 250`, `n <= sizeof(buf)`. Equality and null
// checks are deliberately excluded -- `if (buf)` and `p == NULL` govern whether the sink RUNS, not how
// much it copies, and counting them would discharge every guarded-against-null overflow there is.
val boundShape = java.util.regex.Pattern.compile("(?:<=|>=|<|>)\\s*[0-9]+|[0-9]+\\s*(?:<=|>=|<|>)")
val function = functionS
val parameterSources = paramSrc
@@ -188,6 +205,29 @@
val arity = realArgs.size
val arg0Literal = realArgs.headOption.map(a => a.isLiteral).getOrElse(false)
// Is there a bound on the data reaching this sink? Only asked of sinks whose CWE says the danger is a
// length, and the bound must mention a name the sink actually uses -- an unrelated `i < 10` loop in the
// same function discharges nothing.
//
// Scoped to the METHOD rather than to the sink's dominators, and that is deliberate. libpng guards
// `strcpy(outname+len, ".png")` with `(len = strlen(inname)) > 250`, but the check sits on an else-if
// chain whose sibling branch skips it entirely, and the protection is carried to the sink through an
// `error` flag: `++error` in the guarded branch, `if (!error)` around the copy. So the bound does NOT
// dominate the sink -- Joern is right about that -- and requiring dominance found nothing.
//
// Which is exactly why this lowers the rung instead of clearing the finding. A bound on this variable
// somewhere in this function means the graph CANNOT ENTAIL: it has not shown the copy is unguarded. It
// has not shown it is guarded either, so the judge is asked. Proving the flag-mediated case needs
// path-sensitive reasoning this arbiter does not do.
val boundedSink = boundedNames.exists(n => sink.name == n)
val argNames = sink.argument.ast.isIdentifier.name.l.distinct
val bounds =
if (!boundedSink || argNames.isEmpty) List.empty
else
sink.method.ast.isCall.code.l.filter(c =>
boundShape.matcher(c).find() && argNames.exists(a => mentionsToken(c, List(a)))
)
flows.map { flow =>
val elements = flow.elements.map(_.code).l
val sanitized = sanitizerNames.nonEmpty && elements.exists(code => mentionsToken(code, sanitizerNames))
@@ -203,6 +243,8 @@
"length" -> elements.size,
"sinkArity" -> arity,
"sinkArg0Literal" -> arg0Literal,
"bounded" -> bounds.nonEmpty,
"bound" -> bounds.headOption.getOrElse("").take(120),
"inLabeledScope" -> (function.isEmpty || nestedInLabeled(sink.method) || reachableMethods.contains(sink.method.fullName))
)
}
@@ -217,11 +259,24 @@
val answers = parsed.map { case (id, req) =>
def field(name: String): String = req.obj.get(name).map(_.str).getOrElse("")
val paramSrc = req.obj.get("parameterSources").map(_.str).getOrElse("false")
id -> ujson.Arr(rowsFor(field("sources"), field("sinks"), field("sanitizers"), field("function"), paramSrc, field("file"), field("callDepth")): _*)
id -> ujson.Arr(
rowsFor(
field("sources"),
field("sinks"),
field("sanitizers"),
field("function"),
paramSrc,
field("file"),
field("callDepth"),
field("boundedSinks")
): _*
)
}
println(ujson.write(ujson.Obj.from(answers)))
} else {
println(ujson.write(ujson.Arr(rowsFor(sources, sinks, sanitizers, function, parameterSources, file, callDepth): _*)))
println(
ujson.write(ujson.Arr(rowsFor(sources, sinks, sanitizers, function, parameterSources, file, callDepth, boundedSinks): _*))
)
}
println("---OUSAST-CPG-END---")
}