From 0863c1328c038a3588081a42faf84ea8492c01b8 Mon Sep 17 00:00:00 2001 From: acoulton Date: Tue, 16 Sep 2025 17:49:15 +0100 Subject: [PATCH] fix: MysqlSession should validate session ID format before checking DB Credential-stuffing and malware bots will commonly attempt requests with manufactured session cookies e.g. to search for SQL, path or URL injection vulnerabilities. Currently, the MysqlSession driver checks all provided session IDs against the database to validate them. This is *NOT* a security issue, since we are using PDO's parameters to properly escape the provided values. However, it does cause an exception (and therefore an error report) because the `GET_LOCK` query uses the user-provided session ID as part of the lock name. If the lock name contains characters that are not valid in a lock, MySQL will give a syntax error, which in turn causes a PDOException. Instead, skip the database lookup and return false immediately (as for any other invalid session ID) if the ID does not match the expected length and character set. --- CHANGELOG.md | 5 + src/Session/MysqlSession.php | 29 ++++ test/phpunit-bootstrap.php | 9 +- test/unit/Session/MysqlSessionTest.php | 175 ++++++++++++++++++++++++- 4 files changed, 211 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 72fd167..20e56e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ ### Unreleased +### v2.5.1 (2025-09-17) + +* Fix MysqlSession to immediately return false (without database check) if session ID does not match the pattern of IDs + generated by the application. + ### v2.5.0 (2025-06-26) * Support PHP 8.4 diff --git a/src/Session/MysqlSession.php b/src/Session/MysqlSession.php index 2314507..9347fc3 100644 --- a/src/Session/MysqlSession.php +++ b/src/Session/MysqlSession.php @@ -12,6 +12,7 @@ use Ingenerator\PHPUtils\StringEncoding\StringSanitiser; use PDO; use SessionHandlerInterface; +use UnexpectedValueException; class MysqlSession implements SessionHandlerInterface, \SessionUpdateTimestampHandlerInterface, \SessionIdInterface { @@ -186,6 +187,12 @@ public function create_sid(): string */ public function validateId($session_id): bool { + if ( ! (is_string($session_id) && preg_match($this->getValidSessionIdRegex(), $session_id))) { + // It cannot be a valid SID if it doesn't match the format that we generate + // It's likely a credential stuffing bot - ignore it + return false; + } + $now = new \DateTimeImmutable(); $this->getLock($session_id); @@ -230,6 +237,28 @@ public function validateId($session_id): bool } } + private function getValidSessionIdRegex(): string + { + // sid_length and sid_bits_per_character can be configured with INI settings, but this is deprecated from 8.4 + // https://wiki.php.net/rfc/deprecations_php_8_4#sessionsid_length_and_sessionsid_bits_per_character + // In future they will always be 32 byte hexadecimal strings. + // In the meantime we need to accommodate the potential that the running app has different defaults. + return sprintf( + match (ini_get('session.sid_bits_per_character')) { + '5' => '/^[0-9a-v]{%d}$/', + '6' => '/^[0-9a-zA-Z,-]{%d}$/', + // The default, which will also become the standard once the INI setting is removed (at which point + // ini_get will return false) + false, + '4' => '/^[0-9a-f]{%d}$/', + default => throw new UnexpectedValueException( + 'Unexpected ini setting for session.sid_bits_per_character', + ), + }, + ini_get('session.sid_length') ?: 32, + ); + } + /** * {@inheritdoc} */ diff --git a/test/phpunit-bootstrap.php b/test/phpunit-bootstrap.php index ea34eb8..05fe8f2 100644 --- a/test/phpunit-bootstrap.php +++ b/test/phpunit-bootstrap.php @@ -6,6 +6,11 @@ // https://github.com/sebastianbergmann/phpunit/issues/2449 \set_error_handler( function ($severity, $message, $file, $line) { - throw new ErrorException($message, 0, $severity, $file, $line); - } + if (error_reporting() & $severity) { + throw new ErrorException($message, 0, $severity, $file, $line); + } + + // This error has been silenced locally, ignore it + return true; + }, ); diff --git a/test/unit/Session/MysqlSessionTest.php b/test/unit/Session/MysqlSessionTest.php index 7fe28d1..78e7493 100644 --- a/test/unit/Session/MysqlSessionTest.php +++ b/test/unit/Session/MysqlSessionTest.php @@ -7,24 +7,189 @@ namespace test\unit\Ingenerator\PHPUtils\Session; +use Closure; use Ingenerator\PHPUtils\Session\MysqlSession; +use PDO; +use PDOStatement; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; +use UnexpectedValueException; class MysqlSessionTest extends TestCase { + private array $old_ini_vars = []; public function test_it_is_initialisable() { $this->assertInstanceOf(MysqlSession::class, $this->newSubject()); } - protected function newSubject() + public static function provider_validate_invalid_sid(): array { - return new MysqlSession(new PDOMock, 'insecure-salt'); + return [ + 'with path injection attempt' => [ + [], + '../../../../../../../../../../../../../../windows/win.ini', + ], + 'with url injection attempt' => [ + [], + 'http://some-inexistent-website.acu/some_inexistent_file_with_long_name?.jpg', + ], + 'with SQL injection attempt' => [ + [], + "'; TRUNCATE sessions;", + ], + 'with invalid chars (in default bits per character)' => [ + [], + str_pad('v', 32, 'a'), + ], + 'with SID too short (with default length configuration)' => [ + [], + str_repeat('a', 31), + ], + 'with SID too long (with default length configuration)' => [ + [], + str_repeat('a', 33), + ], + 'with invalid characters (using custom bits)' => [ + ['session.sid_bits_per_character' => '5'], + str_pad(',', 32, 'a'), + ], + 'too long (with custom length)' => [ + ['session.sid_length' => '44'], + str_repeat('a', 45), + ], + 'too short (with custom length)' => [ + ['session.sid_length' => '44'], + str_repeat('a', 43), + ], + ]; } -} + #[DataProvider('provider_validate_invalid_sid')] + public function test_it_rejects_invalid_session_ids_without_checking_database(array $config, string $sid): void + { + $this->configurePhpIniVars($config); + $subject = $this->newSubject(pdo: $this->mockPDOExpectingNoCalls()); + $this->assertFalse($subject->validateId($sid)); + } + + public static function provider_validate_own_sid(): array + { + return [ + 'with default config' => [ + [], + '/^[0-9a-f]{32}$/', + ], + 'with 5 bits per char' => [ + ['session.sid_bits_per_character' => '5'], + '/^[0-9a-v]{32}$/', + ], + 'with 6 bits per char' => [ + ['session.sid_bits_per_character' => '6'], + '/^[0-9a-zA-Z,-]{32}$/', + ], + 'with custom length' => [ + ['session.sid_length' => '22'], + '/^[0-9a-f]{22}$/', + ], + 'with custom length and chars' => [ + ['session.sid_length' => '22', 'session.sid_bits_per_character' => '5'], + '/^[0-9a-v,-]{22}$/', + ], + ]; + } + + #[DataProvider('provider_validate_own_sid')] + public function test_sid_that_it_creates_is_valid(array $config, string $expected_pattern): void + { + $this->configurePhpIniVars($config); + + // This mocking isn't very nice - it is coupled to the implementation details of the SQL queries and the + // results the class expects (and how it fetches them internally). I've tried to limit that as much as possible, + // it would obviously be better if this ran as an integration test against an actual mysql instance - however + // really here I only want to test that the validation is done against the database e.g. not short-circuited by + // the pattern matching. + $pdo = $this->mockPDOToPrepareQueries(function ($query) { + $result = $this->createMock(PDOStatement::class); + + if (str_starts_with($query, 'INSERT INTO `sessions`')) { + // Result of the query is never read + return $result; + } + + if (str_starts_with($query, 'SELECT GET_LOCK')) { + // Just needs to return 1 to show that the lock is acquired. Note that validateSid does not release the + // lock if it successfully loads a session (it'll be released on session_write_close or end of request) + $result->expects($this->once())->method('fetchColumn')->willReturn('1'); + return $result; + } + + if (str_starts_with($query, 'SELECT `session_data`')) { + // Just needs to return some data, even empty + $result->expects($this->once())->method('fetchAll')->willReturn([['session_data' => serialize([])]]); + return $result; + } + + throw new UnexpectedValueException('Un-mocked query '.$query); + }); + + $subject = $this->newSubject(pdo: $pdo); + + $sid = $subject->create_sid(); + // Sanity check that the settings applied as expected + $this->assertMatchesRegularExpression($expected_pattern, $sid, 'Should generate SID in expected format'); + + $this->assertTrue($subject->validateId($sid), 'Should validate SID "'.$sid.'"'); + } + + protected function setUp(): void + { + parent::setUp(); + // Force to the normal default configs + $this->configurePhpIniVars([ + 'session.sid_bits_per_character' => '4', + 'session.sid_length' => '32', + ]); + } + + protected function tearDown(): void + { + parent::tearDown(); + foreach ($this->old_ini_vars as $key => $value) { + @ini_set($key, $value); + } + } + + protected function newSubject( + ?PDO $pdo = null + ) + { + return new MysqlSession( + $pdo ?? $this->mockPDOExpectingNoCalls(), + 'insecure-salt', + ); + } + + private function mockPDOExpectingNoCalls(): PDO + { + $pdo = $this->createMock(PDO::class); + $pdo->expects($this->never())->method($this->anything()); + return $pdo; + } + + private function configurePhpIniVars(array $config): void + { + foreach ($config as $setting => $value) { + $this->old_ini_vars[$setting] = @ini_set($setting, $value); + } + } + + private function mockPDOToPrepareQueries(Closure $callback): PDO + { + $pdo = $this->createMock(PDO::class); + $pdo->expects($this->any())->method('prepare')->willReturnCallback($callback); + return $pdo; + } -class PDOMock extends \PDO { - public function __construct() {} }