FAQ
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

Search Discussions

  • Stas Malyshev at Jul 21, 2011 at 1:21 am
    Hi!
    On 7/19/11 4:44 PM, Solar Designer wrote:
    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
  • Solar Designer at Jul 21, 2011 at 10:08 pm

    On Wed, Jul 20, 2011 at 06:21:16PM -0700, Stas Malyshev wrote:
    On 7/19/11 4:44 PM, Solar Designer wrote:
    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
  • Stas Malyshev at Jul 31, 2011 at 9:43 pm
    Hi!
    On 7/19/11 4:44 PM, Solar Designer wrote:
    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
  • Solar Designer at Jul 31, 2011 at 10:33 pm
    Hi Stas, Pierre -
    On Sun, Jul 31, 2011 at 02:43:12PM -0700, Stas Malyshev wrote:
    On 7/19/11 4:44 PM, Solar Designer wrote:
    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
  • Solar Designer at Jul 31, 2011 at 10:54 pm

    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
  • Solar Designer at Aug 1, 2011 at 12:24 am

    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
    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
  • Stas Malyshev at Jul 31, 2011 at 11:05 pm
    Hi!
    On 7/31/11 3:33 PM, Solar Designer wrote:
    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
  • Pierre Joye at Aug 22, 2011 at 1:05 pm
    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?
    On Wed, Jul 20, 2011 at 1:44 AM, Solar Designer wrote:
    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

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php


    --
    Pierre

    @pierrejoye | http://blog.thepimp.net | http://www.libgd.org
  • Ferenc Kovacs at Aug 22, 2011 at 1:19 pm
    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

    On Mon, Aug 22, 2011 at 3:05 PM, Pierre Joye wrote:
    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?
    On Wed, Jul 20, 2011 at 1:44 AM, Solar Designer wrote:
    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

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php


    --
    Pierre

    @pierrejoye | http://blog.thepimp.net | http://www.libgd.org

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php


    --
    Ferenc Kovács
    @Tyr43l - http://tyrael.hu
  • Solar Designer at Aug 22, 2011 at 1:52 pm

    Definitely.
    On Mon, Aug 22, 2011 at 3:05 PM, Pierre Joye wrote:
    it seems that the changes break BC too, pls see
    https://bugs.php.net/bug.php?id=55477
    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
  • Pierre Joye at Aug 22, 2011 at 2:01 pm

    On Mon, Aug 22, 2011 at 3:52 PM, Solar Designer wrote:
    Definitely.
    On Mon, Aug 22, 2011 at 3:05 PM, Pierre Joye wrote:
    it seems that the changes break BC too, pls see
    https://bugs.php.net/bug.php?id=55477
    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 :)
  • Solar Designer at Aug 22, 2011 at 3:36 pm

    On Mon, Aug 22, 2011 at 04:01:46PM +0200, Pierre Joye wrote:
    On Mon, Aug 22, 2011 at 3:52 PM, Solar Designer wrote:
    On Mon, Aug 22, 2011 at 3:05 PM, Pierre Joye wrote:
    it seems that the changes break BC too, pls see
    https://bugs.php.net/bug.php?id=55477
    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.
    I just did - I added a more verbose comment, though. I think you may
    use this for documentation:

    ---
    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).

    In versions of PHP older than 5.3.7, $2a$ inadvertently resulted in
    system-specific behavior for passwords with non-ASCII characters in them. On
    some installs (mostly on PowerPC and ARM, as well as sometimes on *BSD's and
    Solaris regardless of CPU architecture), they were processed correctly. On
    most installs (most Linux, many others), they were processed incorrectly most
    of the time (but not always), and moreover in a way where security was
    weakened.

    In PHP 5.3.7, $2a$ results in almost the correct behavior, but with an
    additional countermeasure against security-weakened old hashes mentioned above.
    $2x$ results in the buggy behavior, so if old hashes are known to be of the
    buggy type, this may be used on them to keep them working, accepting the
    associated security risk.

    $2y$ results in perfectly correct behavior (without the countermeasure), so it
    may be used (if desired) when hashing newly set passwords. For practical
    purposes, it does not really matter if you use $2a$ or $2y$ for newly set
    passwords, as the countermeasure is only triggered on some obscure passwords
    (not even valid UTF-8) that are unlikely to be seen outside of a deliberate
    attack (trying to match hashes produced by buggy pre-5.3.7 code).

    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.

    Summary: for passwords without characters with the 8th bit set, there's no
    issue, all three prefixes work exactly the same. For occasional passwords with
    characters with the 8th bit set, if the app prefers security and correctness
    over backwards compatibility, no action is needed - just upgrade to new PHP and
    use its new behavior (with $2a$). However, if an app install admin truly
    prefers backwards compatibility over security, and the problem is seen on the
    specific install (which is not always the case because not all platforms/builds
    were affected), then $2a$ in existing hashes in the database may be changed to
    $2x$. Alternatively, a similar thing may be achieved by changing $2a$ to $2x$
    in PHP app code after database queries, and using $2y$ on newly set passwords
    (such that the app's automatic change to $2x$ on queries is not triggered for
    them).
    ---

    BTW, the discrepancy in formatting for comment preview and actually
    posted comments is nasty.

    Alexander
  • Hannes Magnusson at Aug 23, 2011 at 9:31 am

    2011/8/22 Solar Designer <solar@openwall.com>:
    On Mon, Aug 22, 2011 at 04:01:46PM +0200, Pierre Joye wrote:
    On Mon, Aug 22, 2011 at 3:52 PM, Solar Designer wrote:
    On Mon, Aug 22, 2011 at 3:05 PM, Pierre Joye wrote:
    it seems that the changes break BC too, pls see
    https://bugs.php.net/bug.php?id=55477
    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.
    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
  • Solar Designer at Aug 23, 2011 at 10:31 am

    On Tue, Aug 23, 2011 at 11:31:02AM +0200, Hannes Magnusson wrote:
    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
  • Hannes Magnusson at Aug 23, 2011 at 10:52 am

    On Tue, Aug 23, 2011 at 12:30, Solar Designer wrote:
    On Tue, Aug 23, 2011 at 11:31:02AM +0200, Hannes Magnusson wrote:
    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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedJul 19, '11 at 11:44p
activeAug 23, '11 at 10:52a
posts16
users5
websitephp.net

People

Translate

site design / logo © 2022 Grokbase