PHP mhash_keygen_s2k() 64 bits bug.

Hi folks,

Since it's the month of PHP security, I've tried to run my old PHP fucking fuzzer against the new versions of PHP and I've found an interesting security bug on 64 bits arch. Here it is.

The bug is located in function zif_mhash_keygen_s2k() in ext/hash/hash.c where we have :

740 PHP_FUNCTION(mhash_keygen_s2k)
741 {
742 long algorithm, bytes; [0]
751 if (bytes <= 0){ [1]
752 php_error_docref(NULL TSRMLS_CC, E_WARNING, "the byte parameter must be greater than 0");
753 RETURN_FALSE;
754 }
(...)
796 ZVAL_STRINGL(z, key, bytes, 1); [2]

ZVAL_STRINGL macro code is :

544 #define ZVAL_STRINGL(z, s, l, duplicate) { \
545 const char *__s=(s); int __l=l; \ [3]
547 Z_STRVAL_P(z) = (duplicate?estrndup(__s, __l):(char*)__s);\

estrndup() is defined in Zend/zend_alloc.c :

2490 ZEND_API char *_estrndup(const char *s, uint length ZEND_FILE_LINE_DC ZEND_FILE_LINE_ORIG_DC) {
2493 p = (char *) _emalloc(length+1 ZEND_FILE_LINE_RELAY_CC [4]
(...)
2497 memcpy(p, s, length); [5]

At [0] we have the variable bytes declared as long. This variable is filled with the last parameter of the mhash_keygen_s2k() PHP function. At [1] we are checking if this variable is less or equal to 0 which is not permitted. On 64 bits, long is 64 bits so we can pass 0x00000000FFFFFFFF (-1 value of a 32 bits integer) and check is passed.

On [2] we call ZVAL_STRINGL(), this macro casts [3] the bytes variable as an int, so at this moment __l is equal to -1. Then [4] estrndup() is called which does a malloc(__l + 1) which is malloc(0) in our case and permitted. Then we have [5] a call to memcpy() to copy __l bytes into the freshly 0 malloc'ed buffer and sbing heap overflow occurs. :-)

To reproduce this overflow, we can use this script :

<?php mhash_keygen_s2k(1, "coin", 867673168, 4294967295); ?>

Code execution seems possible from different code paths. Also, key is indirectly controlled by the user. Unfortunately I currently have no time to play with it and to confirm this. Geeeeeeeez... :-(

PHP security folks have fixed this vulnerability by switching bytes type from long to int.

Others possible vulnerabilities, not found during the MoPS, are currently under analyze, more info soon. ;-)