Skip to content

Commit

Permalink
Add SSRF sinks (#4592)
Browse files Browse the repository at this point in the history
  • Loading branch information
LukasReschke committed Nov 18, 2020
1 parent ab3961d commit 5ba4681
Show file tree
Hide file tree
Showing 8 changed files with 69 additions and 0 deletions.
1 change: 1 addition & 0 deletions config.xsd
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,7 @@
<xs:element name="TaintedInput" type="IssueHandlerType" minOccurs="0" />
<xs:element name="TaintedShell" type="IssueHandlerType" minOccurs="0" />
<xs:element name="TaintedSql" type="IssueHandlerType" minOccurs="0" />
<xs:element name="TaintedSSRF" type="IssueHandlerType" minOccurs="0" />
<xs:element name="TaintedSystemSecret" type="IssueHandlerType" minOccurs="0" />
<xs:element name="TaintedText" type="IssueHandlerType" minOccurs="0" />
<xs:element name="TaintedUnserialize" type="IssueHandlerType" minOccurs="0" />
Expand Down
2 changes: 2 additions & 0 deletions dictionaries/InternalTaintSinkMap.php
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,6 @@
'unserialize' => [['unserialize']],
'popen' => [['shell']],
'proc_open' => [['shell']],
'curl_init' => [['ssrf']],
'curl_setopt' => [[], [], ['ssrf']],
];
36 changes: 36 additions & 0 deletions docs/running_psalm/issues/TaintedSSRF.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# TaintedSSRF

Potential Server-Side Request Forgery vulnerability. This rule is emitted when user-controlled input can be passed into a network request.

## Risk

Passing untrusted user input to network requests could be dangerous.

If an attacker can fully control a HTTP request they could connect to internal services. Depending on the nature of these, this can pose a security risk. (e.g. backend services, admin interfaces, AWS metadata, ...)

## Example

```php
<?php
$ch = curl_init();

curl_setopt($ch, CURLOPT_URL, $_GET['url']);

curl_exec($ch);

curl_close($ch);
```

## Mitigations

Mitigating SSRF vulnerabilities can be tricky. Disallowing IPs would likely not work as an attacker could create a malicious domain that points to an internal DNS name.

Consider:

1. Having an allow list of domains that can be connected to.
2. Pointing cURL to a proxy that has no access to internal resources.

## Further resources

- [OWASP Wiki for Server Side Request Forgery](https://owasp.org/www-community/attacks/Server_Side_Request_Forgery)
- [CWE-918](https://cwe.mitre.org/data/definitions/918)
10 changes: 10 additions & 0 deletions src/Psalm/Internal/Codebase/TaintFlowGraph.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use Psalm\Issue\TaintedHtml;
use Psalm\Issue\TaintedInclude;
use Psalm\Issue\TaintedShell;
use Psalm\Issue\TaintedSSRF;
use Psalm\Issue\TaintedSql;
use Psalm\Issue\TaintedSystemSecret;
use Psalm\Issue\TaintedText;
Expand Down Expand Up @@ -355,6 +356,15 @@ private function getChildNodes(
);
break;

case TaintKind::INPUT_SSRF:
$issue = new TaintedSSRF(
'Detected tainted network request',
$issue_location,
$issue_trace,
$path
);
break;

default:
$issue = new TaintedCustom(
'Detected tainted ' . $matching_taint,
Expand Down
7 changes: 7 additions & 0 deletions src/Psalm/Issue/TaintedSSRF.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php
namespace Psalm\Issue;

class TaintedSSRF extends TaintedInput
{
public const SHORTCODE = 253;
}
1 change: 1 addition & 0 deletions src/Psalm/Type/TaintKind.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ class TaintKind
public const INPUT_SQL = 'sql';
public const INPUT_HTML = 'html';
public const INPUT_SHELL = 'shell';
public const INPUT_SSRF = 'ssrf';
public const USER_SECRET = 'user_secret';
public const SYSTEM_SECRET = 'system_secret';
}
1 change: 1 addition & 0 deletions src/Psalm/Type/TaintKindGroup.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,6 @@ class TaintKindGroup
TaintKind::INPUT_EVAL,
TaintKind::INPUT_UNSERIALIZE,
TaintKind::INPUT_INCLUDE,
TaintKind::INPUT_SSRF,
];
}
11 changes: 11 additions & 0 deletions tests/TaintTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -1640,6 +1640,17 @@ function some_stub(string $r): string {}
$cb = proc_open($_POST[\'x\'], [], []);',
'error_message' => 'TaintedShell',
],
'taintedCurlInit' => [
'<?php
$ch = curl_init($_GET[\'url\']);',
'error_message' => 'TaintedSSRF',
],
'taintedCurlSetOpt' => [
'<?php
$ch = curl_init();
curl_setopt($ch, CURLOPT_URL, $_GET[\'url\']);',
'error_message' => 'TaintedSSRF',
],
/*
// 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.
Expand Down

0 comments on commit 5ba4681

Please sign in to comment.