diff --git a/src/openultrasast/cpg/backend.py b/src/openultrasast/cpg/backend.py index 97250da..4105683 100644 --- a/src/openultrasast/cpg/backend.py +++ b/src/openultrasast/cpg/backend.py @@ -73,14 +73,39 @@ def extract_payload(stdout: str) -> object | None: if start < 0 or end < 0: return None body = stdout[start + len(BEGIN) : end].strip() - try: - payload: object = json.loads(body) - except json.JSONDecodeError: + # Joern logs to STDOUT, and it does so while the script is running -- so a log line can land INSIDE the + # fence, ahead of the payload. `[INFO ] Attempting to determine flows from empty list of sources.` did + # 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") - return None 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) class CpgResult: """A built CPG and the ways to query it. Opaque: only ``model/`` reads through these. diff --git a/src/openultrasast/cpg/queries/taint.sc b/src/openultrasast/cpg/queries/taint.sc index 44cd18c..017b3f8 100644 --- a/src/openultrasast/cpg/queries/taint.sc +++ b/src/openultrasast/cpg/queries/taint.sc @@ -164,6 +164,22 @@ // 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. + // + // 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 = if (parameterSources != "true") Iterator.empty else if (function.isEmpty) cpg.method.parameter.iterator @@ -171,6 +187,14 @@ 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 // full name (`os.py:.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 // test. Real arguments start at index 1. 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 // rather than a sanitizing call: `execute(sql, params)` binds where `execute(sql + x)` interpolates, and diff --git a/tests/test_cpg_backend.py b/tests/test_cpg_backend.py index 22a6586..bdf3af2 100644 --- a/tests/test_cpg_backend.py +++ b/tests/test_cpg_backend.py @@ -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="") assert _unparsed_files(completed) == () 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]}