Date: Fri, 4 May 2012 19:31:51 +0400 From: Solar Designer <solar@...nwall.com> To: oss-security@...ts.openwall.com Subject: Debian/Ubuntu php_crypt_revamped.patch Hi, Boaz Rymland reported to me what he thought was a phpass bug, where any password would be valid when authenticated against a NULL or empty password hash. (Indeed, the password hash shouldn't normally be NULL or empty, but it is better for authentication code to be fail-close rather than fail-open.) At first, I was not able to reproduce the problem, but after exchanging a few e-mails we were able to narrow it down to PHP crypt() call returning an empty string when called with NULL or an empty string for the salt argument on Boaz' Ubuntu 11.04 system with php5 5.3.5-1ubuntu7.7. I was still not able to reproduce that on other systems (with other versions of PHP). Today, I downloaded and built clean PHP 5.3.5 on an Owl system. I still could not trigger the problem. Then I applied debian/patches/php_crypt_revamped.patch from Debian's php5_5.3.5-1.diff.gz - and the problem finally appeared. Original: php@owl:~ $ ~/php-5.3.5/bin/php -r 'echo crypt("pass", null), "\n";' $1$l5Nwx5hu$NhostJ7i8jP1B.4C4zaiM78. With Debian patch: php@owl:~ $ ~/php-5.3.5-debian/bin/php -r 'echo crypt("pass", null), "\n";' php@owl:~ $ (empty string was printed). It turns out that the patch first appeared in Debian's 5.3.2-1 in response to almost a non-issue (different behavior across PHP versions for an invalid salt string) and general feeling that PHP should be using system-provided crypto instead of its bundled code when possible (questionable to me: each approach has its pros and cons): http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=572601 The handling of NULL/empty salt strings was corrected in 5.3.6-1, as well as in 5.3.3-7+squeeze4 (stable-security): http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=581170 Apparently, that fix never made it into Ubuntu 11.04 updates - so I guess this should happen now. Overall, the patch looks problematic to me. Here's another problem with it, still reproducible on 5.3.10-1ubuntu3 (Ubuntu 12.04): user@...ntu:~$ php -r 'echo crypt("pass", "_J9..Salt"), "\n";' _J9..Saltr2Hq6I3ZH0s user@...ntu:~$ php -r 'echo crypt("pass", "_J9..Saltr2Hq6I3ZH0s"), "\n";' _J0LlWX63dRZg That non-security bug is with the "salt_len == 9" check added with the patch. So phpass' authentication against CRYPT_EXT_DES hashes, which it tries to support, would be failing on Debian/Ubuntu systems. I guess I need to introduce a workaround for it now, complicating the code. :-( I think it may be best to drop this patch from further versions of Debian/Ubuntu - and not reintroduce it even in response to the likely "bug" reports from Debian users complaining about the behavior change from previous versions of Debian. I agree that the code in upstream PHP may need improvement, but that patch does not improve it, and the deviation from upstream is bad. Altering the behavior of PHP on specific distros beyond what may normally happen due to PHP's ./configure is undesirable and should only be done for very good reasons. Sorry for the rant. Alexander
Powered by blists - more mailing lists
Please check out the Open Source Software Security Wiki, which is counterpart to this mailing list.
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.