Home > Cryptography > PHP crypt() bug

PHP crypt() bug

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:

Description:
------------
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
php5-5.3.6.201108112132-94.1.x86_64

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.

About these ads
  1. No comments yet.
  1. No trackbacks yet.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Follow

Get every new post delivered to your Inbox.

%d bloggers like this: