Skip to content

Commit 0ef4bbe

Browse files
acoultonacoulton
committed
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.
1 parent 2fe3e4a commit 0ef4bbe

File tree

4 files changed

+208
-5
lines changed

4 files changed

+208
-5
lines changed

‎CHANGELOG.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
### Unreleased
22

3+
### v2.5.1 (2025-09-17)
4+
5+
* Fix MysqlSession to immediately return false (without database check) if session ID does not match the pattern of IDs
6+
generated by the application.
7+
38
### v2.5.0 (2025-06-26)
49

510
* Support PHP 8.4

‎src/Session/MysqlSession.php‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
use Ingenerator\PHPUtils\StringEncoding\StringSanitiser;
1313
use PDO;
1414
use SessionHandlerInterface;
15+
use UnexpectedValueException;
1516

1617
class MysqlSession implements SessionHandlerInterface, \SessionUpdateTimestampHandlerInterface, \SessionIdInterface
1718
{
@@ -186,6 +187,12 @@ public function create_sid(): string
186187
*/
187188
public function validateId($session_id): bool
188189
{
190+
if ( ! (is_string($session_id) && preg_match($this->getValidSessionIdRegex(), $session_id))) {
191+
// It cannot be a valid SID if it doesn't match the format that we generate
192+
// It's likely a credential stuffing bot - ignore it
193+
return false;
194+
}
195+
189196
$now = new \DateTimeImmutable();
190197
$this->getLock($session_id);
191198

@@ -230,6 +237,28 @@ public function validateId($session_id): bool
230237
}
231238
}
232239

240+
private function getValidSessionIdRegex(): string
241+
{
242+
// sid_length and sid_bits_per_character can be configured with INI settings, but this is deprecated from 8.4
243+
// https://wiki.php.net/rfc/deprecations_php_8_4#sessionsid_length_and_sessionsid_bits_per_character
244+
// In future they will always be 32 byte hexadecimal strings.
245+
// In the meantime we need to accommodate the potential that the running app has different defaults.
246+
return sprintf(
247+
match (ini_get('session.sid_bits_per_character')) {
248+
'5' => '/^[0-9a-v]{%d}$/',
249+
'6' => '/^[0-9a-zA-Z,-]{%d}$/',
250+
// The default, which will also become the standard once the INI setting is removed (at which point
251+
// ini_get will return false)
252+
false,
253+
'4' => '/^[0-9a-f]{%d}$/',
254+
default => throw new UnexpectedValueException(
255+
'Unexpected ini setting for session.sid_bits_per_character',
256+
),
257+
},
258+
ini_get('session.sid_length') ?: 32,
259+
);
260+
}
261+
233262
/**
234263
* {@inheritdoc}
235264
*/

‎test/phpunit-bootstrap.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@
66
// https://github.com/sebastianbergmann/phpunit/issues/2449
77
\set_error_handler(
88
function ($severity, $message, $file, $line) {
9+
if (error_reporting() & ~$severity) {
10+
// This error has been silenced locally, ignore it
11+
return false;
12+
}
913
throw new ErrorException($message, 0, $severity, $file, $line);
1014
}
1115
);

‎test/unit/Session/MysqlSessionTest.php‎

Lines changed: 170 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,24 +7,189 @@
77
namespace test\unit\Ingenerator\PHPUtils\Session;
88

99

10+
use Closure;
1011
use Ingenerator\PHPUtils\Session\MysqlSession;
12+
use PDO;
13+
use PDOStatement;
14+
use PHPUnit\Framework\Attributes\DataProvider;
1115
use PHPUnit\Framework\TestCase;
16+
use UnexpectedValueException;
1217

1318
class MysqlSessionTest extends TestCase
1419
{
20+
private array $old_ini_vars = [];
1521

1622
public function test_it_is_initialisable()
1723
{
1824
$this->assertInstanceOf(MysqlSession::class, $this->newSubject());
1925
}
2026

21-
protected function newSubject()
27+
public static function provider_validate_invalid_sid(): array
2228
{
23-
return new MysqlSession(new PDOMock, 'insecure-salt');
29+
return [
30+
'with path injection attempt' => [
31+
[],
32+
'../../../../../../../../../../../../../../windows/win.ini',
33+
],
34+
'with url injection attempt' => [
35+
[],
36+
'http://some-inexistent-website.acu/some_inexistent_file_with_long_name?.jpg',
37+
],
38+
'with SQL injection attempt' => [
39+
[],
40+
"'; TRUNCATE sessions;",
41+
],
42+
'with invalid chars (in default bits per character)' => [
43+
[],
44+
str_pad('v', 32, 'a'),
45+
],
46+
'with SID too short (with default length configuration)' => [
47+
[],
48+
str_repeat('a', 31),
49+
],
50+
'with SID too long (with default length configuration)' => [
51+
[],
52+
str_repeat('a', 33),
53+
],
54+
'with invalid characters (using custom bits)' => [
55+
['session.sid_bits_per_character' => '5'],
56+
str_pad(',', 32, 'a'),
57+
],
58+
'too long (with custom length)' => [
59+
['session.sid_length' => '44'],
60+
str_repeat('a', 45),
61+
],
62+
'too short (with custom length)' => [
63+
['session.sid_length' => '44'],
64+
str_repeat('a', 43),
65+
],
66+
];
2467
}
2568

26-
}
69+
#[DataProvider('provider_validate_invalid_sid')]
70+
public function test_it_rejects_invalid_session_ids_without_checking_database(array $config, string $sid): void
71+
{
72+
$this->configurePhpIniVars($config);
73+
$subject = $this->newSubject(pdo: $this->mockPDOExpectingNoCalls());
74+
$this->assertFalse($subject->validateId($sid));
75+
}
76+
77+
public static function provider_validate_own_sid(): array
78+
{
79+
return [
80+
'with default config' => [
81+
[],
82+
'/^[0-9a-f]{32}$/',
83+
],
84+
'with 5 bits per char' => [
85+
['session.sid_bits_per_character' => '5'],
86+
'/^[0-9a-v]{32}$/',
87+
],
88+
'with 6 bits per char' => [
89+
['session.sid_bits_per_character' => '6'],
90+
'/^[0-9a-zA-Z,-]{32}$/',
91+
],
92+
'with custom length' => [
93+
['session.sid_length' => '22'],
94+
'/^[0-9a-f]{22}$/',
95+
],
96+
'with custom length and chars' => [
97+
['session.sid_length' => '22', 'session.sid_bits_per_character' => '5'],
98+
'/^[0-9a-v,-]{22}$/',
99+
],
100+
];
101+
}
102+
103+
#[DataProvider('provider_validate_own_sid')]
104+
public function test_sid_that_it_creates_is_valid(array $config, string $expected_pattern): void
105+
{
106+
$this->configurePhpIniVars($config);
107+
108+
// This mocking isn't very nice - it is coupled to the implementation details of the SQL queries and the
109+
// results the class expects (and how it fetches them internally). I've tried to limit that as much as possible,
110+
// it would obviously be better if this ran as an integration test against an actual mysql instance - however
111+
// really here I only want to test that the validation is done against the database e.g. not short-circuited by
112+
// the pattern matching.
113+
$pdo = $this->mockPDOToPrepareQueries(function ($query) {
114+
$result = $this->createMock(PDOStatement::class);
115+
116+
if (str_starts_with($query, 'INSERT INTO `sessions`')) {
117+
// Result of the query is never read
118+
return $result;
119+
}
120+
121+
if (str_starts_with($query, 'SELECT GET_LOCK')) {
122+
// Just needs to return 1 to show that the lock is acquired. Note that validateSid does not release the
123+
// lock if it successfully loads a session (it'll be released on session_write_close or end of request)
124+
$result->expects($this->once())->method('fetchColumn')->willReturn('1');
125+
return $result;
126+
}
127+
128+
if (str_starts_with($query, 'SELECT `session_data`')) {
129+
// Just needs to return some data, even empty
130+
$result->expects($this->once())->method('fetchAll')->willReturn([['session_data' => serialize([])]]);
131+
return $result;
132+
}
133+
134+
throw new UnexpectedValueException('Un-mocked query '.$query);
135+
});
136+
137+
$subject = $this->newSubject(pdo: $pdo);
138+
139+
$sid = $subject->create_sid();
140+
// Sanity check that the settings applied as expected
141+
$this->assertMatchesRegularExpression($expected_pattern, $sid, 'Should generate SID in expected format');
142+
143+
$this->assertTrue($subject->validateId($sid), 'Should validate SID "'.$sid.'"');
144+
}
145+
146+
protected function setUp(): void
147+
{
148+
parent::setUp();
149+
// Force to the normal default configs
150+
$this->configurePhpIniVars([
151+
'session.sid_bits_per_character' => '4',
152+
'session.sid_length' => '32',
153+
]);
154+
}
155+
156+
protected function tearDown(): void
157+
{
158+
parent::tearDown();
159+
foreach ($this->old_ini_vars as $key => $value) {
160+
@ini_set($key, $value);
161+
}
162+
}
163+
164+
protected function newSubject(
165+
?PDO $pdo = null
166+
)
167+
{
168+
return new MysqlSession(
169+
$pdo ?? $this->mockPDOExpectingNoCalls(),
170+
'insecure-salt',
171+
);
172+
}
173+
174+
private function mockPDOExpectingNoCalls(): PDO
175+
{
176+
$pdo = $this->createMock(PDO::class);
177+
$pdo->expects($this->never())->method($this->anything());
178+
return $pdo;
179+
}
180+
181+
private function configurePhpIniVars(array $config): void
182+
{
183+
foreach ($config as $setting => $value) {
184+
$this->old_ini_vars[$setting] = @ini_set($setting, $value);
185+
}
186+
}
187+
188+
private function mockPDOToPrepareQueries(Closure $callback): PDO
189+
{
190+
$pdo = $this->createMock(PDO::class);
191+
$pdo->expects($this->any())->method('prepare')->willReturnCallback($callback);
192+
return $pdo;
193+
}
27194

28-
class PDOMock extends \PDO {
29-
public function __construct() {}
30195
}

0 commit comments

Comments
 (0)