Split LDAP into custom category (#4604)

- Adds ldap_escape as sanitizer
- Defines the right parameters to ldap_search as sink
- Wrote documentation
- Added tests

config.xsd

@@ -380,6 +380,7 @@
             <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="TaintedLdap" 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" />

dictionaries/InternalTaintSinkMap.php

@@ -10,7 +10,7 @@ return [
 'fopen' => [['shell']],
 'header' => [['text']],
 'igbinary_unserialize' => [['unserialize']],
-'ldap_search' => [['text']],
+'ldap_search' => [[], ['ldap'], ['ldap']],
 'mysqli_query' => [[], ['sql']],
 'mysqli::query' => [['sql']],
 'mysqli_real_query' => [[], ['sql']],

docs/running_psalm/issues/TaintedLdap.md

@@ -0,0 +1,32 @@
+# TaintedLdap
+
+Potential LDAP injection. This rule is emitted when user-controlled input can be passed into a LDAP request.
+
+## Risk
+
+Passing untrusted user input to LDAP requests could be dangerous. 
+
+If LDAP requests like these are used for login purposes, it could result in an authentication bypass. An attacker could write a filter that would evaluate to `true` for any user, and thus bruteforce credentials easily. 
+
+
+## Example
+
+```php
+<?php
+
+$ds = ldap_connect('example.com');
+$dn = 'o=Psalm, c=US';
+$filter = $_GET['filter']);
+ldap_search($ds, $dn, $filter, []);
+```
+
+## Mitigations
+
+Use [`ldap_escape`](https://www.php.net/manual/en/function.ldap-escape.php) to escape user input to the LDAP filter and DN.
+
+
+## Further resources
+
+- [OWASP Wiki for LDAP Injections](https://owasp.org/www-community/attacks/LDAP_Injection)
+- [LDAP Injection Prevention Cheatsheet](https://cheatsheetseries.owasp.org/cheatsheets/LDAP_Injection_Prevention_Cheat_Sheet.html)
+- [CWE-90](https://cwe.mitre.org/data/definitions/90.html)

src/Psalm/Internal/Codebase/TaintFlowGraph.php

@@ -10,6 +10,7 @@ use Psalm\Issue\TaintedCustom;
 use Psalm\Issue\TaintedEval;
 use Psalm\Issue\TaintedHtml;
 use Psalm\Issue\TaintedInclude;
+use Psalm\Issue\TaintedLdap;
 use Psalm\Issue\TaintedShell;
 use Psalm\Issue\TaintedSSRF;
 use Psalm\Issue\TaintedSql;
@@ -365,6 +366,15 @@ class TaintFlowGraph extends DataFlowGraph
                                 );
                                 break;
 
+                            case TaintKind::INPUT_LDAP:
+                                $issue = new TaintedLdap(
+                                    'Detected tainted LDAP request',
+                                    $issue_location,
+                                    $issue_trace,
+                                    $path
+                                );
+                                break;
+                                
                             default:
                                 $issue = new TaintedCustom(
                                     'Detected tainted ' . $matching_taint,

src/Psalm/Issue/TaintedLdap.php

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

src/Psalm/Type/TaintKind.php

@@ -11,6 +11,7 @@ class TaintKind
     public const INPUT_UNSERIALIZE = 'unserialize';
     public const INPUT_INCLUDE = 'include';
     public const INPUT_EVAL = 'eval';
+    public const INPUT_LDAP = 'ldap';
     public const INPUT_SQL = 'sql';
     public const INPUT_HTML = 'html';
     public const INPUT_SHELL = 'shell';

src/Psalm/Type/TaintKindGroup.php

@@ -16,5 +16,6 @@ class TaintKindGroup
         TaintKind::INPUT_UNSERIALIZE,
         TaintKind::INPUT_INCLUDE,
         TaintKind::INPUT_SSRF,
+        TaintKind::INPUT_LDAP,
     ];
 }

stubs/CoreGenericFunctions.phpstub

@@ -938,3 +938,11 @@ function convert_uudecode(string $data): string
 function convert_uuencode(string $data) : string 
 {
 }
+
+/**
+ * @psalm-pure
+ *
+ * @psalm-taint-escape ldap
+ * @psalm-flow ($value) -> return
+ */
+function ldap_escape(string $value, string $ignore = "", int $flag = 0) : string {}

tests/TaintTest.php

@@ -230,6 +230,13 @@ class TaintTest extends TestCase
                         echo $a;
                     }'
             ],
+            'taintLdapEscape' => [
+                '<?php
+                    $ds = ldap_connect(\'example.com\');
+                    $dn = \'o=Psalm, c=US\';
+                    $filter = ldap_escape($_GET[\'filter\']);
+                    ldap_search($ds, $dn, $filter, []);'
+            ],
             'taintOnStrReplaceCallRemovedInFunction' => [
                 '<?php
                     class U {
@@ -1767,6 +1774,14 @@ class TaintTest extends TestCase
                     echo $b->getTaint();',
                 'error_message' => 'TaintedHtml',
             ],
+            'taintedLdapSearch' => [
+                '<?php
+                    $ds = ldap_connect(\'example.com\');
+                    $dn = \'o=Psalm, c=US\';
+                    $filter = $_GET[\'filter\'];
+                    ldap_search($ds, $dn, $filter, []);',
+                'error_message' => 'TaintedLdap',
+            ],
             /*
             // 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.