Skip to content

Instantly share code, notes, and snippets.

@ircmaxell
Created December 17, 2013 14:42
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save ircmaxell/2c0e5484b1bf8475b3d5 to your computer and use it in GitHub Desktop.
Save ircmaxell/2c0e5484b1bf8475b3d5 to your computer and use it in GitHub Desktop.

While I very much appreciate the bug reports (I have seen them, just haven't had the time to actually dig through it yet), please be careful not to blow them out of proportion.

The first thing I'd like to point out, is that none of these "bugs" are issues if you let the library generate the salt for you. Which means that the only way that there is any impact at all is if you explicitly do not follow the recommendations in the documentation:

Caution It is strongly recommended that you do not generate your own salt for this function. It will create a secure salt automatically for you if you do not specify one.

Now, as to the bugs themselves:

Improper Base64 - This should not be an issue. The worst-case that can happen is that you lose 2 bits of entropy from a user-supplied salt (from the right most character which is a partial salt, partial hash value).

And considering that crypt() is specifically designed to store the salt with the hash, if you are storing it separately then you are already breaking the standard functionality.

With all of that said, this is something that can (and will) be fixed. But it's most definitely not a major issue. And it's most definitely not a security breach worth writing a warning post over (2 bits out of 128 is not significant, it's worth fixing, but it's not going to impact security at all).

mb.func_overload - This has the potential to be an issue. With that said, I've never actually come across an environment with it on. So it's definitely worth fixing, but it's not a massive issue.

Digging through the library again, there are three places that could be effected by this:

In password_hash, if you supply your own salt then it could raise an error that the provided salt was too short. This means that it would error and refuse to generate the hash. Annoying, but not a security issue.

In password_get_info, the hash will always be US-ASCII, so the string length will always be the same (BIG5 has a possibility of being different, but with the allowed characters in a hash, they are all single bytes). So the potential is there, but it won't have an effect.

In password_verify, the same thing happens as password_get_info. So there would be no effect.

So the only possible bug here (that changes expected behavior) is if you provide your own salt (which we already discussed is not-recommended).

open_basedir -You'll get a warning for every call. And that's because you're using a poor setup of PHP. Openbasedir is a relic along side safe_mode and register_globals that was added by people who did not understand how security should work. Running a server with openbasedir in 2013 is the bug. You should be using far more secure alternatives like chroot jails and the such.

Silencing the error is unacceptable. There may be a better way of fixing it, but just adding @ in front of the is_readable call is not going to happen. The correct way to fix this is to fix the server it's running on. But I will look at a more robust "fix" for Windows in the library (but I won't support error suppression for this)...

Side Note

And as a side note, please be more careful with your wording. I appreciate letting others know about the issues. But nothing that you posted here is going to affect 99.99% of users. You would need to be using the library in a non-recommended way, on an edge-case system to see any sort of effect. And even that wouldn't be enough to make a security concern.

Thanks for the research, and I will fix the issues when I get a chance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment