password_compat
password_compat copied to clipboard
ircmaxell
Type checks of custom salt too permissive
Why is null or a boolean a valid salt? Sure, it will be caught later by the length check, but that's only because those values happen to have a string representation with less than 22 characters. Semantically, this makes no sense.
Why is a number a valid salt? 22 decimal digits are hardly acceptable.
Is it really a good idea to accept objects? Just because it has a __toString() method doesn't mean it was actually intended to mimic a string. This is very error-prone, because the user might accidentally pass some object which merely contains the salt string (an instance of a randomness class or whatever).
In my opinion, the function should only accept a real, actual string. This might be against “the PHP way”, but I don't think a security-related function is the right place for type juggling, duck typing or whatever.
Because different systems have different ways of generating strong random numbers. The automatic salt generation tries out a few possible sources, but it can't cover every single server setup on this planet.
Linux is different from Windows. A new Windows is different from an old one. OpenBSD is different from both Linux and Windows and also has differences accross its versions. Maybe the server only has a hardware RNG. Nobody knows, which is why there's an option to manually generate the salt.
I think this is something that is caused by PHP's bad behaviour. In PHP you can pass any scalar values (+null) where the documentation says string. There are only a few exceptional cases. In a backported function, the same functionality must be provided. So if PHP does it badly, ...
Not only is the custom salt bad, rather than forcing openssl or mcrypt, falling back to /dev/urandom (just use /dev/urandom, maintain less code?). There is a fourth option available in the code that should at the very least generate a notice; to say it is not an invariant as it uses ^ ord(mt_rand())... See http://phpsecurity.readthedocs.org/en/latest/Insufficient-Entropy-For-Random-Values.html
