Hi,
I released crypt_blowfish 1.2 earlier today:
http://www.openwall.com/lists/announce/2011/07/17/1
Since these updates are so important because of a bug of mine (sorry!),
I felt like saving PHP developers some time and updating both 5.3 and 5.4
to the new version myself. It turns out 5.4 was already updated to
crypt_blowfish 1.1 (thanks, Stas!), whereas 5.3 was still at 1.0.4.
The attached patches update both to 1.2. Please apply these before you
release 5.4 and 5.3.7 proper.
Obviously, I reviewed all changes you had made against the corresponding
versions of crypt_blowfish and I merged the relevant ones of those into
these patches.
Oh, one thing I did not add yet is additional test vectors from 1.2's
wrapper.c. You may add them, you may skip that, or you may ask me to
add them. Anyhow, the important thing is to update the crypt_blowfish
code itself (which now includes a quick self-test at runtime) before you
release the new versions of PHP. So I suggest that you start by
applying these patches as-is.
Another thing I forgot is the " (CVE-2011-2483)" reference in NEWS for
5.3 - please add that. It was not needed for 5.4 because of Stas'
earlier update to 1.1 (which already refers to the CVE).
Thanks,
Alexander
- For actual implementation, we set an array index in the variable "bug"
- (0 means no bug, 1 means sign extension bug emulation) and a flag in the
- variable "safety" (bit 16 is set when the safety measure is requested).
- Valid combinations of settings are:
- Prefix "$2a$": bug = 0, safety = 0x10000
- Prefix "$2x$": bug = 1, safety = 0
- Prefix "$2y$": bug = 0, safety = 0
If I'm understanding the change correctly, we should now be advising
users to transition their code to '$2y$' rather than '$2a$', with
perhaps a note mentioning the '$2x$' prefix for "transitioning users
with passwords that contain non-ASCII characters with the 8th bit set".
Obviously, any documentation change in this regard will need to be
pending on the version these patches get rolled into...
- For actual implementation, we set an array index in the variable "bug"
- (0 means no bug, 1 means sign extension bug emulation) and a flag in the
- variable "safety" (bit 16 is set when the safety measure is requested).
- Valid combinations of settings are:
- Prefix "$2a$": bug = 0, safety = 0x10000
- Prefix "$2x$": bug = 1, safety = 0
- Prefix "$2y$": bug = 0, safety = 0
If I'm understanding the change correctly, we should now be advising
users to transition their code to '$2y$' rather than '$2a$', with
Yes, but this is not terribly important. In practice, "$2a$" is almost
the same as "$2y$". For passwords that don't contain the '\xff'
character (which is not even valid in UTF-8 sequences), these two are
100% equivalent. For realistic passwords that do contain this
character, I had one "hit" in 150,000+ such passwords tested:
http://www.openwall.com/lists/oss-security/2011/07/08/1
So this is negligible, and even for the affected passwords (where "$2y$"
and "$2a$" hashes differ by more than just the prefix) this only matters
if those password hashes are ever migrated to other systems (non-PHP).
The reason why I went for this is that I consider the security advantage
of avoiding easy collisions with the buggy hashes non-negligible.
perhaps a note mentioning the '$2x$' prefix for "transitioning users
with passwords that contain non-ASCII characters with the 8th bit set".
We need to be careful here such that no one starts using this for newly
set passwords. This bit of documentation should be available to those
few who actually need it (I expect that most sites won't care), but
maybe it should not be on the function crypt()
documentation page.
Obviously, any documentation change in this regard will need to be
pending on the version these patches get rolled into...
Yes - need to release PHP versions with this code first.
Thanks,
Alexander
hi!
Thanks for the patches! Very welcome :)
Yes, but this is not terribly important. In practice, "$2a$" is almost
the same as "$2y$". For passwords that don't contain the '\xff'
character (which is not even valid in UTF-8 sequences), these two are
100% equivalent. For realistic passwords that do contain this
character, I had one "hit" in 150,000+ such passwords tested:http://www.openwall.com/lists/oss-security/2011/07/08/1
So this is negligible, and even for the affected passwords (where "$2y$"
and "$2a$" hashes differ by more than just the prefix) this only matters
if those password hashes are ever migrated to other systems (non-PHP).The reason why I went for this is that I consider the security advantage
of avoiding easy collisions with the buggy hashes non-negligible.
Makes full sense.
perhaps a note mentioning the '$2x$' prefix for "transitioning users
with passwords that contain non-ASCII characters with the 8th bit set".We need to be careful here such that no one starts using this for newly
set passwords. This bit of documentation should be available to those
few who actually need it (I expect that most sites won't care), but
maybe it should not be on the functioncrypt()
documentation page.Obviously, any documentation change in this regard will need to be
pending on the version these patches get rolled into...Yes - need to release PHP versions with this code first.
I think we should push this patch to 5.3 now as well, so it will be in
5.3.7, it is important enough.
Cheers,
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
Yes - need to release PHP versions with this code first.
I think we should push this patch to 5.3 now as well, so it will be in
5.3.7, it is important enough.
Got the OK for 5.3, I will apply the patches later today or tomorrow.
Or if anyone willing to do it already, pls go ahead :)
--
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
hi!
Thanks for the patches, applied to all active branches.
About the tests, it would be very good to have them ported as phpt. As
far as I remember I did that back then when I first ported it to php.
Cheers,
Hi,
I released crypt_blowfish 1.2 earlier today:
http://www.openwall.com/lists/announce/2011/07/17/1
Since these updates are so important because of a bug of mine (sorry!),
I felt like saving PHP developers some time and updating both 5.3 and 5.4
to the new version myself. It turns out 5.4 was already updated to
crypt_blowfish 1.1 (thanks, Stas!), whereas 5.3 was still at 1.0.4.The attached patches update both to 1.2. Please apply these before you
release 5.4 and 5.3.7 proper.Obviously, I reviewed all changes you had made against the corresponding
versions of crypt_blowfish and I merged the relevant ones of those into
these patches.Oh, one thing I did not add yet is additional test vectors from 1.2's
wrapper.c. You may add them, you may skip that, or you may ask me to
add them. Anyhow, the important thing is to update the crypt_blowfish
code itself (which now includes a quick self-test at runtime) before you
release the new versions of PHP. So I suggest that you start by
applying these patches as-is.Another thing I forgot is the " (CVE-2011-2483)" reference in NEWS for
5.3 - please add that. It was not needed for 5.4 because of Stas'
earlier update to 1.1 (which already refers to the CVE).Thanks,
Alexander
--
--
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
Hi,
Thanks for the patches, applied to all active branches.
Thank you!
About the tests, it would be very good to have them ported as phpt. As
far as I remember I did that back then when I first ported it to php.
No, your tests appear to be specific to PHP's peculiarities. The tests
ported from crypt_blowfish's wrapper.c only appear in 5.4, from
crypt_blowfish 1.1 - probably Stas ported them (thanks!)
Anyway, attached are patches adding the tests from crypt_blowfish 1.2 to
trunk, 5.4, and 5.3. These patches also fix a bug in crypt.c.
I did not port tests of behavior on error, though. Some of these are
redundant with other tests that you already have in other .phpt files,
others would not fit in the existing array() format - would require a
separate loop, which I was too lazy to add. The tests that I did not
port are these (from my wrapper.c):
{"*0", "", "$2a$03$CCCCCCCCCCCCCCCCCCCCC."},
{"*0", "", "$2a$32$CCCCCCCCCCCCCCCCCCCCC."},
{"*0", "", "$2z$05$CCCCCCCCCCCCCCCCCCCCC."},
{"*0", "", "$2`$05$CCCCCCCCCCCCCCCCCCCCC."},
{"*0", "", "$2{$05$CCCCCCCCCCCCCCCCCCCCC."},
{"*1", "", "*0"},
Alexander
Hi,
Please apply the patches from:
http://news.php.net/php.internals/54098
at least the crypt.c bugfix is a must to apply before releasing 5.3.7
and 5.4.0.
...
Anyway, attached are patches adding the tests from crypt_blowfish 1.2 to
trunk, 5.4, and 5.3. These patches also fix a bug in crypt.c.
Alexander
hi,
Hi,
Please apply the patches from:
http://news.php.net/php.internals/54098
at least the crypt.c bugfix is a must to apply before releasing 5.3.7
and 5.4.0.
The patches are applied already, they are in 5.3.7RC4 and should be in
5.4.0a3 next week.
However, I just added the test to 5.3's branch as it was missing there.
Thanks for the headup :)
...
Anyway, attached are patches adding the tests from crypt_blowfish 1.2 to
trunk, 5.4, and 5.3. These patches also fix a bug in crypt.c.Alexander
--
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
Pierre,
Thanks for the prompt response.
http://news.php.net/php.internals/54098
at least the crypt.c bugfix is a must to apply before releasing 5.3.7
and 5.4.0.The patches are applied already, they are in 5.3.7RC4 and should be in
5.4.0a3 next week.
The reason why I sent this reminder was precisely that I could not find
the patches in php5.3-201107310630 and php5.4-201107310630 (I downloaded
the -latest tarballs). Now I also downloaded php-5.3.7RC4.tar.bz2, and
indeed it does not have the patch either.
ext/standard/crypt.c in php-5.3.7RC4 has:
salt[2] == 'a' &&
which means that it doesn't support the new $2x$ and $2y$ prefixes.
In 5.4, that check is totally ridiculous (weird mix of ANDs with OR):
} else if (
salt[0] == '$' &&
salt[1] == '2' &&
(salt[2] != 'a' && salt[2] != 'x') ||
salt[3] == '$' &&
salt[4] >= '0' && salt[4] <= '3' &&
salt[5] >= '0' && salt[5] <= '9' &&
salt[6] == '$') {
Both were fixed by the patches I posted on July 19, but those patches
were not yet applied to these branches (as of yesterday). I did not
check trunk.
Am I missing something?
Alexander
Pierre,
Thanks for the prompt response.
http://news.php.net/php.internals/54098
at least the crypt.c bugfix is a must to apply before releasing 5.3.7
and 5.4.0.The patches are applied already, they are in 5.3.7RC4 and should be in
5.4.0a3 next week.The reason why I sent this reminder was precisely that I could not find
the patches in php5.3-201107310630 and php5.4-201107310630 (I downloaded
the -latest tarballs). Now I also downloaded php-5.3.7RC4.tar.bz2, and
indeed it does not have the patch either.ext/standard/crypt.c in php-5.3.7RC4 has:
salt[2] == 'a' &&
which means that it doesn't support the new $2x$ and $2y$ prefixes.
In 5.4, that check is totally ridiculous (weird mix of ANDs with OR):
} else if (
salt[0] == '$' &&
salt[1] == '2' &&
(salt[2] != 'a' && salt[2] != 'x') ||
salt[3] == '$' &&
salt[4] >= '0' && salt[4] <= '3' &&
salt[5] >= '0' && salt[5] <= '9' &&
salt[6] == '$') {Both were fixed by the patches I posted on July 19, but those patches
were not yet applied to these branches (as of yesterday). I did not
check trunk.Am I missing something?
It looks like your original patch did not change anything in crypt.c
For the record here, that's the commit using your patches:
http://svn.php.net/viewvc?view=revision&revision=313406
I see now the other patch posted on the 20th, I missed it and it
indeed fixes the checks in crypt.c :) I will apply it shortly!
Cheers,
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
It looks like your original patch did not change anything in crypt.c
That's correct. I overlooked the need to modify crypt.c initially.
I only got to that when adding the extra tests, at which time I also
found the incorrect check in 5.4's crypt.c from the upgrade to
crypt_blowfish 1.1 a month ago.
I see now the other patch posted on the 20th, I missed it and it
indeed fixes the checks in crypt.c :) I will apply it shortly!
Yes, please. Thanks!
Alexander
applied :)
Thanks again!
It looks like your original patch did not change anything in crypt.c
That's correct. I overlooked the need to modify crypt.c initially.
I only got to that when adding the extra tests, at which time I also
found the incorrect check in 5.4's crypt.c from the upgrade to
crypt_blowfish 1.1 a month ago.I see now the other patch posted on the 20th, I missed it and it
indeed fixes the checks in crypt.c :) I will apply it shortly!Yes, please. Thanks!
Alexander
--
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org