diff --git a/docs/changelogs/v4.7.7.md b/docs/changelogs/v4.7.7.md index ac104dc1bb40..d0e13eabf7ef 100644 --- a/docs/changelogs/v4.7.7.md +++ b/docs/changelogs/v4.7.7.md @@ -19,6 +19,7 @@ Release Date: Unreleased - **CORS:** Fixed a bug where `Cors::hasResponseHeaders()` matched header substrings via `str_contains()`, causing false positives when existing exposed headers shared a prefix with configured exposed headers. - **HTTP:** Fixed a bug where `UploadedFile::move()` and `UploadedFile::store()` on Windows could store a file under a different name than `getName()` reported, or write its contents to an NTFS alternate data stream. Filenames ending in a period or space or containing a colon are now rejected, including explicitly supplied names. - **Security:** Fixed a bug where `Security::__construct()` threw a `TypeError` when the CSRF cookie was an array, causing an HTTP 500 response for affected requests. +- **Session:** Fixed a bug where `PostgreHandler::lockSession()` and `releaseLock()` interpolated unescaped session IDs and IP addresses into SQL queries. They are now escaped with `BaseConnection::escape()` and executed with `simpleQuery()`. - **Worker Mode:** Fixed a bug where every request that ended with an uncaught exception, including a `404 Page Not Found` without a 404 override, called `exit()` in the exception handler. This ended the worker script, so FrankenPHP booted a new one, and the worker loop skipped its cleanup after the request. The exception handler now returns to the worker loop. See the repo's [CHANGELOG.md](https://github.com/codeigniter4/CodeIgniter4/blob/develop/CHANGELOG.md) for a complete list of bugs fixed. diff --git a/system/Session/Handlers/Database/PostgreHandler.php b/system/Session/Handlers/Database/PostgreHandler.php index d8c6d3c8b794..4f6bac599c77 100644 --- a/system/Session/Handlers/Database/PostgreHandler.php +++ b/system/Session/Handlers/Database/PostgreHandler.php @@ -70,7 +70,9 @@ public function gc($max_lifetime): false|int */ protected function lockSession(string $sessionID): bool { - $arg = "hashtext('{$sessionID}')" . ($this->matchIP ? ", hashtext('{$this->ipAddress}')" : ''); + $arg = 'hashtext(' . $this->db->escape($sessionID) . ')' + . ($this->matchIP ? ', hashtext(' . $this->db->escape($this->ipAddress) . ')' : ''); + if ($this->db->simpleQuery("SELECT pg_advisory_lock({$arg})") !== false) { $this->lock = $arg; diff --git a/tests/system/Session/Handlers/Database/PostgreHandlerMockTest.php b/tests/system/Session/Handlers/Database/PostgreHandlerMockTest.php new file mode 100644 index 000000000000..6b97cbb06f4e --- /dev/null +++ b/tests/system/Session/Handlers/Database/PostgreHandlerMockTest.php @@ -0,0 +1,142 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace CodeIgniter\Session\Handlers\Database; + +use CodeIgniter\Events\Events; +use CodeIgniter\Test\CIUnitTestCase; +use CodeIgniter\Test\Mock\MockConnection; +use CodeIgniter\Test\Mock\MockResult; +use Config\Session as SessionConfig; +use PHPUnit\Framework\Attributes\Group; + +/** + * @internal + */ +#[Group('Others')] +final class PostgreHandlerMockTest extends CIUnitTestCase +{ + private const IP_ADDRESS = "192.168.1.1' OR '1'='1"; + private const SESSION_ID = "sess'id; DROP TABLE users; --"; + + private function createHandler(bool $matchIP, MockConnection $mockDb): PostgreHandler + { + $config = new SessionConfig(); + $config->driver = PostgreHandler::class; + $config->savePath = 'ci_sessions'; + $config->matchIP = $matchIP; + + $handler = new PostgreHandler($config, self::IP_ADDRESS); + $this->setPrivateProperty($handler, 'db', $mockDb); + + return $handler; + } + + private function createMockDb(): PostgreHandlerMockConnection + { + return new PostgreHandlerMockConnection(['DBDriver' => 'Postgre']); + } + + public function testLockAndReleaseSessionWithoutMatchIP(): void + { + $db = $this->createMockDb(); + $handler = $this->createHandler(false, $db); + + $queryCount = 0; + $listener = static function () use (&$queryCount): void { + $queryCount++; + }; + Events::on('DBQuery', $listener); + + $lockSession = self::getPrivateMethodInvoker($handler, 'lockSession'); + $releaseLock = self::getPrivateMethodInvoker($handler, 'releaseLock'); + + $this->assertTrue($lockSession(self::SESSION_ID)); + $this->assertSame( + "hashtext('sess''id; DROP TABLE users; --')", + self::getPrivateProperty($handler, 'lock'), + ); + + $this->assertTrue($releaseLock()); + $this->assertFalse(self::getPrivateProperty($handler, 'lock')); + + Events::removeListener('DBQuery', $listener); + + $this->assertSame(0, $queryCount); + $this->assertSame([ + "SELECT pg_advisory_lock(hashtext('sess''id; DROP TABLE users; --'))", + "SELECT pg_advisory_unlock(hashtext('sess''id; DROP TABLE users; --'))", + ], $db->executed); + + $this->assertStringNotContainsString(self::SESSION_ID, $db->executed[0]); + $this->assertStringNotContainsString(self::SESSION_ID, $db->executed[1]); + } + + public function testLockAndReleaseSessionWithMatchIP(): void + { + $db = $this->createMockDb(); + $handler = $this->createHandler(true, $db); + + $queryCount = 0; + $listener = static function () use (&$queryCount): void { + $queryCount++; + }; + Events::on('DBQuery', $listener); + + $lockSession = self::getPrivateMethodInvoker($handler, 'lockSession'); + $releaseLock = self::getPrivateMethodInvoker($handler, 'releaseLock'); + + $this->assertTrue($lockSession(self::SESSION_ID)); + $this->assertSame( + "hashtext('sess''id; DROP TABLE users; --'), hashtext('192.168.1.1'' OR ''1''=''1')", + self::getPrivateProperty($handler, 'lock'), + ); + + $this->assertTrue($releaseLock()); + $this->assertFalse(self::getPrivateProperty($handler, 'lock')); + + Events::removeListener('DBQuery', $listener); + + $this->assertSame(0, $queryCount); + $this->assertSame([ + "SELECT pg_advisory_lock(hashtext('sess''id; DROP TABLE users; --'), hashtext('192.168.1.1'' OR ''1''=''1'))", + "SELECT pg_advisory_unlock(hashtext('sess''id; DROP TABLE users; --'), hashtext('192.168.1.1'' OR ''1''=''1'))", + ], $db->executed); + + $this->assertStringNotContainsString(self::SESSION_ID, $db->executed[0]); + $this->assertStringNotContainsString(self::IP_ADDRESS, $db->executed[0]); + } +} + +/** + * Mock connection that records the raw SQL passed to simpleQuery(). + * + * PostgreHandler uses simpleQuery() with escaped values, so the queries are not + * bound and must be captured as they are sent to the database. + * + * @internal + */ +final class PostgreHandlerMockConnection extends MockConnection +{ + /** + * @var list + */ + public array $executed = []; + + public function simpleQuery(string $sql): MockResult + { + $this->executed[] = $sql; + + return new MockResult($this->connID, $this->resultID); + } +}