diff --git a/src/Handlers/Rules/TimingUnsafeComparisonHandler.php b/src/Handlers/Rules/TimingUnsafeComparisonHandler.php new file mode 100644 index 00000000..44dbe2ef --- /dev/null +++ b/src/Handlers/Rules/TimingUnsafeComparisonHandler.php @@ -0,0 +1,160 @@ +getExpr(); + $source = $event->getStatementsSource(); + + if (!$source instanceof StatementsAnalyzer) { + return null; + } + + $taintFlowGraph = $source->taint_flow_graph; + + if (!$taintFlowGraph instanceof \Psalm\Internal\Codebase\TaintFlowGraph) { + return null; + } + + if ($expr instanceof BinaryOp\Identical + || $expr instanceof BinaryOp\Equal + || $expr instanceof BinaryOp\NotIdentical + || $expr instanceof BinaryOp\NotEqual + ) { + self::addSinksForOperands( + $source, + $taintFlowGraph, + $expr, + $source->getNodeTypeProvider()->getType($expr->left), + $source->getNodeTypeProvider()->getType($expr->right), + ); + + return null; + } + + // strcmp() and strcasecmp() are also timing-unsafe — they compare + // character-by-character and the return value reveals partial ordering + if ($expr instanceof FuncCall + && $expr->name instanceof Name + && \in_array($expr->name->toLowerString(), self::TIMING_UNSAFE_FUNCTIONS, true) + && \count($expr->args) >= 2 + && $expr->args[0] instanceof Arg + && $expr->args[1] instanceof Arg + ) { + self::addSinksForOperands( + $source, + $taintFlowGraph, + $expr, + $source->getNodeTypeProvider()->getType($expr->args[0]->value), + $source->getNodeTypeProvider()->getType($expr->args[1]->value), + ); + } + + return null; + } + + /** + * Add taint sinks for both operands of a timing-unsafe comparison. + * + * Each operand's data flow parent nodes are connected to a new sink node + * that matches user_secret and system_secret taints. If either operand + * carries secret taint, Psalm's taint resolution will report the issue. + * + * @psalm-external-mutation-free + */ + private static function addSinksForOperands( + StatementsAnalyzer $source, + TaintFlowGraph $graph, + Expr $comparisonExpr, + ?Union $leftType, + ?Union $rightType, + ): void { + $codeLocation = new CodeLocation($source, $comparisonExpr); + $locationId = \strtolower($codeLocation->file_name) + . ':' . $codeLocation->raw_file_start + . '-' . $codeLocation->raw_file_end; + + self::addSinkForType($graph, $leftType, 'timing-comparison-left', $locationId, $codeLocation); + self::addSinkForType($graph, $rightType, 'timing-comparison-right', $locationId, $codeLocation); + } + + /** + * Create a single taint sink for an operand and connect all its data flow + * parent nodes to it. One sink per operand side avoids duplicate reports + * and keeps the taint graph compact. + * + * The sink matches USER_SECRET | SYSTEM_SECRET, so only secret-tainted + * data triggers an issue — ordinary input taint is not affected. + * + * @psalm-external-mutation-free + */ + private static function addSinkForType( + TaintFlowGraph $graph, + ?Union $type, + string $sinkLabel, + string $locationId, + CodeLocation $codeLocation, + ): void { + if (!$type instanceof \Psalm\Type\Union || $type->parent_nodes === []) { + return; + } + + $sinkId = $sinkLabel . '-' . $locationId; + + $sink = DataFlowNode::make( + $sinkId, + $sinkLabel, + $codeLocation, + null, + self::SECRET_TAINTS, + ); + + $graph->addSink($sink); + + foreach ($type->parent_nodes as $parentNode) { + $graph->addPath($parentNode, $sink, 'timing-comparison'); + } + } + +} diff --git a/src/Plugin.php b/src/Plugin.php index b944b4c4..394e5995 100644 --- a/src/Plugin.php +++ b/src/Plugin.php @@ -247,6 +247,9 @@ private function registerHandlers(RegistrationInterface $registration, PluginCon require_once __DIR__ . '/Handlers/Rules/NoEnvOutsideConfigHandler.php'; $registration->registerHooksFromClass(Handlers\Rules\NoEnvOutsideConfigHandler::class); + require_once __DIR__ . '/Handlers/Rules/TimingUnsafeComparisonHandler.php'; + $registration->registerHooksFromClass(Handlers\Rules\TimingUnsafeComparisonHandler::class); + // Unlike TranslationKeyHandler (which always runs for type narrowing), // MissingViewHandler provides no type information — skip entirely when disabled if ($pluginConfig->findMissingViews) { diff --git a/tests/Type/tests/TaintAnalysis/SafeHashEqualsSecret.phpt b/tests/Type/tests/TaintAnalysis/SafeHashEqualsSecret.phpt new file mode 100644 index 00000000..e47f77e8 --- /dev/null +++ b/tests/Type/tests/TaintAnalysis/SafeHashEqualsSecret.phpt @@ -0,0 +1,20 @@ +--ARGS-- +--no-progress --no-diff --config=./tests/Type/psalm.xml --taint-analysis +--FILE-- + +--EXPECTF-- + diff --git a/tests/Type/tests/TaintAnalysis/SafeNonSecretComparison.phpt b/tests/Type/tests/TaintAnalysis/SafeNonSecretComparison.phpt new file mode 100644 index 00000000..ce74aac2 --- /dev/null +++ b/tests/Type/tests/TaintAnalysis/SafeNonSecretComparison.phpt @@ -0,0 +1,15 @@ +--ARGS-- +--no-progress --no-diff --config=./tests/Type/psalm.xml --taint-analysis +--FILE-- +input('role') === 'admin'; +} +?> +--EXPECTF-- + diff --git a/tests/Type/tests/TaintAnalysis/SafeNonSecretStrcmp.phpt b/tests/Type/tests/TaintAnalysis/SafeNonSecretStrcmp.phpt new file mode 100644 index 00000000..d6c081db --- /dev/null +++ b/tests/Type/tests/TaintAnalysis/SafeNonSecretStrcmp.phpt @@ -0,0 +1,17 @@ +--ARGS-- +--no-progress --no-diff --config=./tests/Type/psalm.xml --taint-analysis +--FILE-- +input('role'); + return strcmp($role, 'admin') === 0; +} +?> +--EXPECTF-- + diff --git a/tests/Type/tests/TaintAnalysis/TaintedSystemSecretIndirectComparison.phpt b/tests/Type/tests/TaintAnalysis/TaintedSystemSecretIndirectComparison.phpt new file mode 100644 index 00000000..85d5cb2e --- /dev/null +++ b/tests/Type/tests/TaintAnalysis/TaintedSystemSecretIndirectComparison.phpt @@ -0,0 +1,22 @@ +--ARGS-- +--no-progress --no-diff --config=./tests/Type/psalm.xml --taint-analysis +--FILE-- + +--EXPECTF-- +%ATaintedSystemSecret on line %d: Detected tainted system secret leaking diff --git a/tests/Type/tests/TaintAnalysis/TaintedSystemSecretNotIdenticalComparison.phpt b/tests/Type/tests/TaintAnalysis/TaintedSystemSecretNotIdenticalComparison.phpt new file mode 100644 index 00000000..534c04cb --- /dev/null +++ b/tests/Type/tests/TaintAnalysis/TaintedSystemSecretNotIdenticalComparison.phpt @@ -0,0 +1,23 @@ +--ARGS-- +--no-progress --no-diff --config=./tests/Type/psalm.xml --taint-analysis +--FILE-- + +--EXPECTF-- +%ATaintedSystemSecret on line %d: Detected tainted system secret leaking diff --git a/tests/Type/tests/TaintAnalysis/TaintedSystemSecretStrcmp.phpt b/tests/Type/tests/TaintAnalysis/TaintedSystemSecretStrcmp.phpt new file mode 100644 index 00000000..58d1a448 --- /dev/null +++ b/tests/Type/tests/TaintAnalysis/TaintedSystemSecretStrcmp.phpt @@ -0,0 +1,21 @@ +--ARGS-- +--no-progress --no-diff --config=./tests/Type/psalm.xml --taint-analysis +--FILE-- + +--EXPECTF-- +%ATaintedSystemSecret on line %d: Detected tainted system secret leaking diff --git a/tests/Type/tests/TaintAnalysis/TaintedSystemSecretTimingComparison.phpt b/tests/Type/tests/TaintAnalysis/TaintedSystemSecretTimingComparison.phpt new file mode 100644 index 00000000..bf97c8ee --- /dev/null +++ b/tests/Type/tests/TaintAnalysis/TaintedSystemSecretTimingComparison.phpt @@ -0,0 +1,21 @@ +--ARGS-- +--no-progress --no-diff --config=./tests/Type/psalm.xml --taint-analysis +--FILE-- + +--EXPECTF-- +%ATaintedSystemSecret on line %d: Detected tainted system secret leaking diff --git a/tests/Type/tests/TaintAnalysis/TaintedUserSecretLeftOperand.phpt b/tests/Type/tests/TaintAnalysis/TaintedUserSecretLeftOperand.phpt new file mode 100644 index 00000000..f72d5434 --- /dev/null +++ b/tests/Type/tests/TaintAnalysis/TaintedUserSecretLeftOperand.phpt @@ -0,0 +1,21 @@ +--ARGS-- +--no-progress --no-diff --config=./tests/Type/psalm.xml --taint-analysis +--FILE-- + +--EXPECTF-- +%ATaintedUserSecret on line %d: Detected tainted user secret leaking diff --git a/tests/Type/tests/TaintAnalysis/TaintedUserSecretNotEqualComparison.phpt b/tests/Type/tests/TaintAnalysis/TaintedUserSecretNotEqualComparison.phpt new file mode 100644 index 00000000..e4c68e35 --- /dev/null +++ b/tests/Type/tests/TaintAnalysis/TaintedUserSecretNotEqualComparison.phpt @@ -0,0 +1,22 @@ +--ARGS-- +--no-progress --no-diff --config=./tests/Type/psalm.xml --taint-analysis +--FILE-- + +--EXPECTF-- +%ATaintedUserSecret on line %d: Detected tainted user secret leaking diff --git a/tests/Type/tests/TaintAnalysis/TaintedUserSecretStrcasecmp.phpt b/tests/Type/tests/TaintAnalysis/TaintedUserSecretStrcasecmp.phpt new file mode 100644 index 00000000..772127fc --- /dev/null +++ b/tests/Type/tests/TaintAnalysis/TaintedUserSecretStrcasecmp.phpt @@ -0,0 +1,20 @@ +--ARGS-- +--no-progress --no-diff --config=./tests/Type/psalm.xml --taint-analysis +--FILE-- + +--EXPECTF-- +%ATaintedUserSecret on line %d: Detected tainted user secret leaking diff --git a/tests/Type/tests/TaintAnalysis/TaintedUserSecretTimingComparison.phpt b/tests/Type/tests/TaintAnalysis/TaintedUserSecretTimingComparison.phpt new file mode 100644 index 00000000..240f6f9a --- /dev/null +++ b/tests/Type/tests/TaintAnalysis/TaintedUserSecretTimingComparison.phpt @@ -0,0 +1,21 @@ +--ARGS-- +--no-progress --no-diff --config=./tests/Type/psalm.xml --taint-analysis +--FILE-- + +--EXPECTF-- +%ATaintedUserSecret on line %d: Detected tainted user secret leaking