Skip to content

Fix #79413: session_create_id() fails for active sessions #5305

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Fix #79413: session_create_id() fails for active sessions #5305

wants to merge 2 commits into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Mar 26, 2020

The comment on PS_VALIDATE_SID_FUNC(files) is very clear that the
function is supposed to return SUCCESS if the session already exists.
So to detect a collision, we have to check for SUCCESS, not
FAILURE.

@cmb69
Fix #79413: session_create_id() fails for active sessions
cfce1bd 
The comment on `PS_VALIDATE_SID_FUNC(files)` is very clear that the
function is supposed to return `SUCCESS` if the session already exists.
So to detect a collision, we have to check for `SUCCESS`, not
`FAILURE`.
nikic
nikic reviewed Mar 26, 2020
View reviewed changes
@cmb69
Fix wrong condition in session_regenerate_id() as well
148d7a3 
If the new SID is invalid, i.e. no such session exists yet, there is no
need to create a new SID.  On the other hand, if such session already
exists, we have to create a new SID to satisfy strict mode (actually,
we would have to repeat this until we find an invalid SID).
@cmb69
Copy link
Member Author

cmb69 commented Mar 30, 2020

In my opinion, we should merge this fix ASAP (ideally before the next RCs are tagged tomorrow). See also https://bugs.php.net/bug.php?id=77178#1542784582 for reasons why (function name in the following quote fixed by me):

PHP_FUNCTION(session_create_id) and PHP_FUNCTION(session_regenerate_id) are both ACTIVELY trying to create collisions, because they are comparing against FAILURE instead of SUCCESS.

Also, @remicollet, what do you think about merging this into PHP-7.2. From https://bugs.php.net/bug.php?id=77178#1542803132:

I don't mind fixing this as minor security fix.

@nikic
Copy link
Member

nikic commented Mar 30, 2020

There's one more call here:

php-src/ext/session/session.c

Line 417 in fbe19a6

} else if (PS(use_strict_mode) && PS(mod)->s_validate_sid &&

Does it also need to be flipped?

@cmb69
Copy link
Member Author

cmb69 commented Mar 30, 2020

No, that call is supposed to check for FAILURE (the session is started, and a SID is given, and this session does not yet exists, in strict mode, a new SID has to be created => prevent session fixation).

@cmb69
Copy link
Member Author

cmb69 commented Mar 31, 2020

Thanks! Applied as b510250.

@cmb69 cmb69 closed this Mar 31, 2020
@cmb69 cmb69 deleted the cmb/79413 branch March 31, 2020 06:42
@nicolas-grekas nicolas-grekas mentioned this pull request Apr 18, 2020
Merged
@nicolas-grekas
Copy link
Contributor

Is this going to be applied to PHP 7.2?
I submitted a PR on Symfony to work around the issue in previous versions of PHP and I'd like to ensure the affected versions are correctly listed, see
https://github.com/symfony/symfony/pull/36490/files#diff-d29419a0d7043fce54a98005c26a82aaR74

nicolas-grekas added a commit to symfony/symfony that referenced this pull request Apr 18, 2020
@nicolas-grekas
bug #36490 [HttpFoundation] workaround PHP bug in the session module …
62565a1 
…(nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[HttpFoundation] workaround PHP bug in the session module

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Current tests fail after php/php-src#5305
Which itself is a patch for a bug in the session module.

This PR works around the issue in older versions of PHP and fixes the tests.

Commits
-------

0cbca19 [HttpFoundation] workaround PHP bug in the session module
@cmb69
Copy link
Member Author

cmb69 commented Apr 18, 2020

@nicolas-grekas, that may be something to consider for @remicollet and @sgolemon.

@nanasess nanasess mentioned this pull request Jun 23, 2020
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cmb69 @nikic @nicolas-grekas