diff --git a/src/openultrasast/cpg/queries/taint.sc b/src/openultrasast/cpg/queries/taint.sc index 815fb33..49e73ff 100644 --- a/src/openultrasast/cpg/queries/taint.sc +++ b/src/openultrasast/cpg/queries/taint.sc @@ -302,12 +302,6 @@ def fieldOf(c: io.shiftleft.codepropertygraph.generated.nodes.Call): String = c.argument.l.find(_.argumentIndex == 2).map(_.code.trim).getOrElse("") - def writtenField(c: io.shiftleft.codepropertygraph.generated.nodes.Call): Boolean = - c.argumentIndex == 1 && (c.astParent match { - case parent: io.shiftleft.codepropertygraph.generated.nodes.Call => parent.name == ".assignment" - case _ => false - }) - def assignedTo(id: io.shiftleft.codepropertygraph.generated.nodes.Identifier): Boolean = id.argumentIndex == 1 && (id.astParent match { case parent: io.shiftleft.codepropertygraph.generated.nodes.Call => parent.name == ".assignment" @@ -324,8 +318,10 @@ if (j < 0) false // the object itself is where the path starts: it IS the tainted value else elements(j) match { - case prior: io.shiftleft.codepropertygraph.generated.nodes.Call if prior.name == ".fieldAccess" => - writtenField(prior) && fieldOf(prior) != fieldOf(read) + // Through the SAME field -- written then read back, or read and read again -- is field-sensitive. + case prior: io.shiftleft.codepropertygraph.generated.nodes.Call + if prior.name == ".fieldAccess" && fieldOf(prior) == fieldOf(read) => + false case _ => // How did the value reach the object? Legitimately only by being ASSIGNED to it // (`$data = json_decode($input)`) or passed in as a parameter. An object merely used beside @@ -340,9 +336,21 @@ } } + // Per element, not per path: the same nodes recur across thousands of paths of one sink, and walking each + // path's calls afresh cost one outlier request 101 s -> 160 s. + val familySinkMemo = scala.collection.mutable.Map.empty[Long, Boolean] + def plausible(flow: io.joern.dataflowengineoss.language.Path): Boolean = { val elements = flow.elements.l - !elements.dropRight(1).exists(familySinkCall) && !absorbedByObject(elements) + val throughSink = elements.dropRight(1).exists { + case c: io.shiftleft.codepropertygraph.generated.nodes.Call if !c.name.startsWith(" + familySinkMemo.getOrElseUpdate(c.id, familySinkCall(c)) + case _ => false + } + !throughSink && !(elements.exists { + case c: io.shiftleft.codepropertygraph.generated.nodes.Call => c.name == ".fieldAccess" + case _ => false + } && absorbedByObject(elements)) } // Framework sources are untrusted wherever they appear. Parameters are untrusted only where the caller has diff --git a/src/openultrasast/model/regions.py b/src/openultrasast/model/regions.py index 0cf5570..6e68c48 100644 --- a/src/openultrasast/model/regions.py +++ b/src/openultrasast/model/regions.py @@ -66,6 +66,10 @@ class ScanRegion: # Does the project's own build declaration name this file? True when it declares nothing at all -- # silence is not exclusion, and no repository is penalised for a build system we cannot read. shipped: bool = True + # What the mapper called the entry: `route`, `parser`, `privileged_surface`, ... or `function`, which is + # its catch-all for a named function with no evidence of receiving a request. Only the scan's parameter + # rule reads it, because only it depends on whether a parameter carries request data. + entry_kind: str = "" def regions_for(entries: Sequence[object], targets: Sequence[object], *, shipped: frozenset[str] | None = None) -> tuple[ScanRegion, ...]: @@ -126,6 +130,7 @@ def regions_for(entries: Sequence[object], targets: Sequence[object], *, shipped families=families, rank=rank, source="module_scope" if module_body else "entry_point", + entry_kind=str(getattr(entry, "kind", "") or ""), ) # A nameless entry point IS the file. Where a named region already covers that file it adds nothing but diff --git a/src/openultrasast/model/scan.py b/src/openultrasast/model/scan.py index 5b7f548..708ef7b 100644 --- a/src/openultrasast/model/scan.py +++ b/src/openultrasast/model/scan.py @@ -559,7 +559,7 @@ def _scan_repository_impl( # A repository's parameters are not all attacker input -- but an ENTRY POINT's are, by # definition. That is the trust boundary, and it is the one place both halves of the old # objection stop applying. - parameter_sources=_is_entry_point(region), + parameter_sources=_parameters_are_input(region), call_depth=ENTRY_POINT_CALL_DEPTH if _is_entry_point(region) else 0, ) except Exception as exc: # noqa: BLE001 -- one bad region must not end the scan @@ -1456,7 +1456,7 @@ def _grouped( spec, function=function, file=region.path, - parameter_sources=entry, + parameter_sources=_parameters_are_input(region), call_depth=ENTRY_POINT_CALL_DEPTH if entry else 0, hook_callbacks=hook_callbacks, ), @@ -1502,6 +1502,20 @@ def _is_entry_point(region: ScanRegion) -> bool: return region.source == "entry_point" and bool(region.function) and region.function != MODULE_SCOPE and region.shipped +def _parameters_are_input(region: ScanRegion) -> bool: + """Are this region's own parameters attacker input? Only where the mapper saw a request arrive. + + `function` is the mapper's catch-all: every named function it found, with no evidence of a request. On a + WordPress plugin that is 3,507 of 4,412 entries, and treating their parameters as input made every internal + helper a source -- `sendCancelAdminEmail($user, $old_level_id)`, a gateway's + `pmpro_checkout_before_change_membership_level($user_id, $morder)` fired by `do_action` with internal ids. + Adjudicated on PMPro 2026-09-23, those were false SQL injections. Routes, parsers and privileged surfaces + keep the rule; JavaScript and Python emit no `function` entries at all. The region is still followed into + its callees -- call depth stays with `_is_entry_point` -- only its parameters stop being sources. + """ + return _is_entry_point(region) and region.entry_kind != "function" + + def _spec_for(family: str, language: str) -> ArbiterSpec | None: """The spec whose arbiter can decide this family. Routing stays a lookup; dispatch stays in `_arbitrate`.""" if family == "access_control": diff --git a/tests/test_model_scope.py b/tests/test_model_scope.py index 85f5cb9..b43281e 100644 --- a/tests/test_model_scope.py +++ b/tests/test_model_scope.py @@ -375,3 +375,16 @@ def test_a_killed_culprit_makes_its_family_dearer_and_nothing_else() -> None: assert sizer.rate("output_encoding") >= 54.0 assert abs(sizer.rate("injection") - 1.0) < 1e-9 assert sizer.fixed == 25.0, "a killed portion must not be read as a fixed-start sample" + + +def test_only_a_request_entry_treats_its_parameters_as_attacker_input() -> None: + """The mapper's `function` kind is its catch-all; its parameters carry whatever the caller had.""" + from openultrasast.model import scan + + route = ScanRegion("api.php", "handle", "php", ("injection",), 1.0, "entry_point", entry_kind="route") + helper = ScanRegion("mail.php", "sendCancelAdminEmail", "php", ("injection",), 0.3, "entry_point", entry_kind="function") + params = scan._grouped([("0", route, scan._spec_for("injection", "php")), ("1", helper, scan._spec_for("injection", "php"))])["taint"] + assert params["0"]["parameterSources"] == "true" + assert params["1"]["parameterSources"] == "false" + # Still followed into its callees: only its own parameters stop being sources. + assert params["1"]["callDepth"] == params["0"]["callDepth"] != "0"