Use Crypt::SysRandom to generate session ids #5
Conversation
This replaces convoluted code for generating session ids with a simple call to retrieve random bytes from the system source of randomness.
robrwo
force-pushed
the
rrwo/improve-sessionid-generation
branch
from
7b8c9ab to
6a259ce
Compare
lib/Catalyst/Plugin/Session.pm
Outdated
|
|
||
| return join( "", ++$counter, time, rand, $$, {}, overload::StrVal($c), ); | ||
| } | ||
| sub session_hash_seed { } |
There was a problem hiding this comment.
A subclass could be using this, so it should still return something useful.
There was a problem hiding this comment.
What would be useful? An empty string? Some random data?
There was a problem hiding this comment.
Alternatively, we remove the method, document it as such in the changes. Any subclass that relies on this value will need to be updated.
There was a problem hiding this comment.
I'd probably not be in favor of removing a method from this, at this point this module is very hard to change given the size of the deployment footprint.
There was a problem hiding this comment.
Providing a reasonable seed seems like a good idea to me:
sub session_hash_seed { Crypt::SysRandom::random_bytes(80) }
Or whatever length seems appropriate. The old string was near 80, although it was of course providing much less entropy.
|
Perhaps instead this should just use UUID to generate identifiers? |
|
I think we need to be careful to change the dependencies of this 20+ year old module that is used in nearly every Catalyst application I've consulted on. I think the main thing we are trying to say here is the old code could be a security problem. If that is completely true, like there are known exploits, that's good reason to change. Crypt::SysRandom seems to have a pretty ok compatibility profile, but it's also fairly new. I don't personally know enough about this security topic to be sure it's an actual improvement. I'd not be excited to change this unless we are sure of all the above. As a total aside, I've never been happy with the design of the session plugin as it gloms too much into the application class directly. If the existing one is problematic, could be a good excuse to rethink. |
In your consulting, have you ever run across users that this change might break? I imagine most customisations would be to use some variant of /dev/random for the seed or generated id anyway.
This is a potential security problem. Session ids are often used to grant access to resources. (Note that securing sessions from cookie stealing attacks or forgery is another issue.) What seemed secure in 2005 is now practical to attack with today's hardware. Digest algorithms like MD5 and SHA are from the early 1990s and not considered secure, and there are data used to seed the hash is not very secret.
The "standard" is Crypt::URandom. That's been around for a long time. But I think Crypt::SysRandom is lighter weight, with XS handled as a separate module that users can install. We can use either though. Unless someone is running Catalyst on a very old system without /dev/urandom (before mid-1990s), I don't see it as being a problem. One alternative is to
If users are customising the session_hash_seed, then it should work. If they are customising the generate_id to use session_hash_seed, then again it should work. Otherwise they can update their config to just use the random data from the seed.
I remember being told in 2013 when I was submitting patches to improve this that it was "deprecated" and that I should consider moving to "Plack::Middleware::Session". I have been tempted to write a wrapper that just uses the Plack middleware. |
The session_hash_seed method returns raw random byes from the random source. The number of bytes can be configured using the "hash_seed_size" option. The generate_session_id method will return a hex string of those raw bytes if the "use_raw_hash_seed" option is true. Othersie (default), it will return a SHA-1 hash of the session_hash_seed. The algorithm can be changed using the "digest" option.
|
I have made some changes in d10be86 that I think will be safer for backwards compatability:
|
|
It might be worth updating the documentation to say that the session_hash_seed method is deprecated and that a future release may not use it at all. |
This replaces convoluted code for generating session ids with a simple call to retrieve random bytes from the system source of randomness.
Besides simplifying the code, this also improves security by using high quality randomness to generate a sessionid rather than a hash of a lot of data, much of which is predictable.
It uses Crypt::SysRandom, which is a lighter-weight alternative to Crypt::URandom, though either is fine.