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() {} }