From 15387d19cd41af4d0377d0d65b31aaf9cd8e6974 Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Thu, 23 Jun 2022 16:43:42 -0400 Subject: [PATCH 1/3] Track taints in static properties --- .../InstancePropertyAssignmentAnalyzer.php | 142 +++++++++++------- .../StaticPropertyAssignmentAnalyzer.php | 10 ++ .../Fetch/AtomicPropertyFetchAnalyzer.php | 115 ++++++++------ .../Fetch/StaticPropertyFetchAnalyzer.php | 20 +++ src/Psalm/Internal/Codebase/DataFlowGraph.php | 5 + tests/TaintTest.php | 16 ++ 6 files changed, 211 insertions(+), 97 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php index 767121217..462544d55 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php @@ -463,9 +463,6 @@ class InstancePropertyAssignmentAnalyzer $data_flow_graph = $statements_analyzer->data_flow_graph; - $var_location = new CodeLocation($statements_analyzer->getSource(), $stmt->var); - $property_location = new CodeLocation($statements_analyzer->getSource(), $stmt); - if ($class_storage->specialize_instance) { $var_id = ExpressionIdentifier::getExtendedVarId( $stmt->var, @@ -487,6 +484,8 @@ class InstancePropertyAssignmentAnalyzer return; } + $var_location = new CodeLocation($statements_analyzer->getSource(), $stmt->var); + $var_node = DataFlowNode::getForAssignment( $var_id, $var_location @@ -494,6 +493,8 @@ class InstancePropertyAssignmentAnalyzer $data_flow_graph->addNode($var_node); + $property_location = new CodeLocation($statements_analyzer->getSource(), $stmt); + $property_node = DataFlowNode::getForAssignment( $var_property_id ?: $var_id . '->$property', $property_location @@ -547,77 +548,108 @@ class InstancePropertyAssignmentAnalyzer $statements_analyzer ); - $localized_property_node = DataFlowNode::getForAssignment( + self::taintUnspecializedProperty( + $statements_analyzer, + $stmt, + $property_id, + $class_storage, + $assignment_value_type, + $context, $var_property_id - ?: $property_id . '-' . $property_location->file_name . ':' . $property_location->raw_file_start, - $property_location ); + } + } - $data_flow_graph->addNode($localized_property_node); + public static function taintUnspecializedProperty( + StatementsAnalyzer $statements_analyzer, + PhpParser\Node\Expr $stmt, + string $property_id, + ClassLikeStorage $class_storage, + Union $assignment_value_type, + Context $context, + ?string $var_property_id + ): void { + $codebase = $statements_analyzer->getCodebase(); - $property_node = new DataFlowNode( - $property_id, - $property_id, - null, - null - ); + $data_flow_graph = $statements_analyzer->data_flow_graph; - $data_flow_graph->addNode($property_node); + if (!$data_flow_graph) { + return; + } - $event = new AddRemoveTaintsEvent($stmt, $context, $statements_analyzer, $codebase); + $property_location = new CodeLocation($statements_analyzer->getSource(), $stmt); - $added_taints = $codebase->config->eventDispatcher->dispatchAddTaints($event); - $removed_taints = $codebase->config->eventDispatcher->dispatchRemoveTaints($event); + $localized_property_node = DataFlowNode::getForAssignment( + $var_property_id ?: $property_id, + $property_location + ); - $data_flow_graph->addPath( - $localized_property_node, - $property_node, - 'property-assignment', - $added_taints, - $removed_taints - ); + $data_flow_graph->addNode($localized_property_node); - if ($assignment_value_type->parent_nodes) { - foreach ($assignment_value_type->parent_nodes as $parent_node) { - $data_flow_graph->addPath( - $parent_node, - $localized_property_node, - '=', - $added_taints, - $removed_taints - ); - } - } + $property_node = new DataFlowNode( + $property_id, + $property_id, + null, + null + ); - $declaring_property_class = $codebase->properties->getDeclaringClassForProperty( - $property_id, - false, - $statements_analyzer - ); + $data_flow_graph->addNode($property_node); - if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph - && $declaring_property_class - && $declaring_property_class !== $class_storage->name - && $stmt->name instanceof PhpParser\Node\Identifier - ) { - $declaring_property_node = new DataFlowNode( - $declaring_property_class . '::$' . $stmt->name, - $declaring_property_class . '::$' . $stmt->name, - null, - null - ); + $event = new AddRemoveTaintsEvent($stmt, $context, $statements_analyzer, $codebase); - $data_flow_graph->addNode($declaring_property_node); + $added_taints = $codebase->config->eventDispatcher->dispatchAddTaints($event); + $removed_taints = $codebase->config->eventDispatcher->dispatchRemoveTaints($event); + $data_flow_graph->addPath( + $localized_property_node, + $property_node, + 'property-assignment', + $added_taints, + $removed_taints + ); + + if ($assignment_value_type->parent_nodes) { + foreach ($assignment_value_type->parent_nodes as $parent_node) { $data_flow_graph->addPath( - $property_node, - $declaring_property_node, - 'property-assignment', + $parent_node, + $localized_property_node, + '=', $added_taints, $removed_taints ); } } + + $declaring_property_class = $codebase->properties->getDeclaringClassForProperty( + $property_id, + false, + $statements_analyzer + ); + + if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph + && $declaring_property_class + && $declaring_property_class !== $class_storage->name + && ($stmt instanceof PhpParser\Node\Expr\PropertyFetch + || $stmt instanceof PhpParser\Node\Expr\StaticPropertyFetch) + && $stmt->name instanceof PhpParser\Node\Identifier + ) { + $declaring_property_node = new DataFlowNode( + $declaring_property_class . '::$' . $stmt->name, + $declaring_property_class . '::$' . $stmt->name, + null, + null + ); + + $data_flow_graph->addNode($declaring_property_node); + + $data_flow_graph->addPath( + $property_node, + $declaring_property_node, + 'property-assignment', + $added_taints, + $removed_taints + ); + } } /** diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/StaticPropertyAssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/StaticPropertyAssignmentAnalyzer.php index 11aa4d50a..e43f70e09 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/StaticPropertyAssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/StaticPropertyAssignmentAnalyzer.php @@ -196,6 +196,16 @@ class StaticPropertyAssignmentAnalyzer $context->vars_in_scope[$var_id] = $assignment_value_type; } + InstancePropertyAssignmentAnalyzer::taintUnspecializedProperty( + $statements_analyzer, + $stmt, + $property_id, + $class_storage, + $assignment_value_type, + $context, + null + ); + $class_property_type = $codebase->properties->getPropertyType( $property_id, true, diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php index 4bc2aca3a..f98f99d5f 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php @@ -773,9 +773,6 @@ class AtomicPropertyFetchAnalyzer $data_flow_graph = $statements_analyzer->data_flow_graph; - $var_location = new CodeLocation($statements_analyzer->getSource(), $stmt->var); - $property_location = new CodeLocation($statements_analyzer->getSource(), $stmt); - $added_taints = []; $removed_taints = []; @@ -811,6 +808,9 @@ class AtomicPropertyFetchAnalyzer return; } + $var_location = new CodeLocation($statements_analyzer->getSource(), $stmt->var); + $property_location = new CodeLocation($statements_analyzer->getSource(), $stmt); + $var_node = DataFlowNode::getForAssignment( $var_id, $var_location @@ -849,51 +849,82 @@ class AtomicPropertyFetchAnalyzer $type->parent_nodes = [$property_node->id => $property_node]; } } else { - $var_property_id = ExpressionIdentifier::getExtendedVarId( + self::processUnspecialTaints( + $statements_analyzer, $stmt, - null, - $statements_analyzer - ); - - $localized_property_node = DataFlowNode::getForAssignment( - $var_property_id - ?: $property_id . '-' . $property_location->file_name . ':' . $property_location->raw_file_start, - $property_location - ); - - $data_flow_graph->addNode($localized_property_node); - - $property_node = new DataFlowNode( + $type, $property_id, - $property_id, - null, - null + $in_assignment, + $added_taints, + $removed_taints ); - - $data_flow_graph->addNode($property_node); - - if ($in_assignment) { - $data_flow_graph->addPath( - $localized_property_node, - $property_node, - 'property-assignment', - $added_taints, - $removed_taints - ); - } else { - $data_flow_graph->addPath( - $property_node, - $localized_property_node, - 'property-fetch', - $added_taints, - $removed_taints - ); - } - - $type->parent_nodes = [$localized_property_node->id => $localized_property_node]; } } + /** + * @param ?array $added_taints + * @param ?array $removed_taints + */ + public static function processUnspecialTaints( + StatementsAnalyzer $statements_analyzer, + PhpParser\Node\Expr $stmt, + Union $type, + string $property_id, + bool $in_assignment, + ?array $added_taints, + ?array $removed_taints + ): void { + if (!$statements_analyzer->data_flow_graph) { + return; + } + + $data_flow_graph = $statements_analyzer->data_flow_graph; + + $var_property_id = ExpressionIdentifier::getExtendedVarId( + $stmt, + null, + $statements_analyzer + ); + + $property_location = new CodeLocation($statements_analyzer->getSource(), $stmt); + + $localized_property_node = DataFlowNode::getForAssignment( + $var_property_id ?: $property_id, + $property_location + ); + + $data_flow_graph->addNode($localized_property_node); + + $property_node = new DataFlowNode( + $property_id, + $property_id, + null, + null + ); + + $data_flow_graph->addNode($property_node); + + if ($in_assignment) { + $data_flow_graph->addPath( + $localized_property_node, + $property_node, + 'property-assignment', + $added_taints, + $removed_taints + ); + } else { + $data_flow_graph->addPath( + $property_node, + $localized_property_node, + 'property-fetch', + $added_taints, + $removed_taints + ); + } + + $type->parent_nodes = [$localized_property_node->id => $localized_property_node]; + } + private static function handleEnumName( StatementsAnalyzer $statements_analyzer, PropertyFetch $stmt, diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/StaticPropertyFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/StaticPropertyFetchAnalyzer.php index e28e3f06f..abb687d96 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/StaticPropertyFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/StaticPropertyFetchAnalyzer.php @@ -231,6 +231,16 @@ class StaticPropertyFetchAnalyzer ); } + AtomicPropertyFetchAnalyzer::processUnspecialTaints( + $statements_analyzer, + $stmt, + $stmt_type, + $property_id, + false, + [], + [] + ); + return true; } @@ -389,6 +399,16 @@ class StaticPropertyFetchAnalyzer $stmt_type->getId() ); } + + AtomicPropertyFetchAnalyzer::processUnspecialTaints( + $statements_analyzer, + $stmt, + $stmt_type, + $property_id, + false, + [], + [] + ); } else { $statements_analyzer->node_data->setType($stmt, Type::getMixed()); } diff --git a/src/Psalm/Internal/Codebase/DataFlowGraph.php b/src/Psalm/Internal/Codebase/DataFlowGraph.php index 4bd3f617f..29ee7d8bb 100644 --- a/src/Psalm/Internal/Codebase/DataFlowGraph.php +++ b/src/Psalm/Internal/Codebase/DataFlowGraph.php @@ -2,6 +2,7 @@ namespace Psalm\Internal\Codebase; +use Exception; use Psalm\Internal\DataFlow\DataFlowNode; use Psalm\Internal\DataFlow\Path; @@ -43,6 +44,10 @@ abstract class DataFlowGraph return; } + if ($from_id === 'A::$last-src/somefile.php:255-src/somefile.php:255-265') { + throw new Exception('bad'); + } + $length = 0; if ($from->code_location diff --git a/tests/TaintTest.php b/tests/TaintTest.php index 8d772e1b4..7bcbdd74b 100644 --- a/tests/TaintTest.php +++ b/tests/TaintTest.php @@ -2349,6 +2349,22 @@ class TaintTest extends TestCase }', 'error_message' => 'TaintedHtml', ], + 'checkMemoizedStaticMethodCallTaints' => [ + 'code' => ' 'TaintedHtml', + ], ]; } From e6c444410c0822e0fac1be36e5e340e817ac3c33 Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Thu, 23 Jun 2022 18:03:33 -0400 Subject: [PATCH 2/3] Remove debug code --- src/Psalm/Internal/Codebase/DataFlowGraph.php | 4 ---- tests/TaintTest.php | 14 +++++++------- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/src/Psalm/Internal/Codebase/DataFlowGraph.php b/src/Psalm/Internal/Codebase/DataFlowGraph.php index 29ee7d8bb..42d00cccf 100644 --- a/src/Psalm/Internal/Codebase/DataFlowGraph.php +++ b/src/Psalm/Internal/Codebase/DataFlowGraph.php @@ -44,10 +44,6 @@ abstract class DataFlowGraph return; } - if ($from_id === 'A::$last-src/somefile.php:255-src/somefile.php:255-265') { - throw new Exception('bad'); - } - $length = 0; if ($from->code_location diff --git a/tests/TaintTest.php b/tests/TaintTest.php index 7bcbdd74b..470c73ccc 100644 --- a/tests/TaintTest.php +++ b/tests/TaintTest.php @@ -2352,17 +2352,17 @@ class TaintTest extends TestCase 'checkMemoizedStaticMethodCallTaints' => [ 'code' => ' 'TaintedHtml', ], ]; From 7be32f2020b77056dc4f863d8a7e8e4157bd2c2a Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Thu, 23 Jun 2022 21:30:12 -0400 Subject: [PATCH 3/3] Remove unused use --- src/Psalm/Internal/Codebase/DataFlowGraph.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Psalm/Internal/Codebase/DataFlowGraph.php b/src/Psalm/Internal/Codebase/DataFlowGraph.php index 42d00cccf..4bd3f617f 100644 --- a/src/Psalm/Internal/Codebase/DataFlowGraph.php +++ b/src/Psalm/Internal/Codebase/DataFlowGraph.php @@ -2,7 +2,6 @@ namespace Psalm\Internal\Codebase; -use Exception; use Psalm\Internal\DataFlow\DataFlowNode; use Psalm\Internal\DataFlow\Path;