diff --git a/.kiro/specs/contributor-scan/tasks.md b/.kiro/specs/contributor-scan/tasks.md index 291d65d..d210327 100644 --- a/.kiro/specs/contributor-scan/tasks.md +++ b/.kiro/specs/contributor-scan/tasks.md @@ -162,7 +162,7 @@ - _Requirements: 8.1, 8.2_ - _Depends: 2.2_ -- [ ] 2.7 A dominance witness that names its line +- [x] 2.7 A dominance witness that names its line - Access-control sites read `api_views/users.py:?:update_password`. Taint and config witnesses carry `(line N)`; dominance does not, so its findings have no line and a contributor cannot open them. - Observable: every entailed finding in a repository scan has an integer line. @@ -249,6 +249,38 @@ - _Requirements: 8.1, 8.2_ - _Depends: 2.12, 5.3_ +- [ ] 2.14 Control dependence is not populated, and two arbiters want it + - Measured, not assumed: **0 of 13,184 calls in the libpng CPG and 0 of 1,152 in the vampi CPG have a + controlling control structure.** `controlledBy` returns nothing in either, so the PDG's control half is + simply absent from the graphs we build. The data half is present and load-bearing -- `reachableByFlows` + is the whole taint arbiter, and `joern-parse` reports `dataflowOss` applied. + - Everything guard-shaped we do therefore uses CFG dominance (`dominatedBy`) or syntactic conditions + (`method.controlStructure.condition`) instead. That is why 2.12's first attempt failed: libpng's bound + reaches its sink through an `error` flag, which is a control-dependence fact and nothing else. + - Find out whether a CDG overlay can be enabled on `joern-parse` or in the script, what it costs on the + envelope checkout, and whether it makes the flag-mediated guard decidable. If it cannot be enabled, + record that -- it bounds what guard reasoning this engine can ever do. + - Observable: a recorded answer with the build-time and query-time cost, and wpng.c:374 decided or a + stated reason it cannot be. + - _Requirements: 4.2, 8.1_ + - _Depends: 2.12_ + +- [ ] 2.15 A C checkout whose CVE goes through a sink we model + - libpng cannot demonstrate a C true positive, and pinning an older vulnerable commit would not change + that. **Every one of the twelve files using `strcpy`/`strcat`/`sprintf`/`gets` is under `contrib/`; the + library itself uses none of them.** Its overflows are array and pointer arithmetic and `memcpy` sizing, + and five buffer-overflow fixes checked (`png_set_quantize`, `png_image_finish_read`, + `png_init_read_transformations`, `png_do_quantize`, `png_write_image_8bit`) touch no modelled sink at + all. + - So there are two ways forward and they are different sizes. Pin a C project whose CVE genuinely runs + through a string function -- cheap, and it gives the memory family its first true positive. Or model how + C actually overflows: a `memcpy` whose size is computed, an index that is not bounded. That is size + arithmetic rather than sink matching, a different abstraction, and it should not be started by accident. + - Observable: a pinned checkout with an in-scope CVE found at a rung, or a recorded decision that the + memory family needs the second abstraction before any C detection claim is made. + - _Requirements: 4.1, 4.4_ + - _Depends: 2.12_ + ## Group 3 — Ship it - [ ] 3.1 Contributor output diff --git a/src/openultrasast/cpg/queries/dominance.sc b/src/openultrasast/cpg/queries/dominance.sc index e9e76ee..23da0e5 100644 --- a/src/openultrasast/cpg/queries/dominance.sc +++ b/src/openultrasast/cpg/queries/dominance.sc @@ -28,7 +28,7 @@ // 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}. +// Output: a fenced JSON array of {operation, opLine, opMethod, opFile, dominatingGuards}. // BATCHED, like taint.sc: `requests` is {id: {operations, dischargers, function}} and the output is // {id: [row, ...]}. One JVM start answers a whole scan. The single-request form is kept so the committed @@ -151,6 +151,9 @@ "operation" -> op.code.take(200), "opLine" -> op.lineNumber.getOrElse(-1).toString, "opMethod" -> method.name, + // Where the operation is. Without it an access-control finding reaches a contributor as + // `api_views/users.py:?:update_password` -- a location no editor can open. + "opFile" -> method.filename, "dominatingGuards" -> ujson.Arr(guards.map(ujson.Str(_)): _*) ) } diff --git a/src/openultrasast/model/dominance.py b/src/openultrasast/model/dominance.py index 650c33b..34b41cb 100644 --- a/src/openultrasast/model/dominance.py +++ b/src/openultrasast/model/dominance.py @@ -77,10 +77,24 @@ def verdict(cpg: CpgResult, spec: DominanceSpec, *, function: str = "", file: st f"while {len(guarded_methods)} sibling handler(s) are guarded " f"({', '.join(guarded_methods[:3])})" ) - return Verdict(rung=Rung.ENTAILED, family=spec.family, witness=witness) + return Verdict(rung=Rung.ENTAILED, family=spec.family, witness=witness, location=_location(target)) witness = f"{target['opMethod']} line {target['opLine']}: obligated operation with no discharging guard, and no guarded sibling" - return Verdict(rung=Rung.CORROBORATED, family=spec.family, witness=witness) + return Verdict(rung=Rung.CORROBORATED, family=spec.family, witness=witness, location=_location(target)) + + +def _location(row: Mapping[str, object]) -> str: + """``path:line:function`` of the obligated operation, when the engine reported a file. + + Taint and config verdicts have always carried this; dominance did not, so every access-control finding + reached a contributor as `api_views/users.py:?:update_password` -- the right file by accident, because + the region was asked about that file, and no line at all. + """ + where = str(row.get("opFile") or "") + line = str(row.get("opLine") or "") + if not where or not line.lstrip("-").isdigit() or int(line) < 0: + return "" + return f"{where}:{line}:{row.get('opMethod') or '?'}" def _operations(rows: object) -> list[dict[str, object]]: @@ -97,6 +111,7 @@ def _operations(rows: object) -> list[dict[str, object]]: "operation": str(row.get("operation", "")), "opLine": str(row.get("opLine", "")), "opMethod": str(row.get("opMethod", "")), + "opFile": str(row.get("opFile", "")), "guards": [str(g) for g in guards] if isinstance(guards, Sequence) and not isinstance(guards, str) else [], } ) diff --git a/tests/test_model_dominance.py b/tests/test_model_dominance.py index 239fe8c..c3fe13f 100644 --- a/tests/test_model_dominance.py +++ b/tests/test_model_dominance.py @@ -114,3 +114,53 @@ def test_the_spec_drives_the_query_parameters() -> None: assert seen["operations"] == ("filter_by", "query.get", "session.delete") assert seen["dischargers"] == ("login_required", "current_user", "check_permission") assert seen["function"] == "f" + + +def test_an_access_control_finding_names_a_line_a_contributor_can_open() -> None: + """contributor-scan 2.7. Taint and config verdicts always carried a location; dominance did not, so every + access-control finding arrived as `api_views/users.py:?:update_password` -- the right file by accident, + because the region was asked about that file, and no line at all.""" + from pathlib import Path + + from openultrasast.cpg.backend import CpgResult + from openultrasast.model.dominance import verdict + from openultrasast.model.specs import dominance_specs + + rows = [ + { + "operation": "User.query.filter_by(username = username)", + "opLine": "187", + "opMethod": "update_password", + "opFile": "api_views/users.py", + "dominatingGuards": [], + }, + { + "operation": "User.query.filter_by(username = resp['sub'])", + "opLine": "33", + "opMethod": "me", + "opFile": "api_views/users.py", + "dominatingGuards": ["resp['sub']"], + }, + ] + spec = dominance_specs(language="python")["access_control"] + + answer = verdict(CpgResult(cpg_path=Path("c.bin"), run=lambda q, p: rows), spec, function="update_password") + + assert answer is not None + assert answer.location == "api_views/users.py:187:update_password" + + +def test_a_verdict_without_a_reported_file_carries_no_location() -> None: + """An engine that could not report the file must not have a location invented for it.""" + from pathlib import Path + + from openultrasast.cpg.backend import CpgResult + from openultrasast.model.dominance import verdict + from openultrasast.model.specs import dominance_specs + + rows = [{"operation": "q.filter_by(x)", "opLine": "-1", "opMethod": "handler", "opFile": "", "dominatingGuards": []}] + spec = dominance_specs(language="python")["access_control"] + + answer = verdict(CpgResult(cpg_path=Path("c.bin"), run=lambda q, p: rows), spec, function="handler") + + assert answer is not None and answer.location == ""