All,
As identified in the previous thread, there is an issue when providing
invalid salts to crypt()
where it will silently fall back to STD_DES.
This was evidenced by a fix to crypt()
when upgrading blowfish to 1.3.
Causing a failing test for Horde_Auth:
$ php -r 'var_dump(crypt("foobar", "*0OayF9ttbxIs"));'
string(13) "*0OayF9ttbxIs"
With 5.4.36 / 5.5.21RC1 (with)
$ php55 -r 'var_dump(crypt("foobar", "*0OayF9ttbxIs"));'
string(2) "*1"
The new behavior is absolutely correct due to *0 and *1 being error
conditions, so any salt with *0 should immediately fail with *1 (to
prevent one equaling the other). Previously it was used as a salt for
STD_DES.
This raises another issue though. Currently, an invalid bcrypt salt
such as $2$10$... would actually result in a DES hash:
string(13) "$2rcByx51ejoM"
Which is completely incorrect.
This not only hides errors, but it falls back to an absurdly insecure
format while hiding them. It's worth noting that this case will cause
HHVM to fail with *0.
Changing this fallback behavior to the correct error should happen.
However, this will likely break a number of live systems which are
currently relying on the incorrect behavior (likely without knowing
it).
It's worth nothing that failing is the currently documented behavior:
http://php.net/crypt
Therefore, I'm suggesting we add an E_DEPRECATED
error when we detect
an invalid STD_DES salt but still execute the fallback:
https://github.com/php/php-src/pull/989
Then in a future version (7.1, 8, whatever) remove the fallback and
keep the error along with returning a failure indication (*0).
I'm open to tweaking the error message, and possibly changing to
E_WARNING
if people think it's worth it.
Thoughts?
Anthony
Hi all,
On Sat, Jan 10, 2015 at 1:45 AM, Anthony Ferrara ircmaxell@gmail.com
wrote:
It's worth nothing that failing is the currently documented behavior:
http://php.net/cryptTherefore, I'm suggesting we add an
E_DEPRECATED
error when we detect
an invalid STD_DES salt but still execute the fallback:
https://github.com/php/php-src/pull/989Then in a future version (7.1, 8, whatever) remove the fallback and
keep the error along with returning a failure indication (*0).I'm open to tweaking the error message, and possibly changing to
E_WARNING
if people think it's worth it.Thoughts?
+1 for merging it as bug fix.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Changing this fallback behavior to the correct error should happen.
However, this will likely break a number of live systems which are
currently relying on the incorrect behavior (likely without knowing
it).
I'd call this a sec fix. Absolutely preferable to have an error than a
silent fallback to broken crypto.
Then in a future version (7.1, 8, whatever) remove the fallback and
keep the error along with returning a failure indication (*0).
Is 7 really too soon? I know we err on the side of compatibility, but
in my opinion the fallback should be removed completely (any salt
starting with a $ must not degrade to any other method).