CMSimple_XH–Forum

Hello Community,

since CMSimple_XH 1.5.4 we adopted PasswordHash for hashing the admin password. For portability we opted in to the portable hashes (based on MD5). However, these hashes are relatively weak[1], and since PHP 5.3.0 (5.3.2?) PasswordHash is able to support the stronger bcrypt in a portable way. Therefore I suggest to switch to its bcrypt hashes. Note that the old passwords would be still valid (only not as safe).

An alternative might be to use PHP's native crypt(), but I don't know how to use that in a portable way. password_hash() appears to be the best option, but that is only available since PHP 5.5.0. So for now (i.e. until we require PHP 5.5) I suggest to stick with PasswordHash.

An additional benefit of PasswordHash would be the get_random_bytes() method. I'm not an expert on CSRNGs, but I'm confident that get_random_bytes() is far superior to mt_rand() or even rand() for cryptographic purposes. A presumably better alternative would be openssl_random_pseudo_bytes(), but that may not be widely available.

Anyhow, if we stick with PasswordHash, I suggest that we fully adopt it, and modify it according to our CS and use a PHP 5 constructor, add visibility specifiers and remove the fallbacks for PHP < 5. Besides the constructor, HashPassword(), CheckPassword() and get_random_bytes() (renamed to getRandomBytes()) should be public; all other methods and properties protected/private. And of course we should rename the class to XH_PasswordHash, so it can be autoloaded by the general autoloader.

I suggest to apply this patch.

Thoughts?

See also:

• http://php.net/manual/en/faq.passwords.php
• http://www.heise.de/security/artikel/Pa ... 53931.html (German)

[1] For once, it uses md5() which is not collision safe, and furthermore the stretching is implemented in PHP, which is slower. On my PC bcrypt is roughly able to use the double amount of rounds (i.e. cost+1) than the md5 based variant. Therefore we can easily switch to cost 9, and maybe we should increase that further.

+1
It's all high discipline, but sounds reasonable.

Done (r1517).

Well, more than two years have passed, and now I see the following severe issue with XH\PasswordHash:

::getRandomBytes() tries to read /dev/urandom (what might not be the best option, anyway; php_random_bytes() uses it as last resort), but if that fails (e.g. on Windows), it falls back to constructing random bytes mostly based on microtime() calls. This is definitely not cryptographically secure, but crypt() relies on a good salt:

Make sure to specify a strong enough salt for better security.

Furthermore, password_hash() and friends are supposed to offer strong password hashes in a simple and portable way as of PHP 5.5.0, and there is a "Polyfill" available for PHP 5.3.7: password_compat.

Therefore I suggest to replace XH\PasswordHash with password_hash() and friends, and to include password_compat (it's only a single file). For maximum portability we can use PASSWORD_BCRYPT as algorithm, which currently doesn't matter anyway as PHP_PASSWORD_DEFAULT is still PASSWORD_BCRYPT in current PHP master. This way users of recent PHP versions have the best support available, and older versions would have an acceptable fallback (I'm not concerned to raise the requirements from PHP 5.3.0 to 5.3.7).

We would need a public replacement for ::getRandomBytes(), though, as it's used elsewhere in the core. random_bytes() is available as of PHP 7.0.0, and for older versions the respective code from password_compat might already be sufficient; otherwise we could use (relevant parts of) random_compat. random_compat has the advantage that it doesn't fall back on weak algorithms, but rather raises an Exception if sufficiently strong sources of randomness are not available, as does random_bytes(). We could still fall back to a weak entropy source in this case, but also show a warning to the user, or maybe simply not offer related functionality (currently, only the password forgotten link and configuration fields of type "random").

Thanks for seeing ahead wisely.

manu wrote:Thanks for seeing ahead wisely.

Hopefully.