Add dedicated types for 'file', 'header' and 'cookie' (#4630)

* [WIP] Add dedicated sinks for 'file', 'header' and 'cookie'

* Add documentation

* Add mapping for taint flows

* Add tests

* Fix test

config.xsd

@@ -371,8 +371,11 @@
             <xs:element name="ReferenceConstraintViolation" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="ReservedWord" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="StringIncrement" type="IssueHandlerType" minOccurs="0" />
+            <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="TaintedFile" type="IssueHandlerType" minOccurs="0" />
+            <xs:element name="TaintedHeader" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="TaintedHtml" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="TaintedInclude" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="TaintedInput" type="IssueHandlerType" minOccurs="0" />

dictionaries/InternalTaintSinkMap.php

@@ -5,10 +5,24 @@
 return [
 'exec' => [['shell']],
 'create_function' => [['text'], ['eval']],
-'file_get_contents' => [['text']],
-'file_put_contents' => [['shell']],
-'fopen' => [['shell']],
-'header' => [['text']],
+'file_get_contents' => [['file']],
+'file_put_contents' => [['file']],
+'fopen' => [['file']],
+'unlink' => [['file']],
+'copy' => [['file'], ['file']],
+'file' => [['file']],
+'link' => [['file'], ['file']],
+'mkdir' => [['file']],
+'move_uploaded_file' => [['file'], ['file']],
+'parse_ini_file' => [['file']],
+'chown' => [['file']],
+'lchown' => [['file']],
+'readfile' => [['file']],
+'rename' => [['file'], ['file']],
+'rmdir' => ['file'],
+'header' => [['header']],
+'symlink' => [['file']],
+'tempnam' => [['file']],
 'igbinary_unserialize' => [['unserialize']],
 'ldap_search' => [[], ['ldap'], ['ldap']],
 'mysqli_query' => [[], ['sql']],
@@ -35,7 +49,7 @@ return [
 'pg_send_prepare' => [[], [], ['sql']],
 'pg_send_query' => [[], ['sql']],
 'pg_send_query_params' => [[], ['sql'], []],
-'setcookie' => [['text'], ['text']],
+'setcookie' => [['cookie'], ['cookie']],
 'shell_exec' => [['shell']],
 'system' => [['shell']],
 'unserialize' => [['unserialize']],

docs/running_psalm/issues/TaintedCookie.md

@@ -0,0 +1,34 @@
+# TaintedCookie
+
+Potential cookie injection. This rule is emitted when user-controlled input can be passed into a cookie.
+
+## Risk
+
+The risk of setting arbitrary cookies depends on further application configuration. 
+
+Examples of potential issues:
+
+- Session Fixation: If the authentication cookie doesn't change after a successful login an attacker could fixate the session cookie. If a victim logs in with a fixated cookie, the attacker can now take over the session of the user.
+- Cross-Site-Scripting (XSS): Some application code could read cookies and print it out unsanitized to the user.
+
+
+
+## Example
+
+```php
+<?php
+
+setcookie('authtoken', $_GET['value'], time() + (86400 * 30), '/');
+```
+
+## Mitigations
+
+If this is required functionality, limit the cookie setting to values and not the name. (e.g. `authtoken` in the example)
+
+Make sure to change session tokens after authentication attempts.
+
+## Further resources
+
+- [OWASP Wiki for Session fixation](https://owasp.org/www-community/attacks/Session_fixation)
+- [Session Management Cheatsheet](https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html)
+- [CWE-384](https://cwe.mitre.org/data/definitions/384.html)

docs/running_psalm/issues/TaintedFile.md

@@ -0,0 +1,43 @@
+# TaintedFile
+
+This rule is emitted when user-controlled input can be passed into a sensitive file operation.
+
+## Risk
+
+The risk here depends on the actual operation that contains user-controlled input, and how it is later on processed.
+
+It could range from:
+
+- Creating files
+  - Example: `file_put_contents`
+  - Risk: Depending on the server configuration this may result in remote code execution. (e.g. writing a file in the web root)
+- Modifying files
+  - Example: `file_put_contents`
+  - Risk: Depending on the server configuration this may result in remote code execution. (e.g. modifying a PHP file)
+- Reading files 
+  - Example: `file_get_contents`
+  - Risk: Sensitive data could be exposed from the filesystem. (e.g. config values, source code, user-submitted files)
+- Deleting files
+  - Example: `unlink`
+  - Risk: Denial of Service or potentially RCE. (e.g. deleting application code, removing a .htaccess file)
+
+## Example
+
+```php
+<?php
+
+$content = file_get_contents($_GET['header']);
+echo $content;
+```
+
+## Mitigations
+
+Use an allowlist approach where possible to verify names on file operations.
+
+Sanitize user-controlled filenames by stripping `..`, `\` and `/`.
+
+## Further resources
+
+- [File Upload Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/File_Upload_Cheat_Sheet.html)
+- [OWASP Wiki for Unrestricted FIle Upload](https://owasp.org/www-community/vulnerabilities/Unrestricted_File_Upload)
+- [CWE-73](https://cwe.mitre.org/data/definitions/73.html)

docs/running_psalm/issues/TaintedHeader.md

@@ -0,0 +1,36 @@
+# TaintedHeader
+
+Potential header injection. This rule is emitted when user-controlled input can be passed into a HTTP header.
+
+## Risk
+
+The risk of a header injection depends hugely on your environment.
+
+If your webserver supports something like [`XSendFile`](https://www.nginx.com/resources/wiki/start/topics/examples/xsendfile/) / [`X-Accel`](https://www.nginx.com/resources/wiki/start/topics/examples/x-accel/), an attacker could potentially access arbitrary files on the systems.
+
+If your system does not do that, there may be other concerns, such as:
+
+- Cookie Injection
+- Open Redirects
+- Proxy Cache Poisoning
+
+## Example
+
+```php
+<?php
+
+header($_GET['header']);
+```
+
+## Mitigations
+
+Make sure only the value and not the key can be set by an attacker. (e.g. `header('Location: ' . $_GET['target']);`)
+
+Verify the set values are sensible. Consider using an allow list. (e.g. for redirections)
+
+## Further resources
+
+- [Unvalidated Redirects and Forwards Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.html)
+- [OWASP Wiki for Cache Poisoning](https://owasp.org/www-community/attacks/Cache_Poisoning)
+- [CWE-601](https://cwe.mitre.org/data/definitions/601.html)
+- [CWE-644](https://cwe.mitre.org/data/definitions/644.html)

src/Psalm/Internal/Codebase/TaintFlowGraph.php

@@ -6,8 +6,11 @@ use Psalm\CodeLocation;
 use Psalm\Internal\DataFlow\DataFlowNode;
 use Psalm\Internal\DataFlow\TaintSink;
 use Psalm\Internal\DataFlow\TaintSource;
+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;
@@ -375,6 +378,33 @@ class TaintFlowGraph extends DataFlowGraph
                                 );
                                 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,

src/Psalm/Issue/TaintedCookie.php

@@ -0,0 +1,7 @@
+<?php
+namespace Psalm\Issue;
+
+class TaintedCookie extends TaintedInput
+{
+    public const SHORTCODE = 257;
+}

src/Psalm/Issue/TaintedFile.php

@@ -0,0 +1,7 @@
+<?php
+namespace Psalm\Issue;
+
+class TaintedFile extends TaintedInput
+{
+    public const SHORTCODE = 255;
+}

src/Psalm/Issue/TaintedHeader.php

@@ -0,0 +1,7 @@
+<?php
+namespace Psalm\Issue;
+
+class TaintedHeader extends TaintedInput
+{
+    public const SHORTCODE = 256;
+}

src/Psalm/Type/TaintKind.php

@@ -16,6 +16,9 @@ class TaintKind
     public const INPUT_HTML = 'html';
     public const INPUT_SHELL = 'shell';
     public const INPUT_SSRF = 'ssrf';
+    public const INPUT_FILE = 'file';
+    public const INPUT_COOKIE = 'cookie';
+    public const INPUT_HEADER = 'header';
     public const USER_SECRET = 'user_secret';
     public const SYSTEM_SECRET = 'system_secret';
 }

src/Psalm/Type/TaintKindGroup.php

@@ -17,5 +17,8 @@ class TaintKindGroup
         TaintKind::INPUT_INCLUDE,
         TaintKind::INPUT_SSRF,
         TaintKind::INPUT_LDAP,
+        TaintKind::INPUT_FILE,
+        TaintKind::INPUT_HEADER,
+        TaintKind::INPUT_COOKIE,
     ];
 }

tests/TaintTest.php

@@ -1810,6 +1810,21 @@ class TaintTest extends TestCase
                     ldap_search($ds, $dn, $filter, []);',
                 'error_message' => 'TaintedLdap',
             ],
+            'taintedFile' => [
+                '<?php
+                file_get_contents($_GET[\'taint\']);',
+            'error_message' => 'TaintedFile',
+            ],
+            'taintedHeader' => [
+                '<?php
+                header($_GET[\'taint\']);',
+            'error_message' => 'TaintedHeader',
+            ],
+            'taintedCookie' => [
+                '<?php
+                setcookie($_GET[\'taint\'], \'value\');',
+            'error_message' => 'TaintedCookie',
+            ],
             'potentialTaintThroughChildClassSettingProperty' => [
                 '<?php
                     class A {