From 67497c4d68972d054e0c4f4b60cded3706242911 Mon Sep 17 00:00:00 2001 From: Marius Ciepluch <11855163+norandom@users.noreply.github.com> Date: Wed, 9 Sep 2026 11:43:00 +0200 Subject: [PATCH] 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) Claude-Session: https://claude.ai/code/session_01T7XD7x5KonMdmGRkp63EQG --- .kiro/specs/contributor-scan/tasks.md | 12 +++++++++++ src/openultrasast/cpg/queries/taint.sc | 29 ++++++++++++++++++-------- tests/test_model_specs.py | 20 ++++++++++++++++++ 3 files changed, 52 insertions(+), 9 deletions(-) diff --git a/.kiro/specs/contributor-scan/tasks.md b/.kiro/specs/contributor-scan/tasks.md index a470883..cf62689 100644 --- a/.kiro/specs/contributor-scan/tasks.md +++ b/.kiro/specs/contributor-scan/tasks.md @@ -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. diff --git a/src/openultrasast/cpg/queries/taint.sc b/src/openultrasast/cpg/queries/taint.sc index 800e1c3..0ab4af6 100644 --- a/src/openultrasast/cpg/queries/taint.sc +++ b/src/openultrasast/cpg/queries/taint.sc @@ -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 == '_')) "(? 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("(? 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:.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)) diff --git a/tests/test_model_specs.py b/tests/test_model_specs.py index 0eb6330..a9676df 100644 --- a/tests/test_model_specs.py +++ b/tests/test_model_specs.py @@ -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