mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-18 12:39:28 +00:00
fix(model-grounded-detection): closure scoping, and the three bugs it uncovered
The group-4 finding: JavaScript is callback-heavy, so the method containing a
sink routinely is not the labeled function. `fs.readFileSync` inside the
callback passed to `fs.stat` has enclosing method `<lambda>0`, and filtering on
`sinkMethod == function` dropped it -- the whole path family measured 0/8.
Scope is now decided by line-range containment within the same file
(astParentFullName is empty for lambdas, so nesting is not readable there), and
reported as `inLabeledScope`; the Python side trusts that flag and keeps the
name comparison only as a fallback, so an engine that cannot report scope never
has its sinks silently treated as scoped. Parameter sources stay bound to the
labeled function's OWN parameters -- a callback's `err`/`stats` are not
attacker input.
Fixing the scope exposed three further bugs, each of which would have raised
the number while making the answer untrue:
* Shortest-flow-wins picked the wrong witness. With closures in scope the
same sink has flows from `req.url...` (10 steps, the vulnerability), `res`
(3) and `this` (7). A flow whose source matches the source model now
outranks a shorter one from a bare captured name; shortest still breaks
ties, so the order stays total and the verdict deterministic.
* The JS fact file had NO sanitizers at all, so no JavaScript fix of any
family could ever be recognised. Added the documented path-normalisation
and HTML-escaping APIs, with provenance.
* Adding them regressed the witness, because `resolveUrl` CONTAINS `resolve`
and the substring test marked the vulnerable flow sanitized. This is the
same bug class already fixed in dominance.sc and not carried across -- the
exact fix-the-instance-miss-the-class failure. taint.sc now matches on word
boundaries too.
And the principled rule the fix turns on: parameter sources are a FALLBACK,
never an override. When a modelled source reaches the sink, those flows are the
evidence; otherwise a captured `res` reaching a sanitized sink reports the fix
as still vulnerable on a flow that names no attacker input.
Also recorded: req.url was NOT wrong as a diagnosis, it was untestable in
isolation -- the scoping bug masked it. Both were needed. On the pair that
motivated all of this:
vuln model_entailed req.url.split('?')[0] -> readFileSync(possibleFilename)
fixed model_corroborated same flow, sanitized by path.normalize
distinguished: true
700 passing, mypy/ruff clean, gates byte-identical.
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
91c0931a05
commit
d651455d23
@@ -45,9 +45,14 @@ def verdict(cpg: CpgResult, spec: TaintSpec, *, function: str = "", parameter_so
|
||||
flows = [flow for flow in flows if not _has_safe_shape(flow, spec)]
|
||||
if not flows:
|
||||
return None
|
||||
unsanitized = [flow for flow in flows if not flow["sanitized"]]
|
||||
# Deterministic by construction: sort, then take the first. Nothing here depends on engine ordering.
|
||||
chosen = min(unsanitized or flows, key=_rank)
|
||||
# Parameter sources are a FALLBACK, never an override. When a modelled source reaches the sink, those
|
||||
# flows are the evidence: a captured `res` parameter also reaching it unsanitized must not report a fix as
|
||||
# still vulnerable on the strength of a flow that names no attacker input.
|
||||
modelled = [flow for flow in flows if _from_modelled_source(flow, spec)]
|
||||
considered = modelled or flows
|
||||
unsanitized = [flow for flow in considered if not flow["sanitized"]]
|
||||
# Deterministic by construction: a total order, then take the first. Nothing depends on engine ordering.
|
||||
chosen = min(unsanitized or considered, key=lambda flow: _rank(flow, spec))
|
||||
rung = Rung.ENTAILED if not chosen["sanitized"] else Rung.CORROBORATED
|
||||
return Verdict(rung=rung, family=spec.family, witness=_witness(chosen))
|
||||
|
||||
@@ -60,12 +65,13 @@ def _flows(rows: object, *, function: str) -> list[dict[str, object]]:
|
||||
for row in rows:
|
||||
if not isinstance(row, Mapping):
|
||||
continue
|
||||
if function and str(row.get("sinkMethod", "")) != function:
|
||||
if function and not _in_scope(row, function):
|
||||
continue
|
||||
kept.append({
|
||||
"sink": str(row.get("sink", "")),
|
||||
"sinkArity": row.get("sinkArity"),
|
||||
"sinkArg0Literal": row.get("sinkArg0Literal"),
|
||||
"inLabeledScope": row.get("inLabeledScope"),
|
||||
"sinkLine": str(row.get("sinkLine", "")),
|
||||
"sinkMethod": str(row.get("sinkMethod", "")),
|
||||
"source": str(row.get("source", "")),
|
||||
@@ -100,9 +106,37 @@ def _as_int(value: object) -> int:
|
||||
return int(value) if isinstance(value, int | float) else 0
|
||||
|
||||
|
||||
def _rank(flow: Mapping[str, object]) -> tuple[int, str, str]:
|
||||
"""Shortest flow first, then lexicographic — a total order, so two runs cannot disagree."""
|
||||
return (_as_int(flow["length"]), str(flow["source"]), str(flow["sink"]))
|
||||
def _in_scope(row: Mapping[str, object], function: str) -> bool:
|
||||
"""Is this sink inside the labeled function, counting closures nested within it?
|
||||
|
||||
JavaScript is callback-heavy, so the method containing a sink routinely is not the labeled function --
|
||||
`fs.readFileSync` inside the callback passed to `fs.stat` has enclosing method `<lambda>0`. The query
|
||||
decides containment by line range and reports it here; a row that carries no flag falls back to the name
|
||||
comparison, because an engine that cannot report scope must not have its sinks silently treated as scoped.
|
||||
"""
|
||||
flag = row.get("inLabeledScope")
|
||||
if isinstance(flag, bool):
|
||||
return flag
|
||||
return str(row.get("sinkMethod", "")) == function
|
||||
|
||||
|
||||
def _from_modelled_source(flow: Mapping[str, object], spec: TaintSpec) -> bool:
|
||||
"""Does this flow start at something the source model names, rather than a bare captured parameter?"""
|
||||
source = str(flow["source"])
|
||||
return any(pattern in source for pattern in spec.sources)
|
||||
|
||||
|
||||
def _rank(flow: Mapping[str, object], spec: TaintSpec) -> tuple[int, int, str, str]:
|
||||
"""A flow from a modelled source first, then shortest, then lexicographic — a total order.
|
||||
|
||||
Shortest alone picks the wrong witness once closures are in scope: a captured `res` reaches the sink in
|
||||
three steps while the actual `req.url -> ... -> possibleFilename` path takes eleven, and reporting the
|
||||
former names no attacker input at all. Naming a real source is what makes a witness evidence rather than
|
||||
an assertion, so it outranks brevity.
|
||||
"""
|
||||
source = str(flow["source"])
|
||||
rank_modelled = 0 if _from_modelled_source(flow, spec) else 1
|
||||
return (rank_modelled, _as_int(flow["length"]), source, str(flow["sink"]))
|
||||
|
||||
|
||||
def _witness(flow: Mapping[str, object]) -> str:
|
||||
|
||||
Reference in New Issue
Block a user