Merge pull request vimeo#10417 from cgocast/tainted_extract

TaintedExtract

UPGRADING.md

@@ -17,7 +17,7 @@
 
 - [BC] Class `Psalm\Issue\MixedInferredReturnType` was removed
 
-- [BC] Value of constant `Psalm\Type\TaintKindGroup::ALL_INPUT` changed to reflect new `TaintKind::INPUT_SLEEP` and `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.
+- [BC] Value of constant `Psalm\Type\TaintKindGroup::ALL_INPUT` changed to reflect new `TaintKind::INPUT_EXTRACT`, `TaintKind::INPUT_SLEEP` and `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.
 
 - [BC] Property `Config::$shepherd_host` was replaced with `Config::$shepherd_endpoint`
 

config.xsd

@@ -433,6 +433,7 @@
             <xs:element name="TaintedCookie" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="TaintedCustom" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="TaintedEval" type="IssueHandlerType" minOccurs="0" />
+            <xs:element name="TaintedExtract" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="TaintedFile" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="TaintedHeader" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="TaintedHtml" type="IssueHandlerType" minOccurs="0" />

docs/running_psalm/error_levels.md

@@ -286,6 +286,7 @@ Level 5 and above allows a more non-verifiable code, and higher levels are even
  - [TaintedCookie](issues/TaintedCookie.md)
  - [TaintedCustom](issues/TaintedCustom.md)
  - [TaintedEval](issues/TaintedEval.md)
+ - [TaintedExtract](issues/TaintedExtract.md)
  - [TaintedFile](issues/TaintedFile.md)
  - [TaintedHeader](issues/TaintedHeader.md)
  - [TaintedHtml](issues/TaintedHtml.md)

docs/running_psalm/issues.md

@@ -234,6 +234,7 @@
  - [TaintedCookie](issues/TaintedCookie.md)
  - [TaintedCustom](issues/TaintedCustom.md)
  - [TaintedEval](issues/TaintedEval.md)
+ - [TaintedExtract](issues/TaintedExtract.md)
  - [TaintedFile](issues/TaintedFile.md)
  - [TaintedHeader](issues/TaintedHeader.md)
  - [TaintedHtml](issues/TaintedHtml.md)

docs/running_psalm/issues/TaintedExtract.md

@@ -0,0 +1,10 @@
+# TaintedExtract
+
+Emitted when user-controlled array can be passed into an `extract` call.
+
+```php
+<?php
+
+$array = $_GET;
+extract($array);
+```

src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentsAnalyzer.php

@@ -1270,7 +1270,7 @@ private static function handleByRefFunctionArg(
 
         $builtin_array_functions = [
             'ksort', 'asort', 'krsort', 'arsort', 'natcasesort', 'natsort',
-            'reset', 'end', 'next', 'prev', 'array_pop', 'array_shift',
+            'reset', 'end', 'next', 'prev', 'array_pop', 'array_shift', 'extract',
         ];
 
         if (($var_id && isset($context->vars_in_scope[$var_id]))

src/Psalm/Internal/Codebase/TaintFlowGraph.php

@@ -14,6 +14,7 @@
 use Psalm\Issue\TaintedCookie;
 use Psalm\Issue\TaintedCustom;
 use Psalm\Issue\TaintedEval;
+use Psalm\Issue\TaintedExtract;
 use Psalm\Issue\TaintedFile;
 use Psalm\Issue\TaintedHeader;
 use Psalm\Issue\TaintedHtml;
@@ -471,6 +472,15 @@ private function getChildNodes(
                                 );
                                 break;
 
+                            case TaintKind::INPUT_EXTRACT:
+                                $issue = new TaintedExtract(
+                                    'Detected tainted extract',
+                                    $issue_location,
+                                    $issue_trace,
+                                    $path,
+                                );
+                                break;
+
                             default:
                                 $issue = new TaintedCustom(
                                     'Detected tainted ' . $matching_taint,

src/Psalm/Issue/TaintedExtract.php

@@ -0,0 +1,10 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Psalm\Issue;
+
+final class TaintedExtract extends TaintedInput
+{
+    public const SHORTCODE = 327;
+}

src/Psalm/Type/TaintKind.php

@@ -24,6 +24,7 @@ final class TaintKind
     public const INPUT_HEADER = 'header';
     public const INPUT_XPATH = 'xpath';
     public const INPUT_SLEEP = 'sleep';
+    public const INPUT_EXTRACT = 'extract';
     public const USER_SECRET = 'user_secret';
     public const SYSTEM_SECRET = 'system_secret';
 }

src/Psalm/Type/TaintKindGroup.php

@@ -25,5 +25,6 @@ final class TaintKindGroup
         TaintKind::INPUT_COOKIE,
         TaintKind::INPUT_XPATH,
         TaintKind::INPUT_SLEEP,
+        TaintKind::INPUT_EXTRACT,
     ];
 }

stubs/CoreGenericFunctions.phpstub

@@ -1825,3 +1825,8 @@ function time_sleep_until(float $timestamp): bool {}
  * @psalm-ignore-falsable-return
  */
 function get_browser(?string $user_agent = null, bool $return_array = false): object|array|false {}
+
+/**
+ * @psalm-taint-sink extract $array
+ */
+function extract(array &$array, int $flags = EXTR_OVERWRITE, string $prefix = ""): int {}

tests/TaintTest.php

@@ -2587,6 +2587,17 @@ function evaluateExpression(DOMXPath $xpath) : mixed {
                     time_sleep_until($_GET["timestamp"]);',
                 'error_message' => 'TaintedSleep',
             ],
+            'taintedExtract' => [
+                'code' => '<?php
+                    $array = $_GET;
+                    extract($array);',
+                'error_message' => 'TaintedExtract',
+            ],
+            'extractPost' => [
+                'code' => '<?php
+                    extract($_POST);',
+                'error_message' => 'TaintedExtract',
+            ],
         ];
     }