@xabbuh Does thePHPUnit / 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 setsession.sid_length to be more than250, otherwise a warning will be emitted between251 and256. Probably because the filenames can not be longer than255 bytes on Windows or Linux OS, and because of the session files prefix length:sess_ contains5 bytes and255 - 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 havingsession.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 havingsession.sid_length set to something greater than 250?

@xabbuh That's why, since the start, I suggested to use the providedsession.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 asession.sid_length greater or equal than251.

if ($sessionId &&$this->saveHandlerinstanceof 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 onesession_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'sNativeSessionStorage 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->saveHandlerinstanceof 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 onesession_id(session_create_id());}

Solution 3: usesession.sid_length (OK)

Here we trust the developer's configuration.

if ($sessionId &&$this->saveHandlerinstanceof 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 onesession_id(session_create_id());}

Solution 4: usesession.sid_length andsession.sid_bits_per_character (OK)

Here again, we trust the developer's configuration.

My previous commits:

if ($sessionId &&$this->saveHandlerinstanceof 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) {thrownew \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')) {case4:$sessionIdRegexCharClass ='0-9a-f';break;case5:$sessionIdRegexCharClass ='0-9a-v';break;case6:$sessionIdRegexCharClass ='0-9a-zA-Z-,';break;default:thrownew \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 onesession_id(session_create_id());}}

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

if ($sessionId &&$this->saveHandlerinstanceof 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')) {case4:$sessionIdRegexCharClass ='0-9a-f';break;case5:$sessionIdRegexCharClass ='0-9a-v';break;case6: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 onesession_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 issess_, 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 thesession.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