Hi,
This commit:
commit 03315d9625dc87515f1dfbf1cc7d53c4451b5ec9
Author: Pierre Joye pajoye@php.net
Date: Mon Jul 18 21:26:29 2011 +0000
- update blowfish to 1.2 (Solar Designer)
documented this hack:
$ 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.
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.
As to the hack above, I didn't review it closely (I disapprove of it
anyhow), but maybe it doesn't even work reliably when the code is
compiled in: some people are reporting that salts are padded with dots
(which correspond to all zero bits) and some that salts are padded with
'$' signs. I've just reproduced the latter on CentOS 7.1:
$ php -r 'echo crypt("", "$2a$05$0123456789"), "\n";'
$2a$05$0123456789$$$$$$$$$$$.UQ9ZZoltOie8zhLfF5KWLrJBeoos.S6
$ php -v
PHP 5.4.16 (cli) (built: Jun 23 2015 21:17:27)
Maybe there's an extra hack somewhere causing this, but I can't find it
easily. I'm not looking at the exact branch/package for CentOS, though.
Maybe it was a temporary PHP 5.4 thing.
While padding with dots (zero bits) produces valid bcrypt hash strings
(it's just that the input salt strings were invalid), padding with '$'
signs produces invalid bcrypt hash strings.
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
Hi Alexander,
Hi,
This commit:
commit 03315d9625dc87515f1dfbf1cc7d53c4451b5ec9
Author: Pierre Joye pajoye@php.net
Date: Mon Jul 18 21:26:29 2011 +0000- update blowfish to 1.2 (Solar Designer)documented this hack:
$ 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:
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?
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'scrypt()
for whatever hash types it has bundled code for - and not only because
of this issue.As to the hack above, I didn't review it closely (I disapprove of it
anyhow), but maybe it doesn't even work reliably when the code is
compiled in: some people are reporting that salts are padded with dots
(which correspond to all zero bits) and some that salts are padded with
'$' signs. I've just reproduced the latter on CentOS 7.1:$ php -r 'echo crypt("", "$2a$05$0123456789"), "\n";'
$2a$05$0123456789$$$$$$$$$$$.UQ9ZZoltOie8zhLfF5KWLrJBeoos.S6
$ php -v
PHP 5.4.16 (cli) (built: Jun 23 2015 21:17:27)Maybe there's an extra hack somewhere causing this, but I can't find it
easily. I'm not looking at the exact branch/package for CentOS, though.
Maybe it was a temporary PHP 5.4 thing.While padding with dots (zero bits) produces valid bcrypt hash strings
(it's just that the input salt strings were invalid), padding with '$'
signs produces invalid bcrypt hash strings.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 bycrypt(). 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.
--
Pierre
@pierrejoye | http://www.libgd.org
Hi Pierre,
$ 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: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'scrypt()
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 bycrypt(). 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