@xabbuh Does the PHPUnit / Tests (8.2) (pull_request) check failure depend on my code? I don't think so.

Hey!

I think @BradJonesLLC has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

I think this change is too strict and breaks legit use cases (typically when migrating from one set of settings to another). The only required change is adding ,256 to me.

Hi @nicolas-grekas,

If we only change the regular expression from {22,} to {22,256}, there will be, in my opinion, some problems depending of the OS.

On my windows, I can't set session.sid_length to be more than 250, otherwise a warning will be emitted between 251 and 256. Probably because the filenames can not be longer than 255 bytes on Windows or Linux OS, and because of the session files prefix length: sess_ contains 5 bytes and 255 - 5 = 250.

For example, in my case, the regular expression would have to check {22,250} and not {22,256} to avoid the warning emitted by PHP. So, that's why I think we should create the regular expression based on the PHP ini configuration provided by the developer.

Or maybe we could just change the regular expression from {22,} to {22,250} instead of {22,256} as you suggest.

What do you think? Do you have another solution to avoid the problem on Windows and Linux OS?

I think this change is too strict and breaks legit use cases (typically when migrating from one set of settings to another).

The code I suggest in this PR is a workaround that I use on my websites, and I have never encountered one problem so far.

I think this change is too strict and breaks legit use cases (typically when migrating from one set of settings to another). The only required change is adding ,256 to me.

@nicolas-grekas I pushed a commit, changing only the regular expression, as you requested. But I set the max limit to 250 (see the comment above).

Thank you @brokensourcecode.

Based on #46790 (comment) I have one question about this change: Wouldn't this now break applications on PHP installations having session.sid_length set to something greater than 250?

For reference, here is the validation logic in php-src:
https://github.com/php/php-src/blob/1a5414cd982dbabf4a9757aec5a5e714f15b40d6/ext/session/session.c#L327-L359

Note this comment there:

Somewhat arbitrary length limit here, but should be way more than anyone needs and avoids file-level warnings later on if we exceed MAX_PATH

And here are some more notes about MAX_PATH:
https://stackoverflow.com/questions/833291/is-there-an-equivalent-to-winapis-max-path-under-linux-unix

Based on that it looks like we should put 256 I agree with you @xabbuh: it's not our job to filter this as another layer will report the issue when there is one.

Based on #46790 (comment) I have one question about this change: Wouldn't this now break applications on PHP installations having session.sid_length set to something greater than 250?

@xabbuh That's why, since the start, I suggested to use the provided session.sid_length to build the regular expression. A warning is emitted by PHP if the directive is not between 22 and 256: PHP Warning: ini_set(): session.configuration "session.sid_length" must be between 22 and 256 in C:\index.php on line 4. But @nicolas-grekas said it wasn't a relevant idea (#46790 (review)), and I don't really agree with him on that.

We can't use {22,256} (see the solution 2). Here are the solutions:

Solution 1: use {22,250} in regex (NOT RELEVANT FOR BREAKING REASONS)

This solution is not relevant for developers which define a session.sid_length greater or equal than 251.

if ($sessionId && $this->saveHandler instanceof AbstractProxy && 'files' === $this->saveHandler->getSaveHandlerName() && !preg_match('/^[a-zA-Z0-9,-]{22,250}$/', $sessionId)) {
	// the session ID in the header is invalid, create a new one
	session_id(session_create_id());
}

Solution 2: use {22,256} in regex (NOT RELEVANT FOR SECURITY REASONS)

This solution is not relevant because if you choose to use {22,256}, each developer using Symfony's NativeSessionStorage class should manually check the max length of the session ID for each project, otherwise a malicious user will always be able to massively generate the warning.

if ($sessionId && $this->saveHandler instanceof AbstractProxy && 'files' === $this->saveHandler->getSaveHandlerName() && !preg_match('/^[a-zA-Z0-9,-]{22,256}$/', $sessionId)) {
	// the session ID in the header is invalid, create a new one
	session_id(session_create_id());
}

Solution 3: use session.sid_length (OK)

Here we trust the developer's configuration.

if ($sessionId && $this->saveHandler instanceof AbstractProxy && 'files' === $this->saveHandler->getSaveHandlerName() && !preg_match('/^[a-zA-Z0-9,-]{22,' . ini_get('session.sid_length') . '}$/', $sessionId)) {
	// the session ID in the header is invalid, create a new one
	session_id(session_create_id());
}

Solution 4: use session.sid_length and session.sid_bits_per_character (OK)

Here again, we trust the developer's configuration.

My previous commits:

if ($sessionId && $this->saveHandler instanceof AbstractProxy && 'files' === $this->saveHandler->getSaveHandlerName()) {
	// Must be between 22 and 256. See https://www.php.net/manual/en/session.configuration.php#ini.session.sid-length.
	$sessionIdRegexLength = (int) \ini_get('session.sid_length');
	if ($sessionIdRegexLength < 22 || $sessionIdRegexLength > 256) {
		throw new \RuntimeException("Failed to start the session because the PHP ini directive named \"session.sid_length\" must be between 22 and 256, {$sessionIdRegexLength} given.");
	}

	// Must be 4, 5 or 6. See https://www.php.net/manual/en/session.configuration.php#ini.session.sid-bits-per-character.
	switch ((int) \ini_get('session.sid_bits_per_character')) {
		case 4:
			$sessionIdRegexCharClass = '0-9a-f';
			break;
		case 5:
			$sessionIdRegexCharClass = '0-9a-v';
			break;
		case 6:
			$sessionIdRegexCharClass = '0-9a-zA-Z-,';
			break;
		default:
			throw new \RuntimeException('Failed to start the session because the PHP ini directive named "session.sid_bits_per_character" must be 4, 5 or 6.');
	}

	$sessionIdRegex = "/^[{$sessionIdRegexCharClass}]{{$sessionIdRegexLength}}$/";
	if (!preg_match($sessionIdRegex, $sessionId)) {
		// the session ID in the header is invalid, create a new one
		session_id(session_create_id());
	}
}

Or maybe without the RuntimeException as PHP emits a warning when there is a wrong directive:

if ($sessionId && $this->saveHandler instanceof AbstractProxy && 'files' === $this->saveHandler->getSaveHandlerName()) {
	// Must be between 22 and 256. See https://www.php.net/manual/en/session.configuration.php#ini.session.sid-length.
	$sessionIdRegexLength = (int) \ini_get('session.sid_length');
	if ($sessionIdRegexLength < 22) {
		$sessionIdRegexLength = 22;
	}
	elseif ($sessionIdRegexLength > 256) {
		$sessionIdRegexLength = 256;
	}

	// Must be 4, 5 or 6. See https://www.php.net/manual/en/session.configuration.php#ini.session.sid-bits-per-character.
	switch ((int) \ini_get('session.sid_bits_per_character')) {
		case 4:
			$sessionIdRegexCharClass = '0-9a-f';
			break;
		case 5:
			$sessionIdRegexCharClass = '0-9a-v';
			break;
		case 6:
		default:
			$sessionIdRegexCharClass = '0-9a-zA-Z-,';
			break;
	}

	$sessionIdRegex = "/^[{$sessionIdRegexCharClass}]{{$sessionIdRegexLength}}$/";
	if (!preg_match($sessionIdRegex, $sessionId)) {
		// the session ID in the header is invalid, create a new one
		session_id(session_create_id());
	}
}

The problem we're trying to solve here is preventing a PHP warning. Being more strict than php itself is certainly not a solution to that problem. For this reason, using ini_get is no-go, since php itself doesn't use it to decide if the warning should be triggered or not (see link to php-src in my previous comment.)

I accepted the argument to use 250 so that on Windows ppl don't hit MAX_PATH, but @xabbuh is right that this might break others' code. We should use 256 instead.

a malicious user will always be able to massively generate the warning.

You mean because on Windows this will generate notices related to MAX_PATH?
Can you give it a try an report back the exact message that is generated in such case?

The problem we're trying to solve here is preventing a PHP warning. Being more strict than php itself is certainly not a solution to that problem. For this reason, using ini_get is no-go, since php itself doesn't use it to decide if the warning should be triggered or not (see link to php-src in my previous comment.)

I don't agree with you on that. I don't understand why trust the developer configuration is strict. If the developer does not know how to configure his application, he will know even less that this security problem exists.

I accepted the argument to use 250 so that on Windows ppl don't hit MAX_PATH, but @xabbuh is right that this might break others' code. We should use 256 instead.

I agree with you that 250 is not a relevant solution.

You mean because on Windows this will generate notices related to MAX_PATH? Can you give it a try an report back the exact message that is generated in such case?

No. There are 2 cases:

Case 1: {22,} (before)

I am a malicious user, I falsify the session id in the cookie with the devtool console by putting more than 256 characters.

Cookie (257 bytes):
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

Warning:
PHP Warning: SessionHandler::read(): Session ID is too long or contains illegal characters. Only the A-Z, a-z, 0-9, "-", and "," characters are allowed

Cause: https://github.com/php/php-src/blob/1a5414cd982dbabf4a9757aec5a5e714f15b40d6/ext/session/session.c#L327-L359

Case 2: {22,256} (what you are suggesting)

I am a malicious user, I falsify the session id in the cookie with the devtool console by putting 251 characters.

Cookie (251 bytes):
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa

Warning:
PHP Warning: SessionHandler::read(): open(C:\app\var\tmp\session\sess_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa, O_RDWR) failed: No such file or directory (2)

Cause: the regular expression of the session id checks a max length of 256, but my OS need a limit of 250. My OS limits the filename to 255 bytes. The session filename prefix is sess_, this is 5 bytes. So, 255 - 5 = 250.

Conclusion

With {22,} and with {22,256}, a malicious user can generate warnings in the back-end. In each case, it is a different warning.

PHP Warning: SessionHandler::read(): open(C:\app\var\tmp\session\sess_a...a, O_RDWR) failed: No such file or directory (2)

Great, that's exactly the warning I was wondering about.

I also see a similar warning on Linux when the name of a file is 256 or longer.
I think that settles your question @xabbuh and we should keep 250 as a higher value is broken anyway.

Great, that's exactly the warning I was wondering about.

I also see a similar warning on Linux when the name of a file is 256 or longer. I think that settles your question @xabbuh and we should keep 250 as a higher value is broken anyway.

It seems that Windows and Linux OS limit filenames to 255 bytes (MAX_PATH). So 250 (255 - 5) seems ok if you don't want to use the session.sid_length configuration.

@nicolas-grekas Maybe we should explain in the code why we use 250? By adding a comment that explains the calculation result?

EDIT: done here #46825