Add simple taint analysis to detect variables containing user input

Author: nicwortel

Diff for: Security/Sniffs/UserInputDetector.php

@@ -5,10 +5,13 @@
 namespace NthRoot\PhpSecuritySniffs\Security\Sniffs;
 
 use PHP_CodeSniffer\Files\File;
+use PHP_CodeSniffer\Util\Tokens;
 
 use function array_slice;
 use function in_array;
+use function range;
 
+use const T_EQUAL;
 use const T_OPEN_PARENTHESIS;
 use const T_VARIABLE;
 
@@ -31,45 +34,83 @@ public function containsVariableInput(File $file, int $start): bool
 
     public function containsUserInput(File $file, int $start): bool
     {
-        $tokens = $this->getArgumentTokens($file, $start);
+        $tokens = $file->getTokens();
 
-        foreach ($tokens as $token) {
-            if ($this->isUserInput($token)) {
+        if ($tokens[$start]['code'] === T_OPEN_PARENTHESIS) {
+            $closingParenthesis = $tokens[$start]['parenthesis_closer'];
+
+            $tokenPositions = range($start + 1, $closingParenthesis - 1);
+        } else {
+            $tokenPositions = range($start, $file->findEndOfStatement($start) - 1);
+        }
+
+        foreach ($tokenPositions as $token) {
+            if ($this->isTainted($file, $token)) {
                 return true;
             }
         }
 
         return false;
     }
 
-    private function getArgumentTokens(File $file, int $start): array
+    /**
+     * Returns true if the token at the given position contains user input.
+     */
+    public function isTainted(File $file, int $tokenPosition): bool
     {
         $tokens = $file->getTokens();
+        $token = $tokens[$tokenPosition];
 
-        if ($tokens[$start]['code'] === T_OPEN_PARENTHESIS) {
-            $closingParenthesis = $tokens[$start]['parenthesis_closer'];
-
-            return array_slice($tokens, $start + 1, $closingParenthesis - $start);
+        if ($token['code'] === T_VARIABLE && in_array($token['content'], self::SUPERGLOBALS, true)) {
+            return true;
         }
 
-        $endOfStatement = $file->findEndOfStatement($start);
+        if ($token['code'] === T_VARIABLE) {
+            $variableName = $token['content'];
 
-        return array_slice($tokens, $start, $endOfStatement - $start);
+            $definition = $this->getVariableDefinition($file, $tokenPosition, $variableName);
+
+            if (!$definition) {
+                // Cannot find the definition of the variable
+                return false;
+            }
+
+            $value = $file->findNext(T_VARIABLE, $definition + 1);
+
+            return $this->isTainted($file, $value);
+        }
+
+        return false;
     }
 
-    /**
-     * @param array{code: int, content: string} $token
-     */
-    private function isUserInput(array $token): bool
+    private function getVariableDefinition(File $file, int $tokenPosition, string $name): int|false
     {
-        if ($token['code'] !== T_VARIABLE) {
-            return false;
-        }
+        $tokens = $file->getTokens();
 
-        if (in_array($token['content'], self::SUPERGLOBALS, true)) {
-            return true;
+        while ($tokenPosition = $file->findPrevious(T_VARIABLE, $tokenPosition - 1, null, false, $name)) {
+            $nextToken = $file->findNext(Tokens::$emptyTokens, $tokenPosition + 1, null, true, null, true);
+            $nextToken = $tokens[$nextToken];
+
+            if ($nextToken['code'] === T_EQUAL) {
+                return $tokenPosition;
+            }
         }
 
         return false;
     }
+
+    private function getArgumentTokens(File $file, int $start): array
+    {
+        $tokens = $file->getTokens();
+
+        if ($tokens[$start]['code'] === T_OPEN_PARENTHESIS) {
+            $closingParenthesis = $tokens[$start]['parenthesis_closer'];
+
+            return array_slice($tokens, $start + 1, $closingParenthesis - $start);
+        }
+
+        $endOfStatement = $file->findEndOfStatement($start);
+
+        return array_slice($tokens, $start, $endOfStatement - $start);
+    }
 }

Diff for: Security/Tests/Injection/OsInjectionUnitTest.Indirect.inc

@@ -0,0 +1,7 @@
+<?php
+
+$target = $_GET['host'];
+
+$foo = $target . ' foo';
+
+shell_exec('ping ' . $foo);

Diff for: Security/Tests/Injection/OsInjectionUnitTest.php

@@ -8,8 +8,14 @@
 
 final class OsInjectionUnitTest extends SecuritySniffTestCase
 {
-    protected function getErrorList(): array
+    protected function getErrorList(string $filename = ''): array
     {
+        if ($filename === 'OsInjectionUnitTest.Indirect.inc') {
+            return [
+                7 => 1,
+            ];
+        }
+
         return [
             3 => 1,
         ];

Diff for: Security/Tests/Utility/UserInputDetectorTest.php

@@ -0,0 +1,84 @@
+<?php
+
+declare(strict_types=1);
+
+namespace NthRoot\PhpSecuritySniffs\Security\Tests\Utility;
+
+use NthRoot\PhpSecuritySniffs\Security\Sniffs\UserInputDetector;
+use PHP_CodeSniffer\Config;
+use PHP_CodeSniffer\Files\LocalFile;
+use PHP_CodeSniffer\Ruleset;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+
+use const T_STRING;
+use const T_VARIABLE;
+
+class UserInputDetectorTest extends TestCase
+{
+    private UserInputDetector $detector;
+
+    protected function setUp(): void
+    {
+        $this->detector = new UserInputDetector();
+    }
+
+    #[DataProvider('getTaintedInputFiles')]
+    public function testMarksSuperglobalsAsTainted(string $filename): void
+    {
+        $file = $this->createFile($filename);
+
+        $position = $file->findNext(T_VARIABLE, 0, null, false, '$_GET');
+
+        $this->assertTrue($this->detector->isTainted($file, $position));
+    }
+
+    public static function getTaintedInputFiles(): array
+    {
+        return [
+            ['direct_input.php'],
+            ['concatenated_input.php'],
+        ];
+    }
+
+    public function testMarksTaintedVariablesAsTainted(): void
+    {
+        $file = $this->createFile('indirect_input.php');
+
+        $position = $file->findNext(T_STRING, 0, null, false, 'shell_exec');
+        $position = $file->findNext(T_VARIABLE, $position, null, false, '$target');
+
+        $this->assertTrue($this->detector->isTainted($file, $position));
+    }
+
+    public function testDetectsDirectUserInput(): void
+    {
+        $file = $this->createFile('direct_input.php');
+
+        $start = $file->findNext(T_STRING, 0, null, false, 'shell_exec');
+
+        $this->assertTrue($this->detector->containsUserInput($file, $start));
+    }
+
+    public function testDetectsIndirectUserInput(): void
+    {
+        $file = $this->createFile('indirect_input.php');
+
+        $start = $file->findNext(T_STRING, 0, null, false, 'shell_exec');
+
+        $this->assertTrue($this->detector->containsUserInput($file, $start));
+    }
+
+    private function createFile(string $path): LocalFile
+    {
+        $config = new Config();
+
+        $path = __DIR__ . '/fixtures/' . $path;
+
+        $file = new LocalFile($path, new Ruleset($config), $config);
+        $file->setContent(file_get_contents($path));
+        $file->parse();
+
+        return $file;
+    }
+}

Diff for: Security/Tests/Utility/fixtures/concatenated_input.php

@@ -0,0 +1,3 @@
+<?php
+
+shell_exec('ping ' . $_GET['target']);

Diff for: Security/Tests/Utility/fixtures/direct_input.php

@@ -0,0 +1,3 @@
+<?php
+
+shell_exec($_GET['target']);

Diff for: Security/Tests/Utility/fixtures/indirect_input.php

@@ -0,0 +1,5 @@
+<?php
+
+$target = $_GET['target'];
+
+shell_exec('ping ' . $target);

Diff for: example/index.php

@@ -4,7 +4,9 @@
 
 require_once $_GET['file'];
 
-shell_exec('ping ' . $_GET['host']);
+$target = $_GET['host'];
+
+shell_exec('ping ' . $target);
 
 echo file_get_contents($_GET['file']);