vimeo / psalm

A static analysis tool for finding errors in PHP applications
https://psalm.dev
MIT License
5.57k stars 660 forks source link

Chained @psalm-taint-specialize calls block valid taints #5860

Open ohader opened 3 years ago

ohader commented 3 years ago

While developing a custom plugin and using individual DataFlowNode instances, I was struggling which specialized code locations to use, and for which particular scenarios it is required. For some invocations I got the expected behavior, for others (chained specialized nodes) I got unexpected results - taints were not resolved anymore.

I was able to reproduce the behavior outside my custom implementation with a simplified example at https://psalm.dev/r/a409d0a997

psalm-github-bot[bot] commented 3 years ago

I found these snippets:

https://psalm.dev/r/a409d0a997 ```php return */ function first(string $value): string { return $value; } /** * @psalm-taint-specialize * @##psalm-flow ($value) -> return */ function second(string $value): string { return $value; } $a = (string)($_GET['a'] ?? ''); $b = (string)($_GET['b'] ?? ''); echo first($a); echo second($b); echo first(second($b)); echo second(first($a)); echo first('a'); echo second('b'); echo first(second('b')); echo second(first('a')); ``` ``` Psalm output (using commit 0e8ca1d): ERROR: TaintedHtml - 19:6 - Detected tainted HTML ERROR: TaintedHtml - 20:6 - Detected tainted HTML ```
ohader commented 3 years ago

Another similar behavior (related to @psalm-taint-specialize) can be seen with builder patterns - this use case seems to be more realistic, than my previous example.

https://psalm.dev/r/0807dc3802

Another example, Doctrine DBAL (e.g. https://github.com/psalm/psalm-plugin-doctrine/blob/1.x/stubs/DBAL/QueryBuilder.phpstub) is also implementing builders.

psalm-github-bot[bot] commented 3 years ago

I found these snippets:

https://psalm.dev/r/0807dc3802 ```php value = $value; return $this; } public function getValue(): string { return $this->value; } } exec( (new Subject())->getValue() ); exec( (new Subject())->setValue((string)($_GET['inject'] ?? ''))->getValue() ); ``` ``` Psalm output (using commit cccfe75): ERROR: TaintedShell - 23:7 - Detected tainted shell code ERROR: TaintedShell - 24:7 - Detected tainted shell code ```
orklah commented 3 years ago

Isn't this behaviour correct? https://psalm.dev/r/dfaa9eb1f9

psalm-github-bot[bot] commented 3 years ago

I found these snippets:

https://psalm.dev/r/dfaa9eb1f9 ```php return */ function first(string $value): string { return $value; } /** * @psalm-taint-specialize * @psalm-flow ($value) -> return */ function second(string $value): string { return $value; } $a = (string)($_GET['a'] ?? ''); $b = (string)($_GET['b'] ?? ''); echo first($a); echo second($b); echo first(second($b)); echo second(first($a)); echo first('a'); echo second('b'); echo first(second('b')); echo second(first('a')); ``` ``` Psalm output (using commit 1439647): ERROR: TaintedHtml - 19:6 - Detected tainted HTML ERROR: TaintedTextWithQuotes - 19:6 - Detected tainted text with possible quotes ERROR: TaintedHtml - 20:6 - Detected tainted HTML ERROR: TaintedTextWithQuotes - 20:6 - Detected tainted text with possible quotes ERROR: TaintedHtml - 21:6 - Detected tainted HTML ERROR: TaintedTextWithQuotes - 21:6 - Detected tainted text with possible quotes ERROR: TaintedHtml - 22:6 - Detected tainted HTML ERROR: TaintedTextWithQuotes - 22:6 - Detected tainted text with possible quotes ```
ohader commented 2 years ago

Isn't this behaviour correct? https://psalm.dev/r/dfaa9eb1f9

This looks correct now and something actually changed here - which is good (somewhere between a0739b1..6d45003, cannot be more precise, that was the last time my local tests were failing, with most recent state that particular use case looks good).

However... 😉 https://psalm.dev/r/fb660ce147 still reveals some strange behavior.

psalm-github-bot[bot] commented 2 years ago

I found these snippets:

https://psalm.dev/r/dfaa9eb1f9 ```php return */ function first(string $value): string { return $value; } /** * @psalm-taint-specialize * @psalm-flow ($value) -> return */ function second(string $value): string { return $value; } $a = (string)($_GET['a'] ?? ''); $b = (string)($_GET['b'] ?? ''); echo first($a); echo second($b); echo first(second($b)); echo second(first($a)); echo first('a'); echo second('b'); echo first(second('b')); echo second(first('a')); ``` ``` Psalm output (using commit 6d45003): ERROR: TaintedHtml - 19:6 - Detected tainted HTML ERROR: TaintedTextWithQuotes - 19:6 - Detected tainted text with possible quotes ERROR: TaintedHtml - 20:6 - Detected tainted HTML ERROR: TaintedTextWithQuotes - 20:6 - Detected tainted text with possible quotes ERROR: TaintedHtml - 21:6 - Detected tainted HTML ERROR: TaintedTextWithQuotes - 21:6 - Detected tainted text with possible quotes ERROR: TaintedHtml - 22:6 - Detected tainted HTML ERROR: TaintedTextWithQuotes - 22:6 - Detected tainted text with possible quotes ```
https://psalm.dev/r/fb660ce147 ```php value = $value; return $this; } public function getValue(): string { return $this->value; } } exec( (new Subject())->getValue() ); # comment the following line to spot the difference exec( (new Subject())->setValue((string)($_GET['inject'] ?? ''))->getValue() ); ``` ``` Psalm output (using commit 6d45003): ERROR: TaintedShell - 23:7 - Detected tainted shell code ERROR: TaintedShell - 25:7 - Detected tainted shell code ```