Hi Marcel,

I've got another fix, this one is for regeneration cookies with non-default parameters such as long expiry, path, secure or HTTP only flags. Without this patch I end up with two distinct cookies in my browser when I use a non-existent session ID and browsers only replace cookies with matching parameters.

Thanks

Scott

Error #2 session_regenerate_id(): Session object destruction failed. ID: user (path: tcp://127.0.0.1:6379?database=2&timeout=2)

Steps to reproduce: enter website flush redis: redis-cli> SELECT 2 redis-cli> FLUSHDB reload website

Get such errors while testing (not always, but very often): Warning: Redis::connect(): php_network_getaddresses: getaddrinfo failed: Name or service not known in /mnt/www/example.com/includes/RedisSessionHandler/uma/redis-session-handler/src/RedisSessionHandler.php on line 105

Warning: Redis::connect(): connect() failed: php_network_getaddresses: getaddrinfo failed: Name or service not known in /mnt/www/example.com/includes/RedisSessionHandler/uma/redis-session-handler/src/RedisSessionHandler.php on line 105

Fatal error: Uncaught exception 'RedisException' with message 'Redis server went away' in /mnt/www/example.com/includes/RedisSessionHandler/uma/redis-session-handler/src/RedisSessionHandler.php:242 Stack trace: #0 /mnt/www/example.com/includes/RedisSessionHandler/uma/redis-session-handler/src/RedisSessionHandler.php(242): Redis->exists('aninlrkd8dijko6...') #1 /mnt/www/example.com/includes/RedisSessionHandler/uma/redis-session-handler/src/RedisSessionHandler.php(139): UMA\RedisSessionHandler->mustRegenerate('aninlrkd8dijko6...') #2 [internal function]: UMA\RedisSessionHandler->read('aninlrkd8dijko6...') #3 /mnt/www/example.com/1askjdha.php(18): session_start() #4 {main} thrown in /mnt/www/example.com/includes/RedisSessionHandler/uma/redis-session-handler/src/RedisSessionHandler.php on line 242

Fatal error: Uncaught exception 'RedisException' with message 'Redis server went away' in /mnt/www/example.com/includes/RedisSessionHandler/uma/redis-session-handler/src/RedisSessionHandler.php:159 Stack trace: #0 /mnt/www/example.com/includes/RedisSessionHandler/uma/redis-session-handler/src/RedisSessionHandler.php(159): Redis->setex('aninlrkd8dijko6...', 1440, '') #1 [internal function]: UMA\RedisSessionHandler->write('aninlrkd8dijko6...', '') #2 [internal function]: session_write_close() #3 {main} thrown in /mnt/www/example.com/includes/RedisSessionHandler/uma/redis-session-handler/src/RedisSessionHandler.php on line 159

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.

Great initiative! I think your spinlock should use a usleep of 150 ms.

https://github.com/1ma/RedisSessionHandler/blob/master/RedisSessionHandler.php#L164

In order not to stress the CPU too much. See also:

https://github.com/php-memcached-dev/php-memcached/blob/master/php_memcached_session.c#L162

and

https://github.com/snc/SncRedisBundle/blob/master/Session/Storage/Handler/RedisSessionHandler.php#L146

I hope this helps.

Hi Marcel,

I know the phpredis team have nearly added locking into the module but I am still using your code as a workaround at the moment. I noticed an issue whereby a script will hang if the execution time is not limited.

Please see attached pull request. I wasn't able to run the tests in Docker (it's nearby impossible to get PECL working in the php-cli image) so please check everything is working.

Kind Regards

Scott

I was thinking about the issue https://github.com/1ma/RedisSessionHandler/issues/3.

The sessions are stored in a cookie in the client for authentication propose. But I use to work with JWT instead and I wanna be able to use a JWT for the session authentication.

Example:

eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzZXNzaW9uSWQiOiJramhiazIzajFodmhvbW9tYXJjZWwxMjMxMmluMTMiLCJuYW1lIjoiVmljdG9yIFJpb3MiLCJyb2xlcyI6WyJsZWdhY3kiXX0.M6vwdTwaXMFrYphDbytCzRwk2ONIYcXaV960oxBDyUs

You can use the JWT token as sessionId or like in this token add a a sessionId inside. This mechanism can give me the possibility of expire a valid JWT token, for example.

The handler will require some modifications to allow custom SessionAware objects and a default CookieSessionAware.

That do you think? Make sense to you?

Hi,

The handler only works with save_path which does not contain any parameters like timeout, database, auth.

Open method should be something like:

public function open($save_path, $name) { $save_path = parse_url($save_path); $timeout = 0; $database = 0; $auth = null;

    if(isset($save_path['query']))
    {
        parse_str($save_path['query']);
        $timeout = floatval($timeout);
        $database = intval($database);
    }

    $result = $this->redis->connect($save_path['host'], $save_path['port'], $timeout);
    if(!$result)
    {
        return false;
    }

    if($auth !== null)
    {
        $result = $this->redis->auth($auth);
        if(!$result)
        {
            return false;
        }
    }

    if($database > 0)
    {
        $result = $this->redis->select($database);
        if(!$result)
        {
            return false;
        }
    }

    return true;
}

Solution based on the recommendation from @waverage, but maintaining compatibility with php>=5.6 while maintaining an enforced security against session fixation

According to https://github.com/phpredis/phpredis/issues/1323 Redis::exists() is now returning integer instead of bool, which breaks

https://github.com/1ma/RedisSessionHandler/blob/061dce59eeadfdc4b15cf29f1e83026196fcc9df/src/RedisSessionHandler.php#L253

Any plans of supporting the newer extension? And perhaps add this info to the known caveats section.

Thanks.

As it said at https://www.php.net/manual/en/class.sessionhandlerinterface.php , any custom session handler must return a string from the read() method.

This ensures the returning value is a string, not a random boolean.

session.save_path = " tcp://127.0.0.1:6379?database=2 " Useless, the session cannot be stored in the specified reids database，What is the problem and how to solve it？

PHP 版本 7.3.8-1+ubuntu16.04.1+deb.sury.org+1 PHPRedis 版本 4.3.0 Redis 服务器 v=6.0.11