From 3edc442e558a98a9a9bc0b9663261e05de7cf772 Mon Sep 17 00:00:00 2001 From: Vladyslav Vildanov <117659936+vladvildanov@users.noreply.github.com> Date: Wed, 16 Sep 2026 10:01:26 +0300 Subject: [PATCH] Fixed CRLF command smuggling and node misrouting in `AbstractAggregateConnection::write()` and `CommandInterface::deserializeCommand()` (#1728) * Fixed CRLF command injection / smuggling in AbstractAggregateConnection::write() * Revert "Fixed CRLF command injection / smuggling in AbstractAggregateConnection::write()" This reverts commit a4720b03ee6b2b6e6a66284cd0540b8805f20cec. * Fixed CRLF command smuggling and node misrouting in AbstractAggregateConnection::write() and CommandInterface::deserializeCommand() * Revert change --- CHANGELOG.md | 1 + src/Command/Command.php | 62 +++++++++++---- src/Command/RawCommand.php | 62 +++++++++++---- .../AbstractAggregateConnection.php | 26 +++--- tests/Predis/Command/CommandTest.php | 11 +++ tests/Predis/Command/RawCommandTest.php | 11 +++ .../Connection/Cluster/PredisClusterTest.php | 70 ++++++++++++++++ .../Connection/Cluster/RedisClusterTest.php | 73 +++++++++++++++++ .../MasterSlaveReplicationTest.php | 63 +++++++++++++++ .../Replication/SentinelReplicationTest.php | 79 +++++++++++++++++++ 10 files changed, 421 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 14790054..86a62eea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ - Fixed RESP3 double parsing returning positive `INF` for `-inf` payloads (#1716) - Fixed `client_info` connection parameter being ignored (#1722) - Fixed `Stream::write()`/`read()` leaving a dead connection when a host error handler throws exception (#1725) +- Fixed CRLF command smuggling and node misrouting in `AbstractAggregateConnection::write()` and `CommandInterface::deserializeCommand()` (CVE GHSA-w6f5-v2h6-g786) ## v3.6.0 (2026-08-14) ### Added diff --git a/src/Command/Command.php b/src/Command/Command.php index 32619305..03f81e00 100644 --- a/src/Command/Command.php +++ b/src/Command/Command.php @@ -164,12 +164,8 @@ abstract class Command implements CommandInterface */ public static function deserializeCommand(string $serializedCommand): CommandInterface { - if ($serializedCommand[0] !== '*') { - throw new UnexpectedValueException('Invalid serializing format'); - } - - $commandArray = explode("\r\n", $serializedCommand); - $commandId = $commandArray[2]; + $items = self::parseMultibulk($serializedCommand); + $commandId = $items[0]; $classPath = __NAMESPACE__ . '\Redis\\'; // Check if given command is a module command. @@ -189,15 +185,53 @@ abstract class Command implements CommandInterface } $command = new $classPath(); - $arguments = []; - - for ($i = 4, $iMax = count($commandArray); $i < $iMax; $i++) { - $arguments[] = $commandArray[$i]; - ++$i; - } - - $command->setArguments($arguments); + $command->setArguments(array_slice($items, 1)); return $command; } + + /** + * Parses a RESP multibulk buffer into its individual bulk-string values + * (command ID followed by its arguments), walking each string by its own + * declared byte length instead of splitting the buffer on "\r\n" -- which + * a bulk string's payload may legitimately contain (see GHSA-w6f5-v2h6-g786). + * + * @param string $buffer + * @return string[] + */ + private static function parseMultibulk(string $buffer): array + { + if ($buffer[0] !== '*') { + throw new UnexpectedValueException('Invalid serializing format'); + } + + $lineEnd = strpos($buffer, "\r\n"); + + if ($lineEnd === false) { + throw new UnexpectedValueException('Invalid serializing format'); + } + + $count = (int) substr($buffer, 1, $lineEnd - 1); + $offset = $lineEnd + 2; + $items = []; + + for ($i = 0; $i < $count; ++$i) { + if (($buffer[$offset] ?? '') !== '$') { + throw new UnexpectedValueException('Invalid serializing format'); + } + + $lineEnd = strpos($buffer, "\r\n", $offset); + + if ($lineEnd === false) { + throw new UnexpectedValueException('Invalid serializing format'); + } + + $bulkLen = (int) substr($buffer, $offset + 1, $lineEnd - $offset - 1); + $dataStart = $lineEnd + 2; + $items[] = substr($buffer, $dataStart, $bulkLen); + $offset = $dataStart + $bulkLen + 2; + } + + return $items; + } } diff --git a/src/Command/RawCommand.php b/src/Command/RawCommand.php index 0e417fcd..ad858600 100644 --- a/src/Command/RawCommand.php +++ b/src/Command/RawCommand.php @@ -161,12 +161,8 @@ final class RawCommand implements CommandInterface */ public static function deserializeCommand(string $serializedCommand): CommandInterface { - if ($serializedCommand[0] !== '*') { - throw new UnexpectedValueException('Invalid serializing format'); - } - - $commandArray = explode("\r\n", $serializedCommand); - $commandId = $commandArray[2]; + $items = self::parseMultibulk($serializedCommand); + $commandId = $items[0]; $classPath = __NAMESPACE__ . '\Redis\\'; // Check if given command is a module command. @@ -186,15 +182,53 @@ final class RawCommand implements CommandInterface } $command = new $classPath(); - $arguments = []; - - for ($i = 4, $iMax = count($commandArray); $i < $iMax; $i++) { - $arguments[] = $commandArray[$i]; - ++$i; - } - - $command->setArguments($arguments); + $command->setArguments(array_slice($items, 1)); return $command; } + + /** + * Parses a RESP multibulk buffer into its individual bulk-string values + * (command ID followed by its arguments), walking each string by its own + * declared byte length instead of splitting the buffer on "\r\n" -- which + * a bulk string's payload may legitimately contain (see GHSA-w6f5-v2h6-g786). + * + * @param string $buffer + * @return string[] + */ + private static function parseMultibulk(string $buffer): array + { + if ($buffer[0] !== '*') { + throw new UnexpectedValueException('Invalid serializing format'); + } + + $lineEnd = strpos($buffer, "\r\n"); + + if ($lineEnd === false) { + throw new UnexpectedValueException('Invalid serializing format'); + } + + $count = (int) substr($buffer, 1, $lineEnd - 1); + $offset = $lineEnd + 2; + $items = []; + + for ($i = 0; $i < $count; ++$i) { + if (($buffer[$offset] ?? '') !== '$') { + throw new UnexpectedValueException('Invalid serializing format'); + } + + $lineEnd = strpos($buffer, "\r\n", $offset); + + if ($lineEnd === false) { + throw new UnexpectedValueException('Invalid serializing format'); + } + + $bulkLen = (int) substr($buffer, $offset + 1, $lineEnd - $offset - 1); + $dataStart = $lineEnd + 2; + $items[] = substr($buffer, $dataStart, $bulkLen); + $offset = $dataStart + $bulkLen + 2; + } + + return $items; + } } diff --git a/src/Connection/AbstractAggregateConnection.php b/src/Connection/AbstractAggregateConnection.php index 26586828..58009d1d 100644 --- a/src/Connection/AbstractAggregateConnection.php +++ b/src/Connection/AbstractAggregateConnection.php @@ -77,17 +77,25 @@ abstract class AbstractAggregateConnection implements AggregateConnectionInterfa */ public function write(string $buffer): void { - $rawCommands = []; - $explodedBuffer = explode("\r\n", trim($buffer)); + $offset = 0; + $length = strlen($buffer); - while (!empty($explodedBuffer)) { - $argsLen = (int) explode('*', $explodedBuffer[0])[1]; - $cmdLen = ($argsLen * 2) + 1; - $rawCommands[] = array_splice($explodedBuffer, 0, $cmdLen); - } + while ($offset < $length) { + $start = $offset; + $lineEnd = strpos($buffer, "\r\n", $offset); + $argsCount = (int) substr($buffer, $offset + 1, $lineEnd - $offset - 1); + $offset = $lineEnd + 2; - foreach ($rawCommands as $command) { - $command = implode("\r\n", $command) . "\r\n"; + // Advance by each bulk string's own declared byte length rather than + // splitting on literal "\r\n", which a bulk string's value may legitimately + // contain (see GHSA-w6f5-v2h6-g786). + for ($i = 0; $i < $argsCount; ++$i) { + $lineEnd = strpos($buffer, "\r\n", $offset); + $bulkLen = (int) substr($buffer, $offset + 1, $lineEnd - $offset - 1); + $offset = $lineEnd + 2 + $bulkLen + 2; + } + + $command = substr($buffer, $start, $offset - $start); $commandObj = Command::deserializeCommand($command); $this->getConnectionByCommand($commandObj)->write($command); } diff --git a/tests/Predis/Command/CommandTest.php b/tests/Predis/Command/CommandTest.php index 69c9d3ae..2852cf82 100644 --- a/tests/Predis/Command/CommandTest.php +++ b/tests/Predis/Command/CommandTest.php @@ -281,6 +281,17 @@ class CommandTest extends PredisTestCase TOPKQUERY::class, ['key'], ], + // Regression cases for GHSA-w6f5-v2h6-g786 (CWE-93): a "\r\n" embedded + // in a bulk string's own byte payload must not be mistaken for a RESP + // line boundary. + 'GET with CRLF embedded in the value' => [ + GET::class, + ["value\r\n*1\r\n\$4\r\nEVIL"], + ], + 'ZADD with CRLF embedded in the key' => [ + ZADD::class, + ["key\r\n*1\r\n\$4\r\nEVIL", 'value', 'key1', 'value1'], + ], ]; } } diff --git a/tests/Predis/Command/RawCommandTest.php b/tests/Predis/Command/RawCommandTest.php index 22edb906..83b0b2c7 100644 --- a/tests/Predis/Command/RawCommandTest.php +++ b/tests/Predis/Command/RawCommandTest.php @@ -238,6 +238,17 @@ class RawCommandTest extends PredisTestCase TOPKQUERY::class, ['key'], ], + // Regression cases for GHSA-w6f5-v2h6-g786 (CWE-93): a "\r\n" embedded + // in a bulk string's own byte payload must not be mistaken for a RESP + // line boundary. + 'GET with CRLF embedded in the value' => [ + GET::class, + ["value\r\n*1\r\n\$4\r\nEVIL"], + ], + 'ZADD with CRLF embedded in the key' => [ + ZADD::class, + ["key\r\n*1\r\n\$4\r\nEVIL", 'value', 'key1', 'value1'], + ], ]; } } diff --git a/tests/Predis/Connection/Cluster/PredisClusterTest.php b/tests/Predis/Connection/Cluster/PredisClusterTest.php index 3b8c5b45..cfd10f8a 100644 --- a/tests/Predis/Connection/Cluster/PredisClusterTest.php +++ b/tests/Predis/Connection/Cluster/PredisClusterTest.php @@ -14,6 +14,7 @@ namespace Predis\Connection\Cluster; use Predis\Command\CommandInterface; use Predis\Command\Redis\GET; +use Predis\Command\Redis\SET; use Predis\Connection\Parameters; use PredisTestCase; @@ -516,4 +517,73 @@ class PredisClusterTest extends PredisTestCase $cluster->write($command1->serializeCommand() . $command2->serializeCommand() . $command3->serializeCommand()); } + + /** + * Regression guard for GHSA-w6f5-v2h6-g786 (CWE-93): a CRLF embedded in a + * bulk string's own value must not be mistaken for a command boundary + * (splitting one command into a smuggled extra command), and must not + * corrupt the argument list used to pick the target node. + * + * @group disconnected + */ + public function testWriteHandlesCRLFEmbeddedInBulkStringValue(): void + { + $command = new SET(); + $command->setArguments(['victim-key', "PAD\r\n*1\r\n\$7\r\nFLUSHDB"]); + + $connection1 = $this->getMockConnection('tcp://127.0.0.1:7001'); + $connection2 = $this->getMockConnection('tcp://127.0.0.1:7002'); + + $cluster = new PredisCluster(new Parameters()); + $cluster->add($connection1); + $cluster->add($connection2); + + $expectedConnection = $cluster->getConnectionByCommand($command); + $otherConnection = $expectedConnection === $connection1 ? $connection2 : $connection1; + + $expectedConnection + ->expects($this->once()) + ->method('write') + ->with($command->serializeCommand()); + + $otherConnection + ->expects($this->never()) + ->method('write'); + + $cluster->write($command->serializeCommand()); + } + + /** + * Regression guard for GHSA-w6f5-v2h6-g786 (CWE-93): a CRLF embedded in a + * bulk string KEY must not corrupt the argument list used to pick the + * target node, which would silently route the command to the wrong node. + * + * @group disconnected + */ + public function testWriteHandlesCRLFEmbeddedInBulkStringKey(): void + { + $command = new SET(); + $command->setArguments(["victim\r\n*1\r\n\$4\r\nEVIL", 'somevalue']); + + $connection1 = $this->getMockConnection('tcp://127.0.0.1:7001'); + $connection2 = $this->getMockConnection('tcp://127.0.0.1:7002'); + + $cluster = new PredisCluster(new Parameters()); + $cluster->add($connection1); + $cluster->add($connection2); + + $expectedConnection = $cluster->getConnectionByCommand($command); + $otherConnection = $expectedConnection === $connection1 ? $connection2 : $connection1; + + $expectedConnection + ->expects($this->once()) + ->method('write') + ->with($command->serializeCommand()); + + $otherConnection + ->expects($this->never()) + ->method('write'); + + $cluster->write($command->serializeCommand()); + } } diff --git a/tests/Predis/Connection/Cluster/RedisClusterTest.php b/tests/Predis/Connection/Cluster/RedisClusterTest.php index 2f70ccc3..4c5ccd1a 100644 --- a/tests/Predis/Connection/Cluster/RedisClusterTest.php +++ b/tests/Predis/Connection/Cluster/RedisClusterTest.php @@ -1676,4 +1676,77 @@ class RedisClusterTest extends PredisTestCase $cluster->write($command1->serializeCommand() . $command2->serializeCommand() . $command3->serializeCommand()); } + + /** + * Regression guard for GHSA-w6f5-v2h6-g786 (CWE-93): a CRLF embedded in a + * bulk string's own value must not be mistaken for a command boundary + * (splitting one command into a smuggled extra command), and must not + * corrupt the argument list used to pick the target node. + * + * @group disconnected + */ + public function testWriteHandlesCRLFEmbeddedInBulkStringValue(): void + { + $command = new Command\Redis\SET(); + $command->setArguments(['victim-key', "PAD\r\n*1\r\n\$7\r\nFLUSHDB"]); + + $factory = $this->getMockBuilder(FactoryInterface::class)->getMock(); + + $connection1 = $this->getMockConnection('tcp://127.0.0.1:7001'); + $connection2 = $this->getMockConnection('tcp://127.0.0.1:7002'); + $connection3 = $this->getMockConnection('tcp://127.0.0.1:7003'); + + $cluster = new RedisCluster($factory, new Parameters()); + $cluster->add($connection1); + $cluster->add($connection2); + $cluster->add($connection3); + + $expectedConnection = $cluster->getConnectionByCommand($command); + + foreach ([$connection1, $connection2, $connection3] as $connection) { + if ($connection === $expectedConnection) { + $connection->expects($this->once())->method('write')->with($command->serializeCommand()); + } else { + $connection->expects($this->never())->method('write'); + } + } + + $cluster->write($command->serializeCommand()); + } + + /** + * Regression guard for GHSA-w6f5-v2h6-g786 (CWE-93): a CRLF embedded in a + * bulk string KEY must not corrupt the argument list used to pick the + * target node, which would silently route the command to the wrong node. + * + * @group disconnected + */ + public function testWriteHandlesCRLFEmbeddedInBulkStringKey(): void + { + $command = new Command\Redis\SET(); + $command->setArguments(["victim\r\n*1\r\n\$4\r\nEVIL", 'somevalue']); + + $factory = $this->getMockBuilder(FactoryInterface::class)->getMock(); + + $connection1 = $this->getMockConnection('tcp://127.0.0.1:7001'); + $connection2 = $this->getMockConnection('tcp://127.0.0.1:7002'); + $connection3 = $this->getMockConnection('tcp://127.0.0.1:7003'); + + $cluster = new RedisCluster($factory, new Parameters()); + $cluster->add($connection1); + $cluster->add($connection2); + $cluster->add($connection3); + + $expectedConnection = $cluster->getConnectionByCommand($command); + + foreach ([$connection1, $connection2, $connection3] as $connection) { + if ($connection === $expectedConnection) { + $connection->expects($this->once())->method('write')->with($command->serializeCommand()); + } else { + $connection->expects($this->never())->method('write'); + } + } + + $cluster->write($command->serializeCommand()); + } } diff --git a/tests/Predis/Connection/Replication/MasterSlaveReplicationTest.php b/tests/Predis/Connection/Replication/MasterSlaveReplicationTest.php index 7383c053..3386eef0 100644 --- a/tests/Predis/Connection/Replication/MasterSlaveReplicationTest.php +++ b/tests/Predis/Connection/Replication/MasterSlaveReplicationTest.php @@ -1496,6 +1496,69 @@ repl_backlog_histlen:12978 $replication->write($command1->serializeCommand() . $command2->serializeCommand() . $command3->serializeCommand()); } + /** + * Regression guard for GHSA-w6f5-v2h6-g786 (CWE-93): a CRLF embedded in a + * bulk string's own value must not be mistaken for a command boundary + * (splitting one command into a smuggled extra command). + * + * @group disconnected + */ + public function testWriteHandlesCRLFEmbeddedInBulkStringValue(): void + { + $command = new Command\Redis\SET(); + $command->setArguments(['victim-key', "PAD\r\n*1\r\n\$7\r\nFLUSHDB"]); + + $master = $this->getMockConnection('tcp://127.0.0.1:6379?role=master'); + $slave1 = $this->getMockConnection('tcp://127.0.0.1:6380?role=slave'); + + $slave1 + ->expects($this->never()) + ->method('write'); + + $master + ->expects($this->once()) + ->method('write') + ->with($command->serializeCommand()); + + $replication = new MasterSlaveReplication(); + $replication->add($master); + $replication->add($slave1); + + $replication->write($command->serializeCommand()); + } + + /** + * Regression guard for GHSA-w6f5-v2h6-g786 (CWE-93): a CRLF embedded in a + * bulk string KEY must not corrupt the argument list used to pick the + * target connection, which would silently route the command to the + * wrong node. + * + * @group disconnected + */ + public function testWriteHandlesCRLFEmbeddedInBulkStringKey(): void + { + $command = new Command\Redis\SET(); + $command->setArguments(["victim\r\n*1\r\n\$4\r\nEVIL", 'somevalue']); + + $master = $this->getMockConnection('tcp://127.0.0.1:6379?role=master'); + $slave1 = $this->getMockConnection('tcp://127.0.0.1:6380?role=slave'); + + $slave1 + ->expects($this->never()) + ->method('write'); + + $master + ->expects($this->once()) + ->method('write') + ->with($command->serializeCommand()); + + $replication = new MasterSlaveReplication(); + $replication->add($master); + $replication->add($slave1); + + $replication->write($command->serializeCommand()); + } + /** * @medium * @group disconnected diff --git a/tests/Predis/Connection/Replication/SentinelReplicationTest.php b/tests/Predis/Connection/Replication/SentinelReplicationTest.php index 4ccc2ffa..46766580 100644 --- a/tests/Predis/Connection/Replication/SentinelReplicationTest.php +++ b/tests/Predis/Connection/Replication/SentinelReplicationTest.php @@ -2020,6 +2020,85 @@ class SentinelReplicationTest extends PredisTestCase $replication->write($command1->serializeCommand() . $command2->serializeCommand() . $command3->serializeCommand()); } + /** + * Regression guard for GHSA-w6f5-v2h6-g786 (CWE-93): a CRLF embedded in a + * bulk string's own value must not be mistaken for a command boundary + * (splitting one command into a smuggled extra command). + * + * @group disconnected + */ + public function testWriteHandlesCRLFEmbeddedInBulkStringValue(): void + { + $command = new Command\Redis\SET(); + $command->setArguments(['victim-key', "PAD\r\n*1\r\n\$7\r\nFLUSHDB"]); + + $sentinel = $this->getMockSentinelConnection('tcp://127.0.0.1:5381?role=sentinel'); + $master = $this->getMockConnection('tcp://127.0.0.1:6379?role=master'); + $slave = $this->getMockConnection('tcp://127.0.0.1:6380?role=slave'); + $strategy = new Replication\ReplicationStrategy(); + $factory = new Connection\Factory(); + + $master + ->expects($this->once()) + ->method('isConnected') + ->willReturn(true); + + $slave + ->expects($this->never()) + ->method('write'); + + $master + ->expects($this->once()) + ->method('write') + ->with($command->serializeCommand()); + + $replication = new SentinelReplication('svc', [$sentinel], $factory, $strategy); + $replication->add($master); + $replication->add($slave); + + $replication->write($command->serializeCommand()); + } + + /** + * Regression guard for GHSA-w6f5-v2h6-g786 (CWE-93): a CRLF embedded in a + * bulk string KEY must not corrupt the argument list used to pick the + * target connection, which would silently route the command to the + * wrong node. + * + * @group disconnected + */ + public function testWriteHandlesCRLFEmbeddedInBulkStringKey(): void + { + $command = new Command\Redis\SET(); + $command->setArguments(["victim\r\n*1\r\n\$4\r\nEVIL", 'somevalue']); + + $sentinel = $this->getMockSentinelConnection('tcp://127.0.0.1:5381?role=sentinel'); + $master = $this->getMockConnection('tcp://127.0.0.1:6379?role=master'); + $slave = $this->getMockConnection('tcp://127.0.0.1:6380?role=slave'); + $strategy = new Replication\ReplicationStrategy(); + $factory = new Connection\Factory(); + + $master + ->expects($this->once()) + ->method('isConnected') + ->willReturn(true); + + $slave + ->expects($this->never()) + ->method('write'); + + $master + ->expects($this->once()) + ->method('write') + ->with($command->serializeCommand()); + + $replication = new SentinelReplication('svc', [$sentinel], $factory, $strategy); + $replication->add($master); + $replication->add($slave); + + $replication->write($command->serializeCommand()); + } + public function connectionsProvider(): array { return [