Merge pull request #6812 from orklah/shell_exec_taint

backticks shell_exec taint
vimeo · Nov 4, 2021 · c2b14e2 · c2b14e2
2 parents 034cb59 + ff83c49
commit c2b14e2
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 2 deletions.
diff --git a/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php
@@ -6,18 +6,21 @@
 use Psalm\Config;
 use Psalm\Context;
 use Psalm\Internal\Analyzer\ClosureAnalyzer;
+use Psalm\Internal\Analyzer\Statements\Expression\Call\ArgumentAnalyzer;
 use Psalm\Internal\Analyzer\Statements\Expression\Call\FunctionCallAnalyzer;
 use Psalm\Internal\Analyzer\Statements\Expression\Call\MethodCallAnalyzer;
 use Psalm\Internal\Analyzer\Statements\Expression\Call\NewAnalyzer;
 use Psalm\Internal\Analyzer\Statements\Expression\Call\StaticCallAnalyzer;
 use Psalm\Internal\Analyzer\StatementsAnalyzer;
-use Psalm\Internal\Codebase\VariableUseGraph;
+use Psalm\Internal\Codebase\TaintFlowGraph;
 use Psalm\Internal\DataFlow\DataFlowNode;
+use Psalm\Internal\DataFlow\TaintSink;
 use Psalm\Internal\FileManipulation\FileManipulationBuffer;
 use Psalm\Issue\ForbiddenCode;
 use Psalm\Issue\UnrecognizedExpression;
 use Psalm\IssueBuffer;
 use Psalm\Plugin\EventHandler\Event\AfterExpressionAnalysisEvent;
+use Psalm\Storage\FunctionLikeParameter;
 use Psalm\Type;
 
 use function get_class;
@@ -338,7 +341,23 @@ private static function handleExpression(
         }
 
         if ($stmt instanceof PhpParser\Node\Expr\ShellExec) {
-            if ($statements_analyzer->data_flow_graph instanceof VariableUseGraph) {
+            if ($statements_analyzer->data_flow_graph) {
+                $call_location = new CodeLocation($statements_analyzer->getSource(), $stmt);
+
+                if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) {
+                    $sink = TaintSink::getForMethodArgument(
+                        'shell_exec',
+                        'shell_exec',
+                        0,
+                        null,
+                        $call_location
+                    );
+
+                    $sink->taints = [\Psalm\Type\TaintKind::INPUT_SHELL];
+
+                    $statements_analyzer->data_flow_graph->addSink($sink);
+                }
+
                 foreach ($stmt->parts as $part) {
                     if ($part instanceof PhpParser\Node\Expr\Variable) {
                         if (self::analyze($statements_analyzer, $part, $context) === false) {
@@ -350,6 +369,32 @@ private static function handleExpression(
                             break;
                         }
 
+                        $shell_exec_param = new FunctionLikeParameter(
+                            'var',
+                            false
+                        );
+
+                        if (ArgumentAnalyzer::verifyType(
+                            $statements_analyzer,
+                            $expr_type,
+                            Type::getString(),
+                            null,
+                            'shell_exec',
+                            null,
+                            0,
+                            $call_location,
+                            $stmt,
+                            $context,
+                            $shell_exec_param,
+                            false,
+                            null,
+                            true,
+                            true,
+                            new CodeLocation($statements_analyzer, $stmt)
+                        ) === false) {
+                            return false;
+                        }
+
                         foreach ($expr_type->parent_nodes as $parent_node) {
                             $statements_analyzer->data_flow_graph->addPath(
                                 $parent_node,

diff --git a/tests/TaintTest.php b/tests/TaintTest.php
@@ -2172,6 +2172,14 @@ function foo(array $arr) : void {
                     foo([$_GET["a"]]);',
                 'error_message' => 'TaintedHtml',
             ],
+            'shellExecBacktick' => [
+                '<?php
+
+                    $input = $_GET["input"];
+                    $x = `$input`;
+                    ',
+                'error_message' => 'TaintedShell',
+            ],
             /*
             // TODO: Stubs do not support this type of inference even with $this->message = $message.
             // Most uses of getMessage() would be with caught exceptions, so this is not representative of real code.