All, Look at the issue, there's a line in there that is the crux of the issue:
> So problem isn't only in ROUNDS_MIN. In fact, the overall algorithm changed. Look at a quick example: http://3v4l.org/Eov3o >From 5.3.2+ (when we pulled in our own implementation of crypt-sha256 and crypt-sha512, and crypt-blowfish): using salt: string(31) "$6$rounds=1000$abcdefghijklmnop" gives hash: string(118) "$6$rounds=1000$abcdefghijklmnop$SgL/Atu7DClkX0qBUuG6FS2bRf2XmLFWY9b8pRttPEj9ZSh4MKE5bKlz4WAKomLuWI.YQ5oIPLO2L.0OioAeW/" great. But in 4.3.0 - 5.2.17: the same salt gives: string(99) "$6$rounds=10$EdPD9pXG2vAIeeyG2E/9Mzey2mA/qZgIBAbccXWtrNiymhFYXNK4r2gphMi4KOksXRubHnBWGVh/p2pP2D3R.." Which is shorter. Also note that the rounds went from the defined 1000, to 10. Things get far weirder: http://3v4l.org/KZoIv $h1='$6$rounds=1$abcdefghijklmnop'; $h2='$6$abcdefghijklmnop'; gives us (on 4.3.0 - 5.2.17): string(102) "$6$rounds=1000$$6TFP.7u1vZP5A9fccvmUPteI8f29BLhgfqL1XEQqcrvqTSKKw5SBa2qw1sOwLEQ41Dhl1u/Jbi2hRHRYCdxuv0" string(99) "$6$abcdefghi$4Dv/xQG4euniWsyrRHGUrNqM9CFl5Pg9EI88OgO4tIzuV952JnT09wVxrzUP9l/dr0wP3YSs1Ufz1qJpifLAA0" Note that these are different all around from the other results from the same version range. Basically, the version of crypt being linked to is either buggy (not likely) or our documentation of it is (likely). And our implementation (since 5.3.2) conforms to our documentation instead of the old behavior. Considering 5.3.2 was released 4 years ago, and 5.2 has been EOL for 3.5 years, I don't think there's anything that can be done about this. If it was a simple limit that needed changing, a polyfill would be easy (took me 10 minutes: https://gist.github.com/ircmaxell/ac0710ce044504f65cf0 ). But seeing the entire algorithm is different, much harder. I think this should be documented, and then called a day. I don't see anything else that could reasonably be done (changing crypt() at this point is out of the question IMHO). > - Add bool crypt_migration INI that is default to OFF for PHP 5.4 and up. > - master will not have this INI. Please don't. As I showed above, it's already long past that point. And we don't need new ini settings. And what that ini setting would control is not the minimum iteration flag (as that's not the only problem), but which version of libcrypt we're binding to (our internal one, or an external one). Which means that the overall behavior *will* change. And unless you do it perfectly, it could effect all of crypts algorithms (potentially impacting password_hash, etc). We internalized the algorithms in 5.3.2 at least partially because the system provided libraries were inconsistent at best (hence why many but not all 5.2 systems don't support bcrypt, it depended on the build of libcrypt you linked against). Please don't make us re-live this inconsistency... Especially when it won't really solve the problem. Regarding password_verify() accepting crypt(), I consider it an implementation detail that it works. I know the RFC specifies it, but it specifies it not as a conceptual fact (that it will always be no more than a reason to be there), but more as an explanation for what it's currently doing. I would not rely on that fact. It may work today. It may work tomorrow, but it shouldn't be documented as such (as it's the complement of password_hash(), not the complement of crypt()). > As far as I'm aware, the only reason for not marking crypt() > E_DEPRECATED right now is for compatibility with external systems, and > as far as those go, changing a default won't effect anything. I want to reinforce that point, because it's spot on the money: I think crypt should live on. password_hash should be the way new systems are built, sure. But as you mention external systems, crypt should be a standard way of interacting with them (heck, that's what the lib was designed for). It shouldn't be a "if you're not using password_hash(), you're doing it wrong". It's "password_hash() should solve the majority of use-cases, but if you have a different need, there are other options". My $0.02 Anthony On Thu, Jul 17, 2014 at 1:50 PM, Adam Harvey <ahar...@php.net> wrote: > On 16 July 2014 23:16, Tjerk Meesters <tjerk.meest...@gmail.com> wrote: >> On Thu, Jul 17, 2014 at 10:25 AM, Yasuo Ohgaki <yohg...@ohgaki.net> wrote: >> >>> Hi Tjerk, >>> >>> On Thu, Jul 17, 2014 at 11:09 AM, Tjerk Meesters <tjerk.meest...@gmail.com >>> > wrote: >>> >>>> Why should `password_verify()` work on a hash that wasn't generated with >>>> `password_hash()`? The fact that it uses `crypt()` internally should not >>>> leak outside of its API, imho. >>> >>> >>> password_*() is designed as crypt() wrapper and this fact is documented >>> since it was released. >>> >>> Obsolete password hash is easy to verify with password_needs_rehash(). >>> Developers can check password database easily with password_needs_rehash(). >>> >> >> The documentation states that the `hash` argument to both >> `password_needs_rehash()` and `password_verify()` is: >> >> hash - A hash created by password_hash(). > > Whoa. Don't put too much stock in that — I think I made that up out of > whole cloth while trying to get _something_ into the manual for one of > the early alpha releases of 5.5, and it wasn't intended as a > restrictive statement. > >> Passing a value from your own crypt() implementation may work, but that >> shouldn't be relied upon. I certainly wouldn't classify it as a problem >> that should be fixed in the password api. > > The original RFC specified that both crypt() and password_hash() > hashes were accepted here, and that's probably what the documentation > should say too. > > Adam > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
