# Asking for a review of crypt() allocation changes - Thread: https://externals.io/message/61037 - Original archive: https://news-web.php.net/php.internals/61037 - Started by Nikita Popov on 2012-06-29T12:43:10+00:00 - Messages: 4 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #61037 — Nikita Popov — 2012-06-29T12:43:10+00:00 URL: https://externals.io/message/61037#61037 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 http://git.php.net/?p=php-src.git;a=commitdiff;h=e6cf7d774519300c08399cae5bfba90e33749727 The complete code is here: http://lxr.php.net/xref/PHP_TRUNK/ext/standard/crypt.c#150 To prevent another `crypt()` mess, I'd really appreciate if some people familiar with the code could look over it. Nikita --- ## #61038 — Anthony Ferrara — 2012-06-29T12:56:59+00:00 URL: https://externals.io/message/61037#61038 · In reply to: #61037 Additionally, it appears that SHA256/512 are way overallocating the buffer. For SHA512: int needed = (sizeof(sha512_salt_prefix) - 1 + sizeof(sha512_rounds_prefix) + 9 + 1 + salt_in_len + 1 + 86 + 1); output = emalloc(needed); salt[salt_in_len] = '\0'; crypt_res = php_sha512_crypt_r(str, salt, output, needed); // snip memset(output, 0, needed); efree(output); The salt_in_len includes the salt_prefix and the rounds_prefix as well. Since salt_in_len (thanks to the allocations before hand) can be up to PHP_MAX_SALT_LEN http://lxr.php.net/xref/PHP_TRUNK/ext/standard/crypt.c#88 which is 123 characters, the output of the function can be no bigger than PHP_MAX_SALT_LEN. Therefore the "needed" variable can be replaced by PHP_MAX_SALT_LEN everywhere... output = emalloc(MAX_SALT_LEN); salt[salt_in_len] = '\0'; crypt_res = php_sha512_crypt_r(str, salt, output, MAX_SALT_LEN); // snip memset(output, 0, MAX_SALT_LEN); efree(output); We can do the same for sha256. But in that case, we'll be overallocating the buffer as well. Although not nearly as bad as we are now. Thoughts? Anthony > 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 > http://git.php.net/?p=php-src.git;a=commitdiff;h=e6cf7d774519300c08399cae5bfba90e33749727 > > The complete code is here: > http://lxr.php.net/xref/PHP_TRUNK/ext/standard/crypt.c#150 > > To prevent another `crypt()` mess, I'd really appreciate if some people > familiar with the code could look over it. > > Nikita --- ## #61043 — Rasmus Lerdorf — 2012-06-29T15:17:53+00:00 URL: https://externals.io/message/61037#61043 · In reply to: #61038 > Additionally, it appears that SHA256/512 are way overallocating the buffer. > > For SHA512: > > int needed = (sizeof(sha512_salt_prefix) - 1 > + sizeof(sha512_rounds_prefix) + 9 + 1 > + salt_in_len + 1 + 86 + 1); > output = emalloc(needed); > salt[salt_in_len] = '\0'; > crypt_res = php_sha512_crypt_r(str, salt, output, needed); > // snip > memset(output, 0, needed); > efree(output); > > The salt_in_len includes the salt_prefix and the rounds_prefix as well. > > Since salt_in_len (thanks to the allocations before hand) can be up to > PHP_MAX_SALT_LEN > http://lxr.php.net/xref/PHP_TRUNK/ext/standard/crypt.c#88 which is 123 > characters, the output of the function can be no bigger than > PHP_MAX_SALT_LEN. Therefore the "needed" variable can be replaced by > PHP_MAX_SALT_LEN everywhere... > > output = emalloc(MAX_SALT_LEN); > salt[salt_in_len] = '\0'; > crypt_res = php_sha512_crypt_r(str, salt, output, MAX_SALT_LEN); > // snip > memset(output, 0, MAX_SALT_LEN); > efree(output); > > We can do the same for sha256. But in that case, we'll be > overallocating the buffer as well. Although not nearly as bad as we > are now. > > Thoughts? We need to get into the habit of using safe_emalloc() on any emalloc() that takes its size from a user-provided argument. -Rasmus --- ## #61052 — keisial@gmail.com — 2012-06-30T07:38:06+00:00 URL: https://externals.io/message/61037#61052 · In reply to: #61037 > 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 ) + 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.