From e13ba93abd69d1739000f8cfb27e63c766f88dc2 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Wed, 5 Aug 2026 09:18:56 -0400 Subject: [PATCH] Honor session.use_strict_mode for the built-in SessionHandler SessionHandler exposes no validateId(), so registering it leaves ps_validate_sid undefined and PS_VALIDATE_SID_FUNC(user) falls through to php_session_validate_sid(), which reports every id as existing. With session.use_strict_mode=1, the new default, an attacker supplied id is adopted rather than regenerated. Implement validateId() alongside the other methods that delegate to the wrapped module. A subclass that overrides open() without calling parent::open() never opened that module, so it cannot answer for the id and keeps its previous behavior. Closes GH-23071 --- NEWS | 4 + UPGRADING | 6 + ext/session/mod_user_class.c | 17 +++ ext/session/session.stub.php | 3 + ext/session/session_arginfo.h | 6 +- .../session_strict_handler_validate.phpt | 127 ++++++++++++++++++ 6 files changed, 162 insertions(+), 1 deletion(-) create mode 100644 ext/session/tests/session_strict_handler_validate.phpt diff --git a/NEWS b/NEWS index 7dd05a2b1c72..e8088f6f86aa 100644 --- a/NEWS +++ b/NEWS @@ -42,6 +42,10 @@ PHP NEWS . Fixed segfault in ReflectionMethod::createFromMethodName() on an uninstantiable subclass. (iliaal) +- Session: + . Fixed session.use_strict_mode being a no-op for the built-in + SessionHandler. (iliaal) + - Standard: . Added the "filter.max_filter_count" stream context option for php://filter URLs. Using more than 16 filters without configuring this option is now diff --git a/UPGRADING b/UPGRADING index 9389c7b04b20..a6e601a636b7 100644 --- a/UPGRADING +++ b/UPGRADING @@ -153,6 +153,12 @@ PHP 8.6 UPGRADE NOTES explicitly set this to "None" (and also set session.cookie_secure to 1). RFC: https://wiki.php.net/rfc/session_security_defaults + . SessionHandler::validateId() has been added, so session.use_strict_mode + now takes effect for the built-in handler. Subclasses that already + declare validateId() must be signature compatible with + validateId(string $id): bool. A subclass that overrides open() without + calling parent::open() keeps its previous behavior, because the parent + handler it would validate against was never opened. - Shmop: . shmop_open() now raises a ValueError when the $key argument is outside the diff --git a/ext/session/mod_user_class.c b/ext/session/mod_user_class.c index a6bd69c91a07..6a906facb64c 100644 --- a/ext/session/mod_user_class.c +++ b/ext/session/mod_user_class.c @@ -152,3 +152,20 @@ PHP_METHOD(SessionHandler, create_sid) RETURN_STR(id); } + +PHP_METHOD(SessionHandler, validateId) +{ + zend_string *key; + + if (zend_parse_parameters(ZEND_NUM_ARGS(), "S", &key) == FAILURE) { + RETURN_THROWS(); + } + + PS_SANITY_CHECK; + + if (!PS(mod_user_is_open)) { + RETURN_TRUE; + } + + RETURN_BOOL(SUCCESS == PS(default_mod)->s_validate_sid(&PS(mod_data), key)); +} diff --git a/ext/session/session.stub.php b/ext/session/session.stub.php index bfb6849f45e7..258715782d2c 100644 --- a/ext/session/session.stub.php +++ b/ext/session/session.stub.php @@ -147,4 +147,7 @@ public function gc(int $max_lifetime): int|false {} /** @tentative-return-type */ public function create_sid(): string {} + + /** @tentative-return-type */ + public function validateId(string $id): bool {} } diff --git a/ext/session/session_arginfo.h b/ext/session/session_arginfo.h index 3860731a535a..dfcccc643410 100644 --- a/ext/session/session_arginfo.h +++ b/ext/session/session_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit session.stub.php instead. - * Stub hash: 6bbbdc8c4a33d1ff9984b3d81e4f5c9b76efcb14 */ + * Stub hash: 5109ef5c81733a112fe20d2626b8572d0969973c */ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_MASK_EX(arginfo_session_name, 0, 0, MAY_BE_STRING|MAY_BE_FALSE) ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, name, IS_STRING, 1, "null") @@ -135,6 +135,8 @@ ZEND_END_ARG_INFO() #define arginfo_class_SessionHandler_create_sid arginfo_class_SessionIdInterface_create_sid +#define arginfo_class_SessionHandler_validateId arginfo_class_SessionHandlerInterface_destroy + ZEND_FUNCTION(session_name); ZEND_FUNCTION(session_module_name); ZEND_FUNCTION(session_save_path); @@ -164,6 +166,7 @@ ZEND_METHOD(SessionHandler, write); ZEND_METHOD(SessionHandler, destroy); ZEND_METHOD(SessionHandler, gc); ZEND_METHOD(SessionHandler, create_sid); +ZEND_METHOD(SessionHandler, validateId); static const zend_function_entry ext_functions[] = { ZEND_FE(session_name, arginfo_session_name) @@ -221,6 +224,7 @@ static const zend_function_entry class_SessionHandler_methods[] = { ZEND_ME(SessionHandler, destroy, arginfo_class_SessionHandler_destroy, ZEND_ACC_PUBLIC) ZEND_ME(SessionHandler, gc, arginfo_class_SessionHandler_gc, ZEND_ACC_PUBLIC) ZEND_ME(SessionHandler, create_sid, arginfo_class_SessionHandler_create_sid, ZEND_ACC_PUBLIC) + ZEND_ME(SessionHandler, validateId, arginfo_class_SessionHandler_validateId, ZEND_ACC_PUBLIC) ZEND_FE_END }; diff --git a/ext/session/tests/session_strict_handler_validate.phpt b/ext/session/tests/session_strict_handler_validate.phpt new file mode 100644 index 000000000000..0b96ac7480b7 --- /dev/null +++ b/ext/session/tests/session_strict_handler_validate.phpt @@ -0,0 +1,127 @@ +--TEST-- +use_strict_mode distinguishes SessionHandler from custom-storage subclasses +--EXTENSIONS-- +session +--INI-- +session.use_strict_mode=1 +session.use_cookies=0 +session.cache_limiter= +session.gc_probability=0 +--FILE-- + 'value|s:6:"loaded";', + ]; + + public function open(string $path, string $name): bool + { + return true; + } + + public function close(): bool + { + return true; + } + + public function read(string $id): string|false + { + return $this->sessions[$id] ?? ''; + } + + public function write(string $id, string $data): bool + { + $this->sessions[$id] = $data; + return true; + } + + public function destroy(string $id): bool + { + unset($this->sessions[$id]); + return true; + } + + public function gc(int $max_lifetime): int|false + { + return 0; + } +} + +session_set_save_handler(new CustomStorageHandler, true); +$id = 'valid-custom-session-id'; +session_id($id); +session_start(); +$custom_handler_preserved = session_id() === $id; +$custom_handler_loaded = $_SESSION['value'] ?? null; +session_write_close(); + +class AugmentingHandler extends SessionHandler +{ + public function read(string $id): string|false + { + return parent::read($id); + } +} + +session_set_save_handler(new AugmentingHandler, true); +$augmenting_invalid_id = 'attackerchosensessionidzz' . bin2hex(random_bytes(4)); +session_id($augmenting_invalid_id); +session_start(); +$augmenting_handler_adopted = session_id() === $augmenting_invalid_id; +session_write_close(); + +session_set_save_handler(new AugmentingHandler, true); +session_id($valid_id); +session_start(); +$augmenting_handler_preserved = session_id() === $valid_id; +session_write_close(); + +foreach (glob($save_path . '/*') as $f) { + @unlink($f); +} +@rmdir($save_path); + +echo "SessionHandler preserved: "; +var_dump($session_handler_preserved); +echo "SessionHandler loaded: "; +var_dump($session_handler_loaded); +echo "SessionHandler adopted unknown: "; +var_dump($session_handler_adopted); +echo "Custom handler preserved: "; +var_dump($custom_handler_preserved); +echo "Custom handler loaded: "; +var_dump($custom_handler_loaded); +echo "Augmenting handler adopted unknown: "; +var_dump($augmenting_handler_adopted); +echo "Augmenting handler preserved: "; +var_dump($augmenting_handler_preserved); +?> +--EXPECT-- +SessionHandler preserved: bool(true) +SessionHandler loaded: string(5) "files" +SessionHandler adopted unknown: bool(false) +Custom handler preserved: bool(true) +Custom handler loaded: string(6) "loaded" +Augmenting handler adopted unknown: bool(false) +Augmenting handler preserved: bool(true)