diff --git a/src/openultrasast/cpg/backend.py b/src/openultrasast/cpg/backend.py index ec75f59..9fddbeb 100644 --- a/src/openultrasast/cpg/backend.py +++ b/src/openultrasast/cpg/backend.py @@ -181,7 +181,7 @@ FRONTEND_BUILD_ATTEMPTS = 4 # across 2,500 requests made a 42.5MB request file of which 41.7MB was the same string over and over. Joern # parsed that with ujson inside a 2GB heap and the whole batch died in ForkJoinPool, which the driver # correctly reported as `query_failed` for all 2,500 regions -- a whole-repository scan that decided nothing. -_SHARED_REQUEST_FIELDS = ("hookCallbacks", "dispatchApply", "contextFilter", "contextPaths") +_SHARED_REQUEST_FIELDS = ("hookCallbacks", "dispatchApply", "dispatchValue", "contextFilter", "contextPaths") Runner = Callable[..., subprocess.CompletedProcess[str]] diff --git a/src/openultrasast/cpg/queries/taint.sc b/src/openultrasast/cpg/queries/taint.sc index 0a6b26f..ba800e2 100644 --- a/src/openultrasast/cpg/queries/taint.sc +++ b/src/openultrasast/cpg/queries/taint.sc @@ -52,6 +52,7 @@ evidenceOnly: String = "false", hookCallbacks: String = "", dispatchApply: String = "", + dispatchValue: String = "", contextFilter: String = "", contextPaths: String = "", requests: String = "", @@ -91,6 +92,7 @@ sanitizersS: String, hookCallbacksS: String, dispatchApplyS: String, + dispatchValueS: String, functionS: String, paramSrc: String, fieldParamSrc: String, @@ -137,7 +139,17 @@ // Framework sources: a call whose code contains one of the patterns. Matching on `code` rather than a // method name keeps `request.args["x"]` (an indexAccess over a fieldAccess) and `req.body.name` both // reachable without a per-framework rule for each. - def frameworkSources = cpg.call.filter(c => sourcePatterns.exists(p => c.code.contains(p))) + // A call IS a source when the pattern names the call itself -- its callee or receiver, or the access it is + // (`$_GET["c"]`, `req.body.x`) -- not when the pattern merely appears inside an argument. Matched on the + // whole text, `apply_filters("h", md5("x"), $_GET["c"])` and `sanitize_text_field($_POST["x"])` were + // sources themselves, so a filter's return was request data whatever it returned. The access inside the + // argument is a source in its own right; nothing that reaches a sink through it is lost. + def isSourceCall(c: io.shiftleft.codepropertygraph.generated.nodes.Call): Boolean = { + val named = if (c.name.startsWith(" named.contains(p)) + } + + def frameworkSources = cpg.call.filter(isSourceCall) // The labeled methods, and the region they lexically own. A closure defined inside the labeled function -- // the callback passed to `fs.stat`, say -- has its own synthetic method (`0`) whose astParentFullName @@ -341,6 +353,34 @@ // path's calls afresh cost one outlier request 101 s -> 160 s. val familySinkMemo = scala.collection.mutable.Map.empty[Long, Boolean] + // A registry read returns ONE of its arguments -- the dispatched value -- or nothing at all, per the facts' + // `returns`/`value_arg`. A path that enters such a call through any other argument has used the engine's + // default "every argument reaches the return", which is how `$this`, passed as filter context, turned a + // random md5 fragment into an SQL injection. + val dispatchValues: Map[String, Int] = split(dispatchValueS).flatMap { entry => + entry.split(":") match { + case Array(name, position) => scala.util.Try(name.trim -> position.trim.toInt).toOption + case _ => None + } + }.toMap + + def offValueDispatch(elements: List[io.shiftleft.codepropertygraph.generated.nodes.AstNode]): Boolean = + dispatchValues.nonEmpty && elements.indices.exists { k => + elements(k) match { + case call: io.shiftleft.codepropertygraph.generated.nodes.Call if k > 0 && dispatchValues.contains(call.name) => + val position = dispatchValues(call.name) + // The argument the path ENTERED by, which is the first of the run of this call's arguments right + // before it: the engine hops between sibling arguments (`$_GET["c"]` -> `md5("x")` -> the call), + // so the one immediately before the call can be the value argument when the data came in as context. + val arguments = call.argument.l.map(a => a.id -> a).toMap + var j = k - 1 + while (j >= 0 && arguments.contains(elements(j).id)) j -= 1 + if (j == k - 1) false + else position == 0 || arguments(elements(j + 1).id).argumentIndex != position + case _ => false + } + } + def plausible(flow: io.joern.dataflowengineoss.language.Path): Boolean = { val elements = flow.elements.l val throughSink = elements.dropRight(1).exists { @@ -348,7 +388,7 @@ familySinkMemo.getOrElseUpdate(c.id, familySinkCall(c)) case _ => false } - !throughSink && !(elements.exists { + !throughSink && !offValueDispatch(elements) && !(elements.exists { case c: io.shiftleft.codepropertygraph.generated.nodes.Call => c.name == ".fieldAccess" case _ => false } && absorbedByObject(elements)) @@ -362,7 +402,7 @@ (sourcesS, fieldParamSrc, fileName), { val methods = methodsIn(fileName) val framework: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] = - methods.flatMap(_.ast.isCall.filter(c => sourcePatterns.exists(p => c.code.contains(p))).l) + methods.flatMap(_.ast.isCall.filter(isSourceCall).l) val params: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] = if (fieldParamSrc == "true") methods.flatMap(_.parameter.l) else Nil framework ++ params @@ -466,7 +506,7 @@ // and only the sink is elsewhere. Task 5.13 has the numbers. def hookSeedsOf(callback: io.shiftleft.codepropertygraph.generated.nodes.Method) = { val framework: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] = - callback.ast.isCall.filter(c => sourcePatterns.exists(p => c.code.contains(p))).l + callback.ast.isCall.filter(isSourceCall).l val fields: List[io.shiftleft.codepropertygraph.generated.nodes.CfgNode] = callback.ast.isCall.nameExact(FIELD_ACCESS).l.filter(r => taintedPrefixes(r.code.trim, callback.filename)) framework ++ fields @@ -1039,6 +1079,7 @@ // -region path. if (field("hookCallbacks").nonEmpty) field("hookCallbacks") else hookCallbacks, if (field("dispatchApply").nonEmpty) field("dispatchApply") else dispatchApply, + if (field("dispatchValue").nonEmpty) field("dispatchValue") else dispatchValue, field("function"), paramSrc, fieldParamSrc, @@ -1063,7 +1104,7 @@ } else { println("---OUSAST-CPG-BEGIN---") println( - ujson.write(ujson.Arr(rowsFor(sources, sinks, sanitizers, hookCallbacks, dispatchApply, function, parameterSources, parameterSources, fieldSources, evidenceOnly, file, callDepth, boundedSinks): _*)) + ujson.write(ujson.Arr(rowsFor(sources, sinks, sanitizers, hookCallbacks, dispatchApply, dispatchValue, function, parameterSources, parameterSources, fieldSources, evidenceOnly, file, callDepth, boundedSinks): _*)) ) } println("---OUSAST-CPG-END---") diff --git a/src/openultrasast/model/specs.py b/src/openultrasast/model/specs.py index dc1b4df..87bcbfa 100644 --- a/src/openultrasast/model/specs.py +++ b/src/openultrasast/model/specs.py @@ -115,6 +115,9 @@ class TaintSpec: # The calls that read a value back out of a string-keyed registry (`apply_filters`, `do_action`). Facts, # not constants, so a second framework is a row in a TOML rather than an edit to the query. dispatch_apply: tuple[str, ...] = () + # Which argument of each apply call its RESULT comes from, as `call:position`; position 0 means the call + # returns nothing a flow can carry. Only calls a fact describes appear here. + dispatch_value: tuple[str, ...] = () @dataclass(frozen=True) @@ -243,6 +246,11 @@ def taint_specs( return bool(getattr(fact, "parameterized", False)) or getattr(fact, "literal_format_arg", None) is not None dispatch_apply = tuple(sorted({call for fact in scoped.dispatches for call in fact.apply})) + dispatch_value = tuple( + sorted( + {f"{call}:{fact.value_arg if call in fact.returns else 0}" for fact in scoped.dispatches if fact.returns for call in fact.apply} + ) + ) sanitizers = tuple(sorted({call for fact in scoped.sanitizers if not _is_shape(fact) for call in fact.calls})) safe_shapes = tuple(sorted({call for fact in scoped.sanitizers if _is_shape(fact) for call in fact.calls})) by_family: dict[str, set[str]] = {} @@ -266,6 +274,7 @@ def taint_specs( bounded_sinks=tuple(sorted(bounded.get(family_id, ()))), safe_shape_sinks=safe_shapes, dispatch_apply=dispatch_apply, + dispatch_value=dispatch_value, ) for family_id, calls in sorted(by_family.items()) } diff --git a/src/openultrasast/model/taint.py b/src/openultrasast/model/taint.py index b1a612c..1818798 100644 --- a/src/openultrasast/model/taint.py +++ b/src/openultrasast/model/taint.py @@ -59,6 +59,7 @@ def request_params( # The registry's own vocabulary, from the fact tables. Without it the query would have to know what # `apply_filters` is, which makes a general mechanism into a WordPress feature. "dispatchApply": spec.dispatch_apply, + "dispatchValue": spec.dispatch_value, } diff --git a/src/openultrasast/ruleset/semantic/php.toml b/src/openultrasast/ruleset/semantic/php.toml index 10f554d..8bd1345 100644 --- a/src/openultrasast/ruleset/semantic/php.toml +++ b/src/openultrasast/ruleset/semantic/php.toml @@ -208,6 +208,11 @@ parameterized = true id = "wordpress_hooks" register = ["add_action", "add_filter", "add_shortcode"] apply = ["apply_filters", "apply_filters_ref_array", "do_action", "do_action_ref_array"] +# A filter returns its VALUE argument, filtered; its further arguments are context and never the result, and +# an action returns nothing. Without this, `apply_filters('pmpro_random_code', $code, $this)` returned +# whatever `$this` carried, and a random md5 fragment became an SQL injection. +returns = ["apply_filters", "apply_filters_ref_array"] +value_arg = 2 # What a PHP repository's paths say about scope. `vendored` trees are out of the scan and out of the graph # (a bundled library is a separate unit); `tests` paths stay in scope but are not product, so their diff --git a/src/openultrasast/semantic/facts.py b/src/openultrasast/semantic/facts.py index 5ff534e..079afcc 100644 --- a/src/openultrasast/semantic/facts.py +++ b/src/openultrasast/semantic/facts.py @@ -72,6 +72,14 @@ class DispatchFact: register: tuple[str, ...] apply: tuple[str, ...] language: str + # What an apply call RETURNS. `returns` names the apply calls whose result is the dispatched value, and + # `value_arg` is that value's argument position (1-based, receiver excluded): WordPress's + # `apply_filters('hook', $value, ...$context)` returns the filtered `$value`, and its context arguments + # never become the result. An apply call not named in `returns` returns nothing a flow can carry + # (`do_action`). Without this the engine let EVERY argument reach the return: PMPro's random order code + # came out "tainted" because `$this` was passed as filter context. Empty keeps the engine's default. + returns: tuple[str, ...] = () + value_arg: int = 0 @dataclass(frozen=True) @@ -164,11 +172,30 @@ def _dispatches(value: object, language: str, path: Path) -> list[DispatchFact]: register=_strings(item.get("register"), "register", path), apply=_strings(item.get("apply"), "apply", path), language=language, + returns=_returning(item, path), + value_arg=_value_arg(item, path), ) ) return facts +def _returning(item: dict[str, object], path: Path) -> tuple[str, ...]: + returns = _strings(item.get("returns", []), "returns", path) + unknown = set(returns) - set(_strings(item.get("apply"), "apply", path)) + if unknown: + raise FactLoadError(f"dispatch {item.get('id')!r} in {path}: returns names calls that are not apply calls: {sorted(unknown)}") + return returns + + +def _value_arg(item: dict[str, object], path: Path) -> int: + value = item.get("value_arg", 0) + if not isinstance(value, int) or isinstance(value, bool) or value < 0: + raise FactLoadError(f"dispatch {item.get('id')!r} in {path}: value_arg must be a non-negative integer") + if item.get("returns") and value < 1: + raise FactLoadError(f"dispatch {item.get('id')!r} in {path}: returns needs the value's argument position") + return value + + def _layouts(value: object, language: str, path: Path) -> list[LayoutFact]: facts: list[LayoutFact] = [] for item in _items(value): diff --git a/tests/test_cpg_queries_compile.py b/tests/test_cpg_queries_compile.py index 42b9716..4354527 100644 --- a/tests/test_cpg_queries_compile.py +++ b/tests/test_cpg_queries_compile.py @@ -745,3 +745,60 @@ def test_a_path_through_a_query_result_or_a_whole_object_is_not_an_injection(tmp assert lines["c"] == [], f"an argument into an object's method, its field read: {lines['c']}" assert lines["d"] == [18], f"a member of a decoded request value must still be found: {lines['d']}" assert lines["e"] == [20], f"the field that was written, read back, must still be found: {lines['e']}" + + +@pytest.mark.skipif(_joern() is None, reason="joern is not installed on this machine") +def test_a_filter_returns_its_value_argument_not_its_context(tmp_path: Path) -> None: + """`apply_filters('h', $value, ...$context)` returns the filtered value. Request data passed as context + made PMPro's random md5 order code an SQL injection; request data passed as the value must still flow.""" + from openultrasast.cpg.backend import extract_payload + + frontend = __import__("shutil").which("php2cpg") + if frontend is None: + pytest.skip("php2cpg is not installed") + src = tmp_path / "src" + src.mkdir() + (src / "app.php").write_text( + "query("DELETE FROM t WHERE code = \'" . $code . "\'"); }\n' + "function value_handler() { global $wpdb; $v = apply_filters('h', $_GET['v'], 'context');\n" + ' return $wpdb->query("DELETE FROM t WHERE code = \'" . $v . "\'"); }\n' + ) + cpg = tmp_path / "cpg.bin" + built = subprocess.run([frontend, str(src), "-o", str(cpg)], capture_output=True, text=True, timeout=600, check=False) + if not cpg.is_file(): + pytest.skip(f"could not build a sample cpg: {(built.stderr or '')[-200:]}") + common = { + "sources": "$_GET", + "sinks": "$wpdb->query", + "file": "app.php", + "dispatchApply": "apply_filters,do_action", + "dispatchValue": "apply_filters:2,do_action:0", + } + requests = tmp_path / "requests.json" + requests.write_text( + json.dumps({"context": {**common, "function": "context_handler"}, "value": {**common, "function": "value_handler"}}) + ) + done = subprocess.run( + [ + _joern() or "joern", + "--script", + str((QUERIES / "taint.sc").resolve()), + "--param", + f"cpgFile={cpg}", + "--param", + f"requestsFile={requests}", + ], + capture_output=True, + text=True, + timeout=900, + check=False, + cwd=str(tmp_path), + ) + assert done.returncode == 0, (done.stderr or done.stdout or "")[-500:] + payload = extract_payload(done.stdout or "") + assert isinstance(payload, dict) + flows = {rid: [r for r in payload[rid] if isinstance(r, dict) and r.get("sourceKind")] for rid in ("context", "value")} + assert not flows["context"], f"filter context became its result: {flows['context']}" + assert flows["value"], "the filtered value no longer carries request data" diff --git a/tests/test_dispatch_value.py b/tests/test_dispatch_value.py new file mode 100644 index 0000000..8bd676f --- /dev/null +++ b/tests/test_dispatch_value.py @@ -0,0 +1,36 @@ +"""A registry read returns its VALUE argument, not its context: a fact, loaded and checked.""" + +import pytest + +from openultrasast.model.specs import taint_specs +from openultrasast.semantic.facts import FactLoadError, load_facts + + +def _write(tmp_path, dispatch: str) -> None: + (tmp_path / "php.toml").write_text('language = "php"\n[[source]]\nid = "g"\npatterns = ["$_GET"]\n' + dispatch) + + +def test_the_shipped_facts_say_which_argument_a_filter_returns() -> None: + spec = taint_specs(language="php")["injection"] + assert "apply_filters:2" in spec.dispatch_value and "do_action:0" in spec.dispatch_value + + +def test_a_dispatch_row_declares_returns_with_a_position(tmp_path) -> None: + row = '[[dispatch]]\nid = "hooks"\nregister = ["add_filter"]\napply = ["apply_filters", "do_action"]\n' + _write(tmp_path, row + 'returns = ["apply_filters"]\nvalue_arg = 2\n') + (fact,) = load_facts(tmp_path).dispatches + assert fact.returns == ("apply_filters",) and fact.value_arg == 2 + + +@pytest.mark.parametrize( + "row", + [ + 'returns = ["apply_filters"]\n', # a returning call without the value's position + 'returns = ["not_an_apply_call"]\nvalue_arg = 2\n', + "value_arg = -1\n", + ], +) +def test_a_malformed_dispatch_row_is_refused(tmp_path, row: str) -> None: + _write(tmp_path, '[[dispatch]]\nid = "hooks"\nregister = ["add_filter"]\napply = ["apply_filters"]\n' + row) + with pytest.raises(FactLoadError): + load_facts(tmp_path)