Skip to content

Commit 78f4a06

Browse files
LukasReschkeLukasReschke
authoredNov 19, 2020
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
1 parent 70c9fd9 commit 78f4a06

12 files changed

+207
-5
lines changed
 

‎config.xsd

+3
Original file line numberDiff line numberDiff line change
@@ -371,8 +371,11 @@
371371
<xs:element name="ReferenceConstraintViolation" type="IssueHandlerType" minOccurs="0" />
372372
<xs:element name="ReservedWord" type="IssueHandlerType" minOccurs="0" />
373373
<xs:element name="StringIncrement" type="IssueHandlerType" minOccurs="0" />
374+
<xs:element name="TaintedCookie" type="IssueHandlerType" minOccurs="0" />
374375
<xs:element name="TaintedCustom" type="IssueHandlerType" minOccurs="0" />
375376
<xs:element name="TaintedEval" type="IssueHandlerType" minOccurs="0" />
377+
<xs:element name="TaintedFile" type="IssueHandlerType" minOccurs="0" />
378+
<xs:element name="TaintedHeader" type="IssueHandlerType" minOccurs="0" />
376379
<xs:element name="TaintedHtml" type="IssueHandlerType" minOccurs="0" />
377380
<xs:element name="TaintedInclude" type="IssueHandlerType" minOccurs="0" />
378381
<xs:element name="TaintedInput" type="IssueHandlerType" minOccurs="0" />

‎dictionaries/InternalTaintSinkMap.php

+19-5
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,24 @@
55
return [
66
'exec' => [['shell']],
77
'create_function' => [['text'], ['eval']],
8-
'file_get_contents' => [['text']],
9-
'file_put_contents' => [['shell']],
10-
'fopen' => [['shell']],
11-
'header' => [['text']],
8+
'file_get_contents' => [['file']],
9+
'file_put_contents' => [['file']],
10+
'fopen' => [['file']],
11+
'unlink' => [['file']],
12+
'copy' => [['file'], ['file']],
13+
'file' => [['file']],
14+
'link' => [['file'], ['file']],
15+
'mkdir' => [['file']],
16+
'move_uploaded_file' => [['file'], ['file']],
17+
'parse_ini_file' => [['file']],
18+
'chown' => [['file']],
19+
'lchown' => [['file']],
20+
'readfile' => [['file']],
21+
'rename' => [['file'], ['file']],
22+
'rmdir' => ['file'],
23+
'header' => [['header']],
24+
'symlink' => [['file']],
25+
'tempnam' => [['file']],
1226
'igbinary_unserialize' => [['unserialize']],
1327
'ldap_search' => [[], ['ldap'], ['ldap']],
1428
'mysqli_query' => [[], ['sql']],
@@ -35,7 +49,7 @@
3549
'pg_send_prepare' => [[], [], ['sql']],
3650
'pg_send_query' => [[], ['sql']],
3751
'pg_send_query_params' => [[], ['sql'], []],
38-
'setcookie' => [['text'], ['text']],
52+
'setcookie' => [['cookie'], ['cookie']],
3953
'shell_exec' => [['shell']],
4054
'system' => [['shell']],
4155
'unserialize' => [['unserialize']],

‎docs/running_psalm/issues/TaintedCookie.md

+34
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
# TaintedCookie
2+
3+
Potential cookie injection. This rule is emitted when user-controlled input can be passed into a cookie.
4+
5+
## Risk
6+
7+
The risk of setting arbitrary cookies depends on further application configuration.
8+
9+
Examples of potential issues:
10+
11+
- 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.
12+
- Cross-Site-Scripting (XSS): Some application code could read cookies and print it out unsanitized to the user.
13+
14+
15+
16+
## Example
17+
18+
```php
19+
<?php
20+
21+
setcookie('authtoken', $_GET['value'], time() + (86400 * 30), '/');
22+
```
23+
24+
## Mitigations
25+
26+
If this is required functionality, limit the cookie setting to values and not the name. (e.g. `authtoken` in the example)
27+
28+
Make sure to change session tokens after authentication attempts.
29+
30+
## Further resources
31+
32+
- [OWASP Wiki for Session fixation](https://owasp.org/www-community/attacks/Session_fixation)
33+
- [Session Management Cheatsheet](https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html)
34+
- [CWE-384](https://cwe.mitre.org/data/definitions/384.html)

‎docs/running_psalm/issues/TaintedFile.md

+43
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
# TaintedFile
2+
3+
This rule is emitted when user-controlled input can be passed into a sensitive file operation.
4+
5+
## Risk
6+
7+
The risk here depends on the actual operation that contains user-controlled input, and how it is later on processed.
8+
9+
It could range from:
10+
11+
- Creating files
12+
- Example: `file_put_contents`
13+
- Risk: Depending on the server configuration this may result in remote code execution. (e.g. writing a file in the web root)
14+
- Modifying files
15+
- Example: `file_put_contents`
16+
- Risk: Depending on the server configuration this may result in remote code execution. (e.g. modifying a PHP file)
17+
- Reading files
18+
- Example: `file_get_contents`
19+
- Risk: Sensitive data could be exposed from the filesystem. (e.g. config values, source code, user-submitted files)
20+
- Deleting files
21+
- Example: `unlink`
22+
- Risk: Denial of Service or potentially RCE. (e.g. deleting application code, removing a .htaccess file)
23+
24+
## Example
25+
26+
```php
27+
<?php
28+
29+
$content = file_get_contents($_GET['header']);
30+
echo $content;
31+
```
32+
33+
## Mitigations
34+
35+
Use an allowlist approach where possible to verify names on file operations.
36+
37+
Sanitize user-controlled filenames by stripping `..`, `\` and `/`.
38+
39+
## Further resources
40+
41+
- [File Upload Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/File_Upload_Cheat_Sheet.html)
42+
- [OWASP Wiki for Unrestricted FIle Upload](https://owasp.org/www-community/vulnerabilities/Unrestricted_File_Upload)
43+
- [CWE-73](https://cwe.mitre.org/data/definitions/73.html)

‎docs/running_psalm/issues/TaintedHeader.md

+36
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
# TaintedHeader
2+
3+
Potential header injection. This rule is emitted when user-controlled input can be passed into a HTTP header.
4+
5+
## Risk
6+
7+
The risk of a header injection depends hugely on your environment.
8+
9+
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.
10+
11+
If your system does not do that, there may be other concerns, such as:
12+
13+
- Cookie Injection
14+
- Open Redirects
15+
- Proxy Cache Poisoning
16+
17+
## Example
18+
19+
```php
20+
<?php
21+
22+
header($_GET['header']);
23+
```
24+
25+
## Mitigations
26+
27+
Make sure only the value and not the key can be set by an attacker. (e.g. `header('Location: ' . $_GET['target']);`)
28+
29+
Verify the set values are sensible. Consider using an allow list. (e.g. for redirections)
30+
31+
## Further resources
32+
33+
- [Unvalidated Redirects and Forwards Cheat Sheet](https://cheatsheetseries.owasp.org/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.html)
34+
- [OWASP Wiki for Cache Poisoning](https://owasp.org/www-community/attacks/Cache_Poisoning)
35+
- [CWE-601](https://cwe.mitre.org/data/definitions/601.html)
36+
- [CWE-644](https://cwe.mitre.org/data/definitions/644.html)

‎src/Psalm/Internal/Codebase/TaintFlowGraph.php

+30
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,11 @@
66
use Psalm\Internal\DataFlow\DataFlowNode;
77
use Psalm\Internal\DataFlow\TaintSink;
88
use Psalm\Internal\DataFlow\TaintSource;
9+
use Psalm\Issue\TaintedCookie;
910
use Psalm\Issue\TaintedCustom;
1011
use Psalm\Issue\TaintedEval;
12+
use Psalm\Issue\TaintedFile;
13+
use Psalm\Issue\TaintedHeader;
1114
use Psalm\Issue\TaintedHtml;
1215
use Psalm\Issue\TaintedInclude;
1316
use Psalm\Issue\TaintedLdap;
@@ -375,6 +378,33 @@ private function getChildNodes(
375378
);
376379
break;
377380

381+
case TaintKind::INPUT_COOKIE:
382+
$issue = new TaintedCookie(
383+
'Detected tainted cookie',
384+
$issue_location,
385+
$issue_trace,
386+
$path
387+
);
388+
break;
389+
390+
case TaintKind::INPUT_FILE:
391+
$issue = new TaintedFile(
392+
'Detected tainted file handling',
393+
$issue_location,
394+
$issue_trace,
395+
$path
396+
);
397+
break;
398+
399+
case TaintKind::INPUT_HEADER:
400+
$issue = new TaintedHeader(
401+
'Detected tainted header',
402+
$issue_location,
403+
$issue_trace,
404+
$path
405+
);
406+
break;
407+
378408
default:
379409
$issue = new TaintedCustom(
380410
'Detected tainted ' . $matching_taint,

‎src/Psalm/Issue/TaintedCookie.php

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
<?php
2+
namespace Psalm\Issue;
3+
4+
class TaintedCookie extends TaintedInput
5+
{
6+
public const SHORTCODE = 257;
7+
}

‎src/Psalm/Issue/TaintedFile.php

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
<?php
2+
namespace Psalm\Issue;
3+
4+
class TaintedFile extends TaintedInput
5+
{
6+
public const SHORTCODE = 255;
7+
}

‎src/Psalm/Issue/TaintedHeader.php

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
<?php
2+
namespace Psalm\Issue;
3+
4+
class TaintedHeader extends TaintedInput
5+
{
6+
public const SHORTCODE = 256;
7+
}

‎src/Psalm/Type/TaintKind.php

+3
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@ class TaintKind
1616
public const INPUT_HTML = 'html';
1717
public const INPUT_SHELL = 'shell';
1818
public const INPUT_SSRF = 'ssrf';
19+
public const INPUT_FILE = 'file';
20+
public const INPUT_COOKIE = 'cookie';
21+
public const INPUT_HEADER = 'header';
1922
public const USER_SECRET = 'user_secret';
2023
public const SYSTEM_SECRET = 'system_secret';
2124
}

‎src/Psalm/Type/TaintKindGroup.php

+3
Original file line numberDiff line numberDiff line change
@@ -17,5 +17,8 @@ class TaintKindGroup
1717
TaintKind::INPUT_INCLUDE,
1818
TaintKind::INPUT_SSRF,
1919
TaintKind::INPUT_LDAP,
20+
TaintKind::INPUT_FILE,
21+
TaintKind::INPUT_HEADER,
22+
TaintKind::INPUT_COOKIE,
2023
];
2124
}

‎tests/TaintTest.php

+15
Original file line numberDiff line numberDiff line change
@@ -1810,6 +1810,21 @@ public function __construct($taint) {
18101810
ldap_search($ds, $dn, $filter, []);',
18111811
'error_message' => 'TaintedLdap',
18121812
],
1813+
'taintedFile' => [
1814+
'<?php
1815+
file_get_contents($_GET[\'taint\']);',
1816+
'error_message' => 'TaintedFile',
1817+
],
1818+
'taintedHeader' => [
1819+
'<?php
1820+
header($_GET[\'taint\']);',
1821+
'error_message' => 'TaintedHeader',
1822+
],
1823+
'taintedCookie' => [
1824+
'<?php
1825+
setcookie($_GET[\'taint\'], \'value\');',
1826+
'error_message' => 'TaintedCookie',
1827+
],
18131828
'potentialTaintThroughChildClassSettingProperty' => [
18141829
'<?php
18151830
class A {

0 commit comments

Comments
 (0)