# todo: crypt_blowfish issue - Thread: https://externals.io/message/53583 - Original archive: https://news-web.php.net/php.internals/53583 - Started by Rasmus Lerdorf on 2011-06-26T08:36:42+00:00 - Messages: 12 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #53583 — Rasmus Lerdorf — 2011-06-26T08:36:42+00:00 URL: https://externals.io/message/53583#53583 See http://seclists.org/oss-sec/2011/q2/632 We are using this code in etc/standard/crypt_blowfish.c End of the day here for me, so if someone could go through that and apply the patch plus figure out the BC issues with the $2x$ stuff discussed here: http://seclists.org/oss-sec/2011/q2/636 I would appreciate it. -Rasmus --- ## #53584 — Pierre Joye — 2011-06-26T08:54:08+00:00 URL: https://externals.io/message/53583#53584 · In reply to: #53583 As far as I remember Stas was working on that, Stas? > See http://seclists.org/oss-sec/2011/q2/632 > We are using this code in etc/standard/crypt_blowfish.c > > End of the day here for me, so if someone could go through that and > apply the patch plus figure out the BC issues with the $2x$ stuff > discussed here: http://seclists.org/oss-sec/2011/q2/636 > I would appreciate it. > > -Rasmus > > -- > > > > -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #53586 — Stas Malyshev — 2011-06-26T20:38:49+00:00 URL: https://externals.io/message/53583#53586 · In reply to: #53584 Hi! > As far as I remember Stas was working on that, Stas? I wasn't yet doing anything as I was waiting for this matter to come to official resolution (on the list there, it looks like Solar Designer has not yet decided which road to take) and then have our solution match whatever is done there. I'll see what is going on there and if there's an official solution I'll add the patch to match it. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #53587 — Stas Malyshev — 2011-06-26T21:37:26+00:00 URL: https://externals.io/message/53583#53587 · In reply to: #53583 Hi! > See http://seclists.org/oss-sec/2011/q2/632 > We are using this code in etc/standard/crypt_blowfish.c > I've committed the patch for 5.4/trunk, not sure what to do about 5.3 since there's some BC breakage in the fix for old hashes. See the ML thread for more details. Any thoughts about if we want this in 5.3? -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #53588 — Pierre Joye — 2011-06-26T23:31:27+00:00 URL: https://externals.io/message/53583#53588 · In reply to: #53587 hi! I did not read the report, do you have the details about the breakage? It could be acceptable in 5.3. > Hi! > > > > […] > > I've committed the patch for 5.4/trunk, not sure what to do about 5.3 since > there's some BC breakage in the fix for old hashes. See the ML thread for > more details. Any thoughts about if we want this in 5.3? > -- > Stanislav Malyshev, Software Architect > SugarCRM: http://www.sugarcrm.com/ > (408)454-6900 ext. 227 > > -- > > > > -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #53589 — johannes@schlueters.de — 2011-06-26T23:36:03+00:00 URL: https://externals.io/message/53583#53589 · In reply to: #53588 > hi! > > I did not read the report, do you have the details about the breakage? > It could be acceptable in 5.3. If the hash changes everybody who stored encrypted passwords or such using the old format can't verify them anymore. My suggestion without looking really deep into these things: Change the default, and an "old_blowfish" for compatibility and advertise it ... not sure it's the best thing. johannes > > > […] > > > > -- > Pierre > > @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #53592 — Stas Malyshev — 2011-06-26T23:55:04+00:00 URL: https://externals.io/message/53583#53592 · In reply to: #53588 Hi! > hi! > > I did not read the report, do you have the details about the breakage? > It could be acceptable in 5.3. The problem is that if the string has 8-bit chars in it, the hash ignores certain characters, making the password much less secure and generating high quantity of collisions. It also makes it incompatible with OpenBSD hashing, which may be lesser problem, depending on the use, and likely is, since nobody reported it beforehand. The solution implemented in the original patch and 5.4 is to fix the new hash and introduce a new hash type (starting with $2x and not with $2a as regular blowfish hash) that would use old, buggy hash type. This allows people that need to have password compatibility to have it at the cost of modifications on their data (which is possible without knowing the original passwords since only the prefix changes), while keeping the algorithm secure. However, it still has a chance somebody's data won't work after the update if he had 8-bit data hashed with old `crypt()`. He would need either to re-hash or to change prefix from $2a to $2x. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #53591 — Stas Malyshev — 2011-06-27T00:00:25+00:00 URL: https://externals.io/message/53583#53591 · In reply to: #53589 Hi! > If the hash changes everybody who stored encrypted passwords or such > using the old format can't verify them anymore. The change will be only for 8-bit data though. > My suggestion without looking really deep into these things: Change the > default, and an "old_blowfish" for compatibility and advertise it ... > not sure it's the best thing. We could add some flag, etc. to it to trigger the old behavior - it needs to pass 1 as the last parameter to BF_set_key then. That would probably require some modifications to crypt_blowfish.c as currently php_crypt_blowfish_rn uses the salt to determine the algorithm. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #53607 — david.zuelke@bitextender.com — 2011-06-28T10:19:56+00:00 URL: https://externals.io/message/53583#53607 · In reply to: #53592 > However, it still has a chance somebody's data won't work after the update if he had 8-bit data hashed with old `crypt()`. He would need either to re-hash or to change prefix from $2a to $2x. IMO that's a fair trade-off; people could even implement this in their app code by replacing "$2a" with "$2x" for a transitional period in the hash if the comparison fails (and then simply re-hash the password again with $2a so it's secure). I'm volunteering to write the necessary code sample for the upgrading notes :p David --- ## #53610 — johannes@schlueters.de — 2011-06-28T12:26:42+00:00 URL: https://externals.io/message/53583#53610 · In reply to: #53607 > > > > […] > update if he had 8-bit data hashed with old `crypt()`. He would need > either to re-hash or to change prefix from $2a to $2x. > > IMO that's a fair trade-off; people could even implement this in their > app code by replacing "$2a" with "$2x" for a transitional period in > the hash if the comparison fails (and then simply re-hash the password > again with $2a so it's secure). I'm volunteering to write the > necessary code sample for the upgrading notes :p if people read it ... what might happen is that people test when upgrading (yay!) all tests and all work and then 1% of the users or so can't login anymore (with an european site for instance where 8bit characters might happen ...) johannes > David --- ## #53611 — david.zuelke@bitextender.com — 2011-06-28T12:34:37+00:00 URL: https://externals.io/message/53583#53611 · In reply to: #53610 > > > > […] > > if people read it ... what might happen is that people test when > upgrading (yay!) all tests and all work and then 1% of the users or so > can't login anymore (with an european site for instance where 8bit > characters might happen ...) That might happen, but it isn't a critical issue I think since the change does not produce unconsumable hashes or silently corrupt data in some other way. I think you're also overestimating the amount of people using bcrypt for password storage; most people unfortunately still use SHA1s (with or without a salt). As Stas said though, whatever the upstream implementation uses as a solution should be mirrored by PHP. The alternative would be to introduce a new hash algorithm code that only works in newer versions of PHP, which hurts portability (which is the major selling point of `crypt()`). Simply "breaking" old hashes (there's not gonna be many of them out there) with the ability to easily and transparently fix it without user interaction in userland code seems like a much better idea to me. David --- ## #53612 — Ferenc Kovacs — 2011-06-28T12:40:22+00:00 URL: https://externals.io/message/53583#53612 · In reply to: #53611 2011/6/28 David Zülke : > > > > […] > > That might happen, but it isn't a critical issue I think since the change does not produce unconsumable hashes or silently corrupt data in some other way. I think you're also overestimating the amount of people using bcrypt for password storage; most people unfortunately still use SHA1s (with or without a salt). > > As Stas said though, whatever the upstream implementation uses as a solution should be mirrored by PHP. The alternative would be to introduce a new hash algorithm code that only works in newer versions of PHP, which hurts portability (which is the major selling point of `crypt()`). Simply "breaking" old hashes (there's not gonna be many of them out there) with the ability to easily and transparently fix it without user interaction in userland code seems like a much better idea to me. > > David > > > it would be good if we could communicate this change more than mentioning in the changelog. Tyrael