Actually fix GH-9583

The issue is that PS(mod)->s_validate_sid is always defined for user modules, thus we need to check that the actual callable is set
Add another regression test to ensure current working behaviour is not broken (which was by the previous incorrect fix)

Closes GH-9638
This commit is contained in:
George Peter Banyard 2022-09-29 13:31:53 +01:00
parent 072dc3c857
commit 499fbcd679
No known key found for this signature in database
GPG Key ID: 3306078E3194AEBD
2 changed files with 69 additions and 14 deletions

View File

@ -1083,9 +1083,10 @@ PHPAPI int php_session_register_module(const ps_module *ptr) /* {{{ */
/* }}} */
/* Dummy PS module function */
/* We consider any ID valid, so we return FAILURE to indicate that a session doesn't exist */
/* We consider any ID valid (thus also implying that a session with such an ID exists),
thus we always return SUCCESS */
PHPAPI int php_session_validate_sid(PS_VALIDATE_SID_ARGS) {
return FAILURE;
return SUCCESS;
}
/* Dummy PS module function */
@ -2255,18 +2256,24 @@ PHP_FUNCTION(session_regenerate_id)
}
RETURN_THROWS();
}
if (PS(use_strict_mode) && PS(mod)->s_validate_sid &&
PS(mod)->s_validate_sid(&PS(mod_data), PS(id)) == SUCCESS) {
zend_string_release_ex(PS(id), 0);
PS(id) = PS(mod)->s_create_sid(&PS(mod_data));
if (!PS(id)) {
PS(mod)->s_close(&PS(mod_data));
PS(session_status) = php_session_none;
if (!EG(exception)) {
zend_throw_error(NULL, "Failed to create session ID by collision: %s (path: %s)", PS(mod)->s_name, PS(save_path));
if (PS(use_strict_mode)) {
if ((!PS(mod_user_implemented) && PS(mod)->s_validate_sid) || !Z_ISUNDEF(PS(mod_user_names).name.ps_validate_sid)) {
int limit = 3;
/* Try to generate non-existing ID */
while (limit-- && PS(mod)->s_validate_sid(&PS(mod_data), PS(id)) == SUCCESS) {
zend_string_release_ex(PS(id), 0);
PS(id) = PS(mod)->s_create_sid(&PS(mod_data));
if (!PS(id)) {
PS(mod)->s_close(&PS(mod_data));
PS(session_status) = php_session_none;
if (!EG(exception)) {
zend_throw_error(NULL, "Failed to create session ID by collision: %s (path: %s)", PS(mod)->s_name, PS(save_path));
}
RETURN_THROWS();
}
}
RETURN_THROWS();
}
// TODO warn that ID cannot be verified? else { }
}
/* Read is required to make new session data at this point. */
if (PS(mod)->s_read(&PS(mod_data), PS(id), &data, PS(gc_maxlifetime)) == FAILURE) {
@ -2293,7 +2300,6 @@ PHP_FUNCTION(session_regenerate_id)
/* }}} */
/* {{{ Generate new session ID. Intended for user save handlers. */
/* This is not used yet */
PHP_FUNCTION(session_create_id)
{
zend_string *prefix = NULL, *new_id;
@ -2317,7 +2323,7 @@ PHP_FUNCTION(session_create_id)
int limit = 3;
while (limit--) {
new_id = PS(mod)->s_create_sid(&PS(mod_data));
if (!PS(mod)->s_validate_sid) {
if (!PS(mod)->s_validate_sid || (PS(mod_user_implemented) && Z_ISUNDEF(PS(mod_user_names).name.ps_validate_sid))) {
break;
} else {
/* Detect collision and retry */

View File

@ -0,0 +1,49 @@
--TEST--
GH-9583: session_create_id() fails with user defined save handler that doesn't have a validateId() method
--EXTENSIONS--
session
--SKIPIF--
<?php include('skipif.inc'); ?>
--FILE--
<?php
class SessionHandlerTester implements \SessionHandlerInterface
{
public function close(): bool { return true; }
public function destroy($id): bool { return true; }
public function gc($max_lifetime): int|false { return 1; }
public function open($path, $name): bool { return true; }
public function read($id): string { return ''; }
public function write($id, $data): bool { return true; }
//public function create_sid() { return uniqid(); }
//public function validateId($key) { return true; }
}
$obj = new SessionHandlerTester();
ini_set('session.use_strict_mode','1');
session_set_save_handler($obj);
session_start();
$originalSessionId = session_id();
session_write_close();
session_start();
$newSessionId = session_id();
echo 'validateId() ', (method_exists($obj, 'validateId') ? ('returns ' . ($obj->validateId(1) ? 'true' : 'false')) : 'is commented out'), "\n";
var_dump($originalSessionId == $newSessionId);
?>
--EXPECT--
validateId() is commented out
bool(true)