diff --git a/CHANGELOG.md b/CHANGELOG.md index 17bec36c..92d2b420 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,9 +1,10 @@ ## Changelog -## Unlreleads +## Unreleased ### Added ### Changed ### Fixed +- Fixed CRLF command injection / smuggling in `AbstractAggregateConnection::write()` (CVE GHSA-w6f5-v2h6-g786, CWE-93) - Fixed RESP3 double parsing returning positive `INF` for `-inf` payloads (#1716) ## v3.6.0 (2026-08-14) diff --git a/src/Connection/AbstractAggregateConnection.php b/src/Connection/AbstractAggregateConnection.php index 26586828..edd7e8b8 100644 --- a/src/Connection/AbstractAggregateConnection.php +++ b/src/Connection/AbstractAggregateConnection.php @@ -12,8 +12,8 @@ namespace Predis\Connection; -use Predis\Command\Command; use Predis\Command\CommandInterface; +use Predis\NotSupportedException; abstract class AbstractAggregateConnection implements AggregateConnectionInterface { @@ -77,19 +77,12 @@ abstract class AbstractAggregateConnection implements AggregateConnectionInterfa */ public function write(string $buffer): void { - $rawCommands = []; - $explodedBuffer = explode("\r\n", trim($buffer)); - - while (!empty($explodedBuffer)) { - $argsLen = (int) explode('*', $explodedBuffer[0])[1]; - $cmdLen = ($argsLen * 2) + 1; - $rawCommands[] = array_splice($explodedBuffer, 0, $cmdLen); - } - - foreach ($rawCommands as $command) { - $command = implode("\r\n", $command) . "\r\n"; - $commandObj = Command::deserializeCommand($command); - $this->getConnectionByCommand($commandObj)->write($command); - } + // Refuse raw buffers: re-splitting them on "\r\n" ignored RESP length + // prefixes and let CRLF-smuggled commands be routed to a node + // (CVE GHSA-w6f5-v2h6-g786). Pipelines write each command individually. + throw new NotSupportedException( + 'Aggregate connections cannot write a raw command buffer; ' + . 'route each command through writeRequest() instead.' + ); } } diff --git a/tests/Predis/Connection/Cluster/PredisClusterTest.php b/tests/Predis/Connection/Cluster/PredisClusterTest.php index 3b8c5b45..843bd17d 100644 --- a/tests/Predis/Connection/Cluster/PredisClusterTest.php +++ b/tests/Predis/Connection/Cluster/PredisClusterTest.php @@ -474,46 +474,31 @@ class PredisClusterTest extends PredisTestCase } /** + * Regression guard for CVE GHSA-w6f5-v2h6-g786 (CWE-93): an aggregate connection + * must refuse a raw, already-serialized command buffer instead of re-splitting it + * on "\r\n". The old parser ignored RESP bulk-length prefixes, so CRLF sequences + * smuggled into a value or key were parsed as extra commands and routed to a node. + * * @group disconnected */ - public function testWrite(): void + public function testWriteRejectsRawCommandBuffer(): void { - $command1 = new GET(); - $command1->setArguments(['arg1']); - - $command2 = new GET(); - $command2->setArguments(['arg2']); - - $command3 = new GET(); - $command3->setArguments(['arg3']); + // A single GET whose key carries a smuggled FLUSHDB payload; the old code + // would have re-parsed and routed the FLUSHDB, this must route nothing. + $command = new GET(); + $command->setArguments(["slug: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'); - $connection3 = $this->getMockConnection('tcp://127.0.0.1:7003'); - $connection1 - ->expects($this->exactly(3)) - ->method('write') - ->withConsecutive( - [$command1->serializeCommand()], - [$command2->serializeCommand()], - [$command3->serializeCommand()] - ); - - $connection2 - ->expects($this->never()) - ->method('write'); - - $connection3 ->expects($this->never()) ->method('write'); $cluster = new PredisCluster(new Parameters()); - $cluster->add($connection1); - $cluster->add($connection2); - $cluster->add($connection3); - $cluster->write($command1->serializeCommand() . $command2->serializeCommand() . $command3->serializeCommand()); + $this->expectException('Predis\NotSupportedException'); + $this->expectExceptionMessage('Aggregate connections cannot write a raw command buffer'); + + $cluster->write($command->serializeCommand()); } } diff --git a/tests/Predis/Connection/Cluster/RedisClusterTest.php b/tests/Predis/Connection/Cluster/RedisClusterTest.php index 2f70ccc3..c10644cc 100644 --- a/tests/Predis/Connection/Cluster/RedisClusterTest.php +++ b/tests/Predis/Connection/Cluster/RedisClusterTest.php @@ -1634,46 +1634,33 @@ class RedisClusterTest extends PredisTestCase } /** + * Regression guard for CVE GHSA-w6f5-v2h6-g786 (CWE-93): an aggregate connection + * must refuse a raw, already-serialized command buffer instead of re-splitting it + * on "\r\n". The old parser ignored RESP bulk-length prefixes, so CRLF sequences + * smuggled into a value or key were parsed as extra commands and routed to a node. + * * @group disconnected */ - public function testWrite(): void + public function testWriteRejectsRawCommandBuffer(): void { - $command1 = new Command\Redis\GET(); - $command1->setArguments(['arg1']); - - $command2 = new Command\Redis\GET(); - $command2->setArguments(['arg2']); - - $command3 = new Command\Redis\GET(); - $command3->setArguments(['arg3']); + // A single GET whose key carries a smuggled FLUSHDB payload; the old code + // would have re-parsed and routed the FLUSHDB, this must route nothing. + $command = new Command\Redis\GET(); + $command->setArguments(["slug: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'); - $connection1 - ->expects($this->once()) - ->method('write') - ->with($command3->serializeCommand()); - - $connection2 - ->expects($this->once()) - ->method('write') - ->with($command2->serializeCommand()); - - $connection3 - ->expects($this->once()) - ->method('write') - ->with($command1->serializeCommand()); + ->expects($this->never()) + ->method('write'); $cluster = new RedisCluster($factory, new Parameters()); - $cluster->add($connection1); - $cluster->add($connection2); - $cluster->add($connection3); - $cluster->write($command1->serializeCommand() . $command2->serializeCommand() . $command3->serializeCommand()); + $this->expectException('Predis\NotSupportedException'); + $this->expectExceptionMessage('Aggregate connections cannot write a raw command buffer'); + + $cluster->write($command->serializeCommand()); } } diff --git a/tests/Predis/Connection/Replication/MasterSlaveReplicationTest.php b/tests/Predis/Connection/Replication/MasterSlaveReplicationTest.php index 7383c053..0a3f1d3a 100644 --- a/tests/Predis/Connection/Replication/MasterSlaveReplicationTest.php +++ b/tests/Predis/Connection/Replication/MasterSlaveReplicationTest.php @@ -1459,41 +1459,40 @@ repl_backlog_histlen:12978 } /** + * Regression guard for CVE GHSA-w6f5-v2h6-g786 (CWE-93): an aggregate connection + * must refuse a raw, already-serialized command buffer instead of re-splitting it + * on "\r\n". The old parser ignored RESP bulk-length prefixes, so CRLF sequences + * smuggled into a value or key were parsed as extra commands and routed to a node. + * * @group disconnected */ - public function testWrite(): void + public function testWriteRejectsRawCommandBuffer(): void { - $command1 = new Command\Redis\Json\JSONGET(); - $command1->setArguments(['arg1']); - - $command2 = new Command\Redis\Json\JSONGET(); - $command2->setArguments(['arg2']); - - $command3 = new Command\Redis\Json\JSONGET(); - $command3->setArguments(['arg3']); + // A single command whose key carries a smuggled FLUSHDB payload; the old code + // would have re-parsed and routed the FLUSHDB, this must route nothing. + $command = new Command\Redis\Json\JSONGET(); + $command->setArguments(["slug: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 + $master ->expects($this->never()) ->method('write'); - $master - ->expects($this->exactly(3)) - ->method('write') - ->withConsecutive( - [$command1->serializeCommand()], - [$command2->serializeCommand()], - [$command3->serializeCommand()] - ); + $slave1 + ->expects($this->never()) + ->method('write'); $replication = new MasterSlaveReplication(); $replication->add($master); $replication->add($slave1); - $replication->write($command1->serializeCommand() . $command2->serializeCommand() . $command3->serializeCommand()); + $this->expectException('Predis\NotSupportedException'); + $this->expectExceptionMessage('Aggregate connections cannot write a raw command buffer'); + + $replication->write($command->serializeCommand()); } /** diff --git a/tests/Predis/Connection/Replication/SentinelReplicationTest.php b/tests/Predis/Connection/Replication/SentinelReplicationTest.php index 4ccc2ffa..08719b21 100644 --- a/tests/Predis/Connection/Replication/SentinelReplicationTest.php +++ b/tests/Predis/Connection/Replication/SentinelReplicationTest.php @@ -1975,18 +1975,19 @@ class SentinelReplicationTest extends PredisTestCase } /** + * Regression guard for CVE GHSA-w6f5-v2h6-g786 (CWE-93): an aggregate connection + * must refuse a raw, already-serialized command buffer instead of re-splitting it + * on "\r\n". The old parser ignored RESP bulk-length prefixes, so CRLF sequences + * smuggled into a value or key were parsed as extra commands and routed to a node. + * * @group disconnected */ - public function testWrite(): void + public function testWriteRejectsRawCommandBuffer(): void { - $command1 = new Command\Redis\Search\FTSEARCH(); - $command1->setArguments(['arg1', '*']); - - $command2 = new Command\Redis\Search\FTSEARCH(); - $command2->setArguments(['arg2', '*']); - - $command3 = new Command\Redis\Search\FTSEARCH(); - $command3->setArguments(['arg3', '*']); + // A single command whose argument carries a smuggled FLUSHDB payload; the old + // code would have re-parsed and routed the FLUSHDB, this must route nothing. + $command = new Command\Redis\Search\FTSEARCH(); + $command->setArguments(["idx: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'); @@ -1995,29 +1996,22 @@ class SentinelReplicationTest extends PredisTestCase $factory = new Connection\Factory(); $master - ->expects($this->exactly(3)) - ->method('isConnected') - ->willReturn(true); + ->expects($this->never()) + ->method('write'); $slave ->expects($this->never()) ->method('write'); - $master - ->expects($this->exactly(3)) - ->method('write') - ->withConsecutive( - [$command1->serializeCommand()], - [$command2->serializeCommand()], - [$command3->serializeCommand()] - ); - $replication = new SentinelReplication('svc', [$sentinel], $factory, $strategy); $replication->add($master); $replication->add($slave); - $replication->write($command1->serializeCommand() . $command2->serializeCommand() . $command3->serializeCommand()); + $this->expectException('Predis\NotSupportedException'); + $this->expectExceptionMessage('Aggregate connections cannot write a raw command buffer'); + + $replication->write($command->serializeCommand()); } public function connectionsProvider(): array