diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/ArrayAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/ArrayAnalyzer.php index 23ede3f3518..baa66f0004b 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/ArrayAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/ArrayAnalyzer.php @@ -204,11 +204,28 @@ public static function analyze( if ($codebase->taint) { if ($item_value_type = $statements_analyzer->node_data->getType($item->value)) { - $parent_taint_nodes = array_merge($parent_taint_nodes, $item_value_type->parent_nodes ?: []); - } + if ($item_value_type->parent_nodes) { + $var_location = new CodeLocation($statements_analyzer->getSource(), $item); + + $new_parent_node = \Psalm\Internal\Taint\TaintNode::getForAssignment( + 'array' + . ($item_key_value !== null ? '[\'' . $item_key_value . '\']' : ''), + $var_location + ); + + $codebase->taint->addTaintNode($new_parent_node); + + foreach ($item_value_type->parent_nodes as $parent_node) { + $codebase->taint->addPath( + $parent_node, + $new_parent_node, + 'array-assignment' + . ($item_key_value !== null ? '-\'' . $item_key_value . '\'' : '') + ); + } - if ($item->key && ($item_key_type = $statements_analyzer->node_data->getType($item->key))) { - $parent_taint_nodes = array_merge($parent_taint_nodes, $item_key_type->parent_nodes ?: []); + $parent_taint_nodes = array_merge($parent_taint_nodes, [$new_parent_node]); + } } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/ArrayAssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/ArrayAssignmentAnalyzer.php index 25d6b7506fd..21b523806ee 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/ArrayAssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/ArrayAssignmentAnalyzer.php @@ -150,15 +150,6 @@ public static function updateArrayType( $child_stmt = null; - $parent_taint_nodes = []; - - if ($codebase->taint - && $assign_value - && ($assign_value_type = $statements_analyzer->node_data->getType($assign_value)) - ) { - $parent_taint_nodes = $assign_value_type->parent_nodes; - } - // First go from the root element up, and go as far as we can to figure out what // array types there are while ($child_stmts) { @@ -170,6 +161,8 @@ public static function updateArrayType( $child_stmt_dim_type = null; + $dim_value = null; + if ($child_stmt->dim) { if (ExpressionAnalyzer::analyze( $statements_analyzer, @@ -189,27 +182,28 @@ public static function updateArrayType( && $child_stmt_dim_type->isSingleStringLiteral()) ) { if ($child_stmt->dim instanceof PhpParser\Node\Scalar\String_) { - $value = $child_stmt->dim->value; + $dim_value = $child_stmt->dim->value; } else { - $value = $child_stmt_dim_type->getSingleStringLiteral()->value; + $dim_value = $child_stmt_dim_type->getSingleStringLiteral()->value; } - if (preg_match('/^(0|[1-9][0-9]*)$/', $value)) { - $var_id_additions[] = '[' . $value . ']'; + if (preg_match('/^(0|[1-9][0-9]*)$/', $dim_value)) { + $var_id_additions[] = '[' . $dim_value . ']'; + } else { + $var_id_additions[] = '[\'' . $dim_value . '\']'; } - $var_id_additions[] = '[\'' . $value . '\']'; } elseif ($child_stmt->dim instanceof PhpParser\Node\Scalar\LNumber || (($child_stmt->dim instanceof PhpParser\Node\Expr\ConstFetch || $child_stmt->dim instanceof PhpParser\Node\Expr\ClassConstFetch) && $child_stmt_dim_type->isSingleIntLiteral()) ) { if ($child_stmt->dim instanceof PhpParser\Node\Scalar\LNumber) { - $value = $child_stmt->dim->value; + $dim_value = $child_stmt->dim->value; } else { - $value = $child_stmt_dim_type->getSingleIntLiteral()->value; + $dim_value = $child_stmt_dim_type->getSingleIntLiteral()->value; } - $var_id_additions[] = '[' . $value . ']'; + $var_id_additions[] = '[' . $dim_value . ']'; } elseif ($child_stmt->dim instanceof PhpParser\Node\Expr\Variable && is_string($child_stmt->dim->name) ) { @@ -302,6 +296,15 @@ public static function updateArrayType( $child_stmt_type = $assignment_type; $statements_analyzer->node_data->setType($child_stmt, $assignment_type); + + self::taintArrayAssignment( + $statements_analyzer, + $child_stmt->var, + $array_type, + $assignment_type, + $array_var_id, + $dim_value + ); } $current_type = $child_stmt_type; @@ -440,11 +443,24 @@ public static function updateArrayType( array_pop($var_id_additions); + $array_var_id = null; + if ($root_var_id) { $array_var_id = $root_var_id . implode('', $var_id_additions); $context->vars_in_scope[$array_var_id] = clone $child_stmt_type; $context->possibly_assigned_var_ids[$array_var_id] = true; } + + if ($codebase->taint) { + self::taintArrayAssignment( + $statements_analyzer, + $child_stmt->var, + $statements_analyzer->node_data->getType($child_stmt->var) ?: Type::getMixed(), + $new_child_type, + $array_var_id, + $key_value + ); + } } $root_is_string = $root_type->isString(); @@ -670,10 +686,6 @@ public static function updateArrayType( $root_type = $new_child_type; } - if ($codebase->taint && $parent_taint_nodes) { - $root_type->parent_nodes = \array_merge($parent_taint_nodes, $root_type->parent_nodes ?: []); - } - $statements_analyzer->node_data->setType($root_array_expr, $root_type); if ($root_array_expr instanceof PhpParser\Node\Expr\PropertyFetch) { @@ -730,4 +742,42 @@ public static function updateArrayType( return null; } + + /** + * @param int|string|null $item_key_value + */ + private static function taintArrayAssignment( + StatementsAnalyzer $statements_analyzer, + PhpParser\Node\Expr $stmt, + Type\Union $stmt_type, + Type\Union $child_stmt_type, + ?string $array_var_id, + $item_key_value + ) : void { + $codebase = $statements_analyzer->getCodebase(); + + if ($codebase->taint + && $child_stmt_type->parent_nodes + ) { + $var_location = new \Psalm\CodeLocation($statements_analyzer->getSource(), $stmt); + + $new_parent_node = \Psalm\Internal\Taint\TaintNode::getForAssignment( + $array_var_id ?: 'array-assignment', + $var_location + ); + + $codebase->taint->addTaintNode($new_parent_node); + + foreach ($child_stmt_type->parent_nodes as $parent_node) { + $codebase->taint->addPath( + $parent_node, + $new_parent_node, + 'array-assignment' + . ($item_key_value !== null ? '-\'' . $item_key_value . '\'' : '') + ); + } + + $stmt_type->parent_nodes[] = $new_parent_node; + } + } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/PropertyAssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/PropertyAssignmentAnalyzer.php index 54fdaa95b5c..8cefb1ca7be 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/PropertyAssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/PropertyAssignmentAnalyzer.php @@ -1098,11 +1098,11 @@ private static function taintProperty( $codebase->taint->addTaintNode($property_node); - $codebase->taint->addPath($localized_property_node, $property_node); + $codebase->taint->addPath($localized_property_node, $property_node, 'property-assignment'); if ($assignment_value_type->parent_nodes) { foreach ($assignment_value_type->parent_nodes as $parent_node) { - $codebase->taint->addPath($parent_node, $localized_property_node); + $codebase->taint->addPath($parent_node, $localized_property_node, '='); } } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php index 66769ba0564..5067d304e02 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php @@ -951,7 +951,7 @@ public static function analyze( $codebase->taint->addTaintNode($new_parent_node); foreach ($context->vars_in_scope[$var_id]->parent_nodes as $parent_node) { - $codebase->taint->addPath($parent_node, $new_parent_node, [], $removed_taints); + $codebase->taint->addPath($parent_node, $new_parent_node, '=', [], $removed_taints); } $context->vars_in_scope[$var_id]->parent_nodes = [$new_parent_node]; @@ -1133,13 +1133,13 @@ public static function analyzeAssignmentOperation( if ($stmt_left_type && $stmt_left_type->parent_nodes) { foreach ($stmt_left_type->parent_nodes as $parent_node) { - $codebase->taint->addPath($parent_node, $new_parent_node); + $codebase->taint->addPath($parent_node, $new_parent_node, 'concat'); } } if ($stmt_right_type && $stmt_right_type->parent_nodes) { foreach ($stmt_right_type->parent_nodes as $parent_node) { - $codebase->taint->addPath($parent_node, $new_parent_node); + $codebase->taint->addPath($parent_node, $new_parent_node, 'concat'); } } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php index 7786e2220b2..cf321747a9e 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOpAnalyzer.php @@ -120,13 +120,13 @@ public static function analyze( if ($stmt_left_type && $stmt_left_type->parent_nodes) { foreach ($stmt_left_type->parent_nodes as $parent_node) { - $codebase->taint->addPath($parent_node, $new_parent_node); + $codebase->taint->addPath($parent_node, $new_parent_node, 'concat'); } } if ($stmt_right_type && $stmt_right_type->parent_nodes) { foreach ($stmt_right_type->parent_nodes as $parent_node) { - $codebase->taint->addPath($parent_node, $new_parent_node); + $codebase->taint->addPath($parent_node, $new_parent_node, 'concat'); } } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php index abc3ea70016..b11aa7ffdca 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php @@ -54,7 +54,7 @@ public static function checkArgumentMatches( ?string $cased_method_id, ?string $self_fq_class_name, ?string $static_fq_class_name, - CodeLocation $function_location, + CodeLocation $function_call_location, ?FunctionLikeParameter $function_param, int $argument_offset, PhpParser\Node\Arg $arg, @@ -127,7 +127,7 @@ public static function checkArgumentMatches( $cased_method_id, $self_fq_class_name, $static_fq_class_name, - $function_location, + $function_call_location, $function_param, $arg_value_type, $argument_offset, @@ -156,7 +156,7 @@ private static function checkFunctionLikeTypeMatches( ?string $cased_method_id, ?string $self_fq_class_name, ?string $static_fq_class_name, - CodeLocation $function_location, + CodeLocation $function_call_location, FunctionLikeParameter $function_param, Type\Union $arg_type, int $argument_offset, @@ -404,7 +404,7 @@ private static function checkFunctionLikeTypeMatches( $unpacked_atomic_array, $specialize_taint, $in_call_map, - $function_location + $function_call_location ) === false) { return false; } @@ -421,7 +421,7 @@ public static function verifyType( ?Type\Union $signature_param_type, ?string $cased_method_id, int $argument_offset, - CodeLocation $code_location, + CodeLocation $arg_location, PhpParser\Node\Expr $input_expr, Context $context, FunctionLikeParameter $function_param, @@ -429,7 +429,7 @@ public static function verifyType( ?Type\Atomic $unpacked_atomic_array, bool $specialize_taint, bool $in_call_map, - CodeLocation $function_location + CodeLocation $function_call_location ) { $codebase = $statements_analyzer->getCodebase(); @@ -468,8 +468,8 @@ public static function verifyType( $statements_analyzer, $cased_method_id, $argument_offset, - $code_location, - $function_location, + $arg_location, + $function_call_location, $function_param, $input_type, $specialize_taint @@ -497,7 +497,7 @@ public static function verifyType( 'Argument ' . ($argument_offset + 1) . $method_identifier . ' cannot be ' . $input_type->getId() . ', expecting ' . $param_type, - $code_location, + $arg_location, $cased_method_id ), $statements_analyzer->getSuppressedIssues() @@ -531,8 +531,8 @@ public static function verifyType( $statements_analyzer, $cased_method_id, $argument_offset, - $code_location, - $function_location, + $arg_location, + $function_call_location, $function_param, $input_type, $specialize_taint @@ -548,7 +548,7 @@ public static function verifyType( if (IssueBuffer::accepts( new NoValue( 'This function or method call never returns output', - $code_location + $arg_location ), $statements_analyzer->getSuppressedIssues() )) { @@ -617,8 +617,8 @@ public static function verifyType( $statements_analyzer, $cased_method_id, $argument_offset, - $code_location, - $function_location, + $arg_location, + $function_call_location, $function_param, $input_type, $specialize_taint @@ -689,7 +689,7 @@ public static function verifyType( new MixedArgumentTypeCoercion( 'Argument ' . ($argument_offset + 1) . $method_identifier . ' expects ' . $param_type->getId() . ', parent type ' . $input_type->getId() . ' provided', - $code_location, + $arg_location, $cased_method_id ), $statements_analyzer->getSuppressedIssues() @@ -701,7 +701,7 @@ public static function verifyType( new ArgumentTypeCoercion( 'Argument ' . ($argument_offset + 1) . $method_identifier . ' expects ' . $param_type->getId() . ', parent type ' . $input_type->getId() . ' provided', - $code_location, + $arg_location, $cased_method_id ), $statements_analyzer->getSuppressedIssues() @@ -716,7 +716,7 @@ public static function verifyType( new ImplicitToStringCast( 'Argument ' . ($argument_offset + 1) . $method_identifier . ' expects ' . $param_type->getId() . ', ' . $input_type->getId() . ' provided with a __toString method', - $code_location + $arg_location ), $statements_analyzer->getSuppressedIssues() )) { @@ -739,7 +739,7 @@ public static function verifyType( new InvalidScalarArgument( 'Argument ' . ($argument_offset + 1) . $method_identifier . ' expects ' . $param_type->getId() . ', ' . $input_type->getId() . ' provided', - $code_location, + $arg_location, $cased_method_id ), $statements_analyzer->getSuppressedIssues() @@ -752,7 +752,7 @@ public static function verifyType( new PossiblyInvalidArgument( 'Argument ' . ($argument_offset + 1) . $method_identifier . ' expects ' . $param_type->getId() . ', possibly different type ' . $input_type->getId() . ' provided', - $code_location, + $arg_location, $cased_method_id ), $statements_analyzer->getSuppressedIssues() @@ -764,7 +764,7 @@ public static function verifyType( new InvalidArgument( 'Argument ' . ($argument_offset + 1) . $method_identifier . ' expects ' . $param_type->getId() . ', ' . $input_type->getId() . ' provided', - $code_location, + $arg_location, $cased_method_id ), $statements_analyzer->getSuppressedIssues() @@ -788,7 +788,7 @@ public static function verifyType( if (ClassLikeAnalyzer::checkFullyQualifiedClassLikeName( $statements_analyzer, $input_expr->value, - $code_location, + $arg_location, $context->self, $context->calling_method_id, $statements_analyzer->getSuppressedIssues() @@ -806,7 +806,7 @@ public static function verifyType( if (ClassLikeAnalyzer::checkFullyQualifiedClassLikeName( $statements_analyzer, $item->value->value, - $code_location, + $arg_location, $context->self, $context->calling_method_id, $statements_analyzer->getSuppressedIssues() @@ -854,7 +854,7 @@ public static function verifyType( if (ClassLikeAnalyzer::checkFullyQualifiedClassLikeName( $statements_analyzer, $callable_fq_class_name, - $code_location, + $arg_location, $context->self, $context->calling_method_id, $statements_analyzer->getSuppressedIssues() @@ -890,7 +890,7 @@ public static function verifyType( if (MethodAnalyzer::checkMethodExists( $codebase, $non_existent_method_ids[0], - $code_location, + $arg_location, $statements_analyzer->getSuppressedIssues() ) === false ) { @@ -903,7 +903,7 @@ public static function verifyType( && CallAnalyzer::checkFunctionExists( $statements_analyzer, $function_id, - $code_location, + $arg_location, false ) === false ) { @@ -921,7 +921,7 @@ public static function verifyType( new NullArgument( 'Argument ' . ($argument_offset + 1) . $method_identifier . ' cannot be null, ' . 'null value provided to parameter with type ' . $param_type->getId(), - $code_location, + $arg_location, $cased_method_id ), $statements_analyzer->getSuppressedIssues() @@ -937,7 +937,7 @@ public static function verifyType( new PossiblyNullArgument( 'Argument ' . ($argument_offset + 1) . $method_identifier . ' cannot be null, possibly ' . 'null value provided', - $code_location, + $arg_location, $cased_method_id ), $statements_analyzer->getSuppressedIssues() @@ -956,7 +956,7 @@ public static function verifyType( new PossiblyFalseArgument( 'Argument ' . ($argument_offset + 1) . $method_identifier . ' cannot be false, possibly ' . 'false value provided', - $code_location, + $arg_location, $cased_method_id ), $statements_analyzer->getSuppressedIssues() @@ -1113,8 +1113,8 @@ private static function processTaintedness( StatementsAnalyzer $statements_analyzer, string $cased_method_id, int $argument_offset, - CodeLocation $code_location, - CodeLocation $function_location, + CodeLocation $arg_location, + CodeLocation $function_call_location, FunctionLikeParameter $function_param, Type\Union &$input_type, bool $specialize_taint @@ -1130,15 +1130,15 @@ private static function processTaintedness( $cased_method_id, $cased_method_id, $argument_offset, - $code_location, - $function_location + $function_param->location, + $function_call_location ); } else { $method_node = TaintNode::getForMethodArgument( $cased_method_id, $cased_method_id, $argument_offset, - $code_location + $function_param->location, ); if (strpos($cased_method_id, '::')) { @@ -1154,12 +1154,12 @@ private static function processTaintedness( $dependent_classlike_lc . '::' . $method_name, $dependent_classlike_storage->name . '::' . $cased_method_name, $argument_offset, - $code_location, + $arg_location, null ); $codebase->taint->addTaintNode($new_sink); - $codebase->taint->addPath($method_node, $new_sink); + $codebase->taint->addPath($method_node, $new_sink, 'arg'); } if (isset($class_storage->overridden_method_ids[$method_name])) { @@ -1168,12 +1168,12 @@ private static function processTaintedness( (string) $parent_method_id, $codebase->methods->getCasedMethodId($parent_method_id), $argument_offset, - $code_location, + $arg_location, null ); $codebase->taint->addTaintNode($new_sink); - $codebase->taint->addPath($method_node, $new_sink); + $codebase->taint->addPath($method_node, $new_sink, 'arg'); } } } @@ -1185,15 +1185,15 @@ private static function processTaintedness( $cased_method_id, $cased_method_id, $argument_offset, - $code_location, - $function_location + $function_param->location, + $function_call_location ); } else { $sink = Sink::getForMethodArgument( $cased_method_id, $cased_method_id, $argument_offset, - $code_location + $function_param->location, ); } @@ -1207,7 +1207,7 @@ private static function processTaintedness( if ($input_type->parent_nodes) { foreach ($input_type->parent_nodes as $parent_node) { $codebase->taint->addTaintNode($method_node); - $codebase->taint->addPath($parent_node, $method_node); + $codebase->taint->addPath($parent_node, $method_node, 'arg'); } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php index db1b722490d..d32806fa3c4 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php @@ -1065,6 +1065,7 @@ private static function getFunctionCallReturnType( $codebase->taint->addPath( $function_param_sink, $function_return_sink, + 'arg', $function_storage->added_taints, $function_storage->removed_taints ); diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/MethodCallReturnTypeFetcher.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/MethodCallReturnTypeFetcher.php index f9150874cfa..3a16e8f44dd 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/MethodCallReturnTypeFetcher.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/Method/MethodCallReturnTypeFetcher.php @@ -198,7 +198,7 @@ public static function fetch( $method_call_node = TaintNode::getForMethodReturn( (string) $method_id, $cased_method_id, - $node_location, + $method_storage->signature_return_type_location ?: $method_storage->location, $method_storage->specialize_call ? $node_location : null ); diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php index 66b3a215ecf..fc050f643b4 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/StaticCallAnalyzer.php @@ -1211,18 +1211,22 @@ function (Assertion $assertion) use ($generic_params) : Assertion { ) { $code_location = new CodeLocation($statements_analyzer->getSource(), $stmt); + $method_location = $method_storage + ? ($method_storage->signature_return_type_location ?: $method_storage->location) + : null; + if ($method_storage && $method_storage->pure) { $method_source = TaintNode::getForMethodReturn( (string) $method_id, $cased_method_id, - $code_location, + $method_location, $code_location ); } else { $method_source = TaintNode::getForMethodReturn( (string) $method_id, $cased_method_id, - $code_location + $method_location ); } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/EvalAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/EvalAnalyzer.php index f53d00ea3d3..7ff874f4231 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/EvalAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/EvalAnalyzer.php @@ -44,7 +44,7 @@ public static function analyze( $codebase->taint->addSink($eval_param_sink); foreach ($expr_type->parent_nodes as $parent_node) { - $codebase->taint->addPath($parent_node, $eval_param_sink); + $codebase->taint->addPath($parent_node, $eval_param_sink, 'arg'); } } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ArrayFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ArrayFetchAnalyzer.php index 7969c82a387..8ed9170f1ed 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ArrayFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/ArrayFetchAnalyzer.php @@ -125,13 +125,13 @@ public static function analyze( $stmt_type ); - if ($codebase->taint) { - $sources = $stmt_var_type->parent_nodes ?: []; - - if ($sources) { - $stmt_type->parent_nodes = $sources; - } - } + self::taintArrayFetch( + $statements_analyzer, + $stmt, + $keyed_array_var_id, + $stmt_type, + $used_key_type + ); return true; } @@ -294,17 +294,55 @@ public static function analyze( $context->hasVariable($keyed_array_var_id, $statements_analyzer); } - if ($codebase->taint && ($stmt_var_type = $statements_analyzer->node_data->getType($stmt->var))) { - $sources = []; + self::taintArrayFetch( + $statements_analyzer, + $stmt, + $keyed_array_var_id, + $stmt_type, + $used_key_type + ); - $sources = \array_merge($sources, $stmt_var_type->parent_nodes ?: []); + return true; + } - if ($sources) { - $stmt_type->parent_nodes = $sources; + private static function taintArrayFetch( + StatementsAnalyzer $statements_analyzer, + PhpParser\Node\Expr\ArrayDimFetch $stmt, + ?string $keyed_array_var_id, + Type\Union $stmt_type, + Type\Union $offset_type + ) : void { + $codebase = $statements_analyzer->getCodebase(); + + if ($codebase->taint + && ($stmt_var_type = $statements_analyzer->node_data->getType($stmt->var)) + && $stmt_var_type->parent_nodes + ) { + $var_location = new CodeLocation($statements_analyzer->getSource(), $stmt->var); + + $new_parent_node = \Psalm\Internal\Taint\TaintNode::getForAssignment( + $keyed_array_var_id ?: 'array-fetch', + $var_location + ); + + $codebase->taint->addTaintNode($new_parent_node); + + $dim_value = $offset_type->isSingleStringLiteral() + ? $offset_type->getSingleStringLiteral()->value + : ($offset_type->isSingleIntLiteral() + ? $offset_type->getSingleIntLiteral()->value + : null); + + foreach ($stmt_var_type->parent_nodes as $parent_node) { + $codebase->taint->addPath( + $parent_node, + $new_parent_node, + 'array-fetch' . ($dim_value !== null ? '-\'' . $dim_value . '\'' : '') + ); } - } - return true; + $stmt_type->parent_nodes = [$new_parent_node]; + } } /** diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/InstancePropertyFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/InstancePropertyFetchAnalyzer.php index c6ac6f72b81..54ecc1452bf 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/InstancePropertyFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/InstancePropertyFetchAnalyzer.php @@ -1063,7 +1063,7 @@ private static function processTaints( $codebase->taint->addTaintNode($property_node); - $codebase->taint->addPath($property_node, $localized_property_node); + $codebase->taint->addPath($property_node, $localized_property_node, 'property-fetch'); $type->parent_nodes = [$localized_property_node]; } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/VariableFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/VariableFetchAnalyzer.php index 53db1958d13..479c03368c5 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/VariableFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/VariableFetchAnalyzer.php @@ -367,7 +367,7 @@ private static function taintVariable( $server_taint_source = new Source( $var_name . ':' . $taint_location->file_name . ':' . $taint_location->raw_file_start, $var_name, - $taint_location, + null, null, Type\TaintKindGroup::ALL_INPUT ); diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/IncludeAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/IncludeAnalyzer.php index 3dd7e9b15f2..67abd837efc 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/IncludeAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/IncludeAnalyzer.php @@ -65,11 +65,10 @@ public static function analyze( $context->inside_call = false; } - $stmt_expr_type = null; + $stmt_expr_type = $statements_analyzer->node_data->getType($stmt->expr); if ($stmt->expr instanceof PhpParser\Node\Scalar\String_ - || (($stmt_expr_type = $statements_analyzer->node_data->getType($stmt->expr)) - && $stmt_expr_type->isSingleStringLiteral()) + || ($stmt_expr_type && $stmt_expr_type->isSingleStringLiteral()) ) { if ($stmt->expr instanceof PhpParser\Node\Scalar\String_) { $path_to_file = $stmt->expr->value; @@ -102,6 +101,30 @@ public static function analyze( ); } + if ($stmt_expr_type + && $codebase->taint + && $stmt_expr_type->parent_nodes + && $codebase->config->trackTaintsInPath($statements_analyzer->getFilePath()) + ) { + $arg_location = new CodeLocation($statements_analyzer->getSource(), $stmt->expr); + + $include_param_sink = Sink::getForMethodArgument( + 'include', + 'include', + 0, + $arg_location, + $arg_location + ); + + $include_param_sink->taints = [\Psalm\Type\TaintKind::INPUT_TEXT]; + + $codebase->taint->addSink($include_param_sink); + + foreach ($stmt_expr_type->parent_nodes as $parent_node) { + $codebase->taint->addPath($parent_node, $include_param_sink, 'arg'); + } + } + if ($path_to_file) { $path_to_file = self::normalizeFilePath($path_to_file); @@ -266,33 +289,6 @@ public static function getPathTo( return $stmt_type->getSingleStringLiteral()->value; } - if ($stmt_type && $statements_analyzer) { - $codebase = $statements_analyzer->getCodebase(); - - if ($codebase->taint - && $stmt_type->parent_nodes - && $codebase->config->trackTaintsInPath($statements_analyzer->getFilePath()) - ) { - $arg_location = new CodeLocation($statements_analyzer->getSource(), $stmt); - - $include_param_sink = Sink::getForMethodArgument( - 'include', - 'include', - 0, - $arg_location, - $arg_location - ); - - $include_param_sink->taints = [\Psalm\Type\TaintKind::INPUT_TEXT]; - - $codebase->taint->addSink($include_param_sink); - - foreach ($stmt_type->parent_nodes as $parent_node) { - $codebase->taint->addPath($parent_node, $include_param_sink); - } - } - } - if ($stmt instanceof PhpParser\Node\Expr\ArrayDimFetch) { if ($stmt->var instanceof PhpParser\Node\Expr\Variable && $stmt->var->name === 'GLOBALS' diff --git a/src/Psalm/Internal/Analyzer/Statements/ReturnAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/ReturnAnalyzer.php index 6555417e5c4..6adee578e6d 100644 --- a/src/Psalm/Internal/Analyzer/Statements/ReturnAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/ReturnAnalyzer.php @@ -506,6 +506,7 @@ private static function handleTaints( $codebase->taint->addPath( $parent_node, $method_node, + 'return', $storage->added_taints, $storage->removed_taints ); diff --git a/src/Psalm/Internal/Codebase/Taint.php b/src/Psalm/Internal/Codebase/Taint.php index ee97f6656a4..979d0d2f368 100644 --- a/src/Psalm/Internal/Codebase/Taint.php +++ b/src/Psalm/Internal/Codebase/Taint.php @@ -23,6 +23,8 @@ use function substr; use function strlen; use function array_intersect; +use function strpos; +use function array_reverse; class Taint { @@ -35,7 +37,7 @@ class Taint /** @var array */ private $sinks = []; - /** @var array, array}>> */ + /** @var array, array}>> */ private $forward_edges = []; /** @var array> */ @@ -73,13 +75,18 @@ public function addTaintNode(TaintNode $node) : void public function addPath( Taintable $from, Taintable $to, + string $path_type, array $added_taints = [], array $removed_taints = [] ) : void { $from_id = $from->id; $to_id = $to->id; - $this->forward_edges[$from_id][$to_id] = [$added_taints, $removed_taints]; + if ($from_id === $to_id) { + return; + } + + $this->forward_edges[$from_id][$to_id] = [$path_type, $added_taints, $removed_taints]; } public function getPredecessorPath(Taintable $source) : string @@ -159,7 +166,7 @@ public function connectSinksAndSources() : void $sources = $this->sources; $sinks = $this->sinks; - for ($i = 0; count($sinks) && count($sources) && $i < 20; $i++) { + for ($i = 0; count($sinks) && count($sources) && $i < 25; $i++) { $new_sources = []; foreach ($sources as $source) { @@ -200,7 +207,9 @@ private function getChildNodes( ) : array { $new_sources = []; - foreach ($this->forward_edges[$generated_source->id] as $to_id => [$added_taints, $removed_taints]) { + foreach ($this->forward_edges[$generated_source->id] as $to_id => $path_data) { + [$path_type, $added_taints, $removed_taints] = $path_data; + if (!isset($this->nodes[$to_id])) { continue; } @@ -220,6 +229,24 @@ private function getChildNodes( continue; } + if (strpos($path_type, 'array-fetch-') === 0) { + $previous_path_types = array_reverse($generated_source->path_types); + + foreach ($previous_path_types as $previous_path_type) { + if ($previous_path_type === 'array-assignment') { + break; + } + + if (strpos($previous_path_type, 'array-assignment-') === 0) { + if (substr($previous_path_type, 17) === substr($path_type, 12)) { + break; + } + + continue 2; + } + } + } + if (isset($sinks[$to_id])) { $matching_taints = array_intersect($sinks[$to_id]->taints, $new_taints); @@ -243,6 +270,7 @@ private function getChildNodes( $new_destination->previous = $generated_source; $new_destination->taints = $new_taints; $new_destination->specialized_calls = $generated_source->specialized_calls; + $new_destination->path_types = array_merge($generated_source->path_types, [$path_type]); $new_sources[$to_id] = $new_destination; } diff --git a/src/Psalm/Internal/Taint/Taintable.php b/src/Psalm/Internal/Taint/Taintable.php index 0489b6959b3..f535ab61b17 100644 --- a/src/Psalm/Internal/Taint/Taintable.php +++ b/src/Psalm/Internal/Taint/Taintable.php @@ -28,6 +28,9 @@ abstract class Taintable /** @var ?Taintable */ public $previous; + /** @var list */ + public $path_types = []; + /** * @var array> */ @@ -63,8 +66,8 @@ final public static function getForMethodArgument( string $method_id, string $cased_method_id, int $argument_offset, - ?CodeLocation $code_location, - ?CodeLocation $function_location = null + ?CodeLocation $arg_location, + ?CodeLocation $code_location = null ) { $arg_id = $method_id . '#' . ($argument_offset + 1); @@ -72,14 +75,14 @@ final public static function getForMethodArgument( $specialization_key = null; - if ($function_location) { - $specialization_key = strtolower($function_location->file_name) . ':' . $function_location->raw_file_start; + if ($code_location) { + $specialization_key = strtolower($code_location->file_name) . ':' . $code_location->raw_file_start; } return new static( \strtolower($arg_id), $label, - $code_location, + $arg_location, $specialization_key ); } @@ -91,7 +94,10 @@ final public static function getForAssignment( string $var_id, CodeLocation $assignment_location ) { - $id = $var_id . '-' . $assignment_location->file_name . ':' . $assignment_location->raw_file_start; + $id = $var_id + . '-' . $assignment_location->file_name + . ':' . $assignment_location->raw_file_start + . '-' . $assignment_location->raw_file_end; return new static($id, $var_id, $assignment_location, null); } diff --git a/tests/TaintTest.php b/tests/TaintTest.php index cc91c7fd219..c471e66e84d 100644 --- a/tests/TaintTest.php +++ b/tests/TaintTest.php @@ -159,6 +159,99 @@ public function deleteUser(PDO $pdo) : void { $this->analyzeFile('somefile.php', new Context()); } + /** + * @return void + */ + public function testTaintedInputInCreatedArrayNotEchoed() + { + $this->project_analyzer->trackTaintedInputs(); + + $this->addFile( + 'somefile.php', + ' $name, "id" => $id]; + + echo "

" . htmlentities($data["name"]) . "

"; + echo "

" . $data["id"] . "

";' + ); + + $this->analyzeFile('somefile.php', new Context()); + } + + /** + * @return void + */ + public function testTaintedInputInCreatedArrayIsEchoed() + { + $this->expectException(\Psalm\Exception\CodeException::class); + $this->expectExceptionMessage('TaintedInput'); + + $this->project_analyzer->trackTaintedInputs(); + + $this->addFile( + 'somefile.php', + ' $name]; + + echo "

" . $data["name"] . "

";' + ); + + $this->analyzeFile('somefile.php', new Context()); + } + + /** + * @return void + */ + public function testTaintedInputInAssignedArrayNotEchoed() + { + $this->project_analyzer->trackTaintedInputs(); + + $this->addFile( + 'somefile.php', + '" . htmlentities($data["name"]) . ""; + echo "

" . $data["id"] . "

";' + ); + + $this->analyzeFile('somefile.php', new Context()); + } + + /** + * @return void + */ + public function testTaintedInputInAssignedArrayIsEchoed() + { + $this->expectException(\Psalm\Exception\CodeException::class); + $this->expectExceptionMessage('TaintedInput'); + + $this->project_analyzer->trackTaintedInputs(); + + $this->addFile( + 'somefile.php', + '" . $data["name"] . "";' + ); + + $this->analyzeFile('somefile.php', new Context()); + } + /** * @return void */ @@ -330,7 +423,7 @@ public function exec(string $sql) : void {} public function testTaintedInputFromParam() { $this->expectException(\Psalm\Exception\CodeException::class); - $this->expectExceptionMessage('TaintedInput - somefile.php:17:36 - Detected tainted sql in path: $_GET (somefile.php:4:41) -> A::getUserId (somefile.php:8:41) -> concat (somefile.php:8:32) -> A::getAppendedUserId (somefile.php:12:35) -> $userId (somefile.php:12:25) -> A::deleteUser#2 (somefile.php:13:49) -> concat (somefile.php:17:36) -> PDO::exec#1 (somefile.php:17:36)'); + $this->expectExceptionMessage('TaintedInput - somefile.php:17:36 - Detected tainted sql in path: $_GET -> $_GET[\'user_id\'] (somefile.php:4:41) -> A::getUserId (somefile.php:3:51) -> concat (somefile.php:8:32) -> A::getAppendedUserId (somefile.php:7:59) -> $userId (somefile.php:12:25) -> A::deleteUser#2 (somefile.php:16:65) -> concat (somefile.php:17:36) -> PDO::exec#1'); $this->project_analyzer->trackTaintedInputs(); @@ -467,7 +560,7 @@ public function deleteUser(PDO $pdo, string $userId) : void { public function testTaintedInputToParamAlternatePath() { $this->expectException(\Psalm\Exception\CodeException::class); - $this->expectExceptionMessage('TaintedInput - somefile.php:23:40 - Detected tainted sql in path: $_GET (somefile.php:7:63) -> A::getAppendedUserId#1 (somefile.php:7:54) -> concat (somefile.php:12:32) -> A::getAppendedUserId (somefile.php:11:37) -> A::deleteUser#3 (somefile.php:7:29) -> concat (somefile.php:23:40) -> PDO::exec#1 (somefile.php:23:40)'); + $this->expectExceptionMessage('TaintedInput - somefile.php:23:40 - Detected tainted sql in path: $_GET -> $_GET[\'user_id\'] (somefile.php:7:63) -> A::getAppendedUserId#1 (somefile.php:11:62) -> concat (somefile.php:12:32) -> A::getAppendedUserId (somefile.php:11:37) -> A::deleteUser#3 (somefile.php:19:81) -> concat (somefile.php:23:40) -> PDO::exec#1'); $this->project_analyzer->trackTaintedInputs(); @@ -510,7 +603,7 @@ public function deleteUser(PDO $pdo, string $userId, string $userId2) : void { public function testTaintedInParentLoader() { $this->expectException(\Psalm\Exception\CodeException::class); - $this->expectExceptionMessage('TaintedInput - somefile.php:16:40 - Detected tainted sql in path: $_GET (somefile.php:28:39) -> C::foo#1 (somefile.php:28:30) -> AGrandChild::loadFull#1 (somefile.php:24:47) -> A::loadFull#1 (somefile.php:24:47) -> A::loadPartial#1 (somefile.php:6:45) -> AChild::loadPartial#1 (somefile.php:6:45) -> concat (somefile.php:16:40) -> PDO::exec#1 (somefile.php:16:40)'); + $this->expectExceptionMessage('TaintedInput - somefile.php:16:40 - Detected tainted sql in path: $_GET -> $_GET[\'user_id\'] (somefile.php:28:39) -> C::foo#1 (somefile.php:23:48) -> AGrandChild::loadFull#1 (somefile.php:5:60) -> A::loadFull#1 (somefile.php:24:47) -> A::loadPartial#1 (somefile.php:3:72) -> AChild::loadPartial#1 (somefile.php:6:45) -> concat (somefile.php:16:40) -> PDO::exec#1'); $this->project_analyzer->trackTaintedInputs();