From f31ef862a775b7d04edfc835eadfffa8e7c453a9 Mon Sep 17 00:00:00 2001 From: Marius Ciepluch <11855163+norandom@users.noreply.github.com> Date: Sat, 19 Sep 2026 20:38:39 +0200 Subject: [PATCH] fix(output-encoding): ask about the server half, and say what the family cannot decide The census recorded this family completing 44 questions on a Node application and finding nothing, which is what a client-only vocabulary must do there: all six declared sinks were DOM sinks. NodeGoat's cross-site scripting is server-side. Decision: the family asks about a value reaching a rendering call, and states in its own declaration that it cannot decide template-mediated escaping. Whether res.render is safe depends on the engine's autoescape setting and on the template's markup, neither of which is at the call, so res.render is not a sink and the family's limits field says so. A new CWE-79 sink covers res.write, res.end, res.send and the response.* spellings; res.json stays out because an application/json body is not a document. Template auto-escaping is claimed where its evidence is, as a configuration defect: swig.setDefaults, nunjucks.configure and marked.setOptions are security settings now. It establishes a finding: app/routes/research.js:25, model_entailed, `req.query.url + req.query.symbol -> res.write(body)`. The handler fetches a URL the caller chooses and writes the response body into a text/html response, so the document served from this origin is the attacker's. The record states how coarse the step is: the path from url to body runs through the client's callback parameter, which the graph treats as reachable from the call's arguments, so the conclusion rests on the call's semantics rather than on what the graph established. Five of seven families now establish findings. Four defects were found on the way, three of them false positives this change would otherwise have shipped. - The permissive-value set was shared across every setting, while a boolean's polarity belongs to the setting: true leaves the door open in cors({origin: true}) and closes it in swig.setDefaults({autoescape: true}). Adding the template engines would have reported the fix as the defect on the line that offers it. ConfigSpec now carries per-setting values, and Jinja2's Environment moved out of Python's shared list for the same latent reason. - config.sc matched a dotted setting on its trailing segment alone, so marked.setOptions would have claimed an editor's setOptions. dominance.sc already required the qualifier. - taint.sc asked its flow query of every argument including the receiver at index 0. On a response sink the receiver is the response object, so res.end() -- which takes no data -- was reported exactly like the res.write(body) two lines above it. Flows now use the real arguments, as arity already did. - The arbiter sorted a file's settings by line number as TEXT, so line 126 preceded line 78 and which setting a file reported depended on the spelling of its line numbers. Template escaping disabled at server.js:135 is detected, measured model_entailed against the real query and the real arbiter, and still not reported: the arbiter returns one verdict per question and that file has an earlier permissive setting. Task 14.6 carries it rather than hiding it. 1,415 tests pass, thirteen skipped; nine Joern-gated query tests pass in the image. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EmM1j46qgKMh3zqD1QMwEo --- .kiro/specs/pre-push-safety-net/status.md | 22 +- .kiro/specs/pre-push-safety-net/tasks.md | 12 +- .../2026-09-19-output-encoding-family.json | 276 ++++++++++++++++++ src/openultrasast/cpg/queries/config.sc | 16 +- src/openultrasast/cpg/queries/taint.sc | 7 +- src/openultrasast/model/config_value.py | 31 +- src/openultrasast/model/specs.py | 14 + src/openultrasast/ruleset/families.toml | 1 + .../ruleset/obligations/javascript.toml | 24 ++ .../ruleset/obligations/python.toml | 11 +- .../ruleset/obligations/typescript.toml | 24 ++ .../ruleset/semantic/javascript.toml | 16 + tests/test_cpg_queries_compile.py | 105 +++++++ tests/test_model_config_value.py | 83 ++++++ 14 files changed, 628 insertions(+), 14 deletions(-) create mode 100644 benchmarks/measurements/2026-09-19-output-encoding-family.json diff --git a/.kiro/specs/pre-push-safety-net/status.md b/.kiro/specs/pre-push-safety-net/status.md index 52ed5d9..e3adb7b 100644 --- a/.kiro/specs/pre-push-safety-net/status.md +++ b/.kiro/specs/pre-push-safety-net/status.md @@ -152,11 +152,21 @@ the word `set` in a code comment, matched because the sink text clause searched assignment node. Nothing about this changes the release gates. No capability is qualified by these subjects. -The first of the three gaps is closed. The untrusted-destination vocabulary now holds the -client surface a Node server actually calls, and NodeGoat's SSRF is reported at -`app/routes/research.js:16` with a witness naming the call. One data change, no query change, -every other family unchanged. Output encoding and access control remain open as tasks 14.3 -and 14.4. +Two of the three gaps are closed. The untrusted-destination vocabulary now holds the client +surface a Node server actually calls, and NodeGoat's SSRF is reported at +`app/routes/research.js:16` with a witness naming the call. Output encoding now asks about the +server half of its own family and establishes a finding at `app/routes/research.js:25`, and it +states in its declaration that template-mediated escaping is not something it can decide. Five +of seven families now establish findings; access control remains open as task 14.4. + +Closing the second gap surfaced four defects, three of which were false positives it would +otherwise have shipped: a permissive-value set shared across settings whose booleans have +opposite polarity, a dotted setting matched on its trailing segment alone, a flow query asked of +a call's receiver as well as its arguments, and a file's settings ordered by the spelling of +their line numbers. Template auto-escaping disabled at `server.js:135` is detected and still not +reported, because the arbiter returns one verdict per question and that file has an earlier +permissive setting; task 14.6 carries it. Evidence: [family detection census](../../../benchmarks/measurements/2026-09-19-family-detection-census.json), -[untrusted destination vocabulary](../../../benchmarks/measurements/2026-09-19-untrusted-destination-vocabulary.json). +[untrusted destination vocabulary](../../../benchmarks/measurements/2026-09-19-untrusted-destination-vocabulary.json), +[output encoding family](../../../benchmarks/measurements/2026-09-19-output-encoding-family.json). diff --git a/.kiro/specs/pre-push-safety-net/tasks.md b/.kiro/specs/pre-push-safety-net/tasks.md index c3bc2f9..a099ce0 100644 --- a/.kiro/specs/pre-push-safety-net/tasks.md +++ b/.kiro/specs/pre-push-safety-net/tasks.md @@ -481,13 +481,23 @@ Preserve all completed task evidence below. - _Boundary: shipped semantic facts_ - _Depends: 14.1_ - _Requirements: 6.2, 8.1_ -- [ ] 14.3 Decide what output encoding claims for server-rendered JavaScript +- [x] 14.3 Decide what output encoding claims for server-rendered JavaScript - **Named by the 14.1 census.** All six declared JavaScript sinks are DOM sinks. NodeGoat's XSS is server-side, rendered into Swig templates and written with `res.write`, so the 44 questions this family completed could not have found it whatever the code did. - Either the family gains server-side sinks and template-rendering sinks, or it declares itself client-side only. Both are honest; silence while declaring XSS coverage is not. - Done when one record states the decision, and the family either asks about `res.write` and template rendering or says in its declaration what it does not cover. + - _Verified 2026-09-19:_ 1,415 tests passed, thirteen skipped; nine Joern-gated query tests pass in the image; Ruff/format/mypy passed. **Decision: the family asks about a value reaching a rendering call and states in its own declaration that it cannot decide template-mediated escaping.** Whether `res.render` is safe depends on the engine's autoescape setting and on the template's markup, neither of which is at the call, so `res.render` is not a sink and the family's `limits` now says so. The setting is claimed where its evidence is instead, as a configuration defect. + - It now establishes a finding: `app/routes/research.js:25`, model_entailed, `req.query.url + req.query.symbol -> res.write(body)`. The handler fetches a URL the caller chooses and writes the response body into a text/html response, so the document served from this origin is the attacker's. The record states how coarse the step is: the path from `url` to `body` runs through the client's callback parameter, which the graph treats as reachable from the call's arguments, so the conclusion rests on the call's semantics rather than on what the graph established. Five of seven families now establish findings. Evidence: `benchmarks/measurements/2026-09-19-output-encoding-family.json`. + - _Four defects found on the way, three of them false positives this change would otherwise have shipped._ The permissive-value set was shared across every setting while a boolean's polarity belongs to the setting, so `swig.setDefaults({autoescape: true})` would have been reported as the defect on the line that repairs it; `ConfigSpec` now carries per-setting values and Jinja2's `Environment` was moved out of Python's shared list for the same latent reason. `config.sc` matched a dotted setting on its trailing segment alone, so `marked.setOptions` would have claimed an editor's `setOptions`. `taint.sc` asked its flow query of every argument including the receiver, so `res.end()` — which takes no data — was reported exactly like the `res.write(body)` two lines above it. And the arbiter sorted a file's settings by line number as text, so which setting a file reported depended on the spelling of its line numbers. - _Boundary: shipped semantic facts, family taxonomy_ - _Depends: 14.1_ - _Requirements: 6.2, 8.1_ +- [ ] 14.6 Report every permissive setting in a file, not the first one + - **Named by the 14.3 measurement.** The configuration arbiter returns one verdict per question. NodeGoat's `server.js` holds a permissive session at line 78 and template auto-escaping disabled at line 135 in the same function, so the escaping defect is detected — measured model_entailed against the real query and the real arbiter — and never reported. A contributor is shown one of two defects and told nothing about the other. + - This is the arbiter's contract, not a data gap: `verdict()` returns `Verdict | None` and the scan collects one per question. Changing it touches every family that returns a verdict, so it is scheduled rather than folded into a vocabulary change. + - Done when a file with two permissive settings reports both, with their own sites and witnesses, and no family reports a defect twice. + - _Boundary: contributor-scan arbitration, Rung ladder_ + - _Depends: 14.3_ + - _Requirements: 6.3, 8.1_ - [ ] 14.4 Raise the access-control obligation where the application splits route from query - **Named by the 14.1 census and measured with the real dominance query.** Every modelled data operation in NodeGoat lives in `app/data/*-dao.js`; none lives in a route handler file. Operations are scoped to the region's own file and access_control regions are the handler files, so no obligation is raised where the question is asked, and inside the DAO files every operation is equally unguarded so the sibling rung has nothing to contradict. The family answers correctly on VAmPI, which keeps handler and query in one file. - The file scope was chosen deliberately, because unscoped rows attributed a model's unguarded query to a handler of the same name. Widening it is not the fix; carrying the obligation along the call the handler actually makes might be. diff --git a/benchmarks/measurements/2026-09-19-output-encoding-family.json b/benchmarks/measurements/2026-09-19-output-encoding-family.json new file mode 100644 index 0000000..7815eee --- /dev/null +++ b/benchmarks/measurements/2026-09-19-output-encoding-family.json @@ -0,0 +1,276 @@ +{ + "schema_version": 1, + "experiment": "output-encoding-family-v1", + "date": "2026-09-19", + "task": "pre-push-safety-net 14.3", + "claim_scope": "What the output-encoding family claims for server-rendered JavaScript, decided and measured on NodeGoat. Detection only. No capability qualified: NodeGoat developed this analyser.", + "decision": "The family asks about a value reaching a rendering call, and says in its own declaration that it cannot decide template-mediated escaping. Whether res.render is safe depends on the engine's autoescape setting and on the template's markup, neither of which is at the call, so res.render is not a sink. That setting is claimed instead where its evidence is, as a configuration defect.", + "changes": { + "semantic_facts": "New CWE-79 sink html.response: res.write, res.end, res.send and the response.* spellings. Every sink this family shipped with was a DOM sink, so on a Node application it could complete every question and find nothing. res.json is excluded: an application/json body is not a document.", + "obligation_facts": "New security_setting template-escaping: swig.setDefaults, nunjucks.configure, marked.setOptions, with a discharger of their own whose permissive value is false.", + "permissive_polarity": "ConfigSpec gained permissive_by_setting and the arbiter consults it. A boolean's meaning belongs to the setting: true leaves the door open in cors({origin: true}) and closes it in swig.setDefaults({autoescape: true}). The shared set holds both true and false, so without this the template engines would have reported the fix as the defect. Jinja2's Environment was moved out of Python's shared list for the same reason, where it had the same latent false positive.", + "config_query_receiver": "config.sc matched a dotted setting on its trailing segment alone, so marked.setOptions would have claimed an editor's setOptions. It now requires the qualifier, which dominance.sc already did.", + "taint_query_receiver": "taint.sc asked reachableByFlows of every argument including the receiver at index 0. On a response sink the receiver is the response object, so res.end() -- which takes no data at all -- was reported exactly like the res.write(body) two lines above it. Flows are now asked of the real arguments, as arity already was.", + "settings_order": "The arbiter returns one verdict per question and sorted its rows by line number as TEXT, so line 126 preceded line 78 and which setting a file reported depended on the spelling of its line numbers. Now numeric." + }, + "measured_on_nodegoat": { + "output_encoding_before": { + "questions_asked": 44, + "findings": 0, + "verdict": "answered, nothing found" + }, + "output_encoding_after": { + "questions_asked": 44, + "findings": 1, + "verdict": "establishes findings", + "finding": { + "site": "app/routes/research.js:25:1", + "rung": "model_entailed", + "witness": "req.query.url + req.query.symbol -> res.write(body) in app/routes/research.js (line 25, 6 steps)", + "why_it_is_real": "The handler fetches a URL the caller chooses and writes the response body into a text/html response. The attacker picks the origin, so the document served from this application's origin is theirs.", + "how_coarse_the_step_is": "The path from url to body runs through the HTTP client's callback parameter, which the graph treats as reachable from the call's arguments. That is an over-approximation rather than a proof that body derives from url. The conclusion holds on the semantics of the call; the graph did not establish it." + }, + "not_reported": "res.write with a literal heading on lines 22 and 24, and res.end() on line 27" + }, + "template_escaping_setting": { + "site": "server.js:135", + "call": "swig.setDefaults({ autoescape: false })", + "arbitrated": "model_entailed, measured directly against the real config query and the real arbiter", + "reported_by_the_census": false, + "why_not": "The arbiter returns ONE verdict per question, and server.js holds a permissive session at line 78 in the same function. The first setting in the file wins, so the escaping defect is detected and masked. Task 14.6 carries that." + }, + "fixed_line_stays_silent": "marked.setOptions({ sanitize: true }) at server.js:126 is NodeGoat's applied A9 fix and yields no finding. Under the shared permissive set it would have fired on true." + }, + "census_after": { + "nodegoat": { + "access_control": { + "regions_offering": 35, + "questions_asked": 35, + "questions_completed": 35, + "findings": 0, + "rungs": {}, + "examples": [], + "verdict": "answered, nothing found" + }, + "config_secrets": { + "regions_offering": 44, + "questions_asked": 44, + "questions_completed": 44, + "findings": 1, + "rungs": { + "model_entailed": 1 + }, + "examples": [ + { + "site": "server.js:78:?", + "rung": "model_entailed", + "witness": "session({\n // genid: (req) => {\n // return genuuid() // use UUIDs for session IDs\n //},\n secret: cookieSecret,\n // Both mandatory in Express v4\n saveUninit", + "reached_from": 1 + } + ], + "verdict": "establishes findings" + }, + "injection": { + "regions_offering": 44, + "questions_asked": 44, + "questions_completed": 44, + "findings": 3, + "rungs": { + "model_entailed": 3 + }, + "examples": [ + { + "site": "app/routes/contributions.js:32:2", + "rung": "model_entailed", + "witness": "req.body.preTax -> eval(req.body.preTax) in app/routes/contributions.js (line 32, 1 steps)", + "reached_from": 1 + }, + { + "site": "app/routes/contributions.js:33:2", + "rung": "model_entailed", + "witness": "req.body.afterTax -> eval(req.body.afterTax) in app/routes/contributions.js (line 33, 1 steps)", + "reached_from": 1 + }, + { + "site": "app/routes/contributions.js:34:2", + "rung": "model_entailed", + "witness": "req.body.roth -> eval(req.body.roth) in app/routes/contributions.js (line 34, 1 steps)", + "reached_from": 1 + } + ], + "verdict": "establishes findings" + }, + "output_encoding": { + "regions_offering": 44, + "questions_asked": 44, + "questions_completed": 44, + "findings": 1, + "rungs": { + "model_entailed": 1 + }, + "examples": [ + { + "site": "app/routes/research.js:25:1", + "rung": "model_entailed", + "witness": "req.query.url + req.query.symbol -> res.write(body) in app/routes/research.js (line 25, 6 steps)", + "reached_from": 1 + } + ], + "verdict": "establishes findings" + }, + "path": { + "regions_offering": 44, + "questions_asked": 44, + "questions_completed": 44, + "findings": 0, + "rungs": {}, + "examples": [], + "verdict": "answered, nothing found" + }, + "prototype": { + "regions_offering": 44, + "questions_asked": 44, + "questions_completed": 44, + "findings": 0, + "rungs": {}, + "examples": [], + "verdict": "answered, nothing found" + }, + "untrusted_destination": { + "regions_offering": 44, + "questions_asked": 44, + "questions_completed": 44, + "findings": 2, + "rungs": { + "model_entailed": 2 + }, + "examples": [ + { + "site": "app/routes/index.js:72:1", + "rung": "model_entailed", + "witness": "req.query.url -> res.redirect(req.query.url) in app/routes/index.js (line 72, 1 steps)", + "reached_from": 1 + }, + { + "site": "app/routes/research.js:16:0", + "rung": "model_entailed", + "witness": "req.query.url + req.query.symbol -> needle.get(url, (error, newResponse, body) => {\n if (!error && newResponse.statusCode === 200) {\n res.writeHead(200, {\n ", + "reached_from": 1 + } + ], + "verdict": "establishes findings" + } + }, + "vampi": { + "access_control": { + "regions_offering": 8, + "questions_asked": 8, + "questions_completed": 8, + "findings": 2, + "rungs": { + "model_entailed": 2 + }, + "examples": [ + { + "site": "api_views/books.py:51:get_by_title", + "rung": "model_entailed", + "witness": "get_by_title line 51: no discharging guard, while 1 sibling handler(s) are guarded (add_new_book)", + "reached_from": 1 + }, + { + "site": "api_views/users.py:187:update_password", + "rung": "model_entailed", + "witness": "update_password line 187: no discharging guard, while 3 sibling handler(s) are guarded (delete_user, me, update_email)", + "reached_from": 1 + } + ], + "verdict": "establishes findings" + }, + "config_secrets": { + "regions_offering": 25, + "questions_asked": 25, + "questions_completed": 25, + "findings": 1, + "rungs": { + "model_entailed": 1 + }, + "examples": [ + { + "site": "app.py:17:", + "rung": "model_entailed", + "witness": "vuln_app.run(host = '0.0.0.0', port = 5000, debug = True) (line 17): permissive literal '0.0.0.0'", + "reached_from": 1 + } + ], + "verdict": "establishes findings" + }, + "deserialization": { + "regions_offering": 25, + "questions_asked": 0, + "questions_completed": 0, + "findings": 0, + "rungs": {}, + "examples": [], + "verdict": "region offered, no question asked" + }, + "injection": { + "regions_offering": 25, + "questions_asked": 2, + "questions_completed": 2, + "findings": 1, + "rungs": { + "model_entailed": 1 + }, + "examples": [ + { + "site": "models/user_model.py:73:get_user", + "rung": "model_entailed", + "witness": "username -> text(user_query) in models/user_model.py (line 73, 13 steps)", + "reached_from": 1 + } + ], + "verdict": "establishes findings" + }, + "output_encoding": { + "regions_offering": 25, + "questions_asked": 0, + "questions_completed": 0, + "findings": 0, + "rungs": {}, + "examples": [], + "verdict": "region offered, no question asked" + }, + "path": { + "regions_offering": 25, + "questions_asked": 0, + "questions_completed": 0, + "findings": 0, + "rungs": {}, + "examples": [], + "verdict": "region offered, no question asked" + }, + "untrusted_destination": { + "regions_offering": 25, + "questions_asked": 0, + "questions_completed": 0, + "findings": 0, + "rungs": {}, + "examples": [], + "verdict": "region offered, no question asked" + } + } + }, + "families_establishing_findings": [ + "injection", + "access_control", + "config_secrets", + "untrusted_destination", + "output_encoding" + ], + "limits": [ + "Template-mediated escaping is never decided by flow; a scan of a server-rendered application reporting nothing from this family has established only that no tainted value reached a modelled rendering call.", + "Response splitting (CWE-113) is claimed by no sink: modern Node rejects a newline in a header value, and the claim would be about an engine this project does not model.", + "res.send cannot be distinguished from its JSON-serialising form at the call, so a tainted object reaching it is reported like a tainted string.", + "One subject exercises these sinks. NodeGoat developed this analyser and qualifies nothing." + ] +} diff --git a/src/openultrasast/cpg/queries/config.sc b/src/openultrasast/cpg/queries/config.sc index dfb85b0..70b517a 100644 --- a/src/openultrasast/cpg/queries/config.sc +++ b/src/openultrasast/cpg/queries/config.sc @@ -38,10 +38,22 @@ val names = split(settingsS) val function = functionS - def matches(name: String): Boolean = names.exists(n => name == n || name == n.split("\\.").last) + // A dotted spec matches on its trailing segment, which is how the call is named in the graph -- but the + // qualification has to matter, or the segment alone claims every unrelated call that ends the same way. + // `marked.setOptions` would otherwise match an editor's `setOptions` and `nunjucks.configure` any + // `configure` at all. dominance.sc carries this fix and the reason for it; the setting matcher had never + // been given the same treatment, and it became load-bearing when the template engines joined the table. + def matches(name: String, code: String): Boolean = + names.exists { n => + if (name == n) true + else { + val parts = n.split("\\.") + parts.length > 1 && name == parts.last && parts.init.forall(q => code.contains(q)) + } + } val calls = { - val all = cpg.call.filter(c => matches(c.name)).l + val all = cpg.call.filter(c => matches(c.name, c.code)).l val inFile = if (fileS.isEmpty) all else all.filter(_.method.filename.endsWith(fileS)) if (function.isEmpty) inFile else inFile.filter(_.method.name == function) } diff --git a/src/openultrasast/cpg/queries/taint.sc b/src/openultrasast/cpg/queries/taint.sc index e0e2406..2fc43dc 100644 --- a/src/openultrasast/cpg/queries/taint.sc +++ b/src/openultrasast/cpg/queries/taint.sc @@ -772,7 +772,12 @@ // makes a one-argument interpolated call look like a two-argument bound one -- the exact inversion of the // test. Real arguments start at index 1. val realArgs = sink.argument.argumentIndexGt(0).l - val flows = if (hasSources) sink.argument.reachableByFlows(sourceNodes).l else Nil + // Asked of the REAL arguments, for the same reason arity is counted over them. `call.argument` carries the + // receiver at index 0, and on a response sink the receiver is the response object itself: `res.end()` takes + // no data at all, yet `res` is reachable from the request in any handler that captures it, so the flow + // query answered yes about a call that receives nothing. Measured on NodeGoat, where `res.write(body)` is a + // real finding and `res.end()` two lines below it was reported identically. + val flows = if (hasSources) sink.argument.argumentIndexGt(0).reachableByFlows(sourceNodes).l else Nil // The SHAPE of the sink call, which is what distinguishes a fix from a bug when the fix is a safe form // rather than a sanitizing call: `execute(sql, params)` binds where `execute(sql + x)` interpolates, and diff --git a/src/openultrasast/model/config_value.py b/src/openultrasast/model/config_value.py index e255c8b..04dbd18 100644 --- a/src/openultrasast/model/config_value.py +++ b/src/openultrasast/model/config_value.py @@ -40,12 +40,14 @@ def verdict(cpg: CpgResult, spec: ConfigSpec, *, function: str = "", file: str = # Both abstractions read the same literal: a permissive flag ("*", True) and a weak algorithm ("md5"). permissive = {value.strip().strip("'\"").lower() for value in spec.permissive} permissive |= {value.strip().strip("'\"").lower() for value in spec.weak_algorithms} + scoped = {setting: {value.strip().strip("'\"").lower() for value in values} for setting, values in spec.permissive_by_setting} for row in settings: literals = row["literals"] if not isinstance(literals, list): continue + allowed = _permissive_for(str(row["setting"]), scoped) or permissive for literal in literals: - if str(literal).strip().strip("'\"").lower() in permissive: + if str(literal).strip().strip("'\"").lower() in allowed: return Verdict( rung=Rung.ENTAILED, family=spec.family, @@ -64,6 +66,21 @@ def verdict(cpg: CpgResult, spec: ConfigSpec, *, function: str = "", file: str = return None +def _permissive_for(setting: str, scoped: Mapping[str, set[str]]) -> set[str]: + """The values permissive for the setting THIS row calls, empty when the facts declare none for it. + + A boolean's meaning is a property of the setting, not of the language: `autoescape: false` disables + escaping and `secure: false` disables a cookie flag, while `autoescape: true` is the fix and `origin: true` + is the defect. Asking per setting is what lets the template engines join the table without reporting every + repository that escapes its output. + + The longest declared name that appears in the call wins, so a specific `swig.setDefaults` is not decided by + a shorter name that happens to be a substring of the same call. + """ + matched = sorted((name for name in scoped if name in setting), key=len, reverse=True) + return set(scoped[matched[0]]) if matched else set() + + def _settings(rows: object, spec: ConfigSpec, *, function: str) -> list[dict[str, object]]: """Rows for calls this spec actually names. A non-list (an engine failure) yields nothing.""" if not isinstance(rows, Sequence) or isinstance(rows, (str, bytes)): @@ -86,8 +103,16 @@ def _settings(rows: object, spec: ConfigSpec, *, function: str) -> list[dict[str "literals": [str(v) for v in literals] if isinstance(literals, Sequence) and not isinstance(literals, str) else [], } ) - # Deterministic order: the first row decides, so it must not depend on engine ordering. - return sorted(kept, key=lambda row: (str(row["method"]), str(row["line"]), str(row["setting"]))) + + # Deterministic order: the first row decides, so it must not depend on engine ordering. The line sorts + # NUMERICALLY, which is what "the first row" was always meant to mean: as text, line 126 precedes line 78, + # and which of a file's settings gets reported was decided by the spelling of its line number. Caught when + # a second permissive setting was added to a file that already had one. + def _line(row: dict[str, object]) -> tuple[int, str]: + text = str(row["line"]) + return (int(text) if text.lstrip("-").isdigit() else 1 << 30, text) + + return sorted(kept, key=lambda row: (str(row["method"]), _line(row), str(row["setting"]))) __all__ = ["request_params", "verdict"] diff --git a/src/openultrasast/model/specs.py b/src/openultrasast/model/specs.py index f6e4a66..80b2ca1 100644 --- a/src/openultrasast/model/specs.py +++ b/src/openultrasast/model/specs.py @@ -173,6 +173,13 @@ class ConfigSpec: # algorithm NAMED, not a setting left open. The data has been on the sink facts (`weak_literals`) since # before this feature and nothing read it. weak_algorithms: tuple[str, ...] = () + # Which values are permissive FOR WHICH setting, as (setting, values) pairs. A flat set cannot hold the + # answer, because the polarity of a boolean is per setting: `true` leaves the door open in + # `cors({origin: true})` and closes it in `swig.setDefaults({autoescape: true})`. With one shared set, + # adding a template engine would have reported the fix as the defect on exactly the line whose comment + # offers it. A setting a discharger names uses that discharger's values; a setting no discharger names + # falls back to `permissive`, which is the committed behaviour for `app.listen` and `createClient`. + permissive_by_setting: tuple[tuple[str, tuple[str, ...]], ...] = () def config_specs( @@ -194,6 +201,12 @@ def config_specs( weak_calls = tuple(sorted({call for sink in flow.sinks if sink.weak_literals for call in sink.calls})) if not settings or not permissive: return {} + scoped_values: dict[str, set[str]] = {} + for fact in scoped.dischargers: + if fact.kind != "non_permissive_value" or not fact.permissive_values: + continue + for call in fact.calls: + scoped_values.setdefault(call, set()).update(fact.permissive_values) return { "config_secrets": ConfigSpec( family="config_secrets", @@ -201,6 +214,7 @@ def config_specs( settings=tuple(sorted(set(settings) | set(weak_calls))), permissive=permissive, weak_algorithms=weak, + permissive_by_setting=tuple((call, tuple(sorted(values))) for call, values in sorted(scoped_values.items())), ) } diff --git a/src/openultrasast/ruleset/families.toml b/src/openultrasast/ruleset/families.toml index 9f7311e..8a248bd 100644 --- a/src/openultrasast/ruleset/families.toml +++ b/src/openultrasast/ruleset/families.toml @@ -46,6 +46,7 @@ description = "Untrusted data reaches a rendered document or a header without en cwes = ["CWE-79", "CWE-80", "CWE-83", "CWE-113", "CWE-116"] mechanisms = [] verifier = "none" +limits = """Models a value reaching a rendering CALL -- a DOM sink, or a response body written directly. It cannot decide template-mediated escaping, which is the commonest shape a server-rendered application has: whether `res.render` is safe depends on the template engine's autoescape configuration and on the template's own markup, neither of which is at the call. That setting is claimed instead as a configuration defect, where a permissive literal can be read: `swig.setDefaults`, `nunjucks.configure` and Jinja2's `Environment` are security settings in the obligation facts, and turning escaping off is reported by config_secrets. Response splitting (CWE-113) is claimed by no sink here at all, because modern Node rejects a newline in a header value and the claim would be about an engine this project does not model. A scan of a server-rendered application that reports nothing from this family has established that no tainted value reached a modelled rendering call, and nothing about whether its templates escape.""" [[family]] id = "untrusted_destination" diff --git a/src/openultrasast/ruleset/obligations/javascript.toml b/src/openultrasast/ruleset/obligations/javascript.toml index 2145340..cd4c1fd 100644 --- a/src/openultrasast/ruleset/obligations/javascript.toml +++ b/src/openultrasast/ruleset/obligations/javascript.toml @@ -38,6 +38,20 @@ resource_arg = 0 requires = ["non_permissive_value"] sensitivity = "medium" +[[operation]] +# Template auto-escaping, which is the setting NodeGoat's cross-site scripting actually turns off: +# `swig.setDefaults({ autoescape: false })` at server.js:135, with `autoescape: true` sitting commented out +# directly beneath it. The 2026-09-19 family census recorded output encoding completing 44 questions on that +# repository and finding nothing, because escaping there is a property of the engine's configuration rather +# than of any call a value flows into. It is a value-shaped defect, so it is claimed here rather than by a +# flow, and `output_encoding` states this limit in its own declaration. +id = "template-escaping" +kind = "security_setting" +calls = ["swig.setDefaults", "nunjucks.configure", "marked.setOptions"] +resource_arg = 0 +requires = ["non_permissive_value"] +sensitivity = "medium" + [[discharger]] id = "route-guard" kind = "path_guard" @@ -62,6 +76,16 @@ kind = "non_permissive_value" calls = ["cors", "cookie", "session", "helmet", "setHeader"] permissive_values = ["'*'", "\"*\"", "true", "0.0.0.0", "null", "false", "origin: true", "secure: false", "httpOnly: false"] +[[discharger]] +# The polarity of a boolean belongs to the setting, not to the language. `true` leaves the door open in +# `cors({origin: true})` and closes it in `swig.setDefaults({autoescape: true})`, so these settings carry +# their OWN permissive value rather than sharing the general one -- which holds both `true` and `false`, and +# would have reported NodeGoat's commented-out fix as the defect. +id = "safe-template-escaping" +kind = "non_permissive_value" +calls = ["swig.setDefaults", "nunjucks.configure", "marked.setOptions"] +permissive_values = ["false"] + [[discharger]] id = "validated-input" kind = "validated_input" diff --git a/src/openultrasast/ruleset/obligations/python.toml b/src/openultrasast/ruleset/obligations/python.toml index e8f32a8..9d2a866 100644 --- a/src/openultrasast/ruleset/obligations/python.toml +++ b/src/openultrasast/ruleset/obligations/python.toml @@ -68,9 +68,18 @@ id = "safe-setting" kind = "non_permissive_value" calls = ["CORS", "cross_origin", "set_cookie", "app.run", "config.update", "add_middleware", "session_setup", "SimpleCookieStorage", "setup_session", "set_secure_cookie", - "Application", "RedisStorage", "Environment"] + "Application", "RedisStorage"] permissive_values = ["'*'", "\"*\"", "True", "0.0.0.0", "None", "False", "allow_all", "disable"] +[[discharger]] +# Jinja2's `Environment(autoescape=...)`, moved out of the general list because `True` is in it and `True` is +# the SAFE value here. Sharing one permissive set made the correct configuration indistinguishable from the +# dangerous one, which is the same defect the JavaScript template engines would have introduced. +id = "safe-template-escaping" +kind = "non_permissive_value" +calls = ["Environment", "select_autoescape"] +permissive_values = ["False"] + [[discharger]] id = "validated-input" kind = "validated_input" diff --git a/src/openultrasast/ruleset/obligations/typescript.toml b/src/openultrasast/ruleset/obligations/typescript.toml index 8e3c118..bffb03d 100644 --- a/src/openultrasast/ruleset/obligations/typescript.toml +++ b/src/openultrasast/ruleset/obligations/typescript.toml @@ -38,6 +38,20 @@ resource_arg = 0 requires = ["non_permissive_value"] sensitivity = "medium" +[[operation]] +# Template auto-escaping, which is the setting NodeGoat's cross-site scripting actually turns off: +# `swig.setDefaults({ autoescape: false })` at server.js:135, with `autoescape: true` sitting commented out +# directly beneath it. The 2026-09-19 family census recorded output encoding completing 44 questions on that +# repository and finding nothing, because escaping there is a property of the engine's configuration rather +# than of any call a value flows into. It is a value-shaped defect, so it is claimed here rather than by a +# flow, and `output_encoding` states this limit in its own declaration. +id = "template-escaping" +kind = "security_setting" +calls = ["swig.setDefaults", "nunjucks.configure", "marked.setOptions"] +resource_arg = 0 +requires = ["non_permissive_value"] +sensitivity = "medium" + [[discharger]] id = "route-guard" kind = "path_guard" @@ -62,6 +76,16 @@ kind = "non_permissive_value" calls = ["cors", "cookie", "session", "helmet", "setHeader"] permissive_values = ["'*'", "\"*\"", "true", "0.0.0.0", "null", "false", "origin: true", "secure: false", "httpOnly: false"] +[[discharger]] +# The polarity of a boolean belongs to the setting, not to the language. `true` leaves the door open in +# `cors({origin: true})` and closes it in `swig.setDefaults({autoescape: true})`, so these settings carry +# their OWN permissive value rather than sharing the general one -- which holds both `true` and `false`, and +# would have reported NodeGoat's commented-out fix as the defect. +id = "safe-template-escaping" +kind = "non_permissive_value" +calls = ["swig.setDefaults", "nunjucks.configure", "marked.setOptions"] +permissive_values = ["false"] + [[discharger]] id = "validated-input" kind = "validated_input" diff --git a/src/openultrasast/ruleset/semantic/javascript.toml b/src/openultrasast/ruleset/semantic/javascript.toml index fb7fe3c..db639ce 100644 --- a/src/openultrasast/ruleset/semantic/javascript.toml +++ b/src/openultrasast/ruleset/semantic/javascript.toml @@ -57,6 +57,22 @@ cwe = "CWE-79" calls = ["innerHTML", "outerHTML", "insertAdjacentHTML", "document.write", "dangerouslySetInnerHTML", "v-html"] rule_ids = [] +[[sink]] +# CWE-79, the SERVER half. Every sink above is a DOM sink, so this family could only ever ask about code +# running in a browser -- and the 2026-09-19 family census recorded it completing 44 questions on a Node +# application and finding nothing, which is exactly what a client-only vocabulary must do there. These are the +# documented ways an Express or raw-Node handler puts bytes in a response body without an encoder in between. +# +# `res.json` is deliberately absent: it sets an application/json content type, so a value reaching it is not +# a script in a document. `res.render` is deliberately absent too, and for a different reason -- whether a +# template escapes is a property of the engine's configuration rather than of the call, so it is claimed as a +# configuration defect (`swig.setDefaults`, `nunjucks.configure` in the obligation facts) and stated as a +# limit on this family rather than guessed at here. +id = "html.response" +cwe = "CWE-79" +calls = ["res.write", "res.end", "res.send", "response.write", "response.end", "response.send"] +rule_ids = [] + [[sink]] # CWE-22. Node filesystem entry points that accept a caller-supplied path. id = "path.open" diff --git a/tests/test_cpg_queries_compile.py b/tests/test_cpg_queries_compile.py index 99e069f..28fd011 100644 --- a/tests/test_cpg_queries_compile.py +++ b/tests/test_cpg_queries_compile.py @@ -191,3 +191,108 @@ def test_a_literal_destination_is_not_an_untrusted_destination(tmp_path: Path) - flows = [row for row in (payload.get("0") or []) if row.get("sink")] assert flows, "the request flowing into the client was not reported at all" assert all("needle" in str(row["sink"]) for row in flows), f"a client called with a literal was reported: {flows}" + + +@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine") +def test_a_response_body_write_is_an_output_encoding_sink(tmp_path: Path) -> None: + """A request value written straight into a response body is reported; the JSON form is not. + + Every sink this family shipped with was a DOM sink, so on a Node application it completed its questions + and could not have found anything: the census measured that on NodeGoat, 44 questions and no findings. + `res.json` stays out because it sets an application/json content type, and that exclusion is asserted here + rather than left to the reader of the fact table. + """ + from openultrasast.cpg.backend import extract_payload + from openultrasast.model.specs import taint_specs + + frontend = __import__("shutil").which("jssrc2cpg") + if frontend is None: + pytest.skip("jssrc2cpg is not installed") + sinks = taint_specs(language="javascript")["output_encoding"].sinks + assert "res.write" in sinks, "the shipped vocabulary no longer holds the sink this control is about" + assert "res.json" not in sinks, "res.json is a JSON content type, not a rendered document" + + source = tmp_path / "render.js" + source.write_text( + "function handler(req, res) {\n" + " const name = req.query.name;\n" + " res.json({ echoed: name });\n" + ' res.write("

" + name + "

");\n' + " return res.end();\n" + "}\n" + ) + cpg = tmp_path / "cpg.bin" + built = subprocess.run([frontend, str(tmp_path), "-o", str(cpg)], capture_output=True, text=True, timeout=600, check=False) + if not cpg.is_file(): + pytest.skip(f"could not build a sample cpg: {(built.stderr or '')[-200:]}") + + requests = tmp_path / "requests.json" + requests.write_text(json.dumps({"0": {"sources": "req.query", "sinks": ",".join(sinks), "function": "handler"}})) + done = subprocess.run( + [ + _joern() or "joern", + "--script", + str((QUERIES / "taint.sc").resolve()), + "--param", + f"cpgFile={cpg}", + "--param", + f"requestsFile={requests}", + ], + capture_output=True, + text=True, + timeout=900, + check=False, + ) + assert done.returncode == 0, f"taint.sc did not run: {(done.stderr or done.stdout or '')[-500:]}" + payload = extract_payload(done.stdout or "") + assert payload is not None, "taint.sc produced no parseable payload" + flows = [row for row in (payload.get("0") or []) if row.get("sink")] + assert flows, "a request value written into the response body was not reported" + assert all("res.json" not in str(row["sink"]) for row in flows), f"the JSON form was reported: {flows}" + + +@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine") +def test_a_qualified_setting_needs_its_receiver(tmp_path: Path) -> None: + """`marked.setOptions` must not be answered by an unrelated `setOptions`. + + The setting matcher accepted a dotted spec's trailing segment alone, which was harmless while every + shipped setting was a bare name and became load-bearing when the template engines joined the table: an + editor's `setOptions` would have been read as a template engine's. + """ + from openultrasast.cpg.backend import extract_payload + + frontend = __import__("shutil").which("jssrc2cpg") + if frontend is None: + pytest.skip("jssrc2cpg is not installed") + source = tmp_path / "setup.js" + source.write_text( + "function configure(editor) {\n editor.setOptions({ readOnly: false });\n marked.setOptions({ sanitize: false });\n}\n" + ) + cpg = tmp_path / "cpg.bin" + built = subprocess.run([frontend, str(tmp_path), "-o", str(cpg)], capture_output=True, text=True, timeout=600, check=False) + if not cpg.is_file(): + pytest.skip(f"could not build a sample cpg: {(built.stderr or '')[-200:]}") + + requests = tmp_path / "requests.json" + requests.write_text(json.dumps({"0": {"settings": "marked.setOptions", "function": "configure"}})) + done = subprocess.run( + [ + _joern() or "joern", + "--script", + str((QUERIES / "config.sc").resolve()), + "--param", + f"cpgFile={cpg}", + "--param", + f"requestsFile={requests}", + ], + capture_output=True, + text=True, + timeout=900, + check=False, + ) + assert done.returncode == 0, f"config.sc did not run: {(done.stderr or done.stdout or '')[-500:]}" + payload = extract_payload(done.stdout or "") + assert payload is not None, "config.sc produced no parseable payload" + settings = [str(row.get("setting", "")) for row in (payload.get("0") or []) if row.get("setting")] + assert any("marked.setOptions" in row for row in settings), f"the template engine's setting was missed: {settings}" + assert all("editor.setOptions" not in row for row in settings), f"an unrelated setOptions was claimed: {settings}" diff --git a/tests/test_model_config_value.py b/tests/test_model_config_value.py index eb6ee34..445566e 100644 --- a/tests/test_model_config_value.py +++ b/tests/test_model_config_value.py @@ -131,3 +131,86 @@ def test_a_strong_algorithm_yields_no_finding() -> None: spec = config_specs(language="python")["config_secrets"] rows = [{"setting": 'hashlib.new("sha256")', "line": "4", "method": "digest", "literalArgs": ['"sha256"'], "args": ['"sha256"']}] assert verdict(_cpg(rows), spec, function="digest") is None + + +def _javascript_spec(): # type: ignore[no-untyped-def] + """The SHIPPED JavaScript spec, so these read the facts rather than a copy of them.""" + from openultrasast.model.specs import config_specs + + return config_specs(language="javascript")["config_secrets"] + + +def test_disabling_template_escaping_is_entailed() -> None: + """The setting NodeGoat's cross-site scripting turns off, claimed where the evidence is: the value.""" + from openultrasast.model.config_value import verdict + from openultrasast.model.ladder import Rung + + rows = [ + { + "setting": "swig.setDefaults({ autoescape: false })", + "line": "135", + "method": "", + "literalArgs": ["false"], + "args": ["{ autoescape: false }"], + } + ] + answer = verdict(_cpg(rows), _javascript_spec(), function="") + assert answer is not None and answer.rung is Rung.ENTAILED + assert "swig.setDefaults" in answer.witness and "false" in answer.witness + + +def test_enabling_template_escaping_is_not_a_finding() -> None: + """The fix must not be the defect. + + `true` is in the general permissive set, because `cors({origin: true})` is how a permissive CORS policy is + written. Escaping reverses that polarity, and with one shared set this repaired line would have been + reported on exactly the code that repairs it -- the commented-out `autoescape: true` two lines below + NodeGoat's defect. + """ + from openultrasast.model.config_value import verdict + + rows = [ + { + "setting": "swig.setDefaults({ autoescape: true })", + "line": "135", + "method": "", + "literalArgs": ["true"], + "args": ["{ autoescape: true }"], + } + ] + assert verdict(_cpg(rows), _javascript_spec(), function="") is None + + +def test_a_setting_whose_polarity_is_unchanged_still_reports_true() -> None: + """The control for the other direction: scoping must not quiet the settings it was not about.""" + from openultrasast.model.config_value import verdict + from openultrasast.model.ladder import Rung + + rows = [ + { + "setting": "cors({ origin: true })", + "line": "12", + "method": "createApp", + "literalArgs": ["true"], + "args": ["{ origin: true }"], + } + ] + answer = verdict(_cpg(rows), _javascript_spec(), function="createApp") + assert answer is not None and answer.rung is Rung.ENTAILED + + +def test_the_first_setting_in_a_file_is_the_one_reported() -> None: + """One verdict is returned per question, so which row comes first decides what a file reports. + + The line sorted as TEXT, so line 126 came before line 78 and a file's reported setting depended on the + spelling of its line numbers. NodeGoat has a permissive session at line 78 and template escaping disabled + at line 135, and which one surfaced flipped on that comparison. + """ + from openultrasast.model.config_value import verdict + + rows = [ + {"setting": "swig.setDefaults({ autoescape: false })", "line": "135", "method": "m", "literalArgs": ["false"], "args": []}, + {"setting": "cors({ origin: true })", "line": "78", "method": "m", "literalArgs": ["true"], "args": []}, + ] + answer = verdict(_cpg(rows), _javascript_spec(), function="m") + assert answer is not None and "line 78" in answer.witness, answer.witness