mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-16 03:49:26 +00:00
fix(scan): a catch-all function's parameters are not attacker input; field reads need the same field
Two classes the traced PMPro survivors named after 1f91c0b:
- The mapper records every named PHP function as an entry of kind
`function` (3,507 of PMPro's 4,412), and every entry's parameters were
sources, so internal helpers became injections: sendCancelAdminEmail's
$user, a gateway hook callback's $user_id/$morder fired by do_action.
Regions now carry the entry kind; parameters are input only for request
entries (route, parser, privileged surface). Call depth is unchanged, so
such a region is still followed into its callees. JavaScript and Python
emit no `function` entries.
- The object rule is general: a field read continues a flow through the
bare object only if the taint came through the SAME field, or was
assigned to the variable or passed in as a parameter. It now also cuts
$this->Gateway -> this -> $this->user_id (memberorder.php:705).
plausible() memoises the sink test per node and skips the object walk for
paths with no field access (the filter had cost one outlier 101 s -> 160 s).
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01EmM1j46qgKMh3zqD1QMwEo
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
1f91c0b089
commit
da0ee576f8
@@ -302,12 +302,6 @@
|
|||||||
def fieldOf(c: io.shiftleft.codepropertygraph.generated.nodes.Call): String =
|
def fieldOf(c: io.shiftleft.codepropertygraph.generated.nodes.Call): String =
|
||||||
c.argument.l.find(_.argumentIndex == 2).map(_.code.trim).getOrElse("")
|
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 == "<operator>.assignment"
|
|
||||||
case _ => false
|
|
||||||
})
|
|
||||||
|
|
||||||
def assignedTo(id: io.shiftleft.codepropertygraph.generated.nodes.Identifier): Boolean =
|
def assignedTo(id: io.shiftleft.codepropertygraph.generated.nodes.Identifier): Boolean =
|
||||||
id.argumentIndex == 1 && (id.astParent match {
|
id.argumentIndex == 1 && (id.astParent match {
|
||||||
case parent: io.shiftleft.codepropertygraph.generated.nodes.Call => parent.name == "<operator>.assignment"
|
case parent: io.shiftleft.codepropertygraph.generated.nodes.Call => parent.name == "<operator>.assignment"
|
||||||
@@ -324,8 +318,10 @@
|
|||||||
if (j < 0) false // the object itself is where the path starts: it IS the tainted value
|
if (j < 0) false // the object itself is where the path starts: it IS the tainted value
|
||||||
else
|
else
|
||||||
elements(j) match {
|
elements(j) match {
|
||||||
case prior: io.shiftleft.codepropertygraph.generated.nodes.Call if prior.name == "<operator>.fieldAccess" =>
|
// Through the SAME field -- written then read back, or read and read again -- is field-sensitive.
|
||||||
writtenField(prior) && fieldOf(prior) != fieldOf(read)
|
case prior: io.shiftleft.codepropertygraph.generated.nodes.Call
|
||||||
|
if prior.name == "<operator>.fieldAccess" && fieldOf(prior) == fieldOf(read) =>
|
||||||
|
false
|
||||||
case _ =>
|
case _ =>
|
||||||
// How did the value reach the object? Legitimately only by being ASSIGNED to it
|
// 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
|
// (`$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 = {
|
def plausible(flow: io.joern.dataflowengineoss.language.Path): Boolean = {
|
||||||
val elements = flow.elements.l
|
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("<operator") =>
|
||||||
|
familySinkMemo.getOrElseUpdate(c.id, familySinkCall(c))
|
||||||
|
case _ => false
|
||||||
|
}
|
||||||
|
!throughSink && !(elements.exists {
|
||||||
|
case c: io.shiftleft.codepropertygraph.generated.nodes.Call => c.name == "<operator>.fieldAccess"
|
||||||
|
case _ => false
|
||||||
|
} && absorbedByObject(elements))
|
||||||
}
|
}
|
||||||
|
|
||||||
// Framework sources are untrusted wherever they appear. Parameters are untrusted only where the caller has
|
// Framework sources are untrusted wherever they appear. Parameters are untrusted only where the caller has
|
||||||
|
|||||||
@@ -66,6 +66,10 @@ class ScanRegion:
|
|||||||
# Does the project's own build declaration name this file? True when it declares nothing at all --
|
# 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.
|
# silence is not exclusion, and no repository is penalised for a build system we cannot read.
|
||||||
shipped: bool = True
|
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, ...]:
|
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,
|
families=families,
|
||||||
rank=rank,
|
rank=rank,
|
||||||
source="module_scope" if module_body else "entry_point",
|
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
|
# A nameless entry point IS the file. Where a named region already covers that file it adds nothing but
|
||||||
|
|||||||
@@ -559,7 +559,7 @@ def _scan_repository_impl(
|
|||||||
# A repository's parameters are not all attacker input -- but an ENTRY POINT's are, by
|
# 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
|
# definition. That is the trust boundary, and it is the one place both halves of the old
|
||||||
# objection stop applying.
|
# 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,
|
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
|
except Exception as exc: # noqa: BLE001 -- one bad region must not end the scan
|
||||||
@@ -1456,7 +1456,7 @@ def _grouped(
|
|||||||
spec,
|
spec,
|
||||||
function=function,
|
function=function,
|
||||||
file=region.path,
|
file=region.path,
|
||||||
parameter_sources=entry,
|
parameter_sources=_parameters_are_input(region),
|
||||||
call_depth=ENTRY_POINT_CALL_DEPTH if entry else 0,
|
call_depth=ENTRY_POINT_CALL_DEPTH if entry else 0,
|
||||||
hook_callbacks=hook_callbacks,
|
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
|
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:
|
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`."""
|
"""The spec whose arbiter can decide this family. Routing stays a lookup; dispatch stays in `_arbitrate`."""
|
||||||
if family == "access_control":
|
if family == "access_control":
|
||||||
|
|||||||
@@ -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 sizer.rate("output_encoding") >= 54.0
|
||||||
assert abs(sizer.rate("injection") - 1.0) < 1e-9
|
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"
|
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"
|
||||||
|
|||||||
Reference in New Issue
Block a user