mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-16 03:49:26 +00:00
fix(access-control): decide the sibling scope, and see identity a destructuring spelled away
Two of NodeGoat's three corroborated claims were correct code. `const { userId } = req.session`
lowers to a temporary and then a field read, so nothing in the graph spells session.userId by the
time the value reaches dao.getByUserId(userId, ...), and the constraint the handler really applies
was invisible.
The arbiter now walks back three hops over the method's own assignments from the names the
operation is GIVEN, and req.session joins the identity sources. The token is only safe because of
that requirement: what discharges an object-level obligation is that the value passed to the
operation comes from the authenticated context, not that a session is mentioned nearby. A handler
that reads the session for logging and keys its query on a path parameter is the bug this family
exists for, and it stays unguarded.
Two exclusions the measurement forced, each caught by a page whose whole purpose is the leak it
demonstrates.
- A callback is not a constraint. Taking identifiers from the whole argument subtree credited
NodeGoat's memo listing with a discharge, because memosDAO.getAllMemos((err, docs) => ... userId
...) reads every memo in the collection and only RENDERS the session's user. Method references
are excluded from the argument names.
- An assignment is not a dominating guard. The session read dominates the query, so that clause
credited it instead and the memo listing went quiet again. An assignment is a read of the
authenticated context, not a check applied to it, so operator calls are excluded there -- the
same exclusion the sink matcher needed. The leak stays reported.
On NodeGoat displayContributions and displayProfile go silent, the allocations insecure direct
object reference, both benefits pages and the memo listing stay, and the login and signup handlers
become honestly unguarded rather than accidentally cleared by a callback's text. Six corroborated
where there were seven, and a better six. VAmPI is unchanged at two entailed findings.
The sibling scope is decided by measurement and kept at the file. On NodeGoat the region's own
file entails two handlers, both public routes and both false, and no true defect at all. Every
handler in the application entails six: four true, and the same two public routes. Both scopes
carry the same false positives, because neither can tell a public route from a protected one --
a semantic question that is already the corroborated band's residual. With the carried rung capped
the scope changes no verdict, only the witness, so widening it would buy a repository-wide walk
per question and no precision. A public-route discriminator would change that; the file scope is
not what holds this family back.
1,415 tests pass, sixteen skipped; thirteen Joern-gated query tests pass in the image.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01EmM1j46qgKMh3zqD1QMwEo
This commit is contained in:
co-authored by
Claude Opus 5
parent
c0427e6ceb
commit
e6bdac46e2
@@ -324,6 +324,16 @@ def _route_and_dao(tmp_path: Path) -> Path:
|
||||
" this.displayMine = (req, res) => {\n"
|
||||
" return notesDAO.getOwned(req.user.id, (err, notes) => res.json(notes));\n"
|
||||
" };\n"
|
||||
"\n"
|
||||
" this.displayDestructured = (req, res) => {\n"
|
||||
" const { userId } = req.session;\n"
|
||||
" return notesDAO.getOwned(userId, (err, notes) => res.json(notes));\n"
|
||||
" };\n"
|
||||
"\n"
|
||||
" this.displayEverything = (req, res) => {\n"
|
||||
" const { userId } = req.session;\n"
|
||||
" return notesDAO.getAll((err, notes) => res.json({ userId, notes }));\n"
|
||||
" };\n"
|
||||
"}\n"
|
||||
"\n"
|
||||
"module.exports = NotesHandler;\n"
|
||||
@@ -339,6 +349,10 @@ def _route_and_dao(tmp_path: Path) -> Path:
|
||||
" this.getOwned = (userId, callback) => {\n"
|
||||
" notes.findOne({ owner: userId }, callback);\n"
|
||||
" };\n"
|
||||
"\n"
|
||||
" this.getAll = (callback) => {\n"
|
||||
" notes.find({}).toArray(callback);\n"
|
||||
" };\n"
|
||||
"}\n"
|
||||
"\n"
|
||||
"module.exports.NotesDAO = NotesDAO;\n"
|
||||
@@ -471,3 +485,34 @@ def test_only_a_collection_read_is_an_obligated_find(tmp_path: Path) -> None:
|
||||
lines = sorted(int(str(row["opLine"])) for row in rows)
|
||||
# 3 the object literal, 4 the whole collection, 5 the builder call.
|
||||
assert lines == [3, 4, 5], f"the wrong find shapes were claimed: {[(r['opLine'], r['operation']) for r in rows]}"
|
||||
|
||||
|
||||
@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine")
|
||||
def test_a_destructured_identity_discharges_what_it_reaches(tmp_path: Path) -> None:
|
||||
"""Reading the session discharges the query it constrains, and nothing else.
|
||||
|
||||
`const { userId } = req.session` lowers to a temporary, so nothing in the graph spells `session.userId` by
|
||||
the time the value reaches the query, and two of NodeGoat's corroborated claims were correct code put to a
|
||||
judge for that reason. The second half is the measurement that keeps it honest: NodeGoat's memo listing
|
||||
reads the same session and RENDERS it while its query reads every memo in the collection, which is the leak
|
||||
that page exists to demonstrate. So the value has to reach the operation's own arguments.
|
||||
"""
|
||||
from openultrasast.cpg.backend import CpgResult
|
||||
from openultrasast.model.dominance import verdict
|
||||
from openultrasast.model.ladder import Rung
|
||||
from openultrasast.model.specs import dominance_specs
|
||||
|
||||
root = _route_and_dao(tmp_path)
|
||||
spec = dominance_specs(language="javascript")["access_control"]
|
||||
|
||||
def answer(function: str): # type: ignore[no-untyped-def]
|
||||
rows = _dominance_rows(root, function=function, file="app/routes/notes.js")
|
||||
cpg = CpgResult(cpg_path=tmp_path / "cpg.bin", run=lambda q, p: rows)
|
||||
return verdict(cpg, spec, function=function, file="app/routes/notes.js")
|
||||
|
||||
constrained = answer("displayDestructured")
|
||||
assert constrained is None, f"a query constrained by the session's own user was reported: {constrained}"
|
||||
|
||||
leaked = answer("displayEverything")
|
||||
assert leaked is not None, "a query that reads the whole collection was not reported"
|
||||
assert leaked.rung is Rung.CORROBORATED, f"{leaked.rung} from {leaked.witness}"
|
||||
|
||||
Reference in New Issue
Block a user