Should we consider LDAP methods to be taint sources?

[mmcev106]

We got a request from our security team to consider data in LDAP to be a taint source. Should the following report a taint?

https://psalm.dev/r/14f897da5a

If so, I'm happy to work on a PR.

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/14f897da5a

```php "; } ``` ``` Psalm output (using commit a75d26a): No issues! ```

[mmcev106]

I went through all PHP's LDAP methods and identified four that could be a taint risk. I've added the following in a stub in our project for now. If/when anyone is able to consider this, I'm still happy to create PR if this behavior is desired by default.

/**
 * @psalm-taint-source input
 */
function ldap_get_attributes() {}

/**
 * @psalm-taint-source input
 */
function ldap_get_values_len() {}

/**
 * @psalm-taint-source input
 */
function ldap_get_values() {}

/**
 * @psalm-taint-source input
 */
function ldap_get_entries() {}

[weirdan]

I'd say it depends on whether your LDAP server is within your secure perimeter, and it likely varies from system to system. For some systems (e.g. phpMyAdmin) even the things we usually think of as an internal component (e.g. database) could be a taint source, especially if other systems have access to that.

[mmcev106]

@weirdan, true, that does mitigate risk significantly. There may still be an argument for being more restrictive by default. I think our security team was specifically concerned about preventing an exploit of a system that manages LDAP entries from leading to greater exploits by injecting JS into PHP applications that reference those LDAP entries (e.g. XSS to mimic login pages & capture passwords). I suppose the risk would be similar if an employee with access only to LDAP systems had malicious intent and wanted to gain access to other systems.

[weirdan]

Perhaps it would make sense to add a config section to turn on / off taintedness for entire groups of functions, e.g.:

<taintSources
   db="true" // PDO, MySQLi, PostgreSQL, OCI, etc
   cache="false" // Memcached, Redis, apc (?)
   config="false" // zookeeper, etc
   ldap="true" // ldap...
   queue="true" // beanstalk, amqp, etc...
   fs="false" // file_get_contents && friends
/>

[mmcev106]

@weirdan, I can't justify working on all those options, but might be able to create a PR that adds support for that config section and a single ldap option to start. Would that PR be considered if I worked on it? If so, do you have a preference on the default value for the ldap option?

[weirdan]

Sure, but I'd like to discuss this with other maintainers and lay out a rough design first. Adding new config options is easy, changing or removing them later is less so.

One question would be whether we want to allow userland libs to opt in into some taint groups. For example, amphp/redis could possibly want to be included into taintSources.cache.

[weirdan]

@danog @orklah thoughts? And also, are there any other contributors we may want to involve? Someone particularly active in taint analysis part perhaps?