session_create_id() fails with user defined save handler that doesn't have a validateId() method

[warenzema]

Description

The following code:

<?php
declare(strict_types=1);

class SessionHandlerTester implements \SessionHandlerInterface
{

    public function close(){return true;}

    public function destroy($id){return true;}

    public function gc($max_lifetime){return true;}

    public function open($path, $name){return true;}

    public function read($id){return '';}

    public function write($id, $data){return true;}

    //public function create_sid(){return uniqid();}

    //public function validateId($key){return true;}
}

$obj = new SessionHandlerTester();
ini_set('session.use_strict_mode','1');
session_set_save_handler($obj);
session_start();

echo "\nvalidateId() ".(method_exists($obj,'validateId')?('returns '.($obj->validateId(1)?'true':'false')):'is commented out');
echo "\n";
$sessionId = session_create_id();
echo "\nSession ID:".$sessionId;
echo "\n";

Resulted in this output:

[vagrant@local-test PHPUnit]$ php sessionCreateIdTest.php

validateId() is commented out
PHP Warning:  session_create_id(): Failed to create new ID in /home/vagrant/sessionCreateIdTest.php on line 31

Session ID:

But I expected this output instead:

[vagrant@local-test PHPUnit]$ php sessionCreateIdTest.php

validateId() is commented out

Session ID:ismv386i33uasd5612o816ua54

Here is a summary of the behavior of the above code across various versions, where I tested commenting out the validateId(), having it always return false, and have it always return true:

Version	no validateId()	validateId() returns false	validateId() returns true
7.2.26
7.2.34		Error
7.4.30	Error		Error
8.0.20	Error		Error

The varying behavior over time is believed to be due to two other bug fixes: Fix #79091 and Fix #79413

(The older versions above are listed because they represent the progression of behavior as those fixes were applied. I know most are out of support.)

Those bug fixes created line 2325 below and changed FAILURE to SUCCESS on line 2323, respectively. (No problem with those bug fixes, them being fixed simply revealed this additional bug, and are merely provided for context.)

php-src/ext/session/session.c

Lines 2315 to 2342 in 1084715

	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) {
	break;
	} else {
	/* Detect collision and retry */
	if (PS(mod)->s_validate_sid(&PS(mod_data), new_id) == SUCCESS) {
	zend_string_release_ex(new_id, 0);
	new_id = NULL;
	continue;
	}
	break;
	}
	}
	} else {
	new_id = php_session_create_id(NULL);
	}
	if (new_id) {
	smart_str_append(&id, new_id);
	zend_string_release_ex(new_id, 0);
	} else {
	smart_str_free(&id);
	php_error_docref(NULL, E_WARNING, "Failed to create new ID");
	RETURN_FALSE;
	}

Hypothesis

I don't know for sure what the issue is, as I'm unfamiliar with the language the source code is written in, but here's is the best I can work out from clues:

Line 2319 contains the actual bug, as (I'm guessing here a little) that line is meant to check if the validateId() method exists on the save handler. If it does not exist, the code skips the collision check and just creates the new id without error (2335 is true).

However, that line doesn't actually do that, as it appears that validateId() is always defined and always returns true if the user defined save handler doesn't override it. (In other words, it claims that every ID is already in use.)

That may be due to the following code (again, guessing):

php-src/ext/session/session.c

Lines 1085 to 1088 in 1084715

	/* Dummy PS module function */
	PHPAPI int php_session_validate_sid(PS_VALIDATE_SID_ARGS) {
	return SUCCESS;
	}

As a result, 2319 is always false, so the 2321 else block always executes, and 2323 is always true, which always ends with new_id = NULL which after 3 loops lands the code at 2340, emitting the warning.

This explanation matches the chart of versions above:

7.2.26: With no new_id = NULL there was no way to fail

7.2.34 added new_id = NULL on 2325, which would cause 2323 to evaluate to true if validateId() returned false

Later versions then fixed 2323 to be true if validateId() returned true, which inverted (relative to 7.2.34) the behavior we see.

(I was unable to test other versions as I need to install PHP via amazon-linux-extras, which only on 7.2 allowed me to select which patch to install, didn't allow 7.3 at all, nor 8.1. Installing 7.4 and 8.0 got me 7.4.30 and 8.0.20.)

Workaround

A user can work around this issue by adding a validateId($key) to their user defined save handler. You just need to have it return true if the passed in $key is already in use, and false otherwise. See documentation.

Comments

I would imagine this bug is pretty widespread in general, as most user defined save handlers will not have a validateId() method defined (AWS PHP SDK doesn't, which is why I encountered this issue.) However, most code probably doesn't ever call create_session_id(), which means this problem is encountered extremely rarely.

(I think I failed at being "brief", so here's a 4 line TLDR. Sorry, I didn't want my many hours of digging to go to waste.)

TLDR

ini_set('session.use_strict_mode','1');
session_set_save_handler(new SessionHandlerTester());
session_start(); 
session_create_id();

Causes the following error if SessionHandlerTester doesn't have a validateId() method:

PHP Warning:  session_create_id(): Failed to create new ID in <the last line>

PHP Version

8.0.20

Operating System

No response

[Girgias]

This regression seems to have occurred in PHP 7.4.5: https://3v4l.org/9HCPE

7.4 is in security fixes only, but the fix should be provided for 8.0 upwards.

[warenzema]

@Girgias Won't your solution of changing the default value to FAILURE just cause the php_session_initialize function to fail? See line 421 and 426.

php-src/ext/session/session.c

Lines 420 to 426 in 1084715

	} 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_ex(PS(id), 0);
	}
	PS(id) = PS(mod)->s_create_sid(&PS(mod_data));

I tried returning false in my own code, initially, and discovered it was changing the session ID on every call, since validateId returns whether the ID is in use.

Since php_session_initialize uses validateId to check if the session record still exists, that means changing the default value will break things for everyone using a user defined save handler, not just those that call session_create_id.

[Girgias]

Can you provide a test case for what you think might now fail? As all tests currently pass.
I'm no session handler expert so I just read the description from the previous commits and thought this was the fix and because all tests passed merged.