mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-16 03:49:26 +00:00
feat(model): an access-control finding names a line, and what the PDG does not give us
contributor-scan 2.7. Taint and config verdicts have always carried a `path:line:function` location; dominance never did, 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. The query now reports the operation's file and the verdict carries the location, with an engine that cannot report one getting no location rather than an invented one. Two questions answered by measurement while doing it. **Does this factor in the PDG?** The data half, centrally: `reachableByFlows` is the entire taint arbiter and `joern-parse` applies the `dataflowOss` overlay. The control half, not at all -- and it is not available to us. Measured: 0 of 13,184 calls in the libpng CPG and 0 of 1,152 in the vampi CPG have a controlling control structure, so `controlledBy` returns nothing in either. Everything guard-shaped therefore runs on CFG dominance or on syntactic conditions, which is exactly why 2.12's first attempt failed: libpng's bound reaches its sink through an `error` flag, and that is a control-dependence fact and nothing else. Recorded as 2.14, including the possibility that it cannot be enabled -- which would bound what guard reasoning this engine can ever do. **Would checking out a vulnerable libpng find a known CVE?** No, and not because of the version. Every one of the twelve files using `strcpy`/`strcat`/`sprintf`/`gets` is under `contrib/`; the library itself uses none of them. libpng overflows through array and pointer arithmetic and `memcpy` sizing, and five of its buffer-overflow fixes touch no modelled sink at all. Pinning an older commit would produce the same silence for the same reason. That leaves two routes of very different size, recorded as 2.15: pin a C project whose CVE genuinely runs through a string function, which is cheap and would give the memory family its first true positive; or model how C actually overflows, which is size arithmetic rather than sink matching and should not be started by accident. 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
9221ba8e40
commit
c1191b3a6b
@@ -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
|
||||
|
||||
@@ -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(_)): _*)
|
||||
)
|
||||
}
|
||||
|
||||
@@ -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 [],
|
||||
}
|
||||
)
|
||||
|
||||
@@ -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 == ""
|
||||
|
||||
Reference in New Issue
Block a user