From e8be2c500eb1d44f27476d6aafc951b9834307fe Mon Sep 17 00:00:00 2001 From: Brown Date: Mon, 22 Jun 2020 17:53:03 -0400 Subject: [PATCH] Support taint flows in more functions --- .../Internal/Analyzer/CommentAnalyzer.php | 5 +-- .../Expression/Call/FunctionCallAnalyzer.php | 4 +-- .../Internal/PhpVisitor/ReflectorVisitor.php | 36 +++++++++++++------ .../Scanner/FunctionDocblockComment.php | 4 +-- .../Stubs/CoreGenericFunctions.phpstub | 35 ++++++++++++++++-- src/Psalm/Storage/FunctionLikeStorage.php | 2 +- tests/TaintTest.php | 19 ++++++++++ 7 files changed, 84 insertions(+), 21 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/CommentAnalyzer.php b/src/Psalm/Internal/Analyzer/CommentAnalyzer.php index 6ef14bce4..b6149d757 100644 --- a/src/Psalm/Internal/Analyzer/CommentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/CommentAnalyzer.php @@ -468,8 +468,9 @@ class CommentAnalyzer } if (isset($parsed_docblock->tags['psalm-flow'])) { - $flow = trim(reset($parsed_docblock->tags['psalm-flow'])); - $info->flow = $flow; + foreach ($parsed_docblock->tags['psalm-flow'] as $param) { + $info->flows[] = trim($param); + } } if (isset($parsed_docblock->tags['psalm-taint-sink'])) { diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php index a0c3f63e7..3aa9ed5a3 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php @@ -1092,7 +1092,7 @@ class FunctionCallAnalyzer extends CallAnalyzer } } - foreach ($function_storage->return_source_params as $i) { + foreach ($function_storage->return_source_params as $i => $path_type) { if (!isset($stmt->args[$i])) { continue; } @@ -1115,7 +1115,7 @@ class FunctionCallAnalyzer extends CallAnalyzer $codebase->taint->addPath( $function_param_sink, $function_return_sink, - 'arg', + $path_type, $function_storage->added_taints, $removed_taints ); diff --git a/src/Psalm/Internal/PhpVisitor/ReflectorVisitor.php b/src/Psalm/Internal/PhpVisitor/ReflectorVisitor.php index c6232734a..ce935be99 100644 --- a/src/Psalm/Internal/PhpVisitor/ReflectorVisitor.php +++ b/src/Psalm/Internal/PhpVisitor/ReflectorVisitor.php @@ -2515,21 +2515,35 @@ class ReflectorVisitor extends PhpParser\NodeVisitorAbstract implements PhpParse $storage->added_taints = $docblock_info->added_taints; $storage->removed_taints = $docblock_info->removed_taints; - if ($docblock_info->flow) { - $flow_parts = explode('->', $docblock_info->flow); + if ($docblock_info->flows) { + foreach ($docblock_info->flows as $flow) { + $path_type = 'arg'; - if (isset($flow_parts[1]) && trim($flow_parts[1]) === 'return') { - $source_param_string = trim($flow_parts[0]); + $fancy_path_regex = '/-\(([a-z\-]+)\)->/'; - if ($source_param_string[0] === '(' && substr($source_param_string, -1) === ')') { - $source_params = preg_split('/, ?/', substr($source_param_string, 1, -1)); + if (preg_match($fancy_path_regex, $flow, $matches)) { + if (isset($matches[1])) { + $path_type = $matches[1]; + } - foreach ($source_params as $source_param) { - $source_param = substr($source_param, 1); + $flow = preg_replace($fancy_path_regex, '->', $flow); + } - foreach ($storage->params as $i => $param_storage) { - if ($param_storage->name === $source_param) { - $storage->return_source_params[] = $i; + $flow_parts = explode('->', $flow); + + if (isset($flow_parts[1]) && trim($flow_parts[1]) === 'return') { + $source_param_string = trim($flow_parts[0]); + + if ($source_param_string[0] === '(' && substr($source_param_string, -1) === ')') { + $source_params = preg_split('/, ?/', substr($source_param_string, 1, -1)); + + foreach ($source_params as $source_param) { + $source_param = substr($source_param, 1); + + foreach ($storage->params as $i => $param_storage) { + if ($param_storage->name === $source_param) { + $storage->return_source_params[$i] = $path_type; + } } } } diff --git a/src/Psalm/Internal/Scanner/FunctionDocblockComment.php b/src/Psalm/Internal/Scanner/FunctionDocblockComment.php index 86da74c85..046ec0272 100644 --- a/src/Psalm/Internal/Scanner/FunctionDocblockComment.php +++ b/src/Psalm/Internal/Scanner/FunctionDocblockComment.php @@ -91,9 +91,9 @@ class FunctionDocblockComment /** * Represents the flow from function params to return type * - * @var ?string + * @var array */ - public $flow; + public $flows = []; /** * @var array diff --git a/src/Psalm/Internal/Stubs/CoreGenericFunctions.phpstub b/src/Psalm/Internal/Stubs/CoreGenericFunctions.phpstub index 944d0912d..96927d317 100644 --- a/src/Psalm/Internal/Stubs/CoreGenericFunctions.phpstub +++ b/src/Psalm/Internal/Stubs/CoreGenericFunctions.phpstub @@ -480,6 +480,27 @@ function strtolower(string $str) : string {} */ function strtoupper(string $str) : string {} +/** + * @psalm-pure + * + * @psalm-flow ($str) -> return + */ +function trim(string $str, string $character_mask = " \t\n\r\0\x0B") : string {} + +/** + * @psalm-pure + * + * @psalm-flow ($str) -> return + */ +function ltrim(string $str, string $character_mask = " \t\n\r\0\x0B") : string {} + +/** + * @psalm-pure + * + * @psalm-flow ($str) -> return + */ +function rtrim(string $str, string $character_mask = " \t\n\r\0\x0B") : string {} + /** * @psalm-pure * @@ -493,10 +514,18 @@ function strtoupper(string $str) : string {} * : string * ) * - * @psalm-flow ($glue, $pieces) -> return + * @psalm-flow ($glue) -> return + * @psalm-flow ($pieces) -(array-fetch)-> return */ function implode($glue, array $pieces = []) : string {} +/** + * @psalm-pure + * + * @psalm-flow ($string) -(array-assignment)-> return + */ +function explode(string $delimiter, string $string, int $limit = -1) : array {} + /** * @param array $input * @@ -514,7 +543,7 @@ function array_sum(array $input) {} /** * @psalm-pure * - * @psalm-taint-remove html + * @psalm-taint-escape html * @psalm-flow ($str) -> return */ function strip_tags(string $str, ?string $allowable_tags = null) : string {} @@ -522,7 +551,7 @@ function strip_tags(string $str, ?string $allowable_tags = null) : string {} /** * @psalm-pure * - * @psalm-taint-remove html + * @psalm-taint-escape html * * @psalm-flow ($string) -> return */ diff --git a/src/Psalm/Storage/FunctionLikeStorage.php b/src/Psalm/Storage/FunctionLikeStorage.php index e69469f10..19041690c 100644 --- a/src/Psalm/Storage/FunctionLikeStorage.php +++ b/src/Psalm/Storage/FunctionLikeStorage.php @@ -205,7 +205,7 @@ abstract class FunctionLikeStorage public $removed_taints = []; /** - * @var list + * @var array */ public $return_source_params = []; diff --git a/tests/TaintTest.php b/tests/TaintTest.php index da3fdbe7d..bbf481333 100644 --- a/tests/TaintTest.php +++ b/tests/TaintTest.php @@ -1698,4 +1698,23 @@ class TaintTest extends TestCase $this->analyzeFile('somefile.php', new Context()); } + + public function testImplodeExplode() : void + { + $this->expectException(\Psalm\Exception\CodeException::class); + $this->expectExceptionMessage('TaintedInput'); + + $this->project_analyzer->trackTaintedInputs(); + + $this->addFile( + 'somefile.php', + 'analyzeFile('somefile.php', new Context()); + } }