|
|
php.net | support | documentation | report a bug | advanced search | search howto | statistics | random bug | login |
[2018-11-19 17:23 UTC] riikka dot kalliomaki at gmail dot com
Description:
------------
There seems to be some security issues with how PHP internally deals with session id collisions. Most notably, there seems to be distinct lack of coherency between the purpose of PS(mod)->s_create_sid and PS(mod)->s_validate_sid calls in /ext/session/session.c.
According to reference implementation in the files handler, s_create_sid is supposed to check for collisions with existing sessions, but confusingly, the sessions.c seems to try to use s_validate_sid for this purpose in number of occasions, when s_validate_sid seems to be supposed to return SUCCESS only if the session id points to an existing sessions.
I discovered at least the following inconsistencies and security issues looking at the session code:
* in php_session_initialize, if strict mode is enabled, the code creates a new sid if php_session_initialize returns FAILURE, since no session exists with the given sid. HOWEVER, if s_create_sid also fails due to collision, PHP will simply create a new session id with internal php_session_create_id() while doing no collision detection (instead of erroring out)
* in PHP_FUNCTION(session_regenerate_id), if strict mode is enabled PHP checks if a session exists with the created sid and creates a new sid if it DOES NOT EXIST. i.e, because it's comparing s_validate_sid against FAILURE (instead of SUCCESS), it creates TWO sids in most occasions, unless, for some reason s_create_sid returns a sid that points to an existing sessions (which I guess it shouldn't, if collision checking is the responsibility of s_create_sid)
* In PHP_FUNCTION(session_create_id) there doesn't seem to be a check if s_create_sid returns a null instead of a string. Additionally, again we are comparing the return value of s_validate_sid against FAILURE, resulting in generating THREE sids in most cases, unless, again, s_create_sid for some reason returns a sid that is already in use.
In short, it seems that both PHP_FUNCTION(session_regenerate_id) and PHP_FUNCTION(session_create_id) expect s_validate_sid to return FAILURE if the session id points to an existing session, while the reverse is true for php_session_initialize. It's as if the session.c is a bit confused about who is supposed to be responsible for checking against id collisions. Does the responsibility fall to s_create_sid or should session.c verify against it with s_validate_sid?
Additionally, both php_session_initialize and PHP_FUNCTION(session_create_id) seems to default to php_session_create_id() in some cases instead of the session module's sid creation, which causes no collision detecting to be performed (and may cause weird issues if a custom session handler returns ids in other format than what is returned by php_session_create_id).
Finally, both php_session_initialize and PHP_FUNCTION(session_create_id) are dangerous in the sense that they can both end up emitting a collisioning id, since php_session_initialize just uses the value of php_session_create_id on collision and PHP_FUNCTION(session_create_id) returns the value of the third "detected" collision instead of erroring out. The call PHP_FUNCTION(session_regenerate_id) only returns a collisioning id if s_create_sid returns a collisioning id.
Disclaimer: I'm neither a C programmer nor a security researcher. Thus I'm not sure about the full impact of these issues.
Test script:
---------------
<?php
header('Content-Type: text/plain');
ini_set('session.save_handler', 'files');
$handler = new class extends SessionHandler {
public function create_sid() {
echo "create_sid\n";
return parent::create_sid();
}
public function validateId($session_id) {
echo "validateId\n";
return file_exists(sys_get_temp_dir() . '/sess_' . $session_id);
}
};
session_set_save_handler($handler, true);
session_start();
echo "Do Create\n";
session_create_id();
$_SESSION['var'] = random_int(0, 10000);
Expected result:
----------------
Do Create
create_sid
Actual result:
--------------
Do Create
create_sid
validateId
create_sid
validateId
create_sid
validateId
PatchesPull RequestsHistoryAllCommentsChangesGit/SVN commits
|
|||||||||||||||||||||||||||||||||||||
Copyright © 2001-2024 The PHP GroupAll rights reserved. |
Last updated: Sat Dec 21 12:01:31 2024 UTC |
Collision detection is done, but it is minium for historical(=compatibility/performance) reasons. Originally, session module was written to work even with invalid states. 1) Many invalid states are disallowed now, but not all. 2) Strict mode is introduced to allow only PHP initialized session ID. i.e. This is the primary purpose and real threat. Collision detection is not primary objective. i.e. Old behavior was kept where it is possible. * in php_session_initialize, if strict mode is enabled, the code creates a new sid if php_session_initialize returns FAILURE, since no session exists with the given sid. HOWEVER, if s_create_sid also fails due to collision, PHP will simply create a new session id with internal php_session_create_id() while doing no collision detection (instead of erroring out) This can be simple error. However, php_session_created_id() is simply called because ID collision with safe random source can be considered secure. My preference is to raise error and stop execution. * in PHP_FUNCTION(session_regenerate_id), if strict mode is enabled PHP checks if a session exists with the created sid and creates a new sid if it DOES NOT EXIST. i.e, because it's comparing s_validate_sid against FAILURE (instead of SUCCESS), it creates TWO sids in most occasions, unless, for some reason s_create_sid returns a sid that points to an existing sessions (which I guess it shouldn't, if collision checking is the responsibility of s_create_sid) Because of 2), once valid active session is initialized with strict mode, new session ID must be PHP initialized session ID, so no collision check used be. Collision check requires additional session storage access. My preference is keep current behavior, increase number of bits in session ID. (192 -> 256) NIST requires 256 bit or more for collision resistant usages. * In PHP_FUNCTION(session_create_id) there doesn't seem to be a check if s_create_sid returns a null instead of a string. Additionally, again we are comparing the return value of s_validate_sid against FAILURE, resulting in generating THREE sids in most cases, unless, again, s_create_sid for some reason returns a sid that is already in use. Strictly speaking, the code may be better to check NULL, but NULL is return only when CSPRNG failure occurred and execution is returned to the function for some reason. i.e. Exception is raised and NULL is never processed unless there is really really strange bug. Current code can be confusing, we may remove "if" in php_session_create_id(). Current if (php_random_bytes_throw(rbuf, PS(sid_length) + PS_EXTRA_RAND_BYTES) == FAILURE) { return NULL; } New php_random_bytes_throw(rbuf, PS(sid_length) + PS_EXTRA_RAND_BYTES); It seems sys_get_temp_dir() points to dir other than session save path, try ini_set('session.save_path', '/tmp'); or like, then it works as it should. Although there could be improvements, there is no real threat. Question is how picky should we against session ID collisions?I understand that the collision detection is rudimentary at best as it can't really deal with race conditions universally anyway, but the main issue is this (apologies for my previous confusing explanation): PHP_FUNCTION(session_regenerate_id) and PHP_FUNCTION(session_regenerate_id) are both ACTIVELY trying to create collisions, because they are comparing against FAILURE instead of SUCCESS. (i.e. this is a bug, the condition is supposed to be reversed). My understanding is that s_validate_sid is supposed to return SUCCESS if and only if a session with the given ID exists. THUS, both of those functions are discarding NEW sids if they do not belong to an existing session. Here is a bit more illustrative code sample: <?php header('Content-Type: text/plain'); $sessionPath = __DIR__ . '/foo'; if (!file_exists($sessionPath)) { mkdir(__DIR__ . '/foo'); } foreach (new DirectoryIterator($sessionPath) as $file) { if (!$file->isDir()) { unlink($file->getPathName()); } } ini_set('session.save_handler', 'files'); ini_set('session.save_path', $sessionPath); ini_set('session.use_cookies', false); ini_set('session.user_only_cookies', true); $handler = new class extends SessionHandler { private $id = 1; public function create_sid(): string { echo "create_sid, returning: $this->id\n"; return $this->id++; } public function validateId($session_id): bool { global $sessionPath; $path = $sessionPath . '/sess_' . $session_id; $exists = file_exists($path); echo "validateId: $session_id \n"; echo "Exists: $path: " . ($exists ? 'true' : 'false') . "\n"; return $exists; } }; touch($sessionPath . '/sess_3'); session_set_save_handler($handler, true); session_start(); echo "Do Create\n"; $id = session_create_id(); echo "returned: $id\n"; Note that the session_create_id() here returns "3", BECAUSE the file "sess_3" exists due to "touch()", i.e. it retried creating a new sid until it found one that exists. If you replace the end of code with the following: touch($sessionPath . '/sess_2'); ini_set('session.use_strict_mode', true); session_set_save_handler($handler, true); session_start(); echo "Do Create\n"; session_regenerate_id(); echo "returned: " . session_id(); You can see that the the session_regenerate_id is fine with the new sid of "2", because a session exists with that ID.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; } }