Archive for August, 2011

PHP crypt() bug

August 22, 2011 Leave a comment

A serious (and kind of funny) bug was reported in PHP crypt() today. Under certain circumstances, crypt() with an MD5 salt would only return the salt (no hash of the password). The bug was introduced on 7 Aug by rasmus.

Take a look at the diff:

That’s right, only one line was changed. Can you spot the problem (vulnerable code on the left)?

Take a look at the man page for strlcat, specifically, what is the third parameter? “Unlike those functions [strncpy and strncat], strlcpy() and strlcat() take the full size of the buffer (not just the length) and guarantee to NUL-terminate the result (as long as size is larger than 0 or, in the case of strlcat(), as long as there is at least one byte free in dst).” So, line 380 in the incorrect code is saying that passwd is only one byte in length.This messes everything up which leads to:

If crypt() is executed with MD5 salts, the return value conists of the salt only.
DES and BLOWFISH salts work as expected.

I tested with php from openSUSE PHP5 repository

> php -v
PHP 5.3.7RC6-dev (cli)
> rpm -q php5

Test script:
printf("MD5: %s\n", crypt('password', '$1$U7AjYB.O$'));

Expected result:
MD5: $1$U7AjYB.O$L1N7ux7twaMIMw0En8UUR1

Actual result:
MD5: $1$U7AjYB.O


So, what is the takeaway? If you look at the commit logs in svn, the bug actually started before the commit shown in the diff above. The author of that change changed a strcat to a strncat in order to “Make static analyzers happy”. The very next commit changed the strncat to strlcat with the comment “…let’s use strlcpy/strlcat instead for these static string copies”. NOTE: same author for both of those commits. So, it was a combination of trying to make a static analyzer happy and not understanding the difference between two function calls. The real kicker though is the most recent commit that fixes the bugs. It has the following commit message “If you want to remove static analyzer messages, be my guest, but please run unit tests after”. It turns out that the PHP unit tests would have found this bug, but someone didn’t run them before committing a fix to the repository. An interesting lesson in software development.