From c16216bc424260368fcccdad12cf6306ae33a55f Mon Sep 17 00:00:00 2001 From: cgocast Date: Wed, 30 Aug 2023 17:22:14 +0200 Subject: [PATCH 1/3] Xpath injection #10162 --- config.xsd | 1 + docs/running_psalm/error_levels.md | 1 + docs/running_psalm/issues.md | 1 + docs/running_psalm/issues/TaintedXpath.md | 12 ++++++ .../Internal/Codebase/TaintFlowGraph.php | 10 +++++ src/Psalm/Issue/TaintedXpath.php | 8 ++++ src/Psalm/Type/TaintKind.php | 1 + src/Psalm/Type/TaintKindGroup.php | 1 + stubs/extensions/dom.phpstub | 5 +++ stubs/extensions/simplexml.phpstub | 5 ++- tests/TaintTest.php | 37 +++++++++++++++++++ 11 files changed, 81 insertions(+), 1 deletion(-) create mode 100644 docs/running_psalm/issues/TaintedXpath.md create mode 100644 src/Psalm/Issue/TaintedXpath.php diff --git a/config.xsd b/config.xsd index 4cf075b6e..eb5f11e2c 100644 --- a/config.xsd +++ b/config.xsd @@ -444,6 +444,7 @@ + diff --git a/docs/running_psalm/error_levels.md b/docs/running_psalm/error_levels.md index 55a18b8fa..90b5d5351 100644 --- a/docs/running_psalm/error_levels.md +++ b/docs/running_psalm/error_levels.md @@ -297,6 +297,7 @@ Level 5 and above allows a more non-verifiable code, and higher levels are even - [TaintedSystemSecret](issues/TaintedSystemSecret.md) - [TaintedUnserialize](issues/TaintedUnserialize.md) - [TaintedUserSecret](issues/TaintedUserSecret.md) + - [TaintedXpath](issues/TaintedXpath.md) - [UncaughtThrowInGlobalScope](issues/UncaughtThrowInGlobalScope.md) - [UnevaluatedCode](issues/UnevaluatedCode.md) - [UnnecessaryVarAnnotation](issues/UnnecessaryVarAnnotation.md) diff --git a/docs/running_psalm/issues.md b/docs/running_psalm/issues.md index d9b3b4f16..592225002 100644 --- a/docs/running_psalm/issues.md +++ b/docs/running_psalm/issues.md @@ -246,6 +246,7 @@ - [TaintedTextWithQuotes](issues/TaintedTextWithQuotes.md) - [TaintedUnserialize](issues/TaintedUnserialize.md) - [TaintedUserSecret](issues/TaintedUserSecret.md) + - [TaintedXpath](issues/TaintedXpath.md) - [TooFewArguments](issues/TooFewArguments.md) - [TooManyArguments](issues/TooManyArguments.md) - [TooManyTemplateParams](issues/TooManyTemplateParams.md) diff --git a/docs/running_psalm/issues/TaintedXpath.md b/docs/running_psalm/issues/TaintedXpath.md new file mode 100644 index 000000000..c0b16bbbc --- /dev/null +++ b/docs/running_psalm/issues/TaintedXpath.md @@ -0,0 +1,12 @@ +# TaintedSql + +Emitted when user-controlled input can be passed into to a xpath query. + +```php +xpath($expression); +} +``` diff --git a/src/Psalm/Internal/Codebase/TaintFlowGraph.php b/src/Psalm/Internal/Codebase/TaintFlowGraph.php index b18b82eb1..ba4f20fcc 100644 --- a/src/Psalm/Internal/Codebase/TaintFlowGraph.php +++ b/src/Psalm/Internal/Codebase/TaintFlowGraph.php @@ -24,6 +24,7 @@ use Psalm\Issue\TaintedSystemSecret; use Psalm\Issue\TaintedTextWithQuotes; use Psalm\Issue\TaintedUnserialize; use Psalm\Issue\TaintedUserSecret; +use Psalm\Issue\TaintedXpath; use Psalm\IssueBuffer; use Psalm\Type\TaintKind; @@ -449,6 +450,15 @@ class TaintFlowGraph extends DataFlowGraph ); break; + case TaintKind::INPUT_XPATH: + $issue = new TaintedXpath( + 'Detected tainted xpath query', + $issue_location, + $issue_trace, + $path, + ); + break; + default: $issue = new TaintedCustom( 'Detected tainted ' . $matching_taint, diff --git a/src/Psalm/Issue/TaintedXpath.php b/src/Psalm/Issue/TaintedXpath.php new file mode 100644 index 000000000..b9e4dbb42 --- /dev/null +++ b/src/Psalm/Issue/TaintedXpath.php @@ -0,0 +1,8 @@ +|false + * @psalm-taint-sink xpath $expression + */ public function evaluate(string $expression, ?DOMNode $contextNode = null, bool $registerNodeNS = true): mixed {} /** * @return DOMNodeList|false + * @psalm-taint-sink xpath $expression */ public function query(string $expression, ?DOMNode $contextNode = null, bool $registerNodeNS = true): mixed {} diff --git a/stubs/extensions/simplexml.phpstub b/stubs/extensions/simplexml.phpstub index 7f0bfa214..d2501f620 100644 --- a/stubs/extensions/simplexml.phpstub +++ b/stubs/extensions/simplexml.phpstub @@ -29,7 +29,10 @@ function simplexml_import_dom(SimpleXMLElement|DOMNode $node, ?string $class_nam */ class SimpleXMLElement implements Traversable, Countable { - /** @return array|null|false */ + /** + * @return array|null|false + * @psalm-taint-sink xpath $expression + */ public function xpath(string $expression) {} public function registerXPathNamespace(string $prefix, string $namespace): bool {} diff --git a/tests/TaintTest.php b/tests/TaintTest.php index d5dd0e1dc..a0cefb61d 100644 --- a/tests/TaintTest.php +++ b/tests/TaintTest.php @@ -749,6 +749,19 @@ class TaintTest extends TestCase $d = mysqli_real_escape_string($_GET["d"]); $mysqli->query("$a$b$c$d");', + ], + 'querySimpleXMLElement' => [ + 'code' => 'xpath($expression); + }', ], ]; } @@ -2503,6 +2516,30 @@ class TaintTest extends TestCase $function->invoke();', 'error_message' => 'TaintedCallable', ], + 'querySimpleXMLElement' => [ + 'code' => 'xpath($expression); + }', + 'error_message' => 'TaintedXpath', + ], + 'queryDOMXPath' => [ + 'code' => 'query($expression); + }', + 'error_message' => 'TaintedXpath', + ], + 'evaluateDOMXPath' => [ + 'code' => 'evaluate($expression); + }', + 'error_message' => 'TaintedXpath', + ], ]; } From 5545873f442d8ecc6052f2fcbb9373b7ebc4f19f Mon Sep 17 00:00:00 2001 From: cgocast Date: Thu, 31 Aug 2023 05:20:39 +0200 Subject: [PATCH 2/3] Fix tests --- docs/running_psalm/issues/TaintedXpath.md | 2 +- tests/TaintTest.php | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/running_psalm/issues/TaintedXpath.md b/docs/running_psalm/issues/TaintedXpath.md index c0b16bbbc..22616446a 100644 --- a/docs/running_psalm/issues/TaintedXpath.md +++ b/docs/running_psalm/issues/TaintedXpath.md @@ -1,4 +1,4 @@ -# TaintedSql +# TaintedXpath Emitted when user-controlled input can be passed into to a xpath query. diff --git a/tests/TaintTest.php b/tests/TaintTest.php index a0cefb61d..7efd9dae9 100644 --- a/tests/TaintTest.php +++ b/tests/TaintTest.php @@ -749,7 +749,7 @@ class TaintTest extends TestCase $d = mysqli_real_escape_string($_GET["d"]); $mysqli->query("$a$b$c$d");', - ], + ], 'querySimpleXMLElement' => [ 'code' => 'xpath($expression); - }', + }', 'error_message' => 'TaintedXpath', ], 'queryDOMXPath' => [ @@ -2529,7 +2529,7 @@ class TaintTest extends TestCase function queryExpression(DOMXPath $xpath) : mixed { $expression = $_GET["expression"]; return $xpath->query($expression); - }', + }', 'error_message' => 'TaintedXpath', ], 'evaluateDOMXPath' => [ @@ -2537,7 +2537,7 @@ class TaintTest extends TestCase function evaluateExpression(DOMXPath $xpath) : mixed { $expression = $_GET["expression"]; return $xpath->evaluate($expression); - }', + }', 'error_message' => 'TaintedXpath', ], ]; From e5b912bb2b543bb9c924ccdca2d99789a7d77c6e Mon Sep 17 00:00:00 2001 From: Bruce Weirdan Date: Thu, 31 Aug 2023 16:30:37 +0200 Subject: [PATCH 3/3] Document BC break --- UPGRADING.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/UPGRADING.md b/UPGRADING.md index 01f1a67dd..0bf6ee605 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -9,6 +9,8 @@ - [BC] The `TDependentListKey` type was removed and replaced with an optional property of the `TIntRange` type. +- [BC] Value of constant `Psalm\Type\TaintKindGroup::ALL_INPUT` changed to reflect a new `TaintKind::INPUT_XPATH` have been added. Accordingly, default values for `$taint` parameters of `Psalm\Codebase::addTaintSource()` and `Psalm\Codebase::addTaintSink()` have been changed as well. + # Upgrading from Psalm 4 to Psalm 5 ## Changed