From 22e89d189dd0ed963090236eede1ea1ce737b09a Mon Sep 17 00:00:00 2001 From: Patrick Remy Date: Fri, 15 Sep 2023 11:56:56 +0200 Subject: [PATCH 1/9] fix: add taint source on plugin-added taints --- .../Statements/Expression/ArrayAnalyzer.php | 13 +++++++++++++ .../InstancePropertyAssignmentAnalyzer.php | 13 +++++++++++++ .../Statements/Expression/AssignmentAnalyzer.php | 7 +++++++ .../Statements/Expression/BinaryOpAnalyzer.php | 7 +++++++ .../Statements/Expression/Call/ArgumentAnalyzer.php | 7 +++++++ .../Expression/Call/FunctionCallAnalyzer.php | 7 +++++++ .../Statements/Expression/Call/NewAnalyzer.php | 7 +++++++ .../Expression/EncapsulatedStringAnalyzer.php | 8 ++++++++ .../Analyzer/Statements/Expression/EvalAnalyzer.php | 7 +++++++ .../Expression/Fetch/ArrayFetchAnalyzer.php | 7 +++++++ .../Fetch/AtomicPropertyFetchAnalyzer.php | 13 +++++++++++++ .../Statements/Expression/IncludeAnalyzer.php | 7 +++++++ src/Psalm/Internal/DataFlow/TaintSource.php | 9 +++++++++ 13 files changed, 112 insertions(+) diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/ArrayAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/ArrayAnalyzer.php index b6380df0fcd..d9af850d03a 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/ArrayAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/ArrayAnalyzer.php @@ -12,6 +12,7 @@ use Psalm\Internal\Codebase\TaintFlowGraph; use Psalm\Internal\Codebase\VariableUseGraph; use Psalm\Internal\DataFlow\DataFlowNode; +use Psalm\Internal\DataFlow\TaintSource; use Psalm\Internal\Type\Comparator\UnionTypeComparator; use Psalm\Internal\Type\TypeCombiner; use Psalm\Issue\DuplicateArrayKey; @@ -454,6 +455,12 @@ private static function analyzeArrayItem( } $array_creation_info->parent_taint_nodes += [$new_parent_node->id => $new_parent_node]; + + if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($new_parent_node); + $statements_analyzer->data_flow_graph->addSource($taint_source); + $item_value_type = $item_value_type->addParentNodes([$taint_source->id => $taint_source]); + } } if ($item_key_type @@ -488,6 +495,12 @@ private static function analyzeArrayItem( } $array_creation_info->parent_taint_nodes += [$new_parent_node->id => $new_parent_node]; + + if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($new_parent_node); + $statements_analyzer->data_flow_graph->addSource($taint_source); + $item_value_type = $item_value_type->addParentNodes([$taint_source->id => $taint_source]); + } } } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php index bedec945c25..839ef486850 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php @@ -26,6 +26,7 @@ use Psalm\Internal\Codebase\TaintFlowGraph; use Psalm\Internal\Codebase\VariableUseGraph; use Psalm\Internal\DataFlow\DataFlowNode; +use Psalm\Internal\DataFlow\TaintSource; use Psalm\Internal\FileManipulation\FileManipulationBuffer; use Psalm\Internal\MethodIdentifier; use Psalm\Internal\Type\Comparator\TypeComparisonResult; @@ -518,6 +519,12 @@ private static function taintProperty( } } + if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($property_node); + $statements_analyzer->data_flow_graph->addSource($taint_source); + $assignment_value_type = $assignment_value_type->addParentNodes([$taint_source->id => $taint_source]); + } + if (isset($context->vars_in_scope[$var_id])) { $stmt_var_type = $context->vars_in_scope[$var_id]->setParentNodes( [$var_node->id => $var_node], @@ -618,6 +625,12 @@ public static function taintUnspecializedProperty( } } + if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($property_node); + $statements_analyzer->data_flow_graph->addSource($taint_source); + $assignment_value_type = $assignment_value_type->addParentNodes([$taint_source->id => $taint_source]); + } + $declaring_property_class = $codebase->properties->getDeclaringClassForProperty( $property_id, false, diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php index 679abaa347a..ab67ea4639f 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php @@ -31,6 +31,7 @@ use Psalm\Internal\Codebase\TaintFlowGraph; use Psalm\Internal\Codebase\VariableUseGraph; use Psalm\Internal\DataFlow\DataFlowNode; +use Psalm\Internal\DataFlow\TaintSource; use Psalm\Internal\FileManipulation\FileManipulationBuffer; use Psalm\Internal\ReferenceConstraint; use Psalm\Internal\Scanner\VarDocblockComment; @@ -832,6 +833,12 @@ private static function taintAssignment( } $type = $type->setParentNodes($new_parent_nodes, false); + + if ($data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($new_parent_node); + $data_flow_graph->addSource($taint_source); + $type = $type->addParentNodes([$taint_source->id => $taint_source]); + } } public static function analyzeAssignmentOperation( diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php index 2b7b8b607ae..c19ca63f53a 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php @@ -16,6 +16,7 @@ use Psalm\Internal\Codebase\TaintFlowGraph; use Psalm\Internal\Codebase\VariableUseGraph; use Psalm\Internal\DataFlow\DataFlowNode; +use Psalm\Internal\DataFlow\TaintSource; use Psalm\Internal\MethodIdentifier; use Psalm\Issue\DocblockTypeContradiction; use Psalm\Issue\ImpureMethodCall; @@ -169,6 +170,12 @@ public static function analyze( $added_taints = $codebase->config->eventDispatcher->dispatchAddTaints($event); $removed_taints = $codebase->config->eventDispatcher->dispatchRemoveTaints($event); + if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($new_parent_node); + $statements_analyzer->data_flow_graph->addSource($taint_source); + $stmt_type = $stmt_type->addParentNodes([$taint_source->id => $taint_source]); + } + if ($stmt_left_type && $stmt_left_type->parent_nodes) { foreach ($stmt_left_type->parent_nodes as $parent_node) { $statements_analyzer->data_flow_graph->addPath( diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php index 2fd7cd1b5a7..d3ba173fb01 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php @@ -20,6 +20,7 @@ use Psalm\Internal\Codebase\TaintFlowGraph; use Psalm\Internal\Codebase\VariableUseGraph; use Psalm\Internal\DataFlow\DataFlowNode; +use Psalm\Internal\DataFlow\TaintSource; use Psalm\Internal\MethodIdentifier; use Psalm\Internal\Type\Comparator\CallableTypeComparator; use Psalm\Internal\Type\Comparator\TypeComparisonResult; @@ -1897,5 +1898,11 @@ private static function processTaintedness( $removed_taints, ); } + + if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($argument_value_node); + $statements_analyzer->data_flow_graph->addSource($taint_source); + $input_type = $input_type->addParentNodes([$taint_source->id => $taint_source]); + } } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php index 98e192c72f7..9b58ee5e7fc 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php @@ -17,6 +17,7 @@ use Psalm\Internal\Codebase\InternalCallMapHandler; use Psalm\Internal\Codebase\TaintFlowGraph; use Psalm\Internal\DataFlow\TaintSink; +use Psalm\Internal\DataFlow\TaintSource; use Psalm\Internal\MethodIdentifier; use Psalm\Internal\Type\Comparator\CallableTypeComparator; use Psalm\Internal\Type\TemplateResult; @@ -866,6 +867,12 @@ private static function getAnalyzeNamedExpression( $removed_taints, ); } + + if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($custom_call_sink); + $statements_analyzer->data_flow_graph->addSource($taint_source); + $stmt_name_type = $stmt_name_type->addParentNodes([$taint_source->id => $taint_source]); + } } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php index 8f35fee56b3..3e784a1aa62 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php @@ -19,6 +19,7 @@ use Psalm\Internal\Codebase\TaintFlowGraph; use Psalm\Internal\DataFlow\DataFlowNode; use Psalm\Internal\DataFlow\TaintSink; +use Psalm\Internal\DataFlow\TaintSource; use Psalm\Internal\MethodIdentifier; use Psalm\Internal\Type\TemplateResult; use Psalm\Internal\Type\TemplateStandinTypeReplacer; @@ -747,6 +748,12 @@ private static function analyzeConstructorExpression( $removed_taints, ); } + + if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($custom_call_sink); + $statements_analyzer->data_flow_graph->addSource($taint_source); + $stmt_class_type = $stmt_class_type->addParentNodes([$taint_source->id => $taint_source]); + } } if (self::checkMethodArgs( diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/EncapsulatedStringAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/EncapsulatedStringAnalyzer.php index 63c815ff521..136c0c896cc 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/EncapsulatedStringAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/EncapsulatedStringAnalyzer.php @@ -8,7 +8,9 @@ use Psalm\Context; use Psalm\Internal\Analyzer\Statements\ExpressionAnalyzer; use Psalm\Internal\Analyzer\StatementsAnalyzer; +use Psalm\Internal\Codebase\TaintFlowGraph; use Psalm\Internal\DataFlow\DataFlowNode; +use Psalm\Internal\DataFlow\TaintSource; use Psalm\Plugin\EventHandler\Event\AddRemoveTaintsEvent; use Psalm\Type; use Psalm\Type\Atomic\TLiteralFloat; @@ -112,6 +114,12 @@ public static function analyze( ); } } + + if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($new_parent_node); + $statements_analyzer->data_flow_graph->addSource($taint_source); + $casted_part_type = $casted_part_type->addParentNodes([$taint_source->id => $taint_source]); + } } } else { $all_literals = false; diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/EvalAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/EvalAnalyzer.php index 3f6b2a8d19b..28f76b78c5b 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/EvalAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/EvalAnalyzer.php @@ -9,6 +9,7 @@ use Psalm\Internal\Analyzer\StatementsAnalyzer; use Psalm\Internal\Codebase\TaintFlowGraph; use Psalm\Internal\DataFlow\TaintSink; +use Psalm\Internal\DataFlow\TaintSource; use Psalm\Issue\ForbiddenCode; use Psalm\IssueBuffer; use Psalm\Plugin\EventHandler\Event\AddRemoveTaintsEvent; @@ -69,6 +70,12 @@ public static function analyze( $removed_taints, ); } + + if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($eval_param_sink); + $statements_analyzer->data_flow_graph->addSource($taint_source); + $expr_type = $expr_type->addParentNodes([$taint_source->id => $taint_source]); + } } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ArrayFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ArrayFetchAnalyzer.php index 9bc860de44d..c9e6505be4f 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ArrayFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ArrayFetchAnalyzer.php @@ -16,6 +16,7 @@ use Psalm\Internal\Codebase\TaintFlowGraph; use Psalm\Internal\Codebase\VariableUseGraph; use Psalm\Internal\DataFlow\DataFlowNode; +use Psalm\Internal\DataFlow\TaintSource; use Psalm\Internal\Type\Comparator\AtomicTypeComparator; use Psalm\Internal\Type\Comparator\TypeComparisonResult; use Psalm\Internal\Type\Comparator\UnionTypeComparator; @@ -460,6 +461,12 @@ public static function taintArrayFetch( $stmt_type = $stmt_type->setParentNodes([$new_parent_node->id => $new_parent_node]); + if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($new_parent_node); + $statements_analyzer->data_flow_graph->addSource($taint_source); + $stmt_type = $stmt_type->addParentNodes([$taint_source->id => $taint_source]); + } + if ($array_key_node) { $offset_type = $offset_type->setParentNodes([$array_key_node->id => $array_key_node]); } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php index 6ad6983b76e..93ba2c51ef5 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php @@ -22,6 +22,7 @@ use Psalm\Internal\Analyzer\StatementsAnalyzer; use Psalm\Internal\Codebase\TaintFlowGraph; use Psalm\Internal\DataFlow\DataFlowNode; +use Psalm\Internal\DataFlow\TaintSource; use Psalm\Internal\FileManipulation\FileManipulationBuffer; use Psalm\Internal\MethodIdentifier; use Psalm\Internal\Type\TemplateInferredTypeReplacer; @@ -899,6 +900,12 @@ public static function processTaints( } $type = $type->setParentNodes([$property_node->id => $property_node], true); + + if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($var_node); + $statements_analyzer->data_flow_graph->addSource($taint_source); + $type = $type->addParentNodes([$taint_source->id => $taint_source]); + } } } else { self::processUnspecialTaints( @@ -975,6 +982,12 @@ public static function processUnspecialTaints( } $type = $type->setParentNodes([$localized_property_node->id => $localized_property_node], true); + + if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($localized_property_node); + $statements_analyzer->data_flow_graph->addSource($taint_source); + $type = $type->addParentNodes([$taint_source->id => $taint_source]); + } } private static function handleEnumName( diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/IncludeAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/IncludeAnalyzer.php index 6c4a36ab2bf..31a71c78a24 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/IncludeAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/IncludeAnalyzer.php @@ -14,6 +14,7 @@ use Psalm\Internal\Analyzer\StatementsAnalyzer; use Psalm\Internal\Codebase\TaintFlowGraph; use Psalm\Internal\DataFlow\TaintSink; +use Psalm\Internal\DataFlow\TaintSource; use Psalm\Internal\Provider\NodeDataProvider; use Psalm\Issue\MissingFile; use Psalm\Issue\UnresolvableInclude; @@ -141,6 +142,12 @@ public static function analyze( $removed_taints, ); } + + if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($include_param_sink); + $statements_analyzer->data_flow_graph->addSource($taint_source); + $stmt_expr_type = $stmt_expr_type->addParentNodes([$taint_source->id => $taint_source]); + } } if ($path_to_file) { diff --git a/src/Psalm/Internal/DataFlow/TaintSource.php b/src/Psalm/Internal/DataFlow/TaintSource.php index 1777afe99f7..81bfdadf624 100644 --- a/src/Psalm/Internal/DataFlow/TaintSource.php +++ b/src/Psalm/Internal/DataFlow/TaintSource.php @@ -7,4 +7,13 @@ */ final class TaintSource extends DataFlowNode { + public static function fromNode(DataFlowNode $node): self { + return new self( + $node->id, + $node->label, + $node->code_location, + $node->specialization_key, + $node->taints + ); + } } From fb6bc7c43daf3cfb2c13a07b1eade51f50208cdf Mon Sep 17 00:00:00 2001 From: Patrick Remy Date: Fri, 15 Sep 2023 12:38:06 +0200 Subject: [PATCH 2/9] style: fix code style and redundant conditions --- .../Assignment/InstancePropertyAssignmentAnalyzer.php | 8 ++++++-- .../Statements/Expression/Call/FunctionCallAnalyzer.php | 8 +++----- .../Analyzer/Statements/Expression/Call/NewAnalyzer.php | 8 +++----- .../Analyzer/Statements/Expression/EvalAnalyzer.php | 8 +++----- .../Analyzer/Statements/Expression/IncludeAnalyzer.php | 8 +++----- src/Psalm/Internal/DataFlow/TaintSource.php | 5 +++-- 6 files changed, 21 insertions(+), 24 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php index 839ef486850..ac274c58e89 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php @@ -522,7 +522,9 @@ private static function taintProperty( if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { $taint_source = TaintSource::fromNode($property_node); $statements_analyzer->data_flow_graph->addSource($taint_source); - $assignment_value_type = $assignment_value_type->addParentNodes([$taint_source->id => $taint_source]); + $assignment_value_type = $assignment_value_type->addParentNodes([ + $taint_source->id => $taint_source, + ]); } if (isset($context->vars_in_scope[$var_id])) { @@ -628,7 +630,9 @@ public static function taintUnspecializedProperty( if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { $taint_source = TaintSource::fromNode($property_node); $statements_analyzer->data_flow_graph->addSource($taint_source); - $assignment_value_type = $assignment_value_type->addParentNodes([$taint_source->id => $taint_source]); + $assignment_value_type = $assignment_value_type->addParentNodes([ + $taint_source->id => $taint_source, + ]); } $declaring_property_class = $codebase->properties->getDeclaringClassForProperty( diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php index 9b58ee5e7fc..a5aa8b1820f 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php @@ -868,11 +868,9 @@ private static function getAnalyzeNamedExpression( ); } - if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { - $taint_source = TaintSource::fromNode($custom_call_sink); - $statements_analyzer->data_flow_graph->addSource($taint_source); - $stmt_name_type = $stmt_name_type->addParentNodes([$taint_source->id => $taint_source]); - } + $taint_source = TaintSource::fromNode($custom_call_sink); + $statements_analyzer->data_flow_graph->addSource($taint_source); + $stmt_name_type = $stmt_name_type->addParentNodes([$taint_source->id => $taint_source]); } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php index 3e784a1aa62..e3a36d4b1d4 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php @@ -749,11 +749,9 @@ private static function analyzeConstructorExpression( ); } - if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { - $taint_source = TaintSource::fromNode($custom_call_sink); - $statements_analyzer->data_flow_graph->addSource($taint_source); - $stmt_class_type = $stmt_class_type->addParentNodes([$taint_source->id => $taint_source]); - } + $taint_source = TaintSource::fromNode($custom_call_sink); + $statements_analyzer->data_flow_graph->addSource($taint_source); + $stmt_class_type = $stmt_class_type->addParentNodes([$taint_source->id => $taint_source]); } if (self::checkMethodArgs( diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/EvalAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/EvalAnalyzer.php index 28f76b78c5b..c089d71ecbb 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/EvalAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/EvalAnalyzer.php @@ -71,11 +71,9 @@ public static function analyze( ); } - if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { - $taint_source = TaintSource::fromNode($eval_param_sink); - $statements_analyzer->data_flow_graph->addSource($taint_source); - $expr_type = $expr_type->addParentNodes([$taint_source->id => $taint_source]); - } + $taint_source = TaintSource::fromNode($eval_param_sink); + $statements_analyzer->data_flow_graph->addSource($taint_source); + $expr_type = $expr_type->addParentNodes([$taint_source->id => $taint_source]); } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/IncludeAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/IncludeAnalyzer.php index 31a71c78a24..173d346e91b 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/IncludeAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/IncludeAnalyzer.php @@ -143,11 +143,9 @@ public static function analyze( ); } - if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { - $taint_source = TaintSource::fromNode($include_param_sink); - $statements_analyzer->data_flow_graph->addSource($taint_source); - $stmt_expr_type = $stmt_expr_type->addParentNodes([$taint_source->id => $taint_source]); - } + $taint_source = TaintSource::fromNode($include_param_sink); + $statements_analyzer->data_flow_graph->addSource($taint_source); + $stmt_expr_type = $stmt_expr_type->addParentNodes([$taint_source->id => $taint_source]); } if ($path_to_file) { diff --git a/src/Psalm/Internal/DataFlow/TaintSource.php b/src/Psalm/Internal/DataFlow/TaintSource.php index 81bfdadf624..a69cf2eb1ce 100644 --- a/src/Psalm/Internal/DataFlow/TaintSource.php +++ b/src/Psalm/Internal/DataFlow/TaintSource.php @@ -7,13 +7,14 @@ */ final class TaintSource extends DataFlowNode { - public static function fromNode(DataFlowNode $node): self { + public static function fromNode(DataFlowNode $node): self + { return new self( $node->id, $node->label, $node->code_location, $node->specialization_key, - $node->taints + $node->taints, ); } } From 14548742a08b3387e43db16cb4cc15e1b93e6554 Mon Sep 17 00:00:00 2001 From: Patrick Remy Date: Tue, 19 Sep 2023 20:06:32 +0200 Subject: [PATCH 3/9] test: add addTaints plugin test --- examples/plugins/TaintActiveRecords.php | 90 +++++++++++++++++++++++++ tests/Config/PluginTest.php | 58 ++++++++++++++++ 2 files changed, 148 insertions(+) create mode 100644 examples/plugins/TaintActiveRecords.php diff --git a/examples/plugins/TaintActiveRecords.php b/examples/plugins/TaintActiveRecords.php new file mode 100644 index 00000000000..d6b315fb35f --- /dev/null +++ b/examples/plugins/TaintActiveRecords.php @@ -0,0 +1,90 @@ + + */ + public static function addTaints(AddRemoveTaintsEvent $event): array + { + $expr = $event->getExpr(); + $statements_source = $event->getStatementsSource(); + + // For all property fetch expressions, walk through the full fetch path + // (e.g. `$model->property->subproperty`) and check if it contains + // any class of namespace \app\models\ + do { + $expr_type = $statements_source->getNodeTypeProvider()->getType($expr); + if (!$expr_type) { + continue; + } + + if (self::containsActiveRecord($expr_type)) { + return TaintKindGroup::ALL_INPUT; + } + } while ($expr = self::getParentNode($expr)); + + return []; + } + + /** + * @return bool `true` if union contains a type of model + */ + private static function containsActiveRecord(Union $union_type): bool + { + foreach ($union_type->getAtomicTypes() as $type) { + if (self::isActiveRecord($type)) { + return true; + } + } + + return false; + } + + /** + * @return bool `true` if namespace of type is in namespace `app\models` + */ + private static function isActiveRecord(Atomic $type): bool + { + if (!$type instanceof TNamedObject) { + return false; + } + + + return strpos($type->value, 'app\models\\') === 0; + } + + + /** + * Return next node that should be followed for active record search + */ + private static function getParentNode(Expr $expr): ?Expr + { + // Model properties are always accessed by a property fetch + if ($expr instanceof PropertyFetch) { + return $expr->var; + } + + return null; + } +} diff --git a/tests/Config/PluginTest.php b/tests/Config/PluginTest.php index 61747f6b35f..8731c587456 100644 --- a/tests/Config/PluginTest.php +++ b/tests/Config/PluginTest.php @@ -917,6 +917,64 @@ function b(int $e): int { return $e; } $this->analyzeFile($file_path, new Context()); } + public function testAddTaints(): void + { + $this->project_analyzer = $this->getProjectAnalyzerWithConfig( + TestConfig::loadFromXML( + dirname(__DIR__, 2) . DIRECTORY_SEPARATOR, + ' + + + + + + + + ', + ), + ); + + $this->project_analyzer->getCodebase()->config->initializePlugins($this->project_analyzer); + + $file_path = getcwd() . '/src/somefile.php'; + + $this->addFile( + $file_path, + ' + */ + public static function findAll(): array { + $mockUser = new self(); + $mockUser->name = "

Micky Mouse

"; + + return [$mockUser]; + } + } + + foreach (User::findAll() as $user) { + echo $user->name; + } + ', + ); + + $this->project_analyzer->trackTaintedInputs(); + + $this->expectException(CodeException::class); + $this->expectExceptionMessageMatches('/TaintedHtml/'); + + $this->analyzeFile($file_path, new Context()); + } + public function testRemoveTaints(): void { $this->project_analyzer = $this->getProjectAnalyzerWithConfig( From b61f0748354538b66d7f859285cd9fd000a7a6f5 Mon Sep 17 00:00:00 2001 From: Patrick Remy Date: Tue, 19 Sep 2023 20:09:03 +0200 Subject: [PATCH 4/9] docs: update custom taint plugin docs --- .../security_analysis/custom_taint_sources.md | 42 +++++-------------- 1 file changed, 10 insertions(+), 32 deletions(-) diff --git a/docs/security_analysis/custom_taint_sources.md b/docs/security_analysis/custom_taint_sources.md index d3bdb0a3205..1ff4062da32 100644 --- a/docs/security_analysis/custom_taint_sources.md +++ b/docs/security_analysis/custom_taint_sources.md @@ -26,49 +26,27 @@ For example this plugin treats all variables named `$bad_data` as taint sources. 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\Plugin\EventHandler\AddTaintsInterface; +use Psalm\Plugin\EventHandler\Event\AddRemoveTaintsEvent; use Psalm\Type\TaintKindGroup; -class BadSqlTainter implements AfterExpressionAnalysisInterface +class BadSqlTainter implements AddTaintsInterface { /** - * Called after an expression has been checked + * Called to see what taints should be added * - * @param PhpParser\Node\Expr $expr - * @param Context $context - * @param FileManipulation[] $file_replacements - * - * @return void + * @return list */ - public static function afterExpressionAnalysis(AfterExpressionAnalysisEvent $event): ?bool { + public static function addTaints(AddRemoveTaintsEvent $event): array + { $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) - ); - } + return TaintKindGroup::ALL_INPUT; } - return null; + + return []; } } ``` From d4c5f9b6aa4e01024ef71aac273aa0e3beac4503 Mon Sep 17 00:00:00 2001 From: Patrick Remy Date: Tue, 19 Sep 2023 20:32:26 +0200 Subject: [PATCH 5/9] refactor: only add taint sources if required --- .../Statements/Expression/ArrayAnalyzer.php | 22 +++++++--------- .../InstancePropertyAssignmentAnalyzer.php | 26 +++++++------------ .../Expression/AssignmentAnalyzer.php | 11 ++++---- .../Expression/BinaryOpAnalyzer.php | 3 +-- .../Expression/Call/ArgumentAnalyzer.php | 3 +-- .../Expression/Call/FunctionCallAnalyzer.php | 7 ++--- .../Expression/Call/NewAnalyzer.php | 9 ++++--- .../Expression/EncapsulatedStringAnalyzer.php | 11 ++++---- .../Statements/Expression/EvalAnalyzer.php | 9 ++++--- .../Expression/Fetch/ArrayFetchAnalyzer.php | 3 +-- .../Fetch/AtomicPropertyFetchAnalyzer.php | 6 ++--- .../Statements/Expression/IncludeAnalyzer.php | 9 ++++--- 12 files changed, 54 insertions(+), 65 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/ArrayAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/ArrayAnalyzer.php index d9af850d03a..01e60cdadc0 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/ArrayAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/ArrayAnalyzer.php @@ -443,6 +443,11 @@ private static function analyzeArrayItem( $added_taints = $codebase->config->eventDispatcher->dispatchAddTaints($event); $removed_taints = $codebase->config->eventDispatcher->dispatchRemoveTaints($event); + if ($added_taints !== [] && $statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($new_parent_node); + $statements_analyzer->data_flow_graph->addSource($taint_source); + } + foreach ($item_value_type->parent_nodes as $parent_node) { $data_flow_graph->addPath( $parent_node, @@ -455,12 +460,6 @@ private static function analyzeArrayItem( } $array_creation_info->parent_taint_nodes += [$new_parent_node->id => $new_parent_node]; - - if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { - $taint_source = TaintSource::fromNode($new_parent_node); - $statements_analyzer->data_flow_graph->addSource($taint_source); - $item_value_type = $item_value_type->addParentNodes([$taint_source->id => $taint_source]); - } } if ($item_key_type @@ -484,6 +483,11 @@ private static function analyzeArrayItem( $added_taints = $codebase->config->eventDispatcher->dispatchAddTaints($event); $removed_taints = $codebase->config->eventDispatcher->dispatchRemoveTaints($event); + if ($added_taints !== [] && $statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($new_parent_node); + $statements_analyzer->data_flow_graph->addSource($taint_source); + } + foreach ($item_key_type->parent_nodes as $parent_node) { $data_flow_graph->addPath( $parent_node, @@ -495,12 +499,6 @@ private static function analyzeArrayItem( } $array_creation_info->parent_taint_nodes += [$new_parent_node->id => $new_parent_node]; - - if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { - $taint_source = TaintSource::fromNode($new_parent_node); - $statements_analyzer->data_flow_graph->addSource($taint_source); - $item_value_type = $item_value_type->addParentNodes([$taint_source->id => $taint_source]); - } } } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php index ac274c58e89..d67616540a5 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php @@ -504,6 +504,11 @@ private static function taintProperty( $added_taints = $codebase->config->eventDispatcher->dispatchAddTaints($event); $removed_taints = $codebase->config->eventDispatcher->dispatchRemoveTaints($event); + if ($added_taints !== [] && $statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($property_node); + $statements_analyzer->data_flow_graph->addSource($taint_source); + } + $data_flow_graph->addPath( $property_node, $var_node, @@ -519,14 +524,6 @@ private static function taintProperty( } } - if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { - $taint_source = TaintSource::fromNode($property_node); - $statements_analyzer->data_flow_graph->addSource($taint_source); - $assignment_value_type = $assignment_value_type->addParentNodes([ - $taint_source->id => $taint_source, - ]); - } - if (isset($context->vars_in_scope[$var_id])) { $stmt_var_type = $context->vars_in_scope[$var_id]->setParentNodes( [$var_node->id => $var_node], @@ -607,6 +604,11 @@ public static function taintUnspecializedProperty( $added_taints = $codebase->config->eventDispatcher->dispatchAddTaints($event); $removed_taints = $codebase->config->eventDispatcher->dispatchRemoveTaints($event); + if ($added_taints !== [] && $statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($property_node); + $statements_analyzer->data_flow_graph->addSource($taint_source); + } + $data_flow_graph->addPath( $localized_property_node, $property_node, @@ -627,14 +629,6 @@ public static function taintUnspecializedProperty( } } - if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { - $taint_source = TaintSource::fromNode($property_node); - $statements_analyzer->data_flow_graph->addSource($taint_source); - $assignment_value_type = $assignment_value_type->addParentNodes([ - $taint_source->id => $taint_source, - ]); - } - $declaring_property_class = $codebase->properties->getDeclaringClassForProperty( $property_id, false, diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php index ab67ea4639f..a55d595e8c2 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php @@ -822,6 +822,11 @@ private static function taintAssignment( $data_flow_graph->addNode($new_parent_node); $new_parent_nodes = [$new_parent_node->id => $new_parent_node]; + if ($added_taints !== [] && $data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($new_parent_node); + $data_flow_graph->addSource($taint_source); + } + foreach ($parent_nodes as $parent_node) { $data_flow_graph->addPath( $parent_node, @@ -833,12 +838,6 @@ private static function taintAssignment( } $type = $type->setParentNodes($new_parent_nodes, false); - - if ($data_flow_graph instanceof TaintFlowGraph) { - $taint_source = TaintSource::fromNode($new_parent_node); - $data_flow_graph->addSource($taint_source); - $type = $type->addParentNodes([$taint_source->id => $taint_source]); - } } public static function analyzeAssignmentOperation( diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php index c19ca63f53a..248a160097b 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php @@ -170,10 +170,9 @@ public static function analyze( $added_taints = $codebase->config->eventDispatcher->dispatchAddTaints($event); $removed_taints = $codebase->config->eventDispatcher->dispatchRemoveTaints($event); - if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + if ($added_taints !== [] && $statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { $taint_source = TaintSource::fromNode($new_parent_node); $statements_analyzer->data_flow_graph->addSource($taint_source); - $stmt_type = $stmt_type->addParentNodes([$taint_source->id => $taint_source]); } if ($stmt_left_type && $stmt_left_type->parent_nodes) { diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php index d3ba173fb01..1c1bf669414 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php @@ -1899,10 +1899,9 @@ private static function processTaintedness( ); } - if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + if ($added_taints !== [] && $statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { $taint_source = TaintSource::fromNode($argument_value_node); $statements_analyzer->data_flow_graph->addSource($taint_source); - $input_type = $input_type->addParentNodes([$taint_source->id => $taint_source]); } } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php index a5aa8b1820f..09ac0a40865 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php @@ -868,9 +868,10 @@ private static function getAnalyzeNamedExpression( ); } - $taint_source = TaintSource::fromNode($custom_call_sink); - $statements_analyzer->data_flow_graph->addSource($taint_source); - $stmt_name_type = $stmt_name_type->addParentNodes([$taint_source->id => $taint_source]); + if ($added_taints !== []) { + $taint_source = TaintSource::fromNode($custom_call_sink); + $statements_analyzer->data_flow_graph->addSource($taint_source); + } } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php index e3a36d4b1d4..ae3ecb72155 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NewAnalyzer.php @@ -739,6 +739,11 @@ private static function analyzeConstructorExpression( $added_taints = $codebase->config->eventDispatcher->dispatchAddTaints($event); $removed_taints = $codebase->config->eventDispatcher->dispatchRemoveTaints($event); + if ($added_taints !== []) { + $taint_source = TaintSource::fromNode($custom_call_sink); + $statements_analyzer->data_flow_graph->addSource($taint_source); + } + foreach ($stmt_class_type->parent_nodes as $parent_node) { $statements_analyzer->data_flow_graph->addPath( $parent_node, @@ -748,10 +753,6 @@ private static function analyzeConstructorExpression( $removed_taints, ); } - - $taint_source = TaintSource::fromNode($custom_call_sink); - $statements_analyzer->data_flow_graph->addSource($taint_source); - $stmt_class_type = $stmt_class_type->addParentNodes([$taint_source->id => $taint_source]); } if (self::checkMethodArgs( diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/EncapsulatedStringAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/EncapsulatedStringAnalyzer.php index 136c0c896cc..c5b774238f4 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/EncapsulatedStringAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/EncapsulatedStringAnalyzer.php @@ -103,6 +103,11 @@ public static function analyze( $added_taints = $codebase->config->eventDispatcher->dispatchAddTaints($event); $removed_taints = $codebase->config->eventDispatcher->dispatchRemoveTaints($event); + if ($added_taints !== [] && $statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + $taint_source = TaintSource::fromNode($new_parent_node); + $statements_analyzer->data_flow_graph->addSource($taint_source); + } + if ($casted_part_type->parent_nodes) { foreach ($casted_part_type->parent_nodes as $parent_node) { $statements_analyzer->data_flow_graph->addPath( @@ -114,12 +119,6 @@ public static function analyze( ); } } - - if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { - $taint_source = TaintSource::fromNode($new_parent_node); - $statements_analyzer->data_flow_graph->addSource($taint_source); - $casted_part_type = $casted_part_type->addParentNodes([$taint_source->id => $taint_source]); - } } } else { $all_literals = false; diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/EvalAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/EvalAnalyzer.php index c089d71ecbb..1f8cb15e4f9 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/EvalAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/EvalAnalyzer.php @@ -61,6 +61,11 @@ public static function analyze( $added_taints = $codebase->config->eventDispatcher->dispatchAddTaints($event); $removed_taints = $codebase->config->eventDispatcher->dispatchRemoveTaints($event); + if ($added_taints !== []) { + $taint_source = TaintSource::fromNode($eval_param_sink); + $statements_analyzer->data_flow_graph->addSource($taint_source); + } + foreach ($expr_type->parent_nodes as $parent_node) { $statements_analyzer->data_flow_graph->addPath( $parent_node, @@ -70,10 +75,6 @@ public static function analyze( $removed_taints, ); } - - $taint_source = TaintSource::fromNode($eval_param_sink); - $statements_analyzer->data_flow_graph->addSource($taint_source); - $expr_type = $expr_type->addParentNodes([$taint_source->id => $taint_source]); } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ArrayFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ArrayFetchAnalyzer.php index c9e6505be4f..b04cea8cc0d 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ArrayFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ArrayFetchAnalyzer.php @@ -461,10 +461,9 @@ public static function taintArrayFetch( $stmt_type = $stmt_type->setParentNodes([$new_parent_node->id => $new_parent_node]); - if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + if ($added_taints !== [] && $statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { $taint_source = TaintSource::fromNode($new_parent_node); $statements_analyzer->data_flow_graph->addSource($taint_source); - $stmt_type = $stmt_type->addParentNodes([$taint_source->id => $taint_source]); } if ($array_key_node) { diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php index 93ba2c51ef5..962ff335c80 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php @@ -901,10 +901,9 @@ public static function processTaints( $type = $type->setParentNodes([$property_node->id => $property_node], true); - if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + if ($added_taints !== [] && $statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { $taint_source = TaintSource::fromNode($var_node); $statements_analyzer->data_flow_graph->addSource($taint_source); - $type = $type->addParentNodes([$taint_source->id => $taint_source]); } } } else { @@ -983,10 +982,9 @@ public static function processUnspecialTaints( $type = $type->setParentNodes([$localized_property_node->id => $localized_property_node], true); - if ($statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { + if ($added_taints !== [] && $statements_analyzer->data_flow_graph instanceof TaintFlowGraph) { $taint_source = TaintSource::fromNode($localized_property_node); $statements_analyzer->data_flow_graph->addSource($taint_source); - $type = $type->addParentNodes([$taint_source->id => $taint_source]); } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/IncludeAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/IncludeAnalyzer.php index 173d346e91b..55f14ae7132 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/IncludeAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/IncludeAnalyzer.php @@ -133,6 +133,11 @@ public static function analyze( $added_taints = $codebase->config->eventDispatcher->dispatchAddTaints($event); $removed_taints = $codebase->config->eventDispatcher->dispatchRemoveTaints($event); + if ($added_taints !== []) { + $taint_source = TaintSource::fromNode($include_param_sink); + $statements_analyzer->data_flow_graph->addSource($taint_source); + } + foreach ($stmt_expr_type->parent_nodes as $parent_node) { $statements_analyzer->data_flow_graph->addPath( $parent_node, @@ -142,10 +147,6 @@ public static function analyze( $removed_taints, ); } - - $taint_source = TaintSource::fromNode($include_param_sink); - $statements_analyzer->data_flow_graph->addSource($taint_source); - $stmt_expr_type = $stmt_expr_type->addParentNodes([$taint_source->id => $taint_source]); } if ($path_to_file) { From 9f8c23d3a79aad4b3f8ef73a51e5aeb05a4cc6bb Mon Sep 17 00:00:00 2001 From: Patrick Remy Date: Tue, 23 Jan 2024 21:16:50 +0100 Subject: [PATCH 6/9] test: move event handler tests to subfolder Remove taint tests from PluginTest.php and extract them into seperate classes. --- examples/plugins/TaintActiveRecords.php | 2 - .../AddTaints/AddTaintsInterfaceTest.php | 253 ++++++++++++++++++ .../RemoveTaints/RemoveAllTaintsPlugin.php | 20 ++ .../RemoveTaintsInterfaceTest.php | 173 ++++++++++++ tests/Config/PluginTest.php | 129 --------- 5 files changed, 446 insertions(+), 131 deletions(-) create mode 100644 tests/Config/Plugin/EventHandler/AddTaints/AddTaintsInterfaceTest.php create mode 100644 tests/Config/Plugin/EventHandler/RemoveTaints/RemoveAllTaintsPlugin.php create mode 100644 tests/Config/Plugin/EventHandler/RemoveTaints/RemoveTaintsInterfaceTest.php diff --git a/examples/plugins/TaintActiveRecords.php b/examples/plugins/TaintActiveRecords.php index d6b315fb35f..3214e56525a 100644 --- a/examples/plugins/TaintActiveRecords.php +++ b/examples/plugins/TaintActiveRecords.php @@ -2,8 +2,6 @@ namespace Psalm\Example\Plugin; -use Exception; -use phpDocumentor\Reflection\DocBlock\Tags\Var_; use PhpParser\Node\Expr\PropertyFetch; use PhpParser\Node\Expr; use Psalm\Plugin\EventHandler\AddTaintsInterface; diff --git a/tests/Config/Plugin/EventHandler/AddTaints/AddTaintsInterfaceTest.php b/tests/Config/Plugin/EventHandler/AddTaints/AddTaintsInterfaceTest.php new file mode 100644 index 00000000000..02edf62e67d --- /dev/null +++ b/tests/Config/Plugin/EventHandler/AddTaints/AddTaintsInterfaceTest.php @@ -0,0 +1,253 @@ +setIncludeCollector(new IncludeCollector()); + return new ProjectAnalyzer( + $config, + new Providers( + $this->file_provider, + new FakeParserCacheProvider(), + ), + new ReportOptions(), + ); + } + + public function setUp(): void + { + RuntimeCaches::clearAll(); + $this->file_provider = new FakeFileProvider(); + } + + public function testTaintBadDataVariables(): void + { + $this->project_analyzer = $this->getProjectAnalyzerWithConfig( + TestConfig::loadFromXML( + dirname(__DIR__, 5) . DIRECTORY_SEPARATOR, + ' + + + + + + + + ', + ), + ); + + $this->project_analyzer->getCodebase()->config->initializePlugins($this->project_analyzer); + + $file_path = getcwd() . '/src/somefile.php'; + + $this->addFile( + $file_path, + 'project_analyzer->trackTaintedInputs(); + + $this->expectException(CodeException::class); + $this->expectExceptionMessageMatches('/TaintedHtml/'); + + $this->analyzeFile($file_path, new Context()); + } + + public function testAddTaintsActiveRecord(): void + { + $this->project_analyzer = $this->getProjectAnalyzerWithConfig( + TestConfig::loadFromXML( + dirname(__DIR__, 5) . DIRECTORY_SEPARATOR, + ' + + + + + + + + ', + ), + ); + + $this->project_analyzer->getCodebase()->config->initializePlugins($this->project_analyzer); + + $file_path = getcwd() . '/src/somefile.php'; + + $this->addFile( + $file_path, + 'Micky Mouse"; + } + + $user = new User(); + echo $user->name; + ', + ); + + $this->project_analyzer->trackTaintedInputs(); + + $this->expectException(CodeException::class); + $this->expectExceptionMessageMatches('/TaintedHtml/'); + + $this->analyzeFile($file_path, new Context()); + } + + public function testAddTaintsActiveRecordKeepInVariables(): void + { + $this->project_analyzer = $this->getProjectAnalyzerWithConfig( + TestConfig::loadFromXML( + dirname(__DIR__, 5) . DIRECTORY_SEPARATOR, + ' + + + + + + + + ', + ), + ); + + $this->project_analyzer->getCodebase()->config->initializePlugins($this->project_analyzer); + + $file_path = getcwd() . '/src/somefile.php'; + + $this->addFile( + $file_path, + 'Micky Mouse"; + } + + $user = new User(); + $userName = $user->name; + echo $userName; + ', + ); + + $this->project_analyzer->trackTaintedInputs(); + + $this->expectException(CodeException::class); + $this->expectExceptionMessageMatches('/TaintedHtml/'); + + $this->analyzeFile($file_path, new Context()); + } + + public function testAddTaintsActiveRecordList(): void + { + $this->project_analyzer = $this->getProjectAnalyzerWithConfig( + TestConfig::loadFromXML( + dirname(__DIR__, 5) . DIRECTORY_SEPARATOR, + ' + + + + + + + + ', + ), + ); + + $this->project_analyzer->getCodebase()->config->initializePlugins($this->project_analyzer); + + $file_path = getcwd() . '/src/somefile.php'; + + $this->addFile( + $file_path, + ' + */ + public static function findAll(): array { + $mockUser = new self(); + $mockUser->name = "

Micky Mouse

"; + + return [$mockUser]; + } + } + + foreach (User::findAll() as $user) { + echo $user->name; + } + ', + ); + + $this->project_analyzer->trackTaintedInputs(); + + $this->expectException(CodeException::class); + $this->expectExceptionMessageMatches('/TaintedHtml/'); + + $this->analyzeFile($file_path, new Context()); + } +} diff --git a/tests/Config/Plugin/EventHandler/RemoveTaints/RemoveAllTaintsPlugin.php b/tests/Config/Plugin/EventHandler/RemoveTaints/RemoveAllTaintsPlugin.php new file mode 100644 index 00000000000..bbc38f14f32 --- /dev/null +++ b/tests/Config/Plugin/EventHandler/RemoveTaints/RemoveAllTaintsPlugin.php @@ -0,0 +1,20 @@ + + */ + public static function removeTaints(AddRemoveTaintsEvent $event): array + { + return TaintKindGroup::ALL_INPUT; + } +} diff --git a/tests/Config/Plugin/EventHandler/RemoveTaints/RemoveTaintsInterfaceTest.php b/tests/Config/Plugin/EventHandler/RemoveTaints/RemoveTaintsInterfaceTest.php new file mode 100644 index 00000000000..0740cee4f6e --- /dev/null +++ b/tests/Config/Plugin/EventHandler/RemoveTaints/RemoveTaintsInterfaceTest.php @@ -0,0 +1,173 @@ +setIncludeCollector(new IncludeCollector()); + return new ProjectAnalyzer( + $config, + new Providers( + $this->file_provider, + new FakeParserCacheProvider(), + ), + new ReportOptions(), + ); + } + + public function setUp(): void + { + RuntimeCaches::clearAll(); + $this->file_provider = new FakeFileProvider(); + } + + + public function testRemoveAllTaints(): void + { + $this->project_analyzer = $this->getProjectAnalyzerWithConfig( + TestConfig::loadFromXML( + dirname(__DIR__, 5) . DIRECTORY_SEPARATOR, + ' + + + + + + + + ', + ), + ); + + $this->project_analyzer->getCodebase()->config->initializePlugins($this->project_analyzer); + + $file_path = getcwd() . '/src/somefile.php'; + + $this->addFile( + $file_path, + 'project_analyzer->trackTaintedInputs(); + + $this->analyzeFile($file_path, new Context()); + } + + public function testRemoveTaintsSafeArrayKeyChecker(): void + { + $this->project_analyzer = $this->getProjectAnalyzerWithConfig( + TestConfig::loadFromXML( + dirname(__DIR__, 5) . DIRECTORY_SEPARATOR, + ' + + + + + + + + ', + ), + ); + + $this->project_analyzer->getCodebase()->config->initializePlugins($this->project_analyzer); + + $file_path = getcwd() . '/src/somefile.php'; + + $this->addFile( + $file_path, + ' [ + "safe_key" => $_GET["input"], + ], + ]; + output($build);', + ); + + $this->project_analyzer->trackTaintedInputs(); + + $this->analyzeFile($file_path, new Context()); + + $this->addFile( + $file_path, + ' [ + "safe_key" => $_GET["input"], + "a" => $_GET["input"], + ], + ]; + output($build);', + ); + + $this->project_analyzer->trackTaintedInputs(); + + $this->expectException(CodeException::class); + $this->expectExceptionMessageMatches('/TaintedHtml/'); + + $this->analyzeFile($file_path, new Context()); + } +} diff --git a/tests/Config/PluginTest.php b/tests/Config/PluginTest.php index 8731c587456..77f2fd8d94e 100644 --- a/tests/Config/PluginTest.php +++ b/tests/Config/PluginTest.php @@ -917,135 +917,6 @@ function b(int $e): int { return $e; } $this->analyzeFile($file_path, new Context()); } - public function testAddTaints(): void - { - $this->project_analyzer = $this->getProjectAnalyzerWithConfig( - TestConfig::loadFromXML( - dirname(__DIR__, 2) . DIRECTORY_SEPARATOR, - ' - - - - - - - - ', - ), - ); - - $this->project_analyzer->getCodebase()->config->initializePlugins($this->project_analyzer); - - $file_path = getcwd() . '/src/somefile.php'; - - $this->addFile( - $file_path, - ' - */ - public static function findAll(): array { - $mockUser = new self(); - $mockUser->name = "

Micky Mouse

"; - - return [$mockUser]; - } - } - - foreach (User::findAll() as $user) { - echo $user->name; - } - ', - ); - - $this->project_analyzer->trackTaintedInputs(); - - $this->expectException(CodeException::class); - $this->expectExceptionMessageMatches('/TaintedHtml/'); - - $this->analyzeFile($file_path, new Context()); - } - - public function testRemoveTaints(): void - { - $this->project_analyzer = $this->getProjectAnalyzerWithConfig( - TestConfig::loadFromXML( - dirname(__DIR__, 2) . DIRECTORY_SEPARATOR, - ' - - - - - - - - ', - ), - ); - - $this->project_analyzer->getCodebase()->config->initializePlugins($this->project_analyzer); - - $file_path = getcwd() . '/src/somefile.php'; - - $this->addFile( - $file_path, - ' [ - "safe_key" => $_GET["input"], - ], - ]; - output($build);', - ); - - $this->project_analyzer->trackTaintedInputs(); - - $this->analyzeFile($file_path, new Context()); - - $this->addFile( - $file_path, - ' [ - "safe_key" => $_GET["input"], - "a" => $_GET["input"], - ], - ]; - output($build);', - ); - - $this->project_analyzer->trackTaintedInputs(); - - $this->expectException(CodeException::class); - $this->expectExceptionMessageMatches('/TaintedHtml/'); - - $this->analyzeFile($file_path, new Context()); - } - public function testFunctionDynamicStorageProviderHook(): void { require_once __DIR__ . '/Plugin/StoragePlugin.php'; From ff7f8ef349d687cb76e4b20204df02550073d11f Mon Sep 17 00:00:00 2001 From: Patrick Remy Date: Tue, 23 Jan 2024 21:19:11 +0100 Subject: [PATCH 7/9] docs: fix example for custom taint sources --- .../security_analysis/custom_taint_sources.md | 18 ++++++----- .../AddTaints/TaintBadDataPlugin.php | 30 +++++++++++++++++++ 2 files changed, 40 insertions(+), 8 deletions(-) create mode 100644 tests/Config/Plugin/EventHandler/AddTaints/TaintBadDataPlugin.php diff --git a/docs/security_analysis/custom_taint_sources.md b/docs/security_analysis/custom_taint_sources.md index 1ff4062da32..776e99ef9e1 100644 --- a/docs/security_analysis/custom_taint_sources.md +++ b/docs/security_analysis/custom_taint_sources.md @@ -23,14 +23,17 @@ For example this plugin treats all variables named `$bad_data` as taint sources. ```php getExpr(); - if ($expr instanceof PhpParser\Node\Expr\Variable - && $expr->name === 'bad_data' - ) { - return TaintKindGroup::ALL_INPUT; + + if (!$expr instanceof Variable) { + return []; } - return []; + return $expr->name === 'bad_data' ? TaintKindGroup::ALL_INPUT : []; } } ``` diff --git a/tests/Config/Plugin/EventHandler/AddTaints/TaintBadDataPlugin.php b/tests/Config/Plugin/EventHandler/AddTaints/TaintBadDataPlugin.php new file mode 100644 index 00000000000..b5047ebaf4f --- /dev/null +++ b/tests/Config/Plugin/EventHandler/AddTaints/TaintBadDataPlugin.php @@ -0,0 +1,30 @@ + + */ + public static function addTaints(AddRemoveTaintsEvent $event): array + { + $expr = $event->getExpr(); + + if (!$expr instanceof Variable) { + return []; + } + + return $expr->name === 'bad_data' ? TaintKindGroup::ALL_INPUT : []; + } +} From 350f2e4c1966af7377cb4067eea38c118c728c71 Mon Sep 17 00:00:00 2001 From: Patrick Remy Date: Tue, 23 Jan 2024 21:50:53 +0100 Subject: [PATCH 8/9] test: simplify taint tests for variable assignment --- .../AddTaints/AddTaintsInterfaceTest.php | 59 +++++++++++++++---- 1 file changed, 46 insertions(+), 13 deletions(-) diff --git a/tests/Config/Plugin/EventHandler/AddTaints/AddTaintsInterfaceTest.php b/tests/Config/Plugin/EventHandler/AddTaints/AddTaintsInterfaceTest.php index 02edf62e67d..2fd9f389282 100644 --- a/tests/Config/Plugin/EventHandler/AddTaints/AddTaintsInterfaceTest.php +++ b/tests/Config/Plugin/EventHandler/AddTaints/AddTaintsInterfaceTest.php @@ -98,7 +98,7 @@ public function testTaintBadDataVariables(): void $this->analyzeFile($file_path, new Context()); } - public function testAddTaintsActiveRecord(): void + public function testTaintsArePassedByTaintedAssignments(): void { $this->project_analyzer = $this->getProjectAnalyzerWithConfig( TestConfig::loadFromXML( @@ -112,7 +112,7 @@ public function testAddTaintsActiveRecord(): void - + ', ), @@ -126,14 +126,8 @@ public function testAddTaintsActiveRecord(): void $file_path, 'Micky Mouse"; - } - - $user = new User(); - echo $user->name; + $foo = $bad_data; + echo $foo; ', ); @@ -145,7 +139,47 @@ class User { $this->analyzeFile($file_path, new Context()); } - public function testAddTaintsActiveRecordKeepInVariables(): void + public function testTaintsAreOverriddenByRawAssignments(): void + { + $this->project_analyzer = $this->getProjectAnalyzerWithConfig( + TestConfig::loadFromXML( + dirname(__DIR__, 5) . DIRECTORY_SEPARATOR, + ' + + + + + + + + ', + ), + ); + + $this->project_analyzer->getCodebase()->config->initializePlugins($this->project_analyzer); + + $file_path = getcwd() . '/src/somefile.php'; + + $this->addFile( + $file_path, + 'project_analyzer->trackTaintedInputs(); + // No exceptions should be thrown + + $this->analyzeFile($file_path, new Context()); + } + + public function testAddTaintsActiveRecord(): void { $this->project_analyzer = $this->getProjectAnalyzerWithConfig( TestConfig::loadFromXML( @@ -180,8 +214,7 @@ class User { } $user = new User(); - $userName = $user->name; - echo $userName; + echo $user->name; ', ); From bd424dc48bf64f219583f8373786a185ac3b50ef Mon Sep 17 00:00:00 2001 From: Patrick Remy Date: Tue, 23 Jan 2024 22:13:52 +0100 Subject: [PATCH 9/9] docs: simplify TaintBadDataPlugin example again --- docs/security_analysis/custom_taint_sources.md | 6 +++--- .../Plugin/EventHandler/AddTaints/TaintBadDataPlugin.php | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/security_analysis/custom_taint_sources.md b/docs/security_analysis/custom_taint_sources.md index 776e99ef9e1..938bd3455ba 100644 --- a/docs/security_analysis/custom_taint_sources.md +++ b/docs/security_analysis/custom_taint_sources.md @@ -44,11 +44,11 @@ class TaintBadDataPlugin implements AddTaintsInterface { $expr = $event->getExpr(); - if (!$expr instanceof Variable) { - return []; + if ($expr instanceof Variable && $expr->name === 'bad_data') { + return TaintKindGroup::ALL_INPUT; } - return $expr->name === 'bad_data' ? TaintKindGroup::ALL_INPUT : []; + return []; } } ``` diff --git a/tests/Config/Plugin/EventHandler/AddTaints/TaintBadDataPlugin.php b/tests/Config/Plugin/EventHandler/AddTaints/TaintBadDataPlugin.php index b5047ebaf4f..437eb6fd4c6 100644 --- a/tests/Config/Plugin/EventHandler/AddTaints/TaintBadDataPlugin.php +++ b/tests/Config/Plugin/EventHandler/AddTaints/TaintBadDataPlugin.php @@ -21,10 +21,10 @@ public static function addTaints(AddRemoveTaintsEvent $event): array { $expr = $event->getExpr(); - if (!$expr instanceof Variable) { - return []; + if ($expr instanceof Variable && $expr->name === 'bad_data') { + return TaintKindGroup::ALL_INPUT; } - return $expr->name === 'bad_data' ? TaintKindGroup::ALL_INPUT : []; + return []; } }