CRYPT_SHA256 fails tests in trunk

Expected:<$5$saltstring$5B8vYYiY.CVt1RlTTf8KbXBH3hsxY/GNooZaBBGWEc5>
Got<$5$saltst$JTS/fkywz8NvjeCGmWDndJPi7ZrRFhQKBLNtQZWE2C3>
[...]
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

Hi Stas, Pierre -

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

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

we expected this imo.
internals@lists.php.net/msg51683.html" rel="nofollow" target="_blank">http://www.mail-archive.com/internals@lists.php.net/msg51683.html
internals@lists.php.net/msg51687.html" rel="nofollow" target="_blank">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?

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

--

--
Ferenc Kovács
@Tyr43l - http://tyrael.hu

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.

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