Re: CRYPT_SHA256 fails tests in trunk

On Mon, Aug 01, 2011 at 02:54:29AM +0400, Solar Designer wrote: > 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~�D� > > > MzD,�=�u > > > > 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? I'm sorry, I misattributed the commit. It was by Ilia, "Fixed bug relating to un-initialized memory access". So I guess it was about the strlen(), unless there's another place accessing beyond salt_len as well. Alexander