[security] CSRF token algorithm improvement

[fesksa]

Using MD5 is a jaw dropping from security prospective. At least Sha2 should be used nowadays.

[Edit from panique, admin of this project:] This issue was originally titled "Md5 is badly broken", it was totally unclear what the commenter meant, so I've renamed it to "[security] CSRF token algorithm". The comment itself has not been touched.

[slaveek]

Any more details???

[fesksa]

I didn't go through the whole project, I just came across Csrf briefly and I noticed MD5 was used.

[slaveek]

If you'll read some about creating CRSF tokens you'll find out that this is OK.

[fesksa]

Moreover, the password resetting uses Sha1 which is also broken.

Indeed you make a joke of yourself when you say that sha1 is "the most advanced password hashing" algorithm.

[panique]

@fesksa

sha1 is "the most advanced password hashing" 

Never said that! This project is mainly about password hashing and user auth. If you really see any problem, then come up with a clear definition and write a proper fix. It's a big difference between hashing a password and providing a simple (pseudo-) random id for quick short-term user id obfuscation.

[fesksa]

I was pointing an important issue in the code and I didn't ask to fix anything. To my surprise, that was trolling in your dictionary.

[fesksa]

The issues are shown to be exist despite your initial denial. Unfortunately, instead of addressing the issue in a professional manner and discipline you chose to take it personal and handle it in a not mature way.

[slaveek]

Could you explain what is wrong in this line:

$user_password_reset_hash = sha1(uniqid(mt_rand(), true));

and what profits can bring eg. sha256 / 512 / or any other hash algoritm to:

generate random hash for email password reset verification (40 char string)

if that feature need unique string like hfd987fdg89843n8r87rfnr890........

[fesksa]

You may read about collision resistance. The uniqueness becomes less likely with a weak algorithm. Why not use sha256 or better given that there is no space constraints?

[geozak]

Can we close this thread and then after looking into the code base more bring up a specific issue with security or performance in a section of the code.

[fesksa]

@geozak Either use a reliable hash algorithm or don't waste valuable CPU cycles by running a useless md5 digest algorithm. Hopefully you find this more "specific."

[geozak]

This thread is not being productive for any of the parties here.

By more specific I mean can you point out what line or section of code is causing this application to be unsecure.

I have read every line in this project and didn't see any problems with the hashing security but maybe you see something we missed and if that's the case please show us where there error is instead of just saying MD5 is bad.

As far as the CRSF token using MD5 that is acceptable as it is being used to generate a TEMPORARY semi-unique identifier that is only stored in the session variables.
I don't know if you have read up on CRSF but MD5 is more than enough to prevent this type of attack. Also the MD5 is not being used to hash any sensitive data, just rand().
CSRF the only location using MD5.

[fesksa]

@geozak it just shows that you already made your mind when you say that this thread was not being productive, and yet, you put an effort into your post.

SHA1 and MD5 are outdated and broken, and should never be used when given that the far better alternatives require only few letters change in coding.

[geozak]

Its not being productive because of the attitudes that has been presented.
I put an effort it to hopefully turn it productive.

I have addressed the usage of MD5 and explained the reasoning for selection beyond explaining specific details of the certain attack is being used to prevent works, and logic of how its prevented (which is described is links within the comments near that section).

As for Sha1, it is used is two locations. In both locations it is a temporary semi-unique identifier. And both do NOT hash sensitive data, again just a rand(). One is for password resting which has a timeout of an hour. And the other is for email verification, something that is not much a of security issue.

Just because they are outdated and less "secure" does not meant that they useless when you factor in performance, storage requirements, and the level of security NEEDED (not the necessarily of whole project but the portion of the project that the hashing is being used for).

As far as I have researched and explained the only portion I see that could NEED more security is the would be the password resetting. But consideration must also be made for performance and storage needs for this change.

[panique]

@fesksa Saying "md5 is outdated and broken" is nothing new. The fact that md5 was (and is) used for password (!) hashing until today in thousands of tutorials, huge Nasdaq-listed companies and even some of the best frameworks in the world (I can only speak for Groovy and PHP scene, but it's quite obviuous that this is a global problem) is the reason why this project was initially created: To offer a modern and better implementation using future-proof updateable blowfish algorithm with proper salting. MD5 is NOT used in any way for password hashing.

MD5 is used to create a simple-as-possible pseudo-random strings for a basic CSRF (!) feature here, nothing more. If you have a proper attack scenario, then please write a short info and / or request a fix or provide one. Thanks :)

[fesksa]

Using sha1 is more serious issue and it should be addressed. The implementation of sha256 is as easy as sha1 while sha256 is far better in terms of security.

[geozak]

What are we securing here?
Literally a rand value that we are generating.
If an attacker does crack the hashed value it is literally of no use for anything because it is a random number that's NOT a key for anything.

It's obvious that you aren't even taking the time read our comments, let alone the code (not even the single line in question) to understand the context in which these hashes are being used.
So please stop expecting effort from others until you put in a little yourself.

[fesksa]

@geozak beside having an illusion that I'm making requests here, your post demonstrates a lack of understanding of what have been discussed so far.

Simply put; given few options with almost identical costs, only a dump person or ignorant would chose the broken one.

This project uses md5 and sha1 despite having other options which have better security and do not require dependencies or additional coding.

[fesksa]

@panique an attacker can make a malicious password-reset request and then attempt to produce a collision.

Moreover, it seems that the implementation of this project doesn't limit the number of validations per password-reset request which make things even worse.