Bug #77178 [Com]: Session id collision detection is broken

Edit report at https://bugs.php.net/bug.php?id=77178&edit=1 ID: 77178 Comment by: derek at garudacrafts dot com Reported by: riikka dot kalliomaki at gmail dot com Summary: Session id collision detection is broken Status: Assigned Type: Bug Package: Session related PHP Version: 7.2.12 Assigned To: yohgaki Block user comment: N Private report: N New Comment: Since php 7.3, whenever I call session_create_id() I get the PHP Warning "session_create_id(): Failed to create new ID in...". However, a new session id IS created, which I can confirm by checking the directory where the session files are saved. So my applications work fine, but my error logs fill up with this new erroneous PHP Warning error message. This does not happen in php 7.2. I have reproduced in both php 7.3 and 7.4, all other things being equal. Could the changes made to the session id collision detection as described in this bug report here have anything to do with it? Previous Comments: ------------------------------------------------------------------------ [2019-02-02 03:00:36] kalle@php.net @yohgaki, given the no response from any RM, I think that since it changes the calling procedures for session handlers, it should possibly only go into 7.4 if the way handler callbacks are called changes for backwards compatibility to not introduce regressions for actively supported releases. As you maintain the session module, then any part that you deep a security risk as apart of this report should be fixed in a BC compliant manner for versions as low as 7.1. I personally do not see a high threat here and therefore I'm gonna open the report from its private state. ------------------------------------------------------------------------ [2018-12-10 09:36:26] yohgaki@php.net RMs, how this should be proceeded? Fix this as normal bug or minor security fix? ------------------------------------------------------------------------ [2018-11-21 12:25:32] yohgaki@php.net I don't mind fixing this as minor security fix. i.e. Fix this from PHP 5.6. This doesn't require CVE, IMO. Any comments? ------------------------------------------------------------------------ [2018-11-21 12:18:47] yohgaki@php.net I suppose this can be fixed as normal bug. ------------------------------------------------------------------------ [2018-11-21 12:15:46] yohgaki@php.net PHP_FUNCTION(session_create_id) // This one has bug if (!PS(in_save_handler) && PS(session_status) == php_session_active) { int limit = 3; while (limit--) { new_id = PS(mod)->s_create_sid(&PS(mod_data)); if (!PS(mod)->s_validate_sid) { <=== should test against FAILURE break; } else { /* Detect collision and retry */ if (PS(mod)->s_validate_sid(&PS(mod_data), new_id) == FAILURE) { <===== This should test against SUCCESS zend_string_release(new_id); continue; } break; } } PHP_FUNCTION(session_regenerate_id) // This one has bug if (PS(use_strict_mode) && PS(mod)->s_validate_sid && PS(mod)->s_validate_sid(&PS(mod_data), PS(id)) == FAILURE) { <===== This should test against SUCCESS zend_string_release(PS(id)); 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; zend_throw_error(NULL, "Failed to create session ID by collision: %s (path: %s)", PS(mod)->s_name, PS(save_path)); <===== Wrong error message. RETURN_FALSE; } } php_session_initialize() // This is ok } else if (PS(use_strict_mode) && PS(mod)->s_validate_sid && PS(mod)->s_validate_sid(&PS(mod_data), PS(id)) == FAILURE) { if (PS(id)) { zend_string_release(PS(id)); } PS(id) = PS(mod)->s_create_sid(&PS(mod_data)); if (!PS(id)) { PS(id) = php_session_create_id(NULL); } if (PS(use_cookies)) { PS(send_cookie) = 1; } } ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=77178 -- Edit this bug report at https://bugs.php.net/bug.php?id=77178&edit=1