Re: Asking for a review of crypt() allocation changes

On 29/06/12 14:43, Nikita Popov wrote: > Hi internals! > > Anthony and me have been looking a lot at the crypt() code recently > and noticed that there are some strange things going on in the buffer > allocations for the sha algorithms. > > We did two commits to fix them up a bit: > > > http://git.php.net/?p=php-src.git;a=commitdiff;h=7e8276ca68fc622124d51d18e4f7b5cde3536de4 It took me a while to realise the problem being fixed. The bug is not the memset (as reported in bug 62443), which is using needed (got fixed in e6cf7d), or php_sha{256,512}_crypt_r (uses a null-terminated string), but the salt[salt_in_len] = '\0'; after allocating only strlen(salt). So that you would be accessing the position PHP_MAX_SALT_LEN of the array but have reserved only a few bytes. Just*sizeof*(sha512_rounds_prefix <http://lxr.php.net/xref/PHP_TRUNK/ext/standard/crypt.c#sha512_rounds_prefix>) + 9 + 1 seem enough for not making bug62443.phpt segfault. I have been able to crash it with var_dump( crypt("foo", '$6$'.chr(0). str_pad('', 500, '*') . '$abc') ); but only if it's the first call.