Skip to content

Commit 337efe4

Browse files
acoultonacoulton
authored
Merge pull request #67 from ingenerator/2.x-bug-validate-session-id
fix: MysqlSession should validate session ID format before checking DB
2 parents 2fe3e4a + 0863c13 commit 337efe4

File tree

4 files changed

+211
-7
lines changed

4 files changed

+211
-7
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: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,11 @@
66
// https://github.com/sebastianbergmann/phpunit/issues/2449
77
\set_error_handler(
88
function ($severity, $message, $file, $line) {
9-
throw new ErrorException($message, 0, $severity, $file, $line);
10-
}
9+
if (error_reporting() & $severity) {
10+
throw new ErrorException($message, 0, $severity, $file, $line);
11+
}
12+
13+
// This error has been silenced locally, ignore it
14+
return true;
15+
},
1116
);

‎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)