php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #77178 Session id collision detection is broken
Submitted: 2018-11-19 17:23 UTC Modified: 2019-02-02 03:00 UTC
Votes:2
Avg. Score:4.0 ± 0.0
Reproduced:2 of 2 (100.0%)
Same Version:1 (50.0%)
Same OS:2 (100.0%)
From: riikka dot kalliomaki at gmail dot com Assigned: yohgaki (profile)
Status: Assigned Package: Session related
PHP Version: 7.2.12 OS:
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: riikka dot kalliomaki at gmail dot com
New email:
PHP Version: OS:

 

 [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


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2018-11-20 08:36 UTC] stas@php.net
-Assigned To: +Assigned To: yohgaki
 [2018-11-21 06:27 UTC] yohgaki@php.net
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?
 [2018-11-21 07:16 UTC] riikka dot kalliomaki at gmail dot com
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.
 [2018-11-21 07:32 UTC] 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.
 [2018-11-21 12:15 UTC] yohgaki@php.net
-Status: Assigned +Status: Analyzed
 [2018-11-21 12:15 UTC] 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 12:18 UTC] yohgaki@php.net
I suppose this can be fixed as normal bug.
 [2018-11-21 12:25 UTC] 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-12-10 09:36 UTC] yohgaki@php.net
RMs, how this should be proceeded? Fix this as normal bug or minor security fix?
 [2019-02-02 03:00 UTC] kalle@php.net
-Status: Analyzed +Status: Assigned -Type: Security +Type: Bug
 [2019-02-02 03:00 UTC] 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.
 
PHP Copyright © 2001-2019 The PHP Group
All rights reserved.
Last updated: Thu Jul 18 08:01:25 2019 UTC