mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-16 03:49:26 +00:00
fix(cpg): one method with no sources must not cost the whole batch its answers
`reachableByFlows` on an empty source list is not a no-op. Joern answers it by
logging
[INFO ] Attempting to determine flows from empty list of sources.
to STDOUT -- while the script is running, so the line lands between the fence
markers, ahead of the payload. `json.loads` then fails on the whole document
and the driver reports `query_failed` for every request in the batch. One
method with nothing to trace from silently cost the answers for all 205 of
the others.
Fixed at both ends, because either alone leaves the seam brittle:
* taint.sc skips the flow query when there is no source to trace from, which
is also the correct thing to do with the question.
* `extract_payload` starts a decode at each line that opens a document and
keeps the first that parses. Trimming to the outermost brackets is the
obvious approach and the wrong one -- `[INFO ]` brings its own bracket.
Found while testing whether `$this` belongs in parameter sources. It does not
belong there on the argument -- the receiver is the object, not request data --
and excluding it cuts Paid Memberships Pro's order class from 16 entailed
findings to 6, all sourced from a named parameter, and makes the 2.9.7/2.9.8
pair separate cleanly.
It also loses CVE-2023-6559: MW WP Form's `_delete_files()` takes no
parameters at all, and the attacker-controlled path arrives as
`$this->attachments`, assigned by another method. So the trade is a VERIFIED
CVE against findings merely suspected of being false, and it is refused. The
receiver stays, with the measurement written next to it. The real defect is
field-sensitivity -- a field should be tainted where it is assigned -- and
deleting the receiver removes the symptom by removing the evidence.
841 tests pass; 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
e0442d146c
commit
45cf43e625
@@ -73,14 +73,39 @@ def extract_payload(stdout: str) -> object | None:
|
|||||||
if start < 0 or end < 0:
|
if start < 0 or end < 0:
|
||||||
return None
|
return None
|
||||||
body = stdout[start + len(BEGIN) : end].strip()
|
body = stdout[start + len(BEGIN) : end].strip()
|
||||||
try:
|
# Joern logs to STDOUT, and it does so while the script is running -- so a log line can land INSIDE the
|
||||||
payload: object = json.loads(body)
|
# fence, ahead of the payload. `[INFO ] Attempting to determine flows from empty list of sources.` did
|
||||||
except json.JSONDecodeError:
|
# exactly that and cost a 205-request batch every one of its answers, reported as `query_failed`. The
|
||||||
|
# payload is always one JSON object or array, so trim to its own brackets rather than trusting the fence
|
||||||
|
# to contain nothing else.
|
||||||
|
payload = _decoded_json(body)
|
||||||
|
if payload is None:
|
||||||
logger.debug("cpg query payload was not valid JSON")
|
logger.debug("cpg query payload was not valid JSON")
|
||||||
return None
|
|
||||||
return payload
|
return payload
|
||||||
|
|
||||||
|
|
||||||
|
def _decoded_json(body: str) -> object | None:
|
||||||
|
"""The first complete JSON document in ``body``, ignoring whatever Joern logged around it.
|
||||||
|
|
||||||
|
Trimming to the outermost brackets is the obvious approach and the wrong one: a log line is `[INFO ] ...`,
|
||||||
|
so its own bracket comes first. This starts a decode at each line that opens a document and keeps the
|
||||||
|
first that parses, which is exactly one line of work when nothing was logged.
|
||||||
|
"""
|
||||||
|
decoder = json.JSONDecoder()
|
||||||
|
offset = 0
|
||||||
|
for line in body.splitlines(keepends=True):
|
||||||
|
if line.lstrip().startswith(("{", "[")):
|
||||||
|
try:
|
||||||
|
payload, _ = decoder.raw_decode(body, offset + len(line) - len(line.lstrip()))
|
||||||
|
except ValueError:
|
||||||
|
pass
|
||||||
|
else:
|
||||||
|
decoded: object = payload
|
||||||
|
return decoded
|
||||||
|
offset += len(line)
|
||||||
|
return None
|
||||||
|
|
||||||
|
|
||||||
@dataclass(frozen=True)
|
@dataclass(frozen=True)
|
||||||
class CpgResult:
|
class CpgResult:
|
||||||
"""A built CPG and the ways to query it. Opaque: only ``model/`` reads through these.
|
"""A built CPG and the ways to query it. Opaque: only ``model/`` reads through these.
|
||||||
|
|||||||
@@ -164,6 +164,22 @@
|
|||||||
// with them on. Scoping matters as much as enabling: a callback's own parameters (`err`, `stats`) are not
|
// 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.
|
// 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.
|
// Off by default: outside a labeled function, treating every parameter as untrusted is not sound.
|
||||||
|
//
|
||||||
|
// The RECEIVER is INCLUDED, and it was measured rather than assumed. `$this` is a parameter in the CPG --
|
||||||
|
// index 0 of every PHP method -- and excluding it is tempting: on Paid Memberships Pro's order class, 12 of
|
||||||
|
// 16 entailed findings are `this -> $wpdb->...`, and one of them survives into 2.9.8 at a query whose every
|
||||||
|
// fragment is `esc_sql`-wrapped, reaching the sink through `$this->sqlQuery`. Excluding it cuts that file
|
||||||
|
// from 16 findings to 6, all sourced from a named parameter, and makes the 2.9.7/2.9.8 pair separate
|
||||||
|
// cleanly.
|
||||||
|
//
|
||||||
|
// It also loses CVE-2023-6559. MW WP Form's `_delete_files()` takes NO parameters: the attacker-controlled
|
||||||
|
// path arrives as `$this->attachments`, assigned by another method entirely. With the receiver excluded
|
||||||
|
// that finding goes to zero on the vulnerable side.
|
||||||
|
//
|
||||||
|
// So the trade is a verified CVE against findings merely SUSPECTED of being false, and it is refused. The
|
||||||
|
// real defect is field-sensitivity -- `$this->attachments` should be tainted where it is assigned, not by
|
||||||
|
// the receiver being tainted everywhere -- and deleting the receiver removes the symptom by removing the
|
||||||
|
// evidence.
|
||||||
def parameterNodes =
|
def parameterNodes =
|
||||||
if (parameterSources != "true") Iterator.empty
|
if (parameterSources != "true") Iterator.empty
|
||||||
else if (function.isEmpty) cpg.method.parameter.iterator
|
else if (function.isEmpty) cpg.method.parameter.iterator
|
||||||
@@ -171,6 +187,14 @@
|
|||||||
|
|
||||||
def sourceNodes = frameworkSources.l.iterator ++ parameterNodes
|
def sourceNodes = frameworkSources.l.iterator ++ parameterNodes
|
||||||
|
|
||||||
|
// Whether there is anything to trace from at all. Asking `reachableByFlows` with an empty source list is
|
||||||
|
// not merely wasted work: Joern answers it by logging "Attempting to determine flows from empty list of
|
||||||
|
// sources." to STDOUT, which lands between the fence markers and makes the whole batch's JSON unparseable.
|
||||||
|
// The driver then reports `query_failed` for every request in the batch, so one method with no sources
|
||||||
|
// silently costs the answers for all two hundred of the others. Excluding the receiver made that common:
|
||||||
|
// a method whose only parameter is `$this` now has none.
|
||||||
|
lazy val hasSources = sourceNodes.hasNext
|
||||||
|
|
||||||
// A sink call: matched by short name (`system`), by leading code (`os.system(...)`), or by resolved
|
// A sink call: matched by short name (`system`), by leading code (`os.system(...)`), or by resolved
|
||||||
// full name (`os.py:<module>.system`), so both bare and dotted forms in the spec hit.
|
// full name (`os.py:<module>.system`), so both bare and dotted forms in the spec hit.
|
||||||
//
|
//
|
||||||
@@ -204,7 +228,7 @@
|
|||||||
// makes a one-argument interpolated call look like a two-argument bound one -- the exact inversion of the
|
// 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.
|
// test. Real arguments start at index 1.
|
||||||
val realArgs = sink.argument.argumentIndexGt(0).l
|
val realArgs = sink.argument.argumentIndexGt(0).l
|
||||||
val flows = sink.argument.reachableByFlows(sourceNodes).l
|
val flows = if (hasSources) sink.argument.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
|
// 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
|
// rather than a sanitizing call: `execute(sql, params)` binds where `execute(sql + x)` interpolates, and
|
||||||
|
|||||||
@@ -368,3 +368,15 @@ def test_a_build_with_nothing_dropped_reports_no_unparsed_files() -> None:
|
|||||||
completed = subprocess.CompletedProcess(args=[], returncode=0, stdout="Successfully wrote graph", stderr="")
|
completed = subprocess.CompletedProcess(args=[], returncode=0, stdout="Successfully wrote graph", stderr="")
|
||||||
assert _unparsed_files(completed) == ()
|
assert _unparsed_files(completed) == ()
|
||||||
assert _unparsed_files(None) == ()
|
assert _unparsed_files(None) == ()
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_joern_log_line_inside_the_fence_does_not_destroy_the_payload() -> None:
|
||||||
|
"""Joern logs to stdout while the script runs, so a log line can land ahead of the payload.
|
||||||
|
|
||||||
|
`[INFO ] Attempting to determine flows from empty list of sources.` did, and because the batch is one
|
||||||
|
JSON document the driver lost all 205 answers in it and reported `query_failed` for every one.
|
||||||
|
"""
|
||||||
|
from openultrasast.cpg.backend import BEGIN, END, extract_payload
|
||||||
|
|
||||||
|
noisy = f'{BEGIN}\n[INFO ] Attempting to determine flows from empty list of sources.\n{{"0": [], "1": [1]}}\n{END}\n'
|
||||||
|
assert extract_payload(noisy) == {"0": [], "1": [1]}
|
||||||
|
|||||||
Reference in New Issue
Block a user