Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements and Bug Fixes for gae-sessions v1.07 #34

Open
TheRook opened this issue Apr 13, 2012 · 3 comments
Open

Improvements and Bug Fixes for gae-sessions v1.07 #34

TheRook opened this issue Apr 13, 2012 · 3 comments
Assignees

Comments

@TheRook
Copy link

TheRook commented Apr 13, 2012

I like gae-sessions, and I have made some great improvements in my fork:
https://github.com/MikeBrooks/gae-sessions

As a cryptographer the use of MD5 bothers me because it is a broken primitive. Although there are numerous vulnerabilities in md5, this doesn't undermine this session handler, however it isn't going to help. If GAE's os.urandom() is anything like /dev/urandom then it is a very good source of entropy and all values returned by this function have already passed though a hash function. In general you should choose one cryptographic hash fiction and use it everywhere, and a member of sha2 isn't a bad choice.

Cryptography should only be used when there is no other choice. It is better to have an absolute solution because when a cryptographic system breaks its bad. A good example is the ASP.NET padding oracle attack:
https://www.owasp.org/index.php/ASP.NET_POET_Vulnerability

An HMAC is useful for protecting a payload from being modified. However they always suffer from the "birthday attack". This attack is refering to the large number of input messages that produce the same output authentication code. For this attack it doesn't matter how long your secret is. It is best to avoid this scenario whenever possible, because its a waste of resources.

When gae-sessions is using the database only the SID value is needed. The introduction of an HMAC doesn't improve security, in fact it introduces the possibility of attack. In that if an attacker used the birthday attack he could create a new message with a timestamp in the future that has a valid signature which would cause the is_active() method to return true.
Hopefully an application's authentication system doesn't rely upon is_active(), but this is easy to avoid by removing the HMAC and verifying this value with the database.

Improvements:
(efficiency)Session id length cut in more than half
(efficiency)timestamp in SID has been reduced form 10 bytes to 4 bytes with bit packing
(efficiency)add a cron job to the demos to delete stale cookies (This should also be added to the install doc)
(efficiency)Hash functions used to generate session id cut from 3 to just 1
(Security)SIDs contain more entropy
(Security)removed use of md5()
(bug)SID_LEN can now be changed without causing offset exceptions
(bug)delete_expired_sessions function was not checking remaining cookies properly

@ghost ghost assigned dound Apr 13, 2012
@dound
Copy link
Owner

dound commented Apr 13, 2012

Wow, this is a serious contribution Mike - thanks a ton for both the code and the detailed writeup. I'd love to get this integrated and push out a new release for gae-sessions with this integrated. I'll review the code changes this weekend!

@dound
Copy link
Owner

dound commented Apr 13, 2012

Can you make a pull request for your changes when you get a chance? :D

@john2x
Copy link

john2x commented Jan 19, 2015

Has this been merged to master?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants