Session ID: extend the regex to match possible hash representations

[virtualtam]

[EDITED]

Improves #306
Relates to #335 & #336
Duplicated by #339

Issues:

• PHP regenerates the session ID if it is not compliant
• the regex checking the session ID does not cover all cases
  • different algorithms: md5, sha1, sha256, etc.
  • bit representations: 4, 5, 6

Fix:

• index.php:
  • remove uniqid() usage
  • call session_regenerate_id() if an invalid cookie is detected
• regex: support all possible characters - [a-zA-Z,-]{2,128}
• tests: add coverage for all algorithms & bit representations

See:

• http://php.net/manual/en/session.configuration.php#ini.session.hash-function
• https://secure.php.net/manual/en/session.configuration.php#ini.session.hash-bits-per-character
• http://php.net/manual/en/function.session-id.php
• http://php.net/manual/en/function.session-regenerate-id.php
• http://php.net/manual/en/function.hash-algos.php

[virtualtam]

ping @ArthurHoaro @Cy4n1d3

[Cy4n1d3]

This still does not fix it for me:
$_COOKIE['shaarli'] contains 4jbqakvj8imes0gk3a53fvguihtpc9ggpbedcdpbj72m664v812r6olgi7m3jk8bbiorldr8997k7pt5g7b10nq2sqp4bfssfjrpdt3 (length 103), whereas sha1(uniqid()) generates a string with (correct) length of 40: 44f97439c02fa4ff8770d3a652d3442db75ebab3.

I'll have to investigate what's going on there - the cookie is being refreshed every page load, ruling an old cookie out.

[virtualtam]

Maybe related: there's no setcookie('shaarli', ...) call in the code -it its creation handled in the first place?

[virtualtam]

It looks like your session ID is being considered invalid (which is to be expected, as it does not match the expected format), and either:

• not regenerated by Shaarli
• overridden by your server settings

In which case, it would be necessary to define a regex that covers all PHP hash settings (or set the regex according to PHP settings -may turn hackish)

[virtualtam]

uniqid() may need to be replaced by session_regenerate_id() for the generated value to be compliant with PHP settings

[Cy4n1d3]

Defining a catch-all regex using the maximum length (should 128) would somehow work against the original intent of #306 I think. Hacking the regex together could turn out just as bad however :)
I guess that somewhat depends on PHPs' behavior - null cookies are already being checked right now, what if PHP expects a session id of length 36 and gets 128? Does that already trigger an explicit error message (eg. 'the session id is too long')?
Might need some testing, if a length of 128 is the maximum and does not trigger any errors, the regex should be fine then?

[virtualtam]

Working on an update & more robust tests, here are some interesting links:

• http://stackoverflow.com/questions/12240922/what-is-the-length-of-a-php-session-id-string
• http://forums.devnetwork.net/viewtopic.php?f=34&t=88685
• http://security.stackexchange.com/questions/56592/configuration-settings-for-sessions-in-php

However, I'm not sure how to reproduce the generation of strings encoded in such a way:

session.hash_bits_per_character allows you to define how many bits are stored in each character when converting the binary hash data to something readable.
The possible values are '4' (0-9, a-f), '5' (0-9, a-v), and '6' (0-9, a-z, A-Z, "-", ",")

[virtualtam]

Note: PHP 5.6.13 session ID generation code: php-src/session.c, specifically php_session_create_id()

[Cy4n1d3]

I've read the first SO thread yesterday, the second link is pretty interesting.
The SE link contains some of the arguments I've been following when setting up SHA512 as hash function in the first place...

The following was quite interesting, although already a few years old. Might serve as base for considerations?
http://www.php-security.org/2010/05/09/mops-submission-04-generating-unpredictable-session-ids-and-hashes/
https://secure.php.net/openssl_random_pseudo_bytes

[ArthurHoaro]

I think you're missing the point of this piece of code. It's only used to regenerate a random session ID if something's wrong and avoid any error. Otherwise, session ID is generated by PHP automatically (when the session starts I think).

Apparently, session ids can take any form depending on the server configuration but always hash.

I've made a few tests. Note that I've updated to PHP 5.6, and:

• I wasn't able to reproduce full path disclosure issue.
• If I tried to force an illegal character (=) in my session ID, I couldn't login.

Which means that our regex is valid, but I suggest that we just remove the length limit in the regex. Plus, [this list] can only get longer.

[Cy4n1d3]

I guess you're right, I've thought this function was adding some additional, own security token. I've re-read the code and get the point now.
PHP generates a session id on start if none is set, which is used afterwards as long as it's valid and you're not requesting it to be destroyed or refreshed.

May I ask why you're not using https://secure.php.net/manual/en/function.session-regenerate-id.php to retrieve a new session id?
Thinking about it, this might also be the reason why it's not working for me - I imagine to remember I've read about the fact, that PHP discards 'wrong' session ids and generates new ones in those cases. Wrong would be anything which doesn't match its own generation pattern in this case?

Also, did you try changing session.hash_bits_per_character to 6? In that case the regex should not work.

[Cy4n1d3]

Here's what I've been imagining to remember ;)
https://secure.php.net/manual/de/function.session-id.php#116836

The function validating the session id:
https://github.com/php/php-src/blob/master/ext/session/session.c#L449

Valid chars are a-z, A-Z, 0-9, ,, - and length has to be >0 and <= 128.

I've made the changes locally and adjusted (+ made) the unit tests:
#339

[virtualtam]

I've come up with the following snippet to generate test values:

<?php
function gen_session_hash($function, $bits_per_character)
{
    ini_set('session.hash_function', $function);
    ini_set('session.hash_bits_per_character', $bits_per_character);
    session_start();
    $sid = session_id();
    session_destroy();
    return $sid;
}

$results = array();

foreach (hash_algos() as $algo) {
    $results[$algo] = array();

    foreach (array(4, 5, 6) as $bpc) {
        $results[$algo][$bpc] = gen_session_hash($algo, $bpc);
    }
}

var_export($results);
?>

It has the advantage of dynamically generating test data for all hash functions embedded by PHP, in the 3 available representations.

Time to toy a bit with session_regenerate_id() :)

[EDIT] replaced print_r() by var_export() to generate reference data

[virtualtam]

PR updated with:

• valid regex
• reference test data generation: algorithm => bit per char => hash array
• test coverage

Todo: replace uniqid() by proper session handling

[virtualtam]

Testin' time!

@Cy4n1d3 was there a reason for moving the session ID regen after the call to session_start()?

@ArthurHoaro I've added a helper class to generate test data -sessions+PHPUnit don't seem to have a strong desire of working side-by-side ;-)

[Cy4n1d3]

Session needs to be started before you can regenerate the ID :)

[virtualtam]

Updated :)

[ArthurHoaro]

Awesome work here guys. Hopefully, this won't bother us anymore.