Skip to content

Honor session.use_strict_mode for the built-in SessionHandler - #23071

Open
iliaal wants to merge 1 commit into
php:masterfrom
iliaal:fix/session-strict-validate-master
Open

Honor session.use_strict_mode for the built-in SessionHandler#23071
iliaal wants to merge 1 commit into
php:masterfrom
iliaal:fix/session-strict-validate-master

Conversation

@iliaal

@iliaal iliaal commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

SessionHandler implements neither SessionUpdateTimestampHandlerInterface nor a validateId() method, so registering it leaves ps_validate_sid undefined and PS_VALIDATE_SID_FUNC(user) falls through to php_session_validate_sid, which reports every id as existing. With session.use_strict_mode=1, the new default in 8.6, session_set_save_handler(new SessionHandler, true) adopts an attacker-supplied id instead of regenerating it.

validateId() is now implemented on the class alongside the other methods that delegate to the wrapped module. A subclass that overrides open() without calling parent::open() never opened that module, so it cannot answer for the id and returns true, which keeps the behavior those handlers have today.

@iliaal
iliaal requested a review from Girgias as a code owner August 5, 2026 13:27
iliaal added a commit to iliaal/php-src that referenced this pull request Aug 5, 2026
SessionHandler does not expose validateId(), so strict mode fell back to
php_session_validate_sid() and accepted unknown IDs. Delegate validation
for exact SessionHandler instances to the wrapped module. Keep the
historical behavior for subclasses, which may implement custom storage.

Closes phpGH-23071
@iliaal
iliaal force-pushed the fix/session-strict-validate-master branch from eeb4cf1 to 36d134d Compare August 5, 2026 13:28

@Girgias Girgias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The methods should just be implemented on the class. Rather than this nonsense.

@jorgsowa

jorgsowa commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

SessionHandler exposes no validateId(), so registering it leaves
ps_validate_sid undefined and PS_VALIDATE_SID_FUNC(user) falls through to
php_session_validate_sid(), which reports every id as existing. With
session.use_strict_mode=1, the new default, an attacker supplied id is
adopted rather than regenerated.

Implement validateId() alongside the other methods that delegate to the
wrapped module. A subclass that overrides open() without calling
parent::open() never opened that module, so it cannot answer for the id
and keeps its previous behavior.

Closes phpGH-23071
@iliaal
iliaal force-pushed the fix/session-strict-validate-master branch from 36d134d to e13ba93 Compare August 9, 2026 14:40
@iliaal

iliaal commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

Done in e13ba93. SessionHandler::validateId() now sits next to the other methods that delegate to the wrapped module, and the check in PS_VALIDATE_SID_FUNC is gone.

One guard was needed to get there: a subclass that overrides open() without calling parent::open() never opened the module it would validate against, so it returns true rather than vetoing the id. Without that, session_set_save_handler_class_002.phpt and _016.phpt lose their session data, since those subclasses replace storage and inherit the method.

@iliaal

iliaal commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

A deprecation would not change the 8.6 behavior though, the id is still adopted. Does it cover new SessionHandler itself? It has create_sid() but no validateId(), so as worded it looks like it lands on the deprecated side. e13ba93 adds the method, which settles that either way.

@iliaal iliaal changed the title Honor use_strict_mode for SessionHandler without validateId Honor session.use_strict_mode for the built-in SessionHandler Aug 9, 2026
@iliaal
iliaal requested a review from Girgias August 9, 2026 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants