Global type not respected during taint analysis

[mmcev106]

With the following psalm.xml:

<?xml version="1.0"?>
<psalm>
    <projectFiles>
        <directory name="." />
    </projectFiles>
    <globals>
        <var name="connection" type="mysqli" />
    </globals>
</psalm>

...and the following file.php:

<?php
$connection->query($_GET['injection']);
// function a(mysqli $b){}

A taint is not reported, even though it should be. Strangely, if you uncomment the function a... line, the taint is reported correctly. It's like psalm doesn't become aware of the mysqli type unless it is referenced in an unrelated location.

This example was distilled from an actual use case in a much larger code base. Let me know if there is any more information I can provide, or if you have a hunch that it's related to a particular part of the psalm source that I could look into.

[psalm-github-bot[bot]]

Hey @mmcev106, can you reproduce the issue on https://psalm.dev ?

[orklah]

Good catch!

I have a small guess that there's a mistake here: https://github.com/vimeo/psalm/blob/f9450656e1666fbf6ceb8b419b2d02d065fb76b5/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/VariableFetchAnalyzer.php#L155

This class is in charge of setting the type for a given variable we just read. For example when Psalm sees $a = $b; it will decompose that in a VariableFetch for $b and then an assignment in $a.

The specific line I linked is in charge of handling the case when we have a read on a global variable (and it taints it if necessary). However, it only does that if the variable name is recognized by self::isSuperGlobal() which only checks hard coded global variables: https://github.com/vimeo/psalm/blob/f9450656e1666fbf6ceb8b419b2d02d065fb76b5/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/VariableFetchAnalyzer.php#L37

This is not optimal because the <global> tag in config extends this list in theory (you can see that on VariableFetchAnalyzer::getGlobalType(), it's much more extended)

So, I think we need a VariableFetchAnalyzer::isGlobalVariable() which checks if the name is a known global (the same way getGlobalTypes checks it) and use this function instead of isSuperGlobal.

I think isSuperGlobal would be ultimately useless after being replaced by isGlobalVariable but it would need a few tests to check that

Are you up to try? :)

[mmcev106]

Thanks very much for the info. It might be a while before I can prioritize it, but I would like to try! It won't hurt my feelings if anyone beats me to it though.