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