Skip to content

Commit

Permalink
Better php.ini overrides for insecure setups
Browse files Browse the repository at this point in the history 
This change is to protect our users against a poorly setup server. PHP
can allow pretty scary things security-wise, so it's best to make sure
things that can only have one valid setting should be enforced.

Thanks to @hyp3rlinx for this.
  • Loading branch information
@nitriques
nitriques committed May 23, 2016
1 parent 6c73f63 commit b329a14
Showing 1 changed file with 3 additions and 0 deletions.
3 changes: 3 additions & 0 deletions symphony/lib/core/class.session.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ public static function start($lifetime = 0, $path = '/', $domain = null, $httpOn

if (session_id() == '') {
ini_set('session.save_handler', 'user');
ini_set('session.use_trans_sid', '0');
ini_set('session.use_strict_mode', '1');
ini_set('session.use_only_cookies', '1');
ini_set('session.gc_maxlifetime', $lifetime);
ini_set('session.gc_probability', '1');
ini_set('session.gc_divisor', Symphony::Configuration()->get('session_gc_divisor', 'symphony'));
Expand Down

15 comments on commit b329a14

@nitriques
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @michael-e do you see any problems with that commit ?

@michael-e
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have always switched off trans_sid if possible. Regarding the other two parameters, I don't know much about 'em. But I like making PHP stuff more secure!

@nitriques
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great thanks!

I have always switched off trans_sid if possible

That's the only recommended value ;)

@nitriques
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michael-e how about the session.hash_function ?
I think we should set it to 1. md5 is weak.

@michael-e
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea if this will improve security, because I don't know what is hashed (by PHP/Symphony) into a session ID.

@nitriques
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what is hashed

Looks to me like it's random bytes from urandom: https://github.com/php/php-src/blob/d5914d19eb5c78b9c1d4e3dc6048cfaeb5b39e68/ext/session/session.c#L866

See for more infos http://stackoverflow.com/questions/5081025/php-session-fixation-hijacking

@michael-e
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

session.hash_function allows you to specify the hash algorithm used to generate the session IDs

I don't get it, despite the long explanation on Stackoverflow. If it's just an ID generated from hashed random numbers, how can this make a difference? If an attacker gets your session ID, are you not lost anyway?

Nevertheless, it surely wouldn't be bad to use a stronger algorithm. (The fact that I don't get it does probably not mean that it's useless!)

@nitriques
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that I don't get it does probably not mean that it's useless

LOL. I, too, do not fully get why a stronger hash on random data would be better. But, as far as I can tell, it's only a matter of "risk of collision", meaning, how hard it is to "guess" or "predicts" valid session ids. It's not a matter of not being able to "steal" it. Since the id persists a lot of time, one could try them all and brute force their way into it. Sha1 produces longer ids, hence, it would be harder to brute force.

@michael-e
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I wonder why my Symphony session IDs are 26 characters only. An MD5 hash should be 32 characters, a SHA-1 hash would be 40 characters long.

@michael-e
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this?

@michael-e
Copy link
Member

@michael-e michael-e commented on b329a14 May 25, 2016 •
edited by nitriques

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, probably. session.hash_bits_per_character is 5, globally and locally.

@nitriques
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hyp3rlinx @brendo Should we set it to 5 or 6 ? The goal here is to provide sensible defaults, so should we check the current value and only act when it's not considered "secure/safe" ?

@brendo
Copy link
Member

@brendo brendo commented on b329a14 May 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could take learnings from the HttpFoundation component and move all of this to the user's control. Theoretically the defaults set by php are already the sensible defaults, so the changes we are making are already assuming that we know better than the defaults (sometimes true!).

@nitriques
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

component and move all of this to the user's control.

Isn't this what we already have, i.e. via custom php.ini files ? Some host do not allow them, so should it be config values ? I would do it for values that can have multiple secure-wise valid values...

why my Symphony session IDs are 26 characters only.

I have 32 chars ones and my session.hash_bits_per_character = 4 so the value works as expected. Funny that one would only want half of the bytes to mean something ;)

@nitriques
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a bunch of reading and testing, hash config should be a server thing. We are not making more secure as custom values can be even more secure. Default values are sensible enough. I will change my php.ini settings accordingly. Case closed. Thanks 👍

Please sign in to comment.