Sec Bug->Bug #77178 [Ana->Asn]: Session id collision detection is broken

Edit report at https://bugs.php.net/bug.php?id=77178&edit=1 ID: 77178 Updated by: kalle@php.net Reported by: riikka dot kalliomaki at gmail dot com Summary: Session id collision detection is broken -Status: Analyzed +Status: Assigned -Type: Security +Type: Bug Package: Session related PHP Version: 7.2.12 Assigned To: yohgaki Block user comment: N Private report: Y New Comment: @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. Previous Comments: ------------------------------------------------------------------------ [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; } } ------------------------------------------------------------------------ [2018-11-21 07:32:33] riikka dot kalliomaki at gmail dot com The elaborate further, the issue with actively trying to create collisions is mitigated in the default files handler precisely because in the default files handler the s_create_sid checks for collisions itself and attempts to avoid returning colliding IDS. Thus, if you have e.g. PS(id) = PS(mod)->s_create_sid(&PS(mod_data)); Then the following condition will never be false: PS(mod)->s_validate_sid(&PS(mod_data), PS(id)) == FAILURE Simply because the creation in the files handler already ensured that the sid does not collide (and thus s_validate_sid will always return FAILURE, because no session exists with the id). However, if you implement a custom create_sid like in my previous example, then you run the risk of more likely collisions, unless you also take care of making sure that create_sid does not return colliding ids. Hence the original confusion about the inconsistency: Is s_create_sid supposed to ensure that it does not create colliding ids? And if so, why is it being validated with s_validate_sid in session_regenerate_id() and session_create_id()? Hope this clarifies my report a bit. ------------------------------------------------------------------------ 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