Open Patrick-Remy opened 1 year ago
Also using the AddTaintsInterface
doesn't make a difference:
<?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 [];
}
}
Just checked again, with the above plugin using AfterExpressionAnalysisInterface
, on v4.30.0 errors get detected, using v5.0.0 no errors are found.
Interestingly, even on v4.30.0 no errors are found using the AddTaintsInterface
.
If I debug the taint graph with the different versions. The TaintGraph in v5.0.0+ has much less edges for the few lines of the test snippet.
Still reproducible with current dev-master
. I created a repository for easy reproduction: https://github.com/Patrick-Remy/repro-psalm-5-taint-analysis-issue/blob/master/README.md
@weirdan do you have an idea why the TaintGraph has so big changed between v4 and v5?
do you have an idea why the TaintGraph has so big changed between v4 and v5?
I don't think there were that many changes between v4 and v5, apart from project-wide stylistic changes (we applied several automatic refactorings when we bumped our minimum PHP version).
In July I had a bigger source file when diffing the TaintFlowGraphs.
I tried to debug it again and dumped in the minimal repro afterconnectSinksAndSources
in Analyzer
. And the main diff seems a missing edge in forward_edges
with length 0
The issue seems to be caused by https://github.com/vimeo/psalm/commit/d0be59e16ef6e55f0349c60eab272947e21a2c11#diff-122c0df94b9a0557e4fd09b8d8c7324f4d6d8fff34f344e1e49318c8e7a0a242
With the commit before (51838a545
) the taint is found, when checking out d0be59e16
, the issues are ignored.
The ArgumentAnalyzer::verifyType
gets called with $arg_value_type
as $input_type
, When dumping $input_type->parent_nodes
, it is empty with d0be59e16
but isn't with the commit before.
Dump with 51838a545
["parent_nodes"]=>
array(1) {
["$bad_data-example.php:81"]=>
object(Psalm\Internal\DataFlow\TaintSource)#90618 (9) {
["id"]=>
string(24) "$bad_data-example.php:81"
["unspecialized_id"]=>
NULL
["label"]=>
string(24) "$bad_data-example.php:81"
So anywhere the parent nodes get lost
I think I got it, with the change of immutability, addTaintSource()
doesn't manipulate the $expr_type
anymore and instead returns the adjusted $expr_type
. But this won't work, as from a plugin perspective I cannot adjust the expression type anymore inside the plugin.
So probably that's expected by you, but then there is the question why the addTaints() also doesn't work.
See my PR for furhter analysis/discussion
I ran into the same bug while trying to add $_FILES
and $argv
as taint sources. As a workaround, I patched psalm with
this file
In the PR mentionned above, I take another approach than @Patrick-Remy in order to solve the issue.
After upgrading from psalm 4.30.0 to psalm 5.13.1, I noticed that adding custom taint source via a psalm plugin is broken. Even the example at https://psalm.dev/docs/security_analysis/custom_taint_sources/ does not work.
Adding taint sources via docblock still works:
This correctly throws
ERROR: TaintedSql
. But if I add taint sources via the plugin described at the page. No error is found for this snippet:This is the exact code I used for the plugin. I have printed
TAINTED
to ensure, the variable really gets tainted.