Redis session missing checking for setting of wrong session id (FIXING PATCH INSIDE)

[misterion]

Using file or sql-lile session storage you got the warning if try to set empty session id.
Try this code

<?php
ini_set('error_reporting', E_ALL);

session_start();

echo 'before id=' . session_id() . PHP_EOL;
session_id("");
echo 'after id=' . session_id() . PHP_EOL;

The result is 'Warning: Unknown: The session id is too long or contains illegal characters, valid characters are a-z, A-Z, 0-9 and '-,' in Unknown on line 0'

So at least you have a chance to locate problem in code and fix it. Using redis session you haven`t got this warning.

But the most important is the next thing.

<?php
ini_set('error_reporting', E_ALL);

echo 'before id=' . session_id() . PHP_EOL;
var_export(session_id(""));
session_start();
echo 'after id=' . session_id() . PHP_EOL;

Using original php handler you got something like this:

before id=
''
Warning: session_start(): The session id is too long or contains illegal characters, valid characters are a-z, A-Z, 0-9 and '-,' in /path_to_sample/file.php on line 8

Call Stack:
    0.0005     327912   1. {main}() /path_to_sample/file.php:0
    0.0006     328032   2. session_start() /path_to_sample/file.php:8

after id=o37rffe3fn3vuuq4vmkcpe0686

but with redis session you got the empty session which would be set to client!!!

[misterion]

Here is a patch fixing this issue misterion@9cb3aa7

[nicolasff]

@misterion Thanks! I'll try to merge this today.

[nicolasff]

@misterion, after review I don't see any reason why we should limit the session key to these characters. Redis doesn't have this limitation. However, the empty string is indeed a special case that should probably be filtered out.

What do you think?

[misterion]

Yes, redis don`t have this limitation but in real world PHP session extension use these characters to generate session id. In general you can ignore this, but 'space', 'tab', control characters like '\r\n\a' must be filtered in any case. This is becouse of possibility of using php function like 'empty'.

The more one reason to add limitation is a back compatibility for session id. I think that using files, sql, memcache or redis for session storage must produce the same results.

So you can use or not this limitation but need to filter characters i write above.

[misterion]

Nicolasff do you need some more information before merge this issue to master?

[igor-pavlenko]

Hey guys,
how it's going with this patch?

I think it's really critical, because it allow someone grab your session.

1. Set empty value to COOKIES "PHPSESSID"
2. Go to site/script & authorise (you just need session_start();)
3. You will get in Redis empty session key: GET "PHPREDIS_SESSION:"
4. Try in other browser go to the same site/script with empty COOKIES "PHPSESSID"
5. You will get foreign session (You will be authorised by "first" user).

Thanks!

[michael-grunder]

Hey,

Interesting. I think disallowing empty sessions is the right way to go. I'll see about getting a hotfix up for this situation.

Cheers,
Mike

[simplewhite]

Is there a patch for redis 2.8.17?
Or better way to handle this issue.

We currently have user logged in as different user because of empty session ID

[fericsepi]

Is this fixed in 3.1.1?