fix: add taint source on plugin-added taints

[Patrick-Remy]

Fixes #10057

Problem

Taints not found in psalm v5 using $codebase->addTaintSource() in plugin

With the change of immutability (d0be59e#diff-122c0df94b9a0557e4fd09b8d8c7324f4d6d8fff34f344e1e49318c8e7a0a242), addTaintSource() doesn't manipulate the $expr_type anymore and instead returns the adjusted $expr_type. But this breaks custom-added taint sources via plugin as shown in the docs currently. From plugin perspective you cannot anymore adjust the expression type inside the plugin.

See also my repro here: https://github.com/Patrick-Remy/repro-psalm-5-taint-analysis-issue/blob/master/README.md

<?php

namespace Some\Ns;

use PhpParser;
use Psalm\CodeLocation;
use Psalm\Context;
use Psalm\FileManipulation;
use Psalm\Plugin\EventHandler\AfterExpressionAnalysisInterface;
use Psalm\Plugin\EventHandler\Event\AfterExpressionAnalysisEvent;
use Psalm\Type\TaintKindGroup;

class BadSqlTainter implements AfterExpressionAnalysisInterface
{
    /**
     * Called after an expression has been checked
     *
     * @param  PhpParser\Node\Expr  $expr
     * @param  Context              $context
     * @param  FileManipulation[]   $file_replacements
     *
     * @return void
     */
    public static function afterExpressionAnalysis(AfterExpressionAnalysisEvent $event): ?bool {
        $expr = $event->getExpr();
        $statements_source = $event->getStatementsSource();
        $codebase = $event->getCodebase();
        if ($expr instanceof PhpParser\Node\Expr\Variable
            && $expr->name === 'bad_data'
        ) {
            $expr_type = $statements_source->getNodeTypeProvider()->getType($expr);

            // should be a globally unique id
            // you can use its line number/start offset
            $expr_identifier = '$bad_data'
                . '-' . $statements_source->getFileName()
                . ':' . $expr->getAttribute('startFilePos');

            if ($expr_type) {
                $codebase->addTaintSource(
                    $expr_type,
                    $expr_identifier,
                    TaintKindGroup::ALL_INPUT,
                    new CodeLocation($statements_source, $expr)
                );

                // Custom addition here to check if variable is really tainted
                echo "\n\n\nTAINTED\n\n\n";
            }
        }

        return null;
    }

This is probably an intended behaviour, but there needs to be another way for adding taints by plugin.

Taints not found in psalm v4 + v5 using AddTaintsInterface

I found the undocumented plugin interface AddTaintsInterface that makes it possible to return an array of taints that should be added to analyzed expression.
Unfortunately this also lead to no findings in both psalm v4 and psalm v5.

<?php

namespace Some\Ns;

use PhpParser;
use Psalm\Plugin\EventHandler\AddTaintsInterface;
use Psalm\Plugin\EventHandler\Event\AddRemoveTaintsEvent;
use Psalm\Type\TaintKindGroup;

class BadSqlTainter implements AddTaintsInterface
{

    /**
     * Called to see what taints should be added
     *
     * @return list<string>
     */
    public static function addTaints(AddRemoveTaintsEvent $event): array
    {
        $expr = $event->getExpr();
        if ($expr instanceof PhpParser\Node\Expr\Variable
            && $expr->name === 'bad_data'
        ) {
            echo "\n\n\nTAINTED\n\n\n";
            return TaintKindGroup::ALL_INPUT;
        }

        return [];
    }
}

Analysis

After deep-diving into the taint analysis, I found the reason why addTaints only sometimes could have an effect.
Other than $codebase->addSource() the callees most time do not add TaintSource via data_flow_graph->addSource(). Seems like only those Analyzers that analyze types that can be annotated with @psalm-taint-source like MethodCallReturnTypeFetcher already added those.

Implemented solution

As anywhere where AddRemoveTaintsEvent is dispatched, a plugin could add new taints (also at Taint sinks, which seemed a bit strange to me), a new TaintSource has to be added, so that the already added data flow paths get connected.
I also added a new plugin example and test for addTaints and updated the documentaiton.

Code review request

I would kindly ask for review, as I just have gone to all results of the AddRemoveTaintsEvent and added the required missing steps.
Specially in which scenarios is it required to replace the typeNode and add new parents, like $codebase->addTaintSource() does? For now I have skipped that, as my newly added test case works wihtout modifying the typeNode.

[Patrick-Remy]

I have just refactored my implementation and more important added a new test case that covers the issue.

As now all checks are passing (ignoring the test-with-real-projects), could you please review it, so that this fix can be integrated into 5.x? As unfortunately this currently blocks our upgrade to psalm v5.

[orklah]

Hi, thanks for this work and sorry for not answering earlier.

Does that mean that any existing plugin that may have tried to add taints or sink was broken?

I'm trying to establish the consequences for other existing plugins that may exist to see if this is a BC break or not

[Patrick-Remy]

As far as I know, it may probably still work when adding taints via either addTaintSource() or AddTaintsInterface if there is already some taint source at a node (due to psalm) and you are adding another to the exact same node. But I haven't tested that so far. But as there weren't any tests related to plugin-added taints since now, it could also be completely broken.

There should probably be a full suite of tests related to plugin-added taints. If you wish I can add some more.

[orklah]

More tests are always good :)

Should we deprecated addTaintSource for removal in Psalm 6?

[Patrick-Remy]

I was adding some more tests, and fastly failed with some odd behaviour where I am not sure, if it is due to my changes or if it was already previously.

Using the TaintBadDataPlugin

    public static function addTaints(AddRemoveTaintsEvent $event): array
    {
        $expr = $event->getExpr();

        if ($expr instanceof Variable && $expr->name === 'bad_data') {
            return TaintKindGroup::ALL_INPUT;
        }

        return [];
    }

The following taint is not recognized

<?php // --taint-analysis

$foo = $bad_data;
echo $foo;

whereas this is still recognized:

<?php // --taint-analysis

echo $bad_data;

Anyway I wanted to fix it, but I am still stucking on how inside the AssignmentAnalyzer. Any hints to me?

[Patrick-Remy]

Any ideas @orklah ?