From d651455d235cb682f2c85a4548c5b470910e03ee Mon Sep 17 00:00:00 2001 From: Marius Ciepluch <11855163+norandom@users.noreply.github.com> Date: Tue, 8 Sep 2026 16:24:23 +0200 Subject: [PATCH] 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 `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) Claude-Session: https://claude.ai/code/session_01T7XD7x5KonMdmGRkp63EQG --- src/openultrasast/cpg/queries/taint.sc | 45 ++++++-- src/openultrasast/model/taint.py | 48 ++++++-- .../ruleset/semantic/javascript.toml | 27 +++++ tests/test_model_taint.py | 105 ++++++++++++++++++ 4 files changed, 210 insertions(+), 15 deletions(-) diff --git a/src/openultrasast/cpg/queries/taint.sc b/src/openultrasast/cpg/queries/taint.sc index 88e5f56..bc17391 100644 --- a/src/openultrasast/cpg/queries/taint.sc +++ b/src/openultrasast/cpg/queries/taint.sc @@ -15,7 +15,8 @@ // * `reachableByFlows` is called ON the sink WITH the source, not the other way about. // // Parameters (comma-separated where plural): -// cpgFile, sources, sinks, sanitizers (optional), function (optional: restrict to this enclosing method), +// cpgFile, sources, sinks, sanitizers (optional), function (optional: restrict to this method AND any +// closure lexically nested inside it -- see `nestedInLabeled`), // parameterSources ("true" to treat the labeled function's parameters as untrusted -- see below) // // Output: a fenced JSON array of {sink, sinkLine, sinkMethod, source, sanitized, length}. The fence exists @@ -33,6 +34,17 @@ def split(raw: String): List[String] = raw.split(",").map(_.trim).filter(_.nonEmpty).toList + // 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. + def mentionsToken(code: String, tokens: List[String]): Boolean = + tokens.exists(t => + java.util.regex.Pattern + .compile("(? sourcePatterns.exists(p => c.code.contains(p))) - // Parameter sources: the labeled function's own parameters. In a function-level pair the function boundary - // IS the trust boundary -- the corpus is built so the labeled function's inputs are attacker-controlled -- - // and 39 of this slice's 50 pairs carry no framework token at all. Our flat-IR baseline counted these as - // sources (`source_kinds: ["parameter"]`), so a comparison against it is only like-for-like with them on. + // The labeled methods, and the region they lexically own. A closure defined inside the labeled function -- + // the callback passed to `fs.stat`, say -- has its own synthetic method (`0`) whose astParentFullName + // is empty, so nesting is decided by line-range containment within the same file, which is reliable. + lazy val labeledMethods = if (function.isEmpty) List.empty else cpg.method.nameExact(function).l + + def nestedInLabeled(m: io.shiftleft.codepropertygraph.generated.nodes.Method): Boolean = + labeledMethods.exists { lm => + m.name == function || ( + m.filename == lm.filename && + m.lineNumber.getOrElse(-1) >= lm.lineNumber.getOrElse(0) && + m.lineNumberEnd.getOrElse(-1) <= lm.lineNumberEnd.getOrElse(Int.MaxValue) + ) + } + + // Parameter sources: the labeled function's OWN parameters, never a nested closure's. In a function-level + // pair the function boundary IS the trust boundary -- the corpus is built so the labeled function's inputs + // are attacker-controlled -- and 39 of the injection slice's 50 pairs carry no framework token at all. Our + // flat-IR baseline counted these (`source_kinds: ["parameter"]`), so the comparison is only like-for-like + // with them on. Scoping matters as much as enabling: a callback's own parameters (`err`, `stats`) are not + // attacker input, and counting them produced flows like `res -> readFileSync` that name no real source. // Off by default: outside a labeled function, treating every parameter as untrusted is not sound. def parameterNodes = if (parameterSources != "true") Iterator.empty @@ -63,7 +91,7 @@ val all = cpg.call.filter(c => sinkNames.exists(n => c.name == n || c.code.startsWith(n + "(") || c.code.startsWith(n) || c.methodFullName.contains(n)) ) - if (function.isEmpty) all else all.filter(_.method.name == function) + if (function.isEmpty) all else all.filter(c => nestedInLabeled(c.method)) } val rows = sinkCalls.l.flatMap { sink => @@ -83,7 +111,7 @@ flows.map { flow => val elements = flow.elements.map(_.code).l - val sanitized = sanitizerNames.nonEmpty && elements.exists(code => sanitizerNames.exists(s => code.contains(s))) + val sanitized = sanitizerNames.nonEmpty && elements.exists(code => mentionsToken(code, sanitizerNames)) ujson.Obj( "sink" -> sink.code.take(200), "sinkLine" -> sink.lineNumber.getOrElse(-1).toString, @@ -92,7 +120,8 @@ "sanitized" -> sanitized, "length" -> elements.size, "sinkArity" -> arity, - "sinkArg0Literal" -> arg0Literal + "sinkArg0Literal" -> arg0Literal, + "inLabeledScope" -> (function.isEmpty || nestedInLabeled(sink.method)) ) } } diff --git a/src/openultrasast/model/taint.py b/src/openultrasast/model/taint.py index 6001866..864813b 100644 --- a/src/openultrasast/model/taint.py +++ b/src/openultrasast/model/taint.py @@ -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 `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: diff --git a/src/openultrasast/ruleset/semantic/javascript.toml b/src/openultrasast/ruleset/semantic/javascript.toml index f5280a8..056fc7f 100644 --- a/src/openultrasast/ruleset/semantic/javascript.toml +++ b/src/openultrasast/ruleset/semantic/javascript.toml @@ -28,6 +28,17 @@ id = "parameterized_query" calls = ["query", "execute"] parameterized = true +# --- model-grounded-detection: raw-Node request sources ------------------------------------------------- +# The shipped `req` source fact carries the Express idioms (req.query, req.body, req.params). A raw Node +# http server -- which is what a static file server is -- reads the path straight off `req.url` +# (http.IncomingMessage.url), and `url.parse(req.url)` is the documented way to split it. Measured need: +# with these absent, the angular-http-server path-traversal pair resolved its flow to the captured `res` +# rather than to attacker input, because no modelled source matched. + +[[source]] +id = "node-request" +patterns = ["req.url", "request.url", "req.rawHeaders", "url.parse", "parsedUrl"] + # --- model-grounded-detection task 4.2 ------------------------------------------------------------------- # See the note in python.toml: documented dangerous APIs, provenance per entry, reviewed as data. @@ -65,3 +76,19 @@ id = "redirect" cwe = "CWE-601" calls = ["redirect", "location.assign", "location.replace"] rule_ids = [] + +# --- model-grounded-detection: sanitizers ----------------------------------------------------------------- +# This file shipped with no [[sanitizer]] entries at all, so no JavaScript fix of any family could be +# recognised and the model entailed a fixed twin exactly as readily as its vulnerable one. Measured on the +# angular-http-server path-traversal pair, whose fix is precisely `path.normalize(...).replace(/^(\.\.[\/\\])+/, '')`. +# Documented normalising / escaping APIs, provenance per entry, reviewed as data. + +[[sanitizer]] +# CWE-22. Node's documented path normalisation; `resolve` and `join` against a fixed base collapse `..`. +id = "path.normalize" +calls = ["path.normalize", "path.resolve"] + +[[sanitizer]] +# CWE-79. The standard encoders: the DOM/global URI encoders and the common escaping helpers. +id = "html.escape" +calls = ["encodeURIComponent", "encodeURI", "escapeHtml", "escape", "sanitizeHtml", "DOMPurify.sanitize"] diff --git a/tests/test_model_taint.py b/tests/test_model_taint.py index 71ca1b3..68c26dc 100644 --- a/tests/test_model_taint.py +++ b/tests/test_model_taint.py @@ -204,3 +204,108 @@ def test_parameter_sources_are_opt_in_and_reach_the_query() -> None: assert seen["parameterSources"] == "false", "treating every parameter as untrusted is not the default" verdict(CpgResult(cpg_path=Path("c.bin"), run=run), _spec(), function="run", parameter_sources=True) assert seen["parameterSources"] == "true" + + +# --- closure scoping (group 4 finding) ----------------------------------------------------------------- +# +# JavaScript is callback-heavy, so the method containing a sink routinely is not the labeled function: +# +# function requestListener(req, res) { // labeled +# fs.stat(possibleFilename, function(err, stats) { +# fs.readFileSync(possibleFilename) // sink, enclosing method is 0 +# }) +# } +# +# Filtering on `sinkMethod == function` dropped every such sink -- the whole `path` family measured 0/8. +# The query now decides scope by line-range containment and reports it; the Python side trusts that flag and +# keeps the name comparison only as a fallback for rows that carry no flag. + + +def test_a_sink_in_a_nested_closure_is_in_scope_when_the_query_says_so() -> None: + from openultrasast.model.ladder import Rung + from openultrasast.model.taint import verdict + + rows = [{ + "sink": "fs.readFileSync(possibleFilename)", "sinkLine": "6", "sinkMethod": "0", + "source": "req", "sanitized": False, "length": 4, "inLabeledScope": True, + }] + answer = verdict(_cpg(rows), _spec(), function="requestListener") + assert answer is not None and answer.rung is Rung.ENTAILED + assert "0" not in answer.witness, "the witness names the flow, not the synthetic closure name" + + +def test_a_sink_in_an_unrelated_function_stays_out_of_scope() -> None: + from openultrasast.model.taint import verdict + + rows = [{ + "sink": "fs.readFileSync('/etc/' + name)", "sinkLine": "11", "sinkMethod": "unrelated", + "source": "name", "sanitized": False, "length": 2, "inLabeledScope": False, + }] + assert verdict(_cpg(rows), _spec(), function="requestListener") is None + + +def test_rows_without_the_scope_flag_fall_back_to_the_method_name() -> None: + """A row from an engine that cannot report scope must not silently become in-scope.""" + from openultrasast.model.taint import verdict + + rows = [{"sink": "os.system(x)", "sinkLine": "9", "sinkMethod": "0", + "source": "request.args['x']", "sanitized": False, "length": 2}] + assert verdict(_cpg(rows), _spec(), function="run") is None + + +def test_a_flow_from_a_real_source_outranks_a_shorter_one_from_a_bare_parameter() -> None: + """Shortest-wins alone picks the wrong witness once closures are in scope. + + In the angular-http-server shape the closure yields three unsanitized flows to one sink: + `req.url.split('?')[0]` (11 steps, the actual vulnerability), `res` (3 steps) and `this` (7). Shortest + would report `res -> fs.readFileSync(...)`, which names no attacker input and is simply untrue. A flow + whose source matches the spec's source model outranks one from a bare captured name; shortest still + breaks ties, so the order stays total and the verdict deterministic. + """ + from openultrasast.model.ladder import Rung + from openultrasast.model.taint import verdict + + rows = [ + {"sink": "fs.readFileSync(p)", "sinkLine": "6", "sinkMethod": "0", "source": "res", + "sanitized": False, "length": 3, "inLabeledScope": True}, + {"sink": "fs.readFileSync(p)", "sinkLine": "6", "sinkMethod": "0", "source": "this", + "sanitized": False, "length": 7, "inLabeledScope": True}, + {"sink": "fs.readFileSync(p)", "sinkLine": "6", "sinkMethod": "0", + "source": "request.args['f']", "sanitized": False, "length": 11, "inLabeledScope": True}, + ] + answer = verdict(_cpg(rows), _spec(), function="requestListener") + assert answer is not None and answer.rung is Rung.ENTAILED + assert "request.args['f']" in answer.witness, f"witness named no real source: {answer.witness}" + assert verdict(_cpg(rows), _spec(), function="requestListener") == answer # still deterministic + + +def test_a_modelled_source_flow_decides_even_when_a_parameter_flow_is_unsanitized() -> None: + """Parameter sources are a fallback, never an override. + + On the fixed angular-http-server side the real flow `req.url -> ... -> readFileSync` IS sanitized by + path.normalize, but the captured `res` parameter also reaches the sink unsanitized. Judging on the union + reports the fix as still vulnerable, on the strength of a flow that names no attacker input. When any + modelled source reaches the sink, those flows are the evidence and the bare-parameter ones are ignored. + """ + from openultrasast.model.ladder import Rung + from openultrasast.model.taint import verdict + + rows = [ + {"sink": "fs.readFileSync(safe)", "sinkLine": "41", "sinkMethod": "0", "source": "res", + "sanitized": False, "length": 3, "inLabeledScope": True}, + {"sink": "fs.readFileSync(safe)", "sinkLine": "41", "sinkMethod": "0", + "source": "request.args['f']", "sanitized": True, "length": 22, "inLabeledScope": True}, + ] + answer = verdict(_cpg(rows), _spec(), function="requestListener") + assert answer is not None and answer.rung is Rung.CORROBORATED, "a sanitized real flow is not an entailment" + assert "request.args['f']" in answer.witness + + +def test_parameter_flows_still_decide_when_no_modelled_source_reaches_the_sink() -> None: + from openultrasast.model.ladder import Rung + from openultrasast.model.taint import verdict + + rows = [{"sink": "os.system(cmd)", "sinkLine": "4", "sinkMethod": "run", "source": "username", + "sanitized": False, "length": 2, "inLabeledScope": True}] + answer = verdict(_cpg(rows), _spec(), function="run") + assert answer is not None and answer.rung is Rung.ENTAILED