# CRYPT_SHA256 fails tests in trunk - Thread: https://externals.io/message/54099 - Original archive: https://news-web.php.net/php.internals/54099 - Started by Solar Designer on 2011-07-19T23:44:06+00:00 - Messages: 15 - 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 --- ## #54099 — Solar Designer — 2011-07-19T23:44:06+00:00 URL: https://externals.io/message/54099#54099 Hi, These tests fail in trunk on my x86_64 build: crypt_sha256.phpt crypt_variation1.phpt The differences are like this: Expected: <$5$saltstring$5B8vYYiY.CVt1RlTTf8KbXBH3hsxY/GNooZaBBGWEc5> Got <$5$saltst$JTS/fkywz8NvjeCGmWDndJPi7ZrRFhQKBLNtQZWE2C3> That is, the salts are truncated. There's a relevant recent change in crypt.c involving the line: salt_in_len = MIN(PHP_MAX_SALT_LEN, salt_in_len); I did not investigate this for real - I am merely passing the info that I readily have. I hope this helps. Alexander --- ## #54114 — Stas Malyshev — 2011-07-21T01:21:16+00:00 URL: https://externals.io/message/54099#54114 · In reply to: #54099 Hi! > Hi, > > These tests fail in trunk on my x86_64 build: > > crypt_sha256.phpt > crypt_variation1.phpt > > The differences are like this: > > Expected:<$5$saltstring$5B8vYYiY.CVt1RlTTf8KbXBH3hsxY/GNooZaBBGWEc5> > Got<$5$saltst$JTS/fkywz8NvjeCGmWDndJPi7ZrRFhQKBLNtQZWE2C3> > > That is, the salts are truncated. There's a relevant recent change in > crypt.c involving the line: Yes, we had buffer overflow error there since the buffer salt[] was PHP_MAX_SALT_LEN+1 but if salt was longer salt[salt_in_len] later wrote 0 into bad place. But for SHA max salt len should be something like 123, so I wonder how comes it got truncated in that case. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #54123 — Solar Designer — 2011-07-21T22:01:11+00:00 URL: https://externals.io/message/54099#54123 · In reply to: #54114 > > > […] [...] > Yes, we had buffer overflow error there since the buffer salt[] was > PHP_MAX_SALT_LEN+1 but if salt was longer salt[salt_in_len] later wrote > 0 into bad place. Is this buffer overflow still not fixed in 5.4, or does 5.4 also have the salt truncation bug above? Either way, it sounds like you need to figure this out and include a fix in 5.4, before 5.4 proper is released. Ditto for 5.3.7. > But for SHA max salt len should be something like 123, so I wonder how > comes it got truncated in that case. I trust that you'll figure it out. Thanks, Alexander --- ## #54267 — Stas Malyshev — 2011-07-31T21:43:12+00:00 URL: https://externals.io/message/54099#54267 · In reply to: #54099 Hi! > Hi, > > These tests fail in trunk on my x86_64 build: > > crypt_sha256.phpt > crypt_variation1.phpt > > The differences are like this: > > Expected:<$5$saltstring$5B8vYYiY.CVt1RlTTf8KbXBH3hsxY/GNooZaBBGWEc5> > Got<$5$saltst$JTS/fkywz8NvjeCGmWDndJPi7ZrRFhQKBLNtQZWE2C3> > > That is, the salts are truncated. There's a relevant recent change in > crypt.c involving the line: > > salt_in_len = MIN(PHP_MAX_SALT_LEN, salt_in_len); > Thanks for the report. This problem seems to be unrelated to this change, but in fact looks like it's related to this code in if ((salt - (char *) 0) % __alignof__(uint32_t) != 0) { char *tmp = (char *) alloca(salt_len + 1 + __alignof__(uint32_t)); salt = copied_salt = memcpy(tmp + __alignof__(uint32_t) - (tmp - (char *) 0) % __alignof__ (uint32_t), salt, salt_len); tmp[salt_len] = 0; } As you can see, the last line cuts the string relative to tmp, but salt is copied to tmp with offest, which leads to salt truncation. Changing it from tmp to copied_salt seems to fix the problem. I'll apply the fix in a minute. 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 -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #54268 — Solar Designer — 2011-07-31T22:33:27+00:00 URL: https://externals.io/message/54099#54268 · In reply to: #54267 Hi Stas, Pierre - > > > […] > > Thanks for the report. > This problem seems to be unrelated to this change, but in fact looks > like it's related to this code in You appear to be correct. > if ((salt - (char *) 0) % __alignof__(uint32_t) != 0) { > char *tmp = (char *) alloca(salt_len + 1 + > __alignof__(uint32_t)); > salt = copied_salt = > memcpy(tmp + __alignof__(uint32_t) - (tmp - (char *) 0) % > __alignof__ (uint32_t), salt, salt_len); > tmp[salt_len] = 0; > } > > As you can see, the last line cuts the string relative to tmp, but salt > is copied to tmp with offest, which leads to salt truncation. Changing > it from tmp to copied_salt seems to fix the problem. I'll apply the fix > in a minute. Right. > 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). 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.) You could move away from using alloca() there or introduce some sane length limits (less than one page). But then you need to decide on the proper action on failure. Another approach would be to avoid the copying, although it may have a performance impact (having to deal with potentially unaligned data inside loops), would involve more invasive changes, and thus would require testing and benchmarking - so probably not an option for the upcoming releases now (and maybe beyond scope of PHP development at all). Oh, there are two more alloca() calls further down the function - one of the salt (not an issue if we limit the salt length above), and one of the key (problematic). This needs to be dealt with as well. So perhaps you need to have the function return `NULL` right after the first strlen(key) if the length turns out to be excessive (more than 2048 bytes? which is half a page on x86). Then the wrapper code in crypt.c will translate this into "*0". crypt_sha512.c has similar issues - both with NUL termination and with unlimited alloca(). We also need to check the MD5-crypt code for this. I hope this helps... Alexander --- ## #54269 — Solar Designer — 2011-07-31T22:54:29+00:00 URL: https://externals.io/message/54099#54269 · In reply to: #54268 > > > […] > > 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 --- ## #54270 — Stas Malyshev — 2011-07-31T23:05:26+00:00 URL: https://externals.io/message/54099#54270 · In reply to: #54268 Hi! > Now that I look at this, I think there are more problems around this > place in the code: I just fixed the immediate problem, but giving a second look to this code I don't really understand why there should be `NULL` termination at all - we know the length anyway, and can use it directly. And underlying functions seem never to rely on null-termination. > 2. alloca() of potentially user-controlled size is unsafe - it may > result in the stack pointer being moved outside of allowable range and This is true. This code doesn't seem to have any limits on key length. We probably should add a check somewhere in crypt.c. I'll look into it soon. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 --- ## #54271 — Solar Designer — 2011-08-01T00:24:22+00:00 URL: https://externals.io/message/54099#54271 · In reply to: #54269 > > > […] > > 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 --- ## #54794 — Pierre Joye — 2011-08-22T13:05:44+00:00 URL: https://externals.io/message/54099#54794 · In reply to: #54099 hi, it seems that the changes break BC too, pls see https://bugs.php.net/bug.php?id=55477 Does that ring a bell to you? > Hi, > > These tests fail in trunk on my x86_64 build: > > crypt_sha256.phpt > crypt_variation1.phpt > > The differences are like this: > > Expected: <$5$saltstring$5B8vYYiY.CVt1RlTTf8KbXBH3hsxY/GNooZaBBGWEc5> > Got       <$5$saltst$JTS/fkywz8NvjeCGmWDndJPi7ZrRFhQKBLNtQZWE2C3> > > That is, the salts are truncated.  There's a relevant recent change in > crypt.c involving the line: > >                salt_in_len = MIN(PHP_MAX_SALT_LEN, salt_in_len); > > I did not investigate this for real - I am merely passing the info that > I readily have.  I hope this helps. > > Alexander > > -- > > > > -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #54796 — Ferenc Kovacs — 2011-08-22T13:19:53+00:00 URL: https://externals.io/message/54099#54796 · In reply to: #54794 we expected this imo. http://www.mail-archive.com/internals@lists.php.net/msg51683.html http://www.mail-archive.com/internals@lists.php.net/msg51687.html > hi, > > it seems that the changes break BC too, pls see > https://bugs.php.net/bug.php?id=55477 > > Does that ring a bell to you? > > > > […] > > > > -- > Pierre > > @pierrejoye | http://blog.thepimp.net | http://www.libgd.org > > -- > > > > -- Ferenc Kovács @Tyr43l - http://tyrael.hu --- ## #54798 — Solar Designer — 2011-08-22T13:52:10+00:00 URL: https://externals.io/message/54099#54798 · In reply to: #54796 > we expected this imo. > http://www.mail-archive.com/internals@lists.php.net/msg51683.html > http://www.mail-archive.com/internals@lists.php.net/msg51687.html Definitely. > > > […] We may recommend to Christian to change $2a$ in existing hashes to $2x$ if the goal is to preserve compatibility for all old passwords despite of the security risk associated with doing so. The change as implemented in PHP 5.3.7+ favors security and correctness over backwards compatibility, but it also lets users (admins of PHP app installs) use the new $2x$ prefix on existing hashes to preserve backwards compatibility for those and incur the associated security risk until all such passwords are changed (using $2a$ or $2y$ for newly changed passwords). No change to the PHP code is needed. BTW, this is not the right thread to discuss this on (the "bug" has nothing to do with CRYPT_SHA256). Alexander --- ## #54799 — Pierre Joye — 2011-08-22T14:01:46+00:00 URL: https://externals.io/message/54099#54799 · In reply to: #54798 > > > […] > > Definitely. > > > […] > > We may recommend to Christian to change $2a$ in existing hashes to $2x$ if > the goal is to preserve compatibility for all old passwords despite of > the security risk associated with doing so.  The change as implemented > in PHP 5.3.7+ favors security and correctness over backwards compatibility, > but it also lets users (admins of PHP app installs) use the new $2x$ > prefix on existing hashes to preserve backwards compatibility for those > and incur the associated security risk until all such passwords are > changed (using $2a$ or $2y$ for newly changed passwords). > > No change to the PHP code is needed. Can you add this comment to the bug please? So every user reading it will be informed. That's also something we have to document better. > BTW, this is not the right thread to discuss this on (the "bug" has > nothing to do with CRYPT_SHA256). Oops, reply to the wrong one, sorry :) -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #54820 — Hannes Magnusson — 2011-08-23T09:31:02+00:00 URL: https://externals.io/message/54099#54820 2011/8/22 Solar Designer : > > > […] > > I just did - I added a more verbose comment, though.  I think you may > use this for documentation: Added to http://php.net/security/crypt, and added a link from the release announcement and changelog. (should show up in an hour or two). -Hannes --- ## #54821 — Solar Designer — 2011-08-23T10:30:56+00:00 URL: https://externals.io/message/54099#54821 · In reply to: #54820 > Added to http://php.net/security/crypt, and added a link from the > release announcement and changelog. > (should show up in an hour or two). Thanks. I suggest the following three changes: 1. Change the title from "crypt() security fix details" to `CRYPT_BLOWFISH` security fix details" to avoid confusion with the CRYPT_MD5 problem inadvertently introduced in 5.3.7. 2. Remove this paragraph: BTW, PHP 5.3.7+ has been updated to crypt_blowfish 1.2, not the intermediate 1.1 release referenced in the previous comment. The differences between 1.1 and 1.2 include introduction of the countermeasure for $2a$ mentioned above and the $2y$ prefix. which made sense in the bug comments (after a preceding comment), but is unneeded here. 3. Maybe the URL should be .../crypt_blowfish rather than .../crypt, since there will definitely be more fixes/changes to PHP's `crypt()`, some of which might need their own release notes. It might be too late to make this change, though. Alexander --- ## #54822 — Hannes Magnusson — 2011-08-23T10:52:10+00:00 URL: https://externals.io/message/54099#54822 · In reply to: #54821 > > > […] > > Thanks.  I suggest the following three changes: > > 1. Change the title from "crypt() security fix details" to > `CRYPT_BLOWFISH` security fix details" to avoid confusion with the > CRYPT_MD5 problem inadvertently introduced in 5.3.7. done > 2. Remove this paragraph: > > BTW, PHP 5.3.7+ has been updated to crypt_blowfish 1.2, not the > intermediate 1.1 release referenced in the previous comment. The > differences between 1.1 and 1.2 include introduction of the > countermeasure for $2a$ mentioned above and the $2y$ prefix. > > which made sense in the bug comments (after a preceding comment), but is > unneeded here. done > > 3. Maybe the URL should be .../crypt_blowfish rather than .../crypt, > since there will definitely be more fixes/changes to PHP's `crypt()`, some > of which might need their own release notes.  It might be too late to > make this change, though. done. Added a fallback from /security/crypt to /security/crypt_blowfish for the time being. -Hannes