Skip to content
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.

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

Session handler broke on PHP 7.3.0 #11

Open
1ma opened this issue Dec 20, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@1ma
Copy link
Owner

commented Dec 20, 2018

Apparently the problem lies in RedisSessionHandler.php:L143

if ($this->mustRegenerate($session_id)) {
    session_id($session_id = $this->create_sid());
    // ...
}

Up until PHP 7.2 calling session_id($session_id) inside RedisSessionHandler::read() updates the ID in PHP's internals, so that when the session is closed later on and PHP calls RedisSessionHandler::write(), $session_id is the new ID. On PHP 7.3.0 the $session_id received in RedisSessionHandler::write() is still the old value (the one that has to change), so the handler generates a new ID, puts it in the Cookie header and sends it back to the client, but in Redis the stored key is the stale one. In other words, session_id($session_id) no longer does anything.

Example

$ curl -i -H "Cookie: PHPSESSID=madeupkey;" php73:32768/visit-counter.php
HTTP/1.1 200 OK
Server: nginx/1.13.12
Date: Thu, 20 Dec 2018 20:03:59 GMT
Content-Type: text/html; charset=UTF-8
Transfer-Encoding: chunked
Connection: keep-alive
X-Powered-By: PHP/7.3.0
Set-Cookie: PHPSESSID=3m5qrhtk9upe63069ilf2h3ogr; path=/      <------ new (but lost) ID
Expires: Thu, 19 Nov 1981 08:52:00 GMT
Cache-Control: no-store, no-cache, must-revalidate
Pragma: no-cache

1

Redis MONITOR log:

1545336238.057942 [0 172.18.0.6:57078] "EXISTS" "PHPREDIS_SESSION:madeupkey"
1545336238.587852 [0 172.18.0.6:57078] "SET" "PHPREDIS_SESSION:3m5qrhtk9upe63069ilf2h3ogr_lock" "" "nx"
1545336239.123639 [0 172.18.0.6:57078] "SETEX" "PHPREDIS_SESSION:madeupkey" "1440" "visits|i:1;"
1545336239.595592 [0 172.18.0.6:57078] "DEL" "PHPREDIS_SESSION:3m5qrhtk9upe63069ilf2h3ogr_lock"

Keys in Redis:

127.0.0.1:6379> KEYS *
1) "PHPREDIS_SESSION:madeupkey"           <---- successful session fixation attack

127.0.0.1:6379> GET PHPREDIS_SESSION:madeupkey
"visits|i:1;"

Test suite results on PHP 7.3.0:

290ed5384ec4:/var/www/redis-sessions.test# TARGET=php73 vendor/bin/phpunit 
PHPUnit 7.2.6 by Sebastian Bergmann and contributors.

...FF....F......                                                  16 / 16 (100%)

Time: 604 ms, Memory: 6.00MB

There were 3 failures:

1) UMA\RedisSessions\Tests\E2E\BasicTest::testMaliciousRequest
Failed asserting that 'visits|i:1;' is false.

/var/www/redis-sessions.test/tests/e2e/BasicTest.php:113

2) UMA\RedisSessions\Tests\E2E\BasicTest::testMaliciousRequestWithCustomCookieParams
Failed asserting that 'visits|i:1;' is false.

/var/www/redis-sessions.test/tests/e2e/BasicTest.php:136

3) UMA\RedisSessions\Tests\E2E\ConcurrentTest::testMaliciousRequests
Failed asserting that 1 is identical to 200.

/var/www/redis-sessions.test/tests/e2e/ConcurrentTest.php:69

FAILURES!
Tests: 16, Assertions: 55, Failures: 3.

@1ma 1ma self-assigned this Dec 20, 2018

@1ma

This comment has been minimized.

Copy link
Owner Author

commented Dec 20, 2018

@1ma

This comment has been minimized.

Copy link
Owner Author

commented Dec 20, 2018

If the problem is not solved in the engine a possible workaround would be to store the regenerated $session_id in a new private attribute, and inside RedisSessionHandler::write() use that attribute (if it has been set) as the session key instead of the PHP supplied $session_id.

@yohgaki

This comment has been minimized.

Copy link

commented Jan 17, 2019

You cannot use session_id() to set new session ID for active sessions.
session_id('new_id') updates session module's internal C struct value. Most C written save handlers do not expect changing session ID for active sessions and can behave badly.

If you would like to set 'custom session ID', you must use proper API. i.e. Use PS_FUNCS_SID or better use PS_FUNCS_UPDATE_TIMESTAMP, and define PS_CREATE_SID_FUNC for custom session ID.

If you really need to change session ID in user(PHP) script, close old one, set new ID, then start new session.

Although PHP 7.3 disallows any harmful usages that can cause "side effect", users can set any session ID for inactive sessions.

@1ma

This comment has been minimized.

Copy link
Owner Author

commented Jan 17, 2019

I'll explore that last option then. Thank you for dropping by :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.