From 126a4b8ed21e2ebf59be35cb3b8c6aa68e8452d6 Mon Sep 17 00:00:00 2001 From: Oliver Hader Date: Thu, 22 Feb 2024 18:30:40 +0100 Subject: [PATCH] Allow to implement custom taint type classes Instead of just having a generic `TaintedCustom` for custom taint - this change allows plugins/extensions to register their own custom taint type classes. Examples ``` $registry = Config::getInstance()->taint_kind_registry; $registry->defineKinds([ 'mine' => \Example\Package\TaintedMine::class, 'yours' => \Example\Package\TaintedYours::class, ], TaintKindGroup::GROUP_INPUT); $registry->defineGroup('my-input', 'html', 'sql', 'shell'); $registry->defineGroupProxy('input-sql', 'input', [ 'sql' => \Example\Package\TaintedSqlSecondOrder::class, ]); $registry->getKind('html'); // returns TaintedHtml::class; $registry->getGroupKinds('input'); // returns ['html', 'sql', ...] ``` --- docs/security_analysis/custom_taint_kinds.md | 100 +++++ .../security_analysis/custom_taint_sources.md | 3 +- docs/security_analysis/index.md | 7 + src/Psalm/Codebase.php | 4 +- src/Psalm/Config.php | 7 + src/Psalm/Config/IssueHandler.php | 3 +- .../Fetch/VariableFetchAnalyzer.php | 2 +- .../Internal/Codebase/TaintFlowGraph.php | 222 +++-------- .../Reflector/FunctionLikeDocblockScanner.php | 15 +- src/Psalm/Issue/TaintKindFactory.php | 56 +++ src/Psalm/Issue/TaintedCallable.php | 1 + src/Psalm/Issue/TaintedCookie.php | 1 + src/Psalm/Issue/TaintedCustom.php | 1 + src/Psalm/Issue/TaintedEval.php | 1 + src/Psalm/Issue/TaintedFile.php | 1 + src/Psalm/Issue/TaintedHeader.php | 1 + src/Psalm/Issue/TaintedHtml.php | 1 + src/Psalm/Issue/TaintedInclude.php | 1 + src/Psalm/Issue/TaintedInput.php | 2 + src/Psalm/Issue/TaintedLdap.php | 1 + src/Psalm/Issue/TaintedSSRF.php | 1 + src/Psalm/Issue/TaintedShell.php | 1 + src/Psalm/Issue/TaintedSql.php | 1 + src/Psalm/Issue/TaintedSystemSecret.php | 1 + src/Psalm/Issue/TaintedTextWithQuotes.php | 1 + src/Psalm/Issue/TaintedUnserialize.php | 1 + src/Psalm/Issue/TaintedUserSecret.php | 1 + src/Psalm/Type/TaintKindGroup.php | 3 + src/Psalm/Type/TaintKindRegistry.php | 349 ++++++++++++++++++ tests/TaintTest.php | 158 ++++++++ .../fixtures/Issue/TaintedTestingAnything.php | 15 + 31 files changed, 782 insertions(+), 180 deletions(-) create mode 100644 docs/security_analysis/custom_taint_kinds.md create mode 100644 src/Psalm/Issue/TaintKindFactory.php create mode 100644 src/Psalm/Type/TaintKindRegistry.php create mode 100644 tests/fixtures/Issue/TaintedTestingAnything.php diff --git a/docs/security_analysis/custom_taint_kinds.md b/docs/security_analysis/custom_taint_kinds.md new file mode 100644 index 00000000000..8ef7881feb6 --- /dev/null +++ b/docs/security_analysis/custom_taint_kinds.md @@ -0,0 +1,100 @@ +# Custom Taint Kinds + +[Psalm\Type\TaintKindRegistry](https://github.com/vimeo/psalm/blob/master/src/Psalm/Type/TaintKindRegistry.php) +allows plugins to define their own taint kinds and groups. The registry can be accessed via the `Psalm\Config` +singleton. + + +## Define Kinds + +```php +$registry = \Psalm\Config::getInstance()->taint_kind_registry; +$registry->defineKinds([ + 'custom-first' => \Example\Package\TaintedCustomA::class, + 'custom-second' => \Example\Package\TaintedCustomB::class, +], 'custom'); +``` + +The example above defines the custom taints `custom-first` and `custom-second`, +maps them to corresponding individual issue classes and organizes them +in a new custom group `custom`. + +```php +/** + * @psalm-taint-source custom-first + */ +function fetchFirst(): string {} +/** + * Stub for the `custom` group - which includes both defined kinds. + * + * @psalm-taint-source custom + */ +function fetchData(): array {} +``` + +## Define Groups + +```php +$registry = \Psalm\Config::getInstance()->taint_kind_registry; +$registry->defineGroup('specific-input', 'html', 'sql'); +``` + +The exmaple above defines a new group `specific-input`, which only holds +a reduced subset (`html` and `sql`) of the default built-in group `input`. + +```php +/** + * @psalm-taint-source specific-input + */ +function fetchData(): array {} +``` + +## Extend Groups + +```php +$registry = \Psalm\Config::getInstance()->taint_kind_registry; +$registry->extendGroup('input', 'custom-first'); +``` + +The example above adds the custom kind `custom-first` to an existing group `input`. + +## Define Group Proxies + +```php +$registry = \Psalm\Config::getInstance()->taint_kind_registry; +$registry->defineGroupProxy('input-sql', 'input', [ + 'sql' => \Example\Package\TaintedSqlSecondOrder::class, +]); +``` + +The example above is special as it defines `input-sql` to be a proxy for +the existing group `input-sql` and keeps track of that usage. In addition, +the build-in kind `sql` is overloaded with a custom issue class. + +This example is known as "Second Order SQL Injection", where data that was +retrieved from a database is reused for new SQL queries. The assumption is, +that the persisted data was not sanitized and might contain user submitted +malicious snippets. + +```php +/** + * @psalm-taint-source input-sql + */ +function fetchAliasFromDatabase(): string {} + +/** + * @psalm-taint-sink sql $query + * @psalm-taint-specialize + */ +function executeDatabaseQuery($query): array {} + +// this would still issue the default `TaintedSql` +$alias = $_GET['alias']; +$query = sprintf('SELECT * FROM comments WHERE alias="%s";', $alias); +$rows = executeDatabaseQuery($query); + +// this would issue the custom `TaintedSqlSecondOrder` +$alias = fetchAliasFromDatabase(); +$query = sprintf('SELECT * FROM comments WHERE alias="%s";', $alias); +$rows = executeDatabaseQuery($query); +``` diff --git a/docs/security_analysis/custom_taint_sources.md b/docs/security_analysis/custom_taint_sources.md index d3bdb0a3205..96fcff479c8 100644 --- a/docs/security_analysis/custom_taint_sources.md +++ b/docs/security_analysis/custom_taint_sources.md @@ -48,6 +48,7 @@ class BadSqlTainter implements AfterExpressionAnalysisInterface $expr = $event->getExpr(); $statements_source = $event->getStatementsSource(); $codebase = $event->getCodebase(); + $taint_type_registry = $codebase->config->taint_kind_registry; if ($expr instanceof PhpParser\Node\Expr\Variable && $expr->name === 'bad_data' ) { @@ -63,7 +64,7 @@ class BadSqlTainter implements AfterExpressionAnalysisInterface $codebase->addTaintSource( $expr_type, $expr_identifier, - TaintKindGroup::ALL_INPUT, + [TaintKindGroup::GROUP_INPUT], new CodeLocation($statements_source, $expr) ); } diff --git a/docs/security_analysis/index.md b/docs/security_analysis/index.md index 451a7d250a4..e22aa49807f 100644 --- a/docs/security_analysis/index.md +++ b/docs/security_analysis/index.md @@ -39,7 +39,14 @@ Psalm recognises a number of taint types by default, defined in the [Psalm\Type\ - `user_secret` - used for strings that could contain user-supplied secrets - `system_secret` - used for strings that could contain system secrets +Each of these default kind types will be mapped to a corresponding class which inherits from abstract class +`TaintedInput` - for instance `TaintedHtml`, `TaintedShell`, `TaintedHeader`, etc. + You're also free to define your own taint types when defining custom taint sources – they're just strings. +Per default those custom types are mapped to the generic class `TaintedCustom`. + +However, plugins can utilize [custom taint kinds](custom_taint_kinds.md) to define more specific classes +for improved semantics and enhanced issue handling in general. ## Taint Sources diff --git a/src/Psalm/Codebase.php b/src/Psalm/Codebase.php index a02f61403f6..0b9369e2a42 100644 --- a/src/Psalm/Codebase.php +++ b/src/Psalm/Codebase.php @@ -2423,7 +2423,7 @@ public function queueClassLikeForScanning( public function addTaintSource( Union $expr_type, string $taint_id, - array $taints = TaintKindGroup::ALL_INPUT, + array $taints = [TaintKindGroup::GROUP_INPUT], ?CodeLocation $code_location = null ): Union { if (!$this->taint_flow_graph) { @@ -2449,7 +2449,7 @@ public function addTaintSource( */ public function addTaintSink( string $taint_id, - array $taints = TaintKindGroup::ALL_INPUT, + array $taints = [TaintKindGroup::GROUP_INPUT], ?CodeLocation $code_location = null ): void { if (!$this->taint_flow_graph) { diff --git a/src/Psalm/Config.php b/src/Psalm/Config.php index a8a08366e43..c1d8c7e3e2d 100644 --- a/src/Psalm/Config.php +++ b/src/Psalm/Config.php @@ -41,6 +41,7 @@ use Psalm\Plugin\PluginInterface; use Psalm\Progress\Progress; use Psalm\Progress\VoidProgress; +use Psalm\Type\TaintKindRegistry; use RuntimeException; use SimpleXMLElement; use Symfony\Component\Filesystem\Path; @@ -727,6 +728,11 @@ class Config /** @var list */ public array $config_warnings = []; + /** + * @readonly + */ + public TaintKindRegistry $taint_kind_registry; + /** @internal */ protected function __construct() { @@ -735,6 +741,7 @@ protected function __construct() $this->universal_object_crates = [ strtolower(stdClass::class), ]; + $this->taint_kind_registry = new TaintKindRegistry(); } /** diff --git a/src/Psalm/Config/IssueHandler.php b/src/Psalm/Config/IssueHandler.php index 48791659e47..d534b3a1ce1 100644 --- a/src/Psalm/Config/IssueHandler.php +++ b/src/Psalm/Config/IssueHandler.php @@ -175,7 +175,8 @@ public static function getAllIssueTypes(): array && $issue_name !== 'ParseError' && $issue_name !== 'PluginIssue' && $issue_name !== 'MixedIssue' - && $issue_name !== 'MixedIssueTrait', + && $issue_name !== 'MixedIssueTrait' + && $issue_name !== 'TaintKindFactory', ); } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/VariableFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/VariableFetchAnalyzer.php index 324dd7b30b0..4f4b31b53f7 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/VariableFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/VariableFetchAnalyzer.php @@ -522,7 +522,7 @@ private static function taintVariable( $var_name, null, null, - TaintKindGroup::ALL_INPUT, + [TaintKindGroup::GROUP_INPUT], ); $statements_analyzer->data_flow_graph->addSource($server_taint_source); diff --git a/src/Psalm/Internal/Codebase/TaintFlowGraph.php b/src/Psalm/Internal/Codebase/TaintFlowGraph.php index bb7cd993879..93cc7eab4ac 100644 --- a/src/Psalm/Internal/Codebase/TaintFlowGraph.php +++ b/src/Psalm/Internal/Codebase/TaintFlowGraph.php @@ -8,30 +8,15 @@ use Psalm\Internal\DataFlow\DataFlowNode; use Psalm\Internal\DataFlow\TaintSink; use Psalm\Internal\DataFlow\TaintSource; -use Psalm\Issue\TaintedCallable; -use Psalm\Issue\TaintedCookie; -use Psalm\Issue\TaintedCustom; -use Psalm\Issue\TaintedEval; -use Psalm\Issue\TaintedFile; -use Psalm\Issue\TaintedHeader; -use Psalm\Issue\TaintedHtml; -use Psalm\Issue\TaintedInclude; -use Psalm\Issue\TaintedLdap; -use Psalm\Issue\TaintedSSRF; -use Psalm\Issue\TaintedShell; -use Psalm\Issue\TaintedSql; -use Psalm\Issue\TaintedSystemSecret; -use Psalm\Issue\TaintedTextWithQuotes; -use Psalm\Issue\TaintedUnserialize; -use Psalm\Issue\TaintedUserSecret; +use Psalm\Issue\TaintKindFactory; use Psalm\IssueBuffer; -use Psalm\Type\TaintKind; use function array_diff; use function array_filter; use function array_intersect; use function array_merge; use function array_unique; +use function array_values; use function count; use function end; use function implode; @@ -204,8 +189,10 @@ public function connectSinksAndSources(): void ksort($this->specializations); ksort($this->forward_edges); + $registry = Config::getInstance()->taint_kind_registry; // reprocess resolved descendants up to a maximum nesting level of 40 + // @todo adjust maximum nesting level (via config, or throw warning when exceeded) for ($i = 0; count($sinks) && count($sources) && $i < 40; $i++) { $new_sources = []; @@ -213,6 +200,7 @@ public function connectSinksAndSources(): void foreach ($sources as $source) { $source_taints = $source->taints; + $source_taints = $registry->resolveAllGroupKinds(...$source_taints); sort($source_taints); $visited_source_ids[$source->id][implode(',', $source_taints)] = true; @@ -297,9 +285,12 @@ private function getChildNodes( } if (isset($sinks[$to_id])) { - $matching_taints = array_intersect($sinks[$to_id]->taints, $new_taints); + $matches = $this->matchSinksAndSources($sinks[$to_id]->taints, $new_taints); + + if (isset($matches['taints']) && $generated_source->code_location) { + $matching_taints = $matches['taints']; + $matching_proxies = $matches['proxies'] ?? []; - if ($matching_taints && $generated_source->code_location) { if ($sinks[$to_id]->code_location && $config->reportIssueInFile('TaintedInput', $sinks[$to_id]->code_location->file_path) ) { @@ -312,152 +303,16 @@ private function getChildNodes( $path = $this->getPredecessorPath($generated_source) . ' -> ' . $this->getSuccessorPath($sinks[$to_id]); - foreach ($matching_taints as $matching_taint) { - switch ($matching_taint) { - case TaintKind::INPUT_CALLABLE: - $issue = new TaintedCallable( - 'Detected tainted text', - $issue_location, - $issue_trace, - $path, - ); - break; - - case TaintKind::INPUT_UNSERIALIZE: - $issue = new TaintedUnserialize( - 'Detected tainted code passed to unserialize or similar', - $issue_location, - $issue_trace, - $path, - ); - break; - - case TaintKind::INPUT_INCLUDE: - $issue = new TaintedInclude( - 'Detected tainted code passed to include or similar', - $issue_location, - $issue_trace, - $path, - ); - break; - - case TaintKind::INPUT_EVAL: - $issue = new TaintedEval( - 'Detected tainted code passed to eval or similar', - $issue_location, - $issue_trace, - $path, - ); - break; - - case TaintKind::INPUT_SQL: - $issue = new TaintedSql( - 'Detected tainted SQL', - $issue_location, - $issue_trace, - $path, - ); - break; - - case TaintKind::INPUT_HTML: - $issue = new TaintedHtml( - 'Detected tainted HTML', - $issue_location, - $issue_trace, - $path, - ); - break; - - case TaintKind::INPUT_HAS_QUOTES: - $issue = new TaintedTextWithQuotes( - 'Detected tainted text with possible quotes', - $issue_location, - $issue_trace, - $path, - ); - break; - - case TaintKind::INPUT_SHELL: - $issue = new TaintedShell( - 'Detected tainted shell code', - $issue_location, - $issue_trace, - $path, - ); - break; - - case TaintKind::USER_SECRET: - $issue = new TaintedUserSecret( - 'Detected tainted user secret leaking', - $issue_location, - $issue_trace, - $path, - ); - break; - - case TaintKind::SYSTEM_SECRET: - $issue = new TaintedSystemSecret( - 'Detected tainted system secret leaking', - $issue_location, - $issue_trace, - $path, - ); - break; - - case TaintKind::INPUT_SSRF: - $issue = new TaintedSSRF( - 'Detected tainted network request', - $issue_location, - $issue_trace, - $path, - ); - break; - - case TaintKind::INPUT_LDAP: - $issue = new TaintedLdap( - 'Detected tainted LDAP request', - $issue_location, - $issue_trace, - $path, - ); - break; - - case TaintKind::INPUT_COOKIE: - $issue = new TaintedCookie( - 'Detected tainted cookie', - $issue_location, - $issue_trace, - $path, - ); - break; - - case TaintKind::INPUT_FILE: - $issue = new TaintedFile( - 'Detected tainted file handling', - $issue_location, - $issue_trace, - $path, - ); - break; - - case TaintKind::INPUT_HEADER: - $issue = new TaintedHeader( - 'Detected tainted header', - $issue_location, - $issue_trace, - $path, - ); - break; - - default: - $issue = new TaintedCustom( - 'Detected tainted ' . $matching_taint, - $issue_location, - $issue_trace, - $path, - ); - } + $factory = new TaintKindFactory($config->taint_kind_registry); + foreach ($matching_taints as $matching_taint) { + $issue = $factory->create( + $matching_taint, + $issue_location, + $issue_trace, + $path, + $matching_proxies, + ); IssueBuffer::maybeAdd($issue); } } @@ -529,4 +384,43 @@ private function doesForwardEdgeExist(DataFlowNode $new_source): bool { return isset($this->forward_edges[$new_source->id]); } + + /** + * @param array $sinks + * @param list $sources + * @return array{proxies?: non-empty-array, taints?: non-empty-array} + */ + private function matchSinksAndSources(array $sinks, array $sources): array + { + $sinks = array_filter($sinks); + $sources = array_filter($sources); + if ($sinks === [] || $sources === []) { + return []; + } + + $matching_taints = array_intersect($sinks, $sources); + if ($matching_taints !== []) { + return ['taints' => array_values($matching_taints)]; + } + + $all_taints = []; + $all_proxies = []; + $registry = Config::getInstance()->taint_kind_registry; + $proxy_kind_map = $registry->resolveProxyKindMap(...$sources); + foreach ($proxy_kind_map as $proxy => $kinds) { + $matching_taints = array_intersect($kinds, $sinks); + if ($matching_taints !== []) { + $all_taints[] = array_values($matching_taints); + $all_proxies[] = $proxy; + } + } + + if ($all_proxies === [] || $all_taints === []) { + return []; + } + return [ + 'taints' => array_merge(...$all_taints), + 'proxies' => $all_proxies, + ]; + } } diff --git a/src/Psalm/Internal/PhpVisitor/Reflector/FunctionLikeDocblockScanner.php b/src/Psalm/Internal/PhpVisitor/Reflector/FunctionLikeDocblockScanner.php index cfeb0b15f30..85a5f72e415 100644 --- a/src/Psalm/Internal/PhpVisitor/Reflector/FunctionLikeDocblockScanner.php +++ b/src/Psalm/Internal/PhpVisitor/Reflector/FunctionLikeDocblockScanner.php @@ -50,8 +50,6 @@ use function array_filter; use function array_merge; -use function array_search; -use function array_splice; use function array_unique; use function array_values; use function count; @@ -357,15 +355,10 @@ public static function addDocblockInfo( $docblock_info->taint_source_types = array_values(array_unique($docblock_info->taint_source_types)); // expand 'input' group to all items, e.g. `['other', 'input']` -> `['other', 'html', 'sql', 'shell', ...]` - $inputIndex = array_search(TaintKindGroup::GROUP_INPUT, $docblock_info->taint_source_types, true); - if ($inputIndex !== false) { - array_splice( - $docblock_info->taint_source_types, - $inputIndex, - 1, - TaintKindGroup::ALL_INPUT, - ); - } + $docblock_info->taint_source_types = $config->taint_kind_registry->resolveGroupKinds( + TaintKindGroup::GROUP_INPUT, + ...$docblock_info->taint_source_types, + ); // merge taints from doc block to storage, enforce uniqueness and having consecutive index keys $storage->taint_source_types = array_merge($storage->taint_source_types, $docblock_info->taint_source_types); $storage->taint_source_types = array_values(array_unique($storage->taint_source_types)); diff --git a/src/Psalm/Issue/TaintKindFactory.php b/src/Psalm/Issue/TaintKindFactory.php new file mode 100644 index 00000000000..383434166f1 --- /dev/null +++ b/src/Psalm/Issue/TaintKindFactory.php @@ -0,0 +1,56 @@ +registry = $registry; + } + + /** + * @param array $proxies + */ + public function create( + string $type, + CodeLocation $code_location, + array $journey, + string $journey_text, + array $proxies = [] + ): CodeIssue { + /** @var TaintedInput $class_name */ + $class_name = $this->resolveKindProxies($type, $proxies) + ?? $this->registry->getKind($type) + ?? $this->registry->getDefaultKind(); + return new $class_name($class_name::MESSAGE, $code_location, $journey, $journey_text); + } + + /** + * @param array $proxies + */ + private function resolveKindProxies(string $type, array $proxies): ?string + { + if ($proxies === []) { + return null; + } + foreach ($proxies as $proxy) { + $class_name = $this->registry->getKindProxy($type, $proxy); + if ($class_name !== null) { + return $class_name; + } + } + return null; + } +} diff --git a/src/Psalm/Issue/TaintedCallable.php b/src/Psalm/Issue/TaintedCallable.php index 4b988029704..ab264215255 100644 --- a/src/Psalm/Issue/TaintedCallable.php +++ b/src/Psalm/Issue/TaintedCallable.php @@ -5,4 +5,5 @@ final class TaintedCallable extends TaintedInput { public const SHORTCODE = 243; + public const MESSAGE = 'Detected tainted text'; } diff --git a/src/Psalm/Issue/TaintedCookie.php b/src/Psalm/Issue/TaintedCookie.php index 424b860c949..a5ddaaa214e 100644 --- a/src/Psalm/Issue/TaintedCookie.php +++ b/src/Psalm/Issue/TaintedCookie.php @@ -5,4 +5,5 @@ final class TaintedCookie extends TaintedInput { public const SHORTCODE = 257; + public const MESSAGE = 'Detected tainted cookie'; } diff --git a/src/Psalm/Issue/TaintedCustom.php b/src/Psalm/Issue/TaintedCustom.php index 2bd91c65caf..097ffe6d1f4 100644 --- a/src/Psalm/Issue/TaintedCustom.php +++ b/src/Psalm/Issue/TaintedCustom.php @@ -5,4 +5,5 @@ final class TaintedCustom extends TaintedInput { public const SHORTCODE = 249; + public const MESSAGE = 'Detected tainted %s'; } diff --git a/src/Psalm/Issue/TaintedEval.php b/src/Psalm/Issue/TaintedEval.php index b003c5373f4..ab5fe87020c 100644 --- a/src/Psalm/Issue/TaintedEval.php +++ b/src/Psalm/Issue/TaintedEval.php @@ -5,4 +5,5 @@ final class TaintedEval extends TaintedInput { public const SHORTCODE = 252; + public const MESSAGE = 'Detected tainted code passed to eval or similar'; } diff --git a/src/Psalm/Issue/TaintedFile.php b/src/Psalm/Issue/TaintedFile.php index 1c1838574b8..287337832a8 100644 --- a/src/Psalm/Issue/TaintedFile.php +++ b/src/Psalm/Issue/TaintedFile.php @@ -5,4 +5,5 @@ final class TaintedFile extends TaintedInput { public const SHORTCODE = 255; + public const MESSAGE = 'Detected tainted file handling'; } diff --git a/src/Psalm/Issue/TaintedHeader.php b/src/Psalm/Issue/TaintedHeader.php index ebbbfc5f239..7ac72984cbe 100644 --- a/src/Psalm/Issue/TaintedHeader.php +++ b/src/Psalm/Issue/TaintedHeader.php @@ -5,4 +5,5 @@ final class TaintedHeader extends TaintedInput { public const SHORTCODE = 256; + public const MESSAGE = 'Detected tainted header'; } diff --git a/src/Psalm/Issue/TaintedHtml.php b/src/Psalm/Issue/TaintedHtml.php index 3c9956da866..593e6069c32 100644 --- a/src/Psalm/Issue/TaintedHtml.php +++ b/src/Psalm/Issue/TaintedHtml.php @@ -5,4 +5,5 @@ final class TaintedHtml extends TaintedInput { public const SHORTCODE = 245; + public const MESSAGE = 'Detected tainted HTML'; } diff --git a/src/Psalm/Issue/TaintedInclude.php b/src/Psalm/Issue/TaintedInclude.php index f1affa5c6c5..6680cd6b52c 100644 --- a/src/Psalm/Issue/TaintedInclude.php +++ b/src/Psalm/Issue/TaintedInclude.php @@ -5,4 +5,5 @@ final class TaintedInclude extends TaintedInput { public const SHORTCODE = 251; + public const MESSAGE = 'Detected tainted code passed to include or similar'; } diff --git a/src/Psalm/Issue/TaintedInput.php b/src/Psalm/Issue/TaintedInput.php index 15713c09335..2d334f42585 100644 --- a/src/Psalm/Issue/TaintedInput.php +++ b/src/Psalm/Issue/TaintedInput.php @@ -10,6 +10,8 @@ abstract class TaintedInput extends CodeIssue public const ERROR_LEVEL = -2; /** @var int<0, max> */ public const SHORTCODE = 205; + /** @var string */ + public const MESSAGE = 'Detected generic tainted input'; /** * @var string diff --git a/src/Psalm/Issue/TaintedLdap.php b/src/Psalm/Issue/TaintedLdap.php index 4ab8e32e2ff..762618bc387 100644 --- a/src/Psalm/Issue/TaintedLdap.php +++ b/src/Psalm/Issue/TaintedLdap.php @@ -5,4 +5,5 @@ final class TaintedLdap extends TaintedInput { public const SHORTCODE = 254; + public const MESSAGE = 'Detected tainted LDAP request'; } diff --git a/src/Psalm/Issue/TaintedSSRF.php b/src/Psalm/Issue/TaintedSSRF.php index def8a2cdc97..236812f1868 100644 --- a/src/Psalm/Issue/TaintedSSRF.php +++ b/src/Psalm/Issue/TaintedSSRF.php @@ -5,4 +5,5 @@ final class TaintedSSRF extends TaintedInput { public const SHORTCODE = 253; + public const MESSAGE = 'Detected tainted network request'; } diff --git a/src/Psalm/Issue/TaintedShell.php b/src/Psalm/Issue/TaintedShell.php index 8ff52b1615f..bd6b4a39285 100644 --- a/src/Psalm/Issue/TaintedShell.php +++ b/src/Psalm/Issue/TaintedShell.php @@ -5,4 +5,5 @@ final class TaintedShell extends TaintedInput { public const SHORTCODE = 246; + public const MESSAGE = 'Detected tainted shell code'; } diff --git a/src/Psalm/Issue/TaintedSql.php b/src/Psalm/Issue/TaintedSql.php index 9d790d71e62..e3525b31996 100644 --- a/src/Psalm/Issue/TaintedSql.php +++ b/src/Psalm/Issue/TaintedSql.php @@ -5,4 +5,5 @@ final class TaintedSql extends TaintedInput { public const SHORTCODE = 244; + public const MESSAGE = 'Detected tainted SQL'; } diff --git a/src/Psalm/Issue/TaintedSystemSecret.php b/src/Psalm/Issue/TaintedSystemSecret.php index 04888a99a37..b51a882e425 100644 --- a/src/Psalm/Issue/TaintedSystemSecret.php +++ b/src/Psalm/Issue/TaintedSystemSecret.php @@ -5,4 +5,5 @@ final class TaintedSystemSecret extends TaintedInput { public const SHORTCODE = 248; + public const MESSAGE = 'Detected tainted system secret leaking'; } diff --git a/src/Psalm/Issue/TaintedTextWithQuotes.php b/src/Psalm/Issue/TaintedTextWithQuotes.php index 9b78c3d4509..21e31e50311 100644 --- a/src/Psalm/Issue/TaintedTextWithQuotes.php +++ b/src/Psalm/Issue/TaintedTextWithQuotes.php @@ -5,4 +5,5 @@ final class TaintedTextWithQuotes extends TaintedInput { public const SHORTCODE = 274; + public const MESSAGE = 'Detected tainted text with possible quotes'; } diff --git a/src/Psalm/Issue/TaintedUnserialize.php b/src/Psalm/Issue/TaintedUnserialize.php index b2e97c9a438..31143f7c0d2 100644 --- a/src/Psalm/Issue/TaintedUnserialize.php +++ b/src/Psalm/Issue/TaintedUnserialize.php @@ -5,4 +5,5 @@ final class TaintedUnserialize extends TaintedInput { public const SHORTCODE = 250; + public const MESSAGE = 'Detected tainted code passed to unserialize or similar'; } diff --git a/src/Psalm/Issue/TaintedUserSecret.php b/src/Psalm/Issue/TaintedUserSecret.php index 5bbfcf17142..27b84abbf54 100644 --- a/src/Psalm/Issue/TaintedUserSecret.php +++ b/src/Psalm/Issue/TaintedUserSecret.php @@ -5,4 +5,5 @@ final class TaintedUserSecret extends TaintedInput { public const SHORTCODE = 247; + public const MESSAGE = 'Detected tainted user secret leaking'; } diff --git a/src/Psalm/Type/TaintKindGroup.php b/src/Psalm/Type/TaintKindGroup.php index 8f144569a5e..1725f1b897b 100644 --- a/src/Psalm/Type/TaintKindGroup.php +++ b/src/Psalm/Type/TaintKindGroup.php @@ -9,6 +9,9 @@ final class TaintKindGroup { public const GROUP_INPUT = 'input'; + /** + * @deprecated since Psalm 5.x, use `[TaintKindGroup::GROUP_INPUT]` instead + */ public const ALL_INPUT = [ TaintKind::INPUT_HTML, TaintKind::INPUT_HAS_QUOTES, diff --git a/src/Psalm/Type/TaintKindRegistry.php b/src/Psalm/Type/TaintKindRegistry.php new file mode 100644 index 00000000000..be3270eb86c --- /dev/null +++ b/src/Psalm/Type/TaintKindRegistry.php @@ -0,0 +1,349 @@ +> + */ + private array $taint_kinds = [ + TaintKind::INPUT_CALLABLE => TaintedCallable::class, + TaintKind::INPUT_COOKIE => TaintedCookie::class, + TaintKind::INPUT_EVAL => TaintedEval::class, + TaintKind::INPUT_FILE => TaintedFile::class, + TaintKind::INPUT_HAS_QUOTES => TaintedTextWithQuotes::class, + TaintKind::INPUT_HEADER => TaintedHeader::class, + TaintKind::INPUT_HTML => TaintedHtml::class, + TaintKind::INPUT_INCLUDE => TaintedInclude::class, + TaintKind::INPUT_LDAP => TaintedLdap::class, + TaintKind::INPUT_SHELL => TaintedShell::class, + TaintKind::INPUT_SQL => TaintedSql::class, + TaintKind::INPUT_SSRF => TaintedSSRF::class, + TaintKind::INPUT_UNSERIALIZE => TaintedUnserialize::class, + TaintKind::SYSTEM_SECRET => TaintedSystemSecret::class, + TaintKind::USER_SECRET => TaintedUserSecret::class, + ]; + /** + * @var non-empty-array> + */ + private array $taint_groups = [ + TaintKindGroup::GROUP_INPUT => [ + TaintKind::INPUT_CALLABLE, + TaintKind::INPUT_COOKIE, + TaintKind::INPUT_EVAL, + TaintKind::INPUT_FILE, + TaintKind::INPUT_HAS_QUOTES, + TaintKind::INPUT_HEADER, + TaintKind::INPUT_HTML, + TaintKind::INPUT_INCLUDE, + TaintKind::INPUT_LDAP, + TaintKind::INPUT_SHELL, + TaintKind::INPUT_SQL, + TaintKind::INPUT_SSRF, + TaintKind::INPUT_UNSERIALIZE, + ], + ]; + + /** + * @var array>}> + */ + private array $group_proxies = []; + + /** + * @var class-string + */ + private string $default_kind = TaintedCustom::class; + + /** + * @internal + */ + public function __construct() + { + } + + /** + * @param array> $kind_to_class_map + */ + public function defineKinds(array $kind_to_class_map, string $group = ''): void + { + foreach ($kind_to_class_map as $kind => $class_name) { + if ($kind === '') { + throw new LogicException('Taint kind cannot be an empty string'); + } + if ($this->hasKind($kind)) { + throw new RuntimeException('Taint kind ' . $kind . ' is already defined'); + } + $this->assertClassName($class_name); + $this->assertDisjunctiveNames($kind); + $this->taint_kinds[$kind] = $class_name; + } + if ($group === '') { + return; + } + if ($this->hasGroup($group)) { + $this->extendGroup($group, ...array_keys($kind_to_class_map)); + } else { + $this->defineGroup($group, ...array_keys($kind_to_class_map)); + } + } + + /** + * @param non-empty-string $group + */ + public function defineGroup(string $group, string ...$kinds): void + { + if ($this->hasGroup($group)) { + throw new LogicException('Taint group ' . $group . ' is already defined'); + } + $kinds = array_filter($kinds); + $this->assertKinds(...$kinds); + $this->assertDisjunctiveNames($group); + $this->taint_groups[$group] = array_values(array_unique($kinds)); + } + + /** + * @param non-empty-string $group + */ + public function extendGroup(string $group, string ...$kinds): void + { + if (!$this->hasGroup($group)) { + throw new LogicException('Taint group ' . $group . ' is not defined'); + } + $kinds = array_filter($kinds); + $this->assertKinds(...$kinds); + $this->taint_groups[$group] = array_values(array_unique(array_merge($this->taint_groups[$group], $kinds))); + } + + /** + * @param array> $refined_kind_to_class_map + */ + public function defineGroupProxy(string $proxy, string $group, array $refined_kind_to_class_map): void + { + if (isset($this->group_proxies[$proxy])) { + throw new LogicException('Group proxy ' . $proxy . ' is already defined'); + } + foreach ($refined_kind_to_class_map as $kind => $class_name) { + if ($kind === '') { + throw new LogicException('Taint kind cannot be an empty string'); + } + if (!$this->hasKind($kind)) { + throw new RuntimeException('Taint kind ' . $kind . ' is not defined'); + } + $this->assertClassName($class_name); + } + $this->assertDisjunctiveNames($proxy); + $this->group_proxies[$proxy] = [ + 'group' => $group, + 'kinds' => $refined_kind_to_class_map, + ]; + } + + /** + * @psalm-suppress PossiblyUnusedMethod + * @param class-string $class_name + */ + public function setDefaultKind(string $class_name): void + { + $this->assertClassName($class_name); + $this->default_kind = $class_name; + } + + /** + * @return class-string|null + */ + public function getKind(string $kind): ?string + { + return $this->taint_kinds[$kind] ?? null; + } + + /** + * @return class-string + */ + public function getDefaultKind(): string + { + return $this->default_kind; + } + + public function getKindProxy(string $kind, string $proxy): ?string + { + return $this->group_proxies[$proxy]['kinds'][$kind] ?? null; + } + + /** + * @param non-empty-string $kind + */ + public function hasKind(string $kind): bool + { + return isset($this->taint_kinds[$kind]); + } + + /** + * Resolves one level of items (e.g. 'html', 'sql', ...) for a named group (e.g. 'index') + * (does not resolve recursively). + * + * @return list + */ + public function getGroupKinds(string $group): array + { + return $this->taint_groups[$group] ?? []; + } + + /** + * @param non-empty-string $group + * @return array + */ + public function resolveGroupKinds(string $group, string ...$names): array + { + if (!$this->hasGroup($group)) { + return $names; + } + $groupIndex = array_search($group, $names, true); + if ($groupIndex !== false && is_int($groupIndex)) { + array_splice( + $names, + $groupIndex, + 1, + $this->taint_groups[$group], + ); + } + return $names; + } + + /** + * @return array + */ + public function resolveAllGroupKinds(string ...$names): array + { + foreach (array_keys($this->taint_groups) as $group) { + $names = $this->resolveGroupKinds($group, ...$names); + } + return $names; + } + + /** + * @param non-empty-string $group + */ + public function hasGroup(string $group): bool + { + return isset($this->taint_groups[$group]); + } + + /** + * @return array> + */ + public function resolveProxyKindMap(string ...$kinds): array + { + if ($this->group_proxies === []) { + return []; + } + $group_proxies = array_filter( + $this->group_proxies, + static fn(string $proxy) => in_array($proxy, $kinds, true), + ARRAY_FILTER_USE_KEY, + ); + return array_map( + fn(array $item) => $this->getGroupKinds($item['group']), + $group_proxies, + ); + } + + /** + * @param non-empty-string ...$kinds + */ + private function assertKinds(string ...$kinds): void + { + foreach ($kinds as $kind) { + if (!$this->hasKind($kind)) { + throw new RuntimeException('Taint kind ' . $kind . ' is not defined'); + } + } + } + + private function assertDisjunctiveNames(string $name): void + { + if (isset($this->taint_kinds[$name])) { + throw new LogicException('All taint names need to be disjunctive, but found kind ' . $name); + } + if (isset($this->taint_groups[$name])) { + throw new LogicException('All taint names need to be disjunctive, but found group ' . $name); + } + if (isset($this->group_proxies[$name])) { + throw new LogicException('All taint names need to be disjunctive, but found proxy ' . $name); + } + } + + private function assertClassName(string $class_name): void + { + if (!class_exists($class_name)) { + throw new LogicException(sprintf( + 'Taint class %s does not exist', + $class_name, + )); + } + if (!is_subclass_of($class_name, TaintedInput::class)) { + throw new LogicException(sprintf( + 'Taint class %s must be a subclass of %s', + $class_name, + TaintedInput::class, + )); + } + if (stripos($this->getShortClassName($class_name), self::CLASS_NAME_PREFIX) !== 0) { + throw new LogicException(sprintf( + 'Taint class name %s must start with "%s"', + $class_name, + self::CLASS_NAME_PREFIX, + )); + } + } + + /** + * Resolves `Vendor\Package\ClassName` to just `ClassName` + * + * @param class-string $class_name + */ + private function getShortClassName(string $class_name): string + { + return (new ReflectionClass($class_name))->getShortName(); + } +} diff --git a/tests/TaintTest.php b/tests/TaintTest.php index 6439b366c1d..2ca12929490 100644 --- a/tests/TaintTest.php +++ b/tests/TaintTest.php @@ -6,6 +6,8 @@ use Psalm\Exception\CodeException; use Psalm\Internal\Analyzer\IssueData; use Psalm\IssueBuffer; +use Psalm\Tests\fixtures\Issue\TaintedTestingAnything; +use Psalm\Type\TaintKindGroup; use function array_map; use function preg_quote; @@ -2609,4 +2611,160 @@ function triggerFile(string $value): string ], ]; } + + /** + * @test + */ + public function customTaintedKindIsDetected(): void + { + // disables issue exceptions - we need all, not just the first + $this->testConfig->throw_exception = false; + $this->project_analyzer->trackTaintedInputs(); + // registers test fixture `\Psalm\Tests\fixtures\Issue\TaintedTestingAnything` for + // the custom taint kind `testing-anything`; in addition, the group `input` now would + // expand to `sql`, `html`, ... including `testing-anything` + // (the group assignment is optional) + $this->testConfig->taint_kind_registry->defineKinds([ + 'testing-anything' => TaintedTestingAnything::class, + ], TaintKindGroup::GROUP_INPUT); + + $code = 'addFile($filePath, $code); + $this->analyzeFile($filePath, new Context(), false); + + $actualIssueTypes = array_map( + static fn(IssueData $issue): string => $issue->type . '{ ' . trim($issue->snippet) . ' }', + IssueBuffer::getIssuesDataForFile($filePath), + ); + $expectedIssuesTypes = [ + 'TaintedTestingAnything{ function sinkTestingAnything($value): void {} }', + 'TaintedTestingAnything{ function sinkInput($value): void {} }', + ]; + + self::assertSame($expectedIssuesTypes, $actualIssueTypes); + } + + /** + * @test + */ + public function customTaintGroupIsUsed(): void + { + // disables issue exceptions - we need all, not just the first + $this->testConfig->throw_exception = false; + $this->project_analyzer->trackTaintedInputs(); + $this->testConfig->taint_kind_registry->defineGroup('custom-input', 'html', 'sql'); + + $code = 'addFile($filePath, $code); + $this->analyzeFile($filePath, new Context(), false); + + $actualIssueTypes = array_map( + static fn(IssueData $issue): string => $issue->type . '{ ' . trim($issue->snippet) . ' }', + IssueBuffer::getIssuesDataForFile($filePath), + ); + $expectedIssuesTypes = [ + 'TaintedHtml{ function sinkHtml($value): void {} }', + 'TaintedSql{ function sinkSql($value): void {} }', + ]; + + self::assertSame($expectedIssuesTypes, $actualIssueTypes); + } + + /** + * @test + */ + public function customTaintGroupProxyIsUsed(): void + { + // disables issue exceptions - we need all, not just the first + $this->testConfig->throw_exception = false; + $this->project_analyzer->trackTaintedInputs(); + // custom source `input-sql` is a proxy for all kinds of group `input` (which includes kind `sql`), + // in case the taint `sql` is detected for a proxied `input-sql` scenario; instead of the default + // `TaintedSql` the overridden `TaintedTestingAnything` class would be used + $this->testConfig->taint_kind_registry->defineGroupProxy('input-sql', 'input', [ + 'sql' => TaintedTestingAnything::class, + ]); + + $code = 'addFile($filePath, $code); + $this->analyzeFile($filePath, new Context(), false); + + $actualIssueTypes = array_map( + static fn(IssueData $issue): string => $issue->type . '{ ' . trim($issue->snippet) . ' }', + IssueBuffer::getIssuesDataForFile($filePath), + ); + $expectedIssuesTypes = [ + 'TaintedTestingAnything{ function executeDatabaseQuery($query): array {} }', + 'TaintedSql{ function executeDatabaseQuery($query): array {} }', + ]; + + self::assertSame($expectedIssuesTypes, $actualIssueTypes); + } } diff --git a/tests/fixtures/Issue/TaintedTestingAnything.php b/tests/fixtures/Issue/TaintedTestingAnything.php new file mode 100644 index 00000000000..0bc60cde5f4 --- /dev/null +++ b/tests/fixtures/Issue/TaintedTestingAnything.php @@ -0,0 +1,15 @@ +