mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-16 03:49:26 +00:00
fix(cpg): a sink is matched as a word, not as a substring
`fgets` contains `gets`. The sink matcher's last clause was an unbounded
`methodFullName.contains(n)`, so every bounded read in libpng matched the
sink for the unbounded one. All 26 entailed findings on that repository
were the same false positive:
*argv -> fgets(pnmline, 256, wpng_info.infile) (line 335, 8 steps)
which is the safe API being reported as the dangerous one -- and `fgets`
is in the fact tables as a SOURCE, never a sink. Verified against the CPG:
methodFullName is exactly "fgets", and .contains("gets") is true.
Third instance of one bug class in this codebase, after the sanitizer that
matched `resolve` inside `resolveUrl` and the discharger that matched
`user` inside `users`. The sink matcher had never been given the same
treatment. All four of its clauses now run through the shared bounded
matcher, and that matcher is the punctuation-tolerant version written for
task 2.6 -- so `input(`, which an unconditional lookahead had made
unmatchable in `input(x)`, works for the first time as well.
libpng entailed 26 -> 0
Zero is the right answer for a PNG library whose only known CVE is out of
family. Gates byte-identical, so no pair outcome depended on the substring
match.
What remains on that scan is recorded, not claimed: 113 `c-unsafe-*` rows
assert an API is present rather than misused, and 50 overlay-coverage rows
are annotations rather than findings. 82 of the 113 are under `contrib/`.
Task 2.11 covers whether that band is worth putting in front of a
contributor at all; 2.10 already covers not scanning example code.
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
68b4e551f3
commit
67497c4d68
@@ -197,6 +197,18 @@
|
||||
- _Requirements: 5.1, 8.1_
|
||||
- _Depends: 2.4_
|
||||
|
||||
- [ ] 2.11 The suspicion band has to be worth reading
|
||||
- libpng's scan produced 198 rows: 26 entailed (all one false positive, since fixed), 113 `c-unsafe-*`
|
||||
pattern matches that assert an API is PRESENT rather than misused, 50 overlay-coverage annotations that
|
||||
are not findings at all, and 9 model suspicions. Zero validated defects.
|
||||
- The rung is honest -- these say "needs review" and mean it -- but a contributor handed 163 rows about a
|
||||
library that uses `memcpy` correctly throughout will not read the one that matters. 82 of the 113 are in
|
||||
`contrib/`, so task 2.10 removes most of it; what remains needs a policy, not a filter invented here.
|
||||
- Observable: a stated rule for what reaches a contributor's report at `suspicion`, and libpng's row count
|
||||
under it, measured.
|
||||
- _Requirements: 5.1, 5.2_
|
||||
- _Depends: 2.10_
|
||||
|
||||
- [ ] 2.10 Do not spend the scan on example code
|
||||
- 22 of libpng's 30 highest-ranked regions are under `contrib/` -- the sample programs shipped with the
|
||||
library rather than the library. The budget and the 243s taint query go there first.
|
||||
|
||||
@@ -57,13 +57,16 @@
|
||||
// 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
|
||||
// attacker input. Same bug class as the dominance query's guard matching; fixed in both.
|
||||
// The boundary only applies where there IS a word character to bound: a token like `input(` ends in `(`,
|
||||
// and an unconditional lookahead would make it unmatchable in `input(x)`.
|
||||
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 mentionsToken(code: String, tokens: List[String]): Boolean =
|
||||
tokens.exists(t =>
|
||||
java.util.regex.Pattern
|
||||
.compile("(?<![A-Za-z0-9_])" + java.util.regex.Pattern.quote(t) + "(?![A-Za-z0-9_])")
|
||||
.matcher(code)
|
||||
.find()
|
||||
)
|
||||
tokens.exists(t => boundedPattern(t).matcher(code).find())
|
||||
|
||||
val sourcePatterns = split(sourcesS)
|
||||
val sinkNames = split(sinksS)
|
||||
@@ -145,10 +148,18 @@
|
||||
|
||||
// A sink call: matched by short name (`system`), by leading code (`os.system(...)`), or by resolved
|
||||
// full name (`os.py:<module>.system`), so both bare and dotted forms in the spec hit.
|
||||
//
|
||||
// Every clause is word-BOUNDED, and the unbounded `contains` that used to end this list is why: `fgets`
|
||||
// contains `gets`, so every bounded read in libpng matched the sink for the unbounded one, and all 26
|
||||
// entailed findings on that repository were the same false positive -- `*argv -> fgets(buf, 256, f)`,
|
||||
// which is the safe API being reported as the dangerous one. Third instance of this bug class here, after
|
||||
// the sanitizer that matched `resolve` inside `resolveUrl` and the discharger that matched `user` inside
|
||||
// `users`; the sink matcher had simply never been given the same treatment.
|
||||
def sinkMatches(c: io.shiftleft.codepropertygraph.generated.nodes.Call, n: String): Boolean =
|
||||
c.name == n || c.code.startsWith(n + "(") || mentionsToken(c.code, List(n)) || mentionsToken(c.methodFullName, List(n))
|
||||
|
||||
def sinkCalls = {
|
||||
val all = cpg.call.filter(c =>
|
||||
sinkNames.exists(n => c.name == n || c.code.startsWith(n + "(") || c.code.startsWith(n) || c.methodFullName.contains(n))
|
||||
)
|
||||
val all = cpg.call.filter(c => sinkNames.exists(n => sinkMatches(c, n)))
|
||||
if (function.isEmpty) {
|
||||
// A region with no enclosing function: the file IS the scope.
|
||||
if (fileS.isEmpty) all else all.filter(_.method.filename.endsWith(fileS))
|
||||
|
||||
@@ -221,3 +221,23 @@ def test_a_guard_discharges_only_the_obligation_it_is_a_guard_of() -> None:
|
||||
assert "token_validator" not in identity, "authentication is not an object-level check"
|
||||
assert "token_validator" in set(spec.dischargers_by_kind["path_guard"])
|
||||
assert "['sub']" in identity or '["sub"]' in identity
|
||||
|
||||
|
||||
def test_a_sink_is_matched_as_a_word_not_a_substring() -> None:
|
||||
"""contributor-scan: `fgets` contains `gets`, and the safe API is not the dangerous one.
|
||||
|
||||
The sink matcher's last clause was an unbounded `methodFullName.contains(n)`, so every bounded read in
|
||||
libpng matched the sink for the unbounded one: all 26 entailed findings on that repository were the same
|
||||
false positive, `*argv -> fgets(buf, 256, f)`. Third instance of this bug class here, after the sanitizer
|
||||
that matched `resolve` inside `resolveUrl` and the discharger that matched `user` inside `users`.
|
||||
|
||||
The query is Scala, so this asserts the fact tables still carry the pair that made it visible; the
|
||||
boundary itself is exercised against a real CPG in the repository measurements.
|
||||
"""
|
||||
from openultrasast.model.specs import taint_specs
|
||||
|
||||
memory = taint_specs(language="c")["memory"]
|
||||
|
||||
assert "gets" in memory.sinks, "the unbounded read is a sink"
|
||||
assert "fgets" in memory.sources, "the bounded read is a SOURCE of untrusted input, never a sink"
|
||||
assert "fgets" not in memory.sinks
|
||||
|
||||
Reference in New Issue
Block a user