php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #77330 session_id() no longer works inside custom SessionHandlerInterface
Submitted: 2018-12-20 21:10 UTC Modified: 2019-01-17 18:32 UTC
Votes:1
Avg. Score:5.0 ± 0.0
Reproduced:1 of 1 (100.0%)
Same Version:1 (100.0%)
Same OS:1 (100.0%)
From: e6990620 at gmail dot com Assigned: yohgaki (profile)
Status: Not a bug Package: Session related
PHP Version: 7.3.0 OS: Linux
Private report: No CVE-ID: None
 [2018-12-20 21:10 UTC] e6990620 at gmail dot com
Description:
------------
Up until PHP 7.3.0 when you wrote a custom SessionHandlerInterface you could signal the PHP engine to regenerate the ID by calling session_id('newvalue') inside its read() method. Then, when the session closed and the engine calls the write() method, $session_id used to be the new value.

Starting from PHP 7.3.0 this pattern no longer works, as write() receives the stale value.

Might be related to https://bugs.php.net/bug.php?id=74941 since it is the only session-related change in this new major release.

Another bug report of the same issue in a real world session handler: https://github.com/1ma/RedisSessionHandler/issues/11

Test script:
---------------
https://3v4l.org/6S4XM

Expected result:
----------------
$ curl -i -H "Cookie: PHPSESSID=madeupkey;" localhost/bug.php;

HTTP/1.1 200 OK
Server: nginx/1.13.12
Date: Thu, 20 Dec 2018 20:51:23 GMT
Content-Type: text/html; charset=UTF-8
Transfer-Encoding: chunked
Connection: keep-alive
X-Powered-By: PHP/7.2.13                 <------------ PHP 7.2
Expires: Thu, 19 Nov 1981 08:52:00 GMT
Cache-Control: no-store, no-cache, must-revalidate
Pragma: no-cache

newsessionid

Actual result:
--------------
$ curl -i -H "Cookie: PHPSESSID=madeupkey;" localhost/bug.php;

HTTP/1.1 200 OK
Server: nginx/1.13.12
Date: Thu, 20 Dec 2018 20:51:25 GMT
Content-Type: text/html; charset=UTF-8
Transfer-Encoding: chunked
Connection: keep-alive
X-Powered-By: PHP/7.3.0                  <------------ PHP 7.3
Expires: Thu, 19 Nov 1981 08:52:00 GMT
Cache-Control: no-store, no-cache, must-revalidate
Pragma: no-cache

madeupkey        <------ successful session fixation attack

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2018-12-21 15:05 UTC] cmb@php.net
-Status: Open +Status: Assigned -Assigned To: +Assigned To: yohgaki
 [2018-12-21 15:05 UTC] cmb@php.net
This behavioral change has been introduced by merging pull request
2406[1].  Yasuo, can you please have a look at this?  There will
also be a warning, if the session ID is changed in
SessionHandlerInterface::open().

[1] <https://github.com/php/php-src/pull/2406>
 [2019-01-02 09:09 UTC] nikic@php.net
Maybe we should revert this change for now? Until someone with good ext/session understanding can look at this, I think it's okay to just drop the warning in the meantime.
 [2019-01-17 09:50 UTC] yohgaki@php.net
Your PHP 7.2 is enabling session.strict_mode=On while 7.3 is not.
 [2019-01-17 09:53 UTC] yohgaki@php.net
While I've been proposed enabling stricrt_mode by default, but it's not enabled yet. Current implementation requires 2 session data read when strict mode is enabled, but this can be save to 1 read. 

This is rather simple change, but it requires save handler API change.
 [2019-01-17 10:01 UTC] yohgaki@php.net
Unless someone changed code and test, there is strict_mode phpt so I suppose stict_mode is not broken. Reporter, please verify.
 [2019-01-17 10:04 UTC] nikic@php.net
@yohgaki: I'm not sure I see what strict mode has to do with this. This change has been introduced in https://github.com/php/php-src/pull/2406. It's a new check, independent of strict mode.
 [2019-01-17 10:08 UTC] yohgaki@php.net
Ok thanks. I'll have a look to see what is going on.
 [2019-01-17 10:16 UTC] e6990620 at gmail dot com
I can confirm that the session.use_strict_mode directive is disabled in all PHP versions (though "strict mode" is actually enforced by the DemoBugSessionHandler code itself).

To verify this I just appended this line at the end of the test script in 3v4l:

echo ini_get('session.use_strict_mode') . PHP_EOL;

and reran.
 [2019-01-17 10:26 UTC] yohgaki@php.net
Reporter, as documented in UPGRADING, PHP 7.3 has more precise session module state management.

Older session allowed abuses that can harm session and session module. PHP 7.3 disallowed these harmful/broken usages. i.e. Any changes that can cause "side effect" are disallowed.

This code is trying to change active session state.
https://3v4l.org/6S4XM

Therefore, the code wouldn't work even with ob_start().

Session module was made to work even with harmful/broken usages, and it caused number of bad bugs in the past. 


    public function read($session_id)
    {
        // Simulate a session ID regeneration.
        \session_id('newsessionid');

        return '';
    }

This code is bad code because it is calling session_id() to set "new" id. Setting new ID in user script for active sessions is bad because it has "side effect" to session module internal. The "side effect" can break session. Detailed description is omitted.

Anyway, the code is broken. User cannot make any changes that can cause "side effect".

I'll check see if strict_mode is broken or not, since reporter claims that 7.3 allows session fixation.
 [2019-01-17 10:38 UTC] yohgaki@php.net
-Status: Assigned +Status: Feedback
 [2019-01-17 10:38 UTC] yohgaki@php.net
ext/session/tests/session_basic2.phpt is not failing, so reporter's 7.3 is not enabling session.strict_mode. To reporter, please verify.
 [2019-01-17 10:42 UTC] e6990620 at gmail dot com
The way I see it it's a valid use case, though. A custom session handler that wants to conform to strict mode needs some way to regenerate the session ID if the one received in the read($session_id) method is not in the storage area, and that new ID must be passed down by the PHP engine when it invokes write($session_id, $session_data). Calling session_id() in this context used to achieve this, but I wasn't aware it was considered an abuse. Are there any other ways to do the same?


Unrelatedly to this issue, I also believe that session.use_strict_mode should default to true, and I'm glad to hear you are pushing in this direction.
 [2019-01-17 10:46 UTC] e6990620 at gmail dot com
@yohgaki another version of the test script, with strict_mode enabled with ini_set: https://3v4l.org/pfKge
 [2019-01-17 13:05 UTC] yohgaki@php.net
-Status: Feedback +Status: Not a bug
 [2019-01-17 13:05 UTC] yohgaki@php.net
You must not ignore errors. Otherwise, it cannot work as it supposed to.

An explanation for the record.
Users must not change session ID for active session. 
Note that session_id() changes session module's internal session ID which is saved in PS globals.

Suppose users are using "files" save handler. 
Files save handler does not care session ID stored in PS globals once session is activated because it only carries file descriptor. session_id() call does not have any effect, since files save handler simply does not care about changing session ID. 

Now suppose one created RDBMS based user save handler.
RDBMS requires different data saving methods, i.e. INSERT for new or UPDATE for existing. 
The save handler author would set flag which saving method is required when reading session data from RDBMS. (There are many ways, but I would do this)
If session_id() is called and changes PS global value for session ID, then writing session would fail.

No matter how session module try, except forbidding bad usages that cause side effect, it is impossible to assure portable code that works any save handler.

For me, these kind of _bad_ code is obvious. However, I cannot expect other users, including PHP core developers, to understand what is good and bad.

Therefore, I forbid any usages that creates side effects. 
Users should be able to write portable/workable code for any save handlers as long as they don't ignore errors. 

If there is uncatched case that causes side effect, please report bug. Thank you.
 [2019-01-17 18:32 UTC] yohgaki@php.net
BTW, session module API lacks protection that is known as 
Seven pernicious kingdoms: a taxonomy of software security errors (7PK) - Encapsulation
https://ieeexplore.ieee.org/document/1556543
or CWE-700
https://cwe.mitre.org/data/definitions/700.html

Users must not change encapsulated session module data structure. i.e. PS globals Since PS globals must not be changed, session module API should protect these. 

Older PHP didn't protect PS globals and let users write bad codes.
 
PHP Copyright © 2001-2019 The PHP Group
All rights reserved.
Last updated: Mon Aug 19 14:01:26 2019 UTC