Hi Pierre, On Mon, Mar 28, 2016 at 02:22:13PM +0700, Pierre Joye wrote: > On Sun, Mar 27, 2016 at 10:16 PM, Solar Designer <so...@openwall.com> wrote: > > $ git show 03315d9625dc87515f1dfbf1cc7d53c4451b5ec9 | fgrep -i hack > > + if (tmp == '$') break; /* PHP hack */ \ > > + while (dptr < end) /* PHP hack */ > > > > I vaguely recall a PHP developer (Pierre?) mentioning this at the time, > > and I guess I didn't object strongly enough - perhaps because the hack > > itself was in there before. > > I cannot remember why I added this comment there but as far as I can > tell now this code was present before, actually since the very first > version I use when I first make blowfish always available for PHP > using your implementation. See: > > https://github.com/php/php-src/commit/1e820eca02dcf322b41fd2fe4ed2a6b8309f8ab5#diff-243b169f3d5e753b5314e8eb994e36bc > > I cannot say why it is specific to PHP. Is it something that may have > been present in your implementation earlier and then removed but kept > in PHP for some BC reasons?
No. As far as I'm aware, this always was a PHP-only hack. IIRC, there might have been a wrong example with too-short bcrypt salt on the PHP crypt() function documentation page back then (2008?), and maybe the code was adapted to match that. As to how that example worked before, I suspect it might have worked through exploiting *BSD implementations' lack of strict checking on salt decoding, when PHP was running on such systems. If so, the padding wasn't necessarily zeroes - it was uninitialized stack data - issue fixed in OpenBSD 2 years ago, rejecting such invalid salts: http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libc/crypt/bcrypt.c.diff?r1=1.37&r2=1.38 - decode_base64(csalt, BCRYPT_MAXSALT, salt); + if (decode_base64(csalt, BCRYPT_MAXSALT, salt)) + return -1; However, PHP isn't actually better in that respect, with its '.' vs. '$' padding discrepancy across versions/builds. I still don't know where exactly this discrepancy comes from. > > Well, this PHP-specific behavior confuses people in several ways. It's > > especially weird that the behavior may be seen or may be gone on the > > same PHP version depending on whether it uses PHP's bundled (and hacked > > as above) copy of crypt_blowfish or an implementation of bcrypt provided > > by the underlying system. Maybe PHP should not use the system's crypt() > > for whatever hash types it has bundled code for - and not only because > > of this issue. > > In practical terms, padding with dots (zero bits) is an unpleasant > > surprise for people who choose to store their invalid salts and hashes > > into separate database fields (and then authentication stops working > > after some upgrade or migration, where the new system provides its > > non-hacked bcrypt), and padding with '$' signs is also a similar > > unpleasant surprise for people who store the full hash strings as > > returned by crypt(). While those strings might work in the same way > > with some bcrypt implementations, they may also be rejected (ideally, > > like upstream crypt_blowfish does) or processed differently. > > > > I'm not sure whether/how PHP should recover from this now. Alexander -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
