8
\$\begingroup\$

I just wrote this function and am wondering if it's secure enough for storing passwords in a database:

public static function hash($salt, $str) {
    $salt = md5($salt);
    $salt = '$2y$12$HfuPgoDY94nHJssqQeVLmH' . $salt . '$';

    return crypt($str, $salt);
}
\$\endgroup\$
6
\$\begingroup\$

Yes

Blowfish is a great algorithm for storing passwords, it makes generating a hash slow such that brute force attacks are unviable. It's also php's recommended means of storing passwords.

But

The salt for blowfish is supposed to be:

22 digits from the alphabet "./0-9A-Za-z".

This applies to the salt string which in the question is:

'HfuPgoDY94nHJssqQeVLmH' . $salt . '$'

That's 22 chars + 32 chars + '$', which is 65 characters long and contains an illegal '$'. The consequence of that is that the salt actually used is a fixed string, the first 22 characters and is the same for all generated hashes.

The salt should be random, and for example you could use something as simple as this:

$salt = '$2y$12$' . substr(md5(time()), 0, 22);
return crypt($str, $salt);

Note however that this code is simple for brevity and can be significantly improved upon since the salt will only be made up of hexadecimal (0-9a-f) characters. Here's an example implementation that could be used to generate a better salt.

Make cost a parameter

A cost of 12 is fine for passwords, but you may wish to make it a parameter so that e.g. unit tests are not needlessly slow whenever they generate a password.

\$\endgroup\$
  • \$\begingroup\$ Thanks very much, marked as top answer. Sorry for late reply, was away over the last couple of days. \$\endgroup\$ – user18933 Nov 6 '12 at 21:39
  • 2
    \$\begingroup\$ If you take partial md5 hash as salt, you are effectively narrowing the namespace down to 16 characters (md5 is 0-9 a-f), when it could be around 60 characters. \$\endgroup\$ – 1615903 May 20 '13 at 6:13
  • \$\begingroup\$ The salt should be at least 64 characters, and it should be generated using a cryptographically secure random number generator. It needs to be unique for each hash. It should be the same size as the hash. Any less is too little entropy. Any more is wasted. Always pass work units into key stretching algorithms like bcrypt, PDKDF2, etc... That way you can increase the work required as CPUs get faster. Double the work every 2 years. \$\endgroup\$ – Eric Elliott Oct 6 '13 at 20:46
  • \$\begingroup\$ @EricElliott The salt should be at least 64 characters - as quoted and stated in the linked reference in the question CRYPT_BLOWFISH - Blowfish hashing with a salt as follows: "$2a$", "$2x$" or "$2y$", a two digit cost parameter, "$", and 22 characters from the alphabet "./0-9A-Za-z" - using 64 characters will simply mean the first 22 chars are used and the rest discarded. As pointed out by @user1615903 using md5(anything) will lead to a lower entropy salt, however it was used as a simple example of generating a 22 char random salt. I'm not sure what you wish to say regarding work units. \$\endgroup\$ – AD7six Oct 6 '13 at 21:03
  • \$\begingroup\$ In that case, I could consider this algorithm obsolete and try to find a PBKDF2 implementation that can take hash length / salt length as parameters, and let you specify iterations. CPU power doubles every year, and botnets are gigantic. \$\endgroup\$ – Eric Elliott Oct 7 '13 at 1:58
3
\$\begingroup\$

No

md5 gives you a hexadecimal output, 0-9 & a-f. Salt for bcrypt takes base64 encoded string. This means that if you use (partial) md5 as salt, you are loosing a lot of entropy. See #9: Bonus 2 from this post. It's a good read on the subject.

I have the same advice as that poster: use a library.

\$\endgroup\$
  • 2
    \$\begingroup\$ Re: use a library, PHP 5.5 will be including simplified password hashing functions. So, in the future it may be as simple as using these. \$\endgroup\$ – Paul May 21 '13 at 9:41
-2
\$\begingroup\$

No

If you can't pass hash length, salt length, and work units (iterations) into the hash function, you have no way of keeping up with faster CPU speeds.

The salt should be at least 64 characters, and it should be generated using a cryptographically secure random number generator. It needs to be unique for each hash. It should be the same size as the hash. Any less is too little entropy. Any more is wasted. Always pass work units into key stretching algorithms like bcrypt, PBKDF2, etc... That way you can increase the work required as CPUs get faster.

In 2014, a PBKDF2/HMAC+Sha1 algorithm needs 128,000 work units. Double the work every 2 years.

You need to store the work units with the hash so that you know how many to use for comparison, and you'll know to ask the users to select a new password when its time to upgrade their hash.

Addressing comments (there wasn't enough room in the comments field):

The salt should be at least 64 characters because the hash should be at least 64 characters. The salt should be the same size as the hash in order to prevent attackers from creating lookup tables for every possible salt. This is why it's a very bad idea to use a username as a salt -- especially now that we're fighting off attackers with enormous amounts of storage, CPU, and memory space compared to just a few years ago. In other words, having an equal length salt guarantees that generating tables for each salt (combining tables + dictionary attacks of the most common passwords) would take at least as long as a brute force attack.

The minimum length of 64 characters is the current recommendation (for the year 2014, 2013 is almost over) in response to changing CPU speeds. The recommendation of 22 was first made years ago -- CPUs double in speed every year, and current botnets exist with over 90,000 nodes, capable of generating billions of salts per second. Super cheap supercomputing clusters can be built today for less than $2k. The old standards just don't cut it anymore.

Salts need to be generated by a CSRNG in order to prevent collisions. Pseudo-random generators that are not CSRNGs are notoriously bad at that, often generating a collision within a few thousand hashes. For sites with hundreds of thousands or millions of users, that's not an acceptable rate.

It's important to prevent collisions because a collision means that the same tables can be used to attack multiple hashes at the same time.

Work units = cost parameter?

Work units = cost parameter, yes, but the original question is about a function which does not take a cost parameter. The cost is hard-coded in the function.

Sha1+HMAC is inappropriate

No, "HMACs are substantially less affected by collisions than their underlying hashing algorithms alone." Sha1+HMAC is far more secure than Sha1, and it is a currently accepted standard for password hashing.

and PBKDF2 is weaker than bcrypt so overall..

No, it isn't. Once you go over 60 characters or so, the cost of PBKDF2 trumps bcrypt by orders of magnitude.

It's ubiquitous and time-tested with an academic pedigree from RSA Labs, you know, the guys who invented much of the cryptographic ecosystem we use today. Like bcrypt, PBKDF2 has an adjustable work factor. Unlike bcrypt, PBKDF2 has been the subject of intense research and still remains the best conservative choice. There has been considerably less research into the soundness of bcrypt as a key derivation function as compared to PBKDF2, and simply for that reason alone bcrypt is much more of an unknown as to what future attacks may be discovered against it.

In other words, bcrypt is overrated. Especially if you think it's better than PBKDF2.

Source: http://www.unlimitednovelty.com/2012/03/dont-use-bcrypt.html

\$\endgroup\$
  • \$\begingroup\$ There are no references given here, and it seems to me (I'm no expert) to be mostly misinformation. The salt should be at least 64 characters - why? [the salt] should be cryptographically secure - this is commonly(?) held to be untrue; Always pass work units - isn't that the cost parameter, if it's not - what is it? I'm not clear on your recommendation but sha1 is inappropriate and [PBKDF2] is weaker than bcrypt so overall.. -1. \$\endgroup\$ – AD7six Oct 7 '13 at 8:27
  • \$\begingroup\$ @AD7six See my edits, above. There wasn't enough space to respond to your comments appropriately here. \$\endgroup\$ – Eric Elliott Oct 7 '13 at 16:20
  • \$\begingroup\$ A much better answer! Though The salt should be at least 64 characters because the hash should be at least 64 characters. this doesn't clarify anything - why? I don't feel the explanation for requiring a cryptographically secure salt (I repeat: salt) addresses why; The cost paramter being hard coded isn't the same thing as a cost parameter not being used (that's how I understood all comments/references to it to date); Once you go over 60 characters or so how many passwords are that long? I'll remove my DV but I think saying bcrypt is inappropriate is still a huge overstatement. \$\endgroup\$ – AD7six Oct 7 '13 at 16:39
  • \$\begingroup\$ Additional point: If the code in the question isn't appropriate a link or example of (php) code that is appropriate would be a good idea, otherwise the read is left with "OK but what can I use instead?". \$\endgroup\$ – AD7six Oct 7 '13 at 16:49
  • \$\begingroup\$ Added: Having an equal length salt guarantees that generating tables for each salt (combining tables + dictionary attacks of the most common passwords) would take at least as long as a brute force attack. \$\endgroup\$ – Eric Elliott Oct 8 '13 at 17:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.