Re: CRYPT_SHA256 fails tests in trunk

On Mon, Aug 01, 2011 at 02:33:27AM +0400, Solar Designer wrote: > On Sun, Jul 31, 2011 at 02:43:12PM -0700, Stas Malyshev wrote: > > The change that introduced this problem is: > > > > http://svn.php.net/viewvc/php/php-src/branches/PHP_5_3/ext/standard/crypt_sha256.c?r1=300427&r2=312952 > > Now that I look at this, I think there are more problems around this > place in the code: > > 1. Pierre's commit that inadvertently introduced the truncation was a > bugfix, adding the previously missed NUL termination of the copied salt. > However, the similar piece of code copying the key appears to still lack > the NUL termination (it works by pure luck). Upon a closer look, the NUL termination might not be needed - neither for the key nor for the salt. Ulrich Drepper's reference implementation similarly does not NUL-terminate these, and does not appear to rely on NUL termination in further code. Has the code in PHP somehow deviated from this convention? Pierre - what was the reason for your commit? ...Oh, I think I see the problem: __php_stpncpy() is not sufficiently correct. It starts by calling strlen(src). Maybe fixing it to more closely match the real thing would be a cleaner fix? Either way, crypt_sha256.c and crypt_sha512.c need to be made similar in this respect. Right now, they are not. > 2. alloca() of potentially user-controlled size is unsafe - it may > result in the stack pointer being moved outside of allowable range and > onto other areas, such as the heap. With a large enough allocation, > it'd jump over the few guard pages there might be. In the worst case, > this will ultimately allow for arbitrary machine code execution via an > overly long password. (BTW, we need to check glibc for similar issues.) This problem appears to be present in Ulrich Drepper's reference implementation as well. > We also need to check the MD5-crypt code for this. No alloca() in the code in ext/standard/php_crypt_r.c. Instead, it assumes that the MD5 implementation itself is able to handle unaligned inputs, which it is. Alexander