People often get the salt generation wrong, e.g. by just using a substring of `md5(mt_rand())`, which is obviously wrong (in several respects) :/
If the salt is allowed to be "less than secret" (by which I mean it can be stored in plain-text, not that it should be published on your website), then what does it matter if it's "pretty random" versus "cryptographically random"?
What's wrong with something like this:
$salt_length = 22;
$cost_factor = 10;
$lower = range('a', 'z');
$upper = range('A', 'Z');
$numeric = range('0', '9');
$special = array('/', '.');
$salt_chars = array_merge(
$lower,
$upper,
$numeric,
$special
);
$char_count = count($salt_chars);
$salt = '';
for ($i = 0; $i < $salt_length; $i++) {
$salt += $salt_chars[mt_rand(0, $char_count -1)];
}
$hash = crypt(
$password,
'$2a$' . $cost_factor . '$' . $salt
);But for this case, the salt generation is much better (assuming that `mt_rand` is a good enough source of entropy, which may or may not be the case).
The rest definitely applies though.
In short, you're using the wrong algorithm ($2y$ is the better one, the one you're using has a known bug). You're not checking for errors from `crypt()` prior to storing the hash. So you can wind up significantly messing up your database and potentially leaving it in a worse state than if you just used `md5($password)`... And the minor note about timing attacks...
Not to mention that you currently have an issue in your code (it needs to be .= for a string, not +=)...
This is the only question I was asking, and you haven't really addressed it in any detail. The reddit comment you linked was replying to some obviously bad code. I mean, limiting your salt to use only 16 possible characters? Really?
> you're using the wrong algorithm ($2y$ is the better one, the one you're using has a known bug)
I didn't know about that bug until just after writing my last comment (don't worry, I don't do this for a living). I just used `2a` because that's a) the example I see most often, and b) that's what was used in this HN comment thread.
The security fix notice[1] linked from the manual page for crypt() mentions that `2a`, on systems where `2y` is available, has countermeasures to try to combat the vulnerability for newly generated hashes, and even says "if the app prefers security and correctness over backwards compatibility, no action is needed - just upgrade to new PHP and use its new behavior (with $2a$)" which doesn't make it sound like it's a huge issue to use `2a` on newer installs, just that you should prefer `2y` where possible.
That said, I'll make a note to use the new one since it is superior. I do find your comment that "if you're on too old of a PHP version to use that (5.3.7 IIRC), then don't even talk about security..." to be needlessly flippant. You don't even bother to offer an alternative to the poor bastards that are stuck on older versions.
> You're not checking for errors from `crypt()` prior to storing the hash.
It's example code, not production code. Maybe I should have made that clearer, but I thought it would be pretty obvious.
> And the minor note about timing attacks...
What's the timing attack on my (non-production, air code)? Your comment on timing in the reddit comment was about verification, which my code doesn't mention.
> Not to mention that you currently have an issue in your code (it needs to be .= for a string, not +=)...
That's just a stupid typo/brain fart. I didn't actually run this; it's just "air code".
---
Can you elaborate more on the salt generation specifically, since that's all I was really trying to ask here? Is the point of using a more cryptographically secure RNG just to make it more likely that each new salt will be unique? How important is absolute uniqueness?
If you could also elaborate on the problems with mt_rand() while you're at it, I'd appreciate it. The only thing the manual mentions, as big a problem as it may be on its own, is that it prefers even numbers on 64-bit systems in certain configurations. Is there more to it than that?
I'm not a PHP pro, as should be clear by now, so I appreciate any information you can pass along. I'm just trying to learn.
I thought you meant the function in its entirety.
So, to your specific point, it's not bad. That doesn't mean it can't be improved upon.
For example, `mt_rand()` is susceptible to certain types of seed poisoning attacks. That's because the state that it uses is process specific. So when running PHP in a case similar to what happens with mod_php, that state is shared among all php instances (just like with APC). What that means is that the security and randomness of your usage depends on everyone else's usage. So if someone calls `mt_srand()` in one app over and over with the same value, your randomness can be thrown out of the window.
Now, that's a very significant edge case with very limited attack potential. However, when it comes to security if there's a better way, why not use it. And in this case, there is (/dev/urandom). Just read from that source (via fopen, via mcrypt_create_iv, via openssl_random_pseudo_bytes, etc).
I'd much rather edge on the safer side as long as there are not significant downsides...
As far as 2a vs 2y, I would stick with 2y unless you have a very good reason for sticking with 2a.
As far as the error checking, I thought it was worth mentioning, since it seems that $hash = crypt(...) is all you need, when in reality it isn't. Which goes to further my point that crypt() is too difficult to use out of the box...
> That's just a stupid typo/brain fart.
I realize that. I was just pointing it out.
> That said, I'll make a note to use the new one since it is superior. I do find your comment that "if you're on too old of a PHP version to use that (5.3.7 IIRC), then don't even talk about security..." to be needlessly flippant. You don't even bother to offer an alternative to the poor bastards that are stuck on older versions.
Correct. Because older versions have fairly significant vulnerabilities associated with them. Two major DOS vulnerabilities come to mind. Is the comment flippant? Perhaps. Does that make it wrong? No...
And as far as "offer an alternative to the poor bastards that are stuck on older versions", there are plenty of those. PHPass supports PHP all the way back to like 4.2... If you need a password hashing algorithm for an unsupported version (or 5.3.x < 5.3.7), just use that.
Which actually brings me to the entire point (I don't need to tell you, just making the point again). Just use a library for this. It may seem easy to just do it yourself, but there's a lot to it. Just use a library and be done with it. There's no reason to re-implement it every time...
Hope that helps...
I apologize if I got a bit crass in my earlier comment. I think the Wil Wheaton's Guide To Depression post has me a bit sensitive today. I probably took the worst view of your comments and got annoyed over my own warped perception.
I probably should just use a library for this, but I've been in a pretty "reinvent the wheel to learn about wheels" mode with the thing that I'm building (the latest in a series of projects which continue to elude actual completion). I even started writing a framework a while back, before switching to CodeIgniter since it's widely used and easy (before switching back to writing a framework after getting annoyed fighting CI... kidding).
Since there's likely only ever going to be a single user for this thing, I doubt the password implementation actually matters much, but it's certainly going to be a debate for a day or two.
Thanks again.
Rainbow tables are not the only "attack vector" that proper use of salts defeats.
The salt which is unknown/unpredictable (and contains enough entropy) to the attacker makes his offline attack against the hash unfeasible (after he has managed to fetch the password hash from the server using timing leaks). I'm not sure if it is possible to fetch the (whole) hash using timing, because it is not a direct comparison. But anyway, if the attacker managed to do that, now because of "a proper salt" he would have to crack a hash that was composed of, say, 128 bits of salt and 20 bits of the actual password. It is unfeasible because of that 128 bits of salt alone.
That's defending against a newly generated rainbow table.
> And it is not possible to see if different entries shares a same password (or to see if they have a different password).
That's repeating the first point with different words! It's defending against a pre-generated table. i.e. a rainbow table.
Timing attacks are not mitigated by salts; they're mitigated by the design of the encryption. You should not rely on salts for this. In fact, if your hash is exposed you should assume your salt is also exposed.
What do salts guard against other than rainbow tables?
You lost me here. Anyway, the attacker does not need a rainbow table at all to attack against multiple hashes at the price of one.
> That's repeating the first point with different words! It's defending against a pre-generated table. i.e. a rainbow table.
Again, no rainbow tables at all are needed to see if different entries shares a password or not.
About timing attacks, see my earlier comment in this comment chain.
> You should not rely on salts for this. In fact, if your hash is exposed you should assume your salt is also exposed.
I see and agree with your point about "relying on salts", but salts just happens to (as a side-effect probably) mitigate the attack. Remember, your salts are not exposed "as-is" if the attacker manages to fetch the password hash using timing.
Yep. Can't wait for GoogleGuy's php-doc changes to get in, then I could downvote suggestions like this: http://www.php.net/manual/en/function.hash.php#101987
What are the ways this is wrong? I know that mt_rand is not a cryptographically secure RNG, but what are the other ones?
(NB: I do not write my own auth/password code.)