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

1. 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. At some of those places where AddRemoveTaintsEvent was implemented, it was intended to only remove events, instead of also allowing it to add new taints
2. At some places AddRemoveTaintsEvent wasn't called before, so it wasn't triggered to analyze some expressions and therefor it was impossible to add taints in some situations
3. I also added a new 5.x+ compatible plugin example and test for addTaints and updated the documentaiton.
4. Independently of AddRemoveTaintsEvent method-return taints weren't correctly inherited by return statements. Multiple return statements now merge and store the taints in the function-storage. When the return type of a method call gets analyzed, a taint source now gets created by the function storage.

[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 ?

[Patrick-Remy]

It's finally done! After a lot of work and deeply digging into the taint system of psalm, I was able to fix many taint bugs since 5.x and added a bunch of tests, that were previously failing. I updated the initial comment for reference.

As @danog is extremely active currently, I would kindly ask to review this and hoping for a fast release, as most (adding) taint plugins are broken since 1 1/2 year right now!

[Patrick-Remy]

Someone? Maybe @orklah?

[orklah]

Thanks!

Sorry for not answering earlier and congrats on this massive work!

[Patrick-Remy]

Thank you so much! 😊

[danog]

Hi, looking through the taint system now, shouldn't this be $node->unspecialized_id ?? $node->id to avoid re-specialization of already specalized nodes?

[Patrick-Remy]

Honestly, I have no idea... maybe the impact can checked when changing and checking if all tests still pass - at least if there are any specialization taint tests.

[Patrick-Remy]

In general my feedback to the taint system is, that it's really hard to overview and internal documentation is extremely short. In sum there are unfortunately only a few comments or docblocks in the codebase with explanations which makes it very hard to get into all the Analyzer classes and their (taint) handling.