Summary
Malformatted BCrypt hashes that include a $ within their salt part will trigger a buffer overread and may erroneously validate any password as valid.
Details
Example:
<?php
var_dump(password_verify("foo", '$2y$04$00000000$')); // bool(true)
?>This issue is caused by a PHP specific modification to the crypt_blowfish implementation that is fittingly named “PHP hack”:
|
if (tmp == '$') break; /* PHP hack */ \ |
The “hack” will allow a salt that is cut short by $ to be detected as valid and allowing it to proceed with the algorithm. At the end the original setting, which is properly NUL terminated, will be copied into the output buffer:
|
memcpy(output, setting, 7 + 22 - 1); |
which is then used to initialize the returned zend_string by leveraging strlen() and thus cutting the string short after the setting:
|
result = zend_string_init(output, strlen(output), 0); |
… thus returning the original input.
Furthermore a buffer overread of the setting input may be triggered when copying the setting into the output buffer, because the memcpy uses fixed sizes, even if the setting might be too short:
|
memcpy(output, setting, 7 + 22 - 1); |
Suggested Fix
The suggested fix would be removing the “PHP Hack” and thus the differences between PHP's crypt_blowfish and Openwall’s implementation which is the basis of PHP’s implementation.
The “PHP Hack” exists since the very first version of PHP’s own crypt_blowfish implementation and no clear reasoning is given for its existence in the commentary or commit history. In any case such a hash is not a valid BCrypt hash and it is not generated by password_hash(), which is the recommended password hashing API in PHP.
While this technically might break backwards compatibility with existing users, the fact that these hashes are never generated by password_hash(), are not accepted by other implementations of BCrypt and introduce possible security vulnerabilities in applications, they should be rejected in every supported PHP version.
PoC
Running the following in valgrind:
<?php
var_dump(password_verify("foo", '$2y$04$00000000$')); // bool(true)
?>results in:
==423829== Memcheck, a memory error detector
==423829== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==423829== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==423829== Command: sapi/cli/php test3.php
==423829==
==423829== Invalid read of size 2
==423829== at 0x4840240: memcpy@GLIBC_2.2.5 (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==423829== by 0x591BE9: BF_crypt (crypt_blowfish.c:763)
==423829== by 0x591DAD: php_crypt_blowfish_rn (crypt_blowfish.c:830)
==423829== by 0x5D76BC: php_crypt (crypt.c:143)
==423829== by 0x68CC98: php_password_bcrypt_verify (password.c:157)
==423829== by 0x68DD3C: zif_password_verify (password.c:635)
==423829== by 0x7A58C1: ZEND_DO_ICALL_SPEC_RETVAL_USED_HANDLER (zend_vm_execute.h:1295)
==423829== by 0x81F2AF: execute_ex (zend_vm_execute.h:55163)
==423829== by 0x824B54: zend_execute (zend_vm_execute.h:59523)
==423829== by 0x769201: zend_execute_scripts (zend.c:1694)
==423829== by 0x6B0B56: php_execute_script (main.c:2545)
==423829== by 0x869719: do_cli (php_cli.c:949)
==423829== Address 0x704da90 is 0 bytes after a block of size 48 alloc'd
==423829== at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==423829== by 0x7286E6: __zend_malloc (zend_alloc.c:3056)
==423829== by 0x726FFF: _malloc_custom (zend_alloc.c:2418)
==423829== by 0x7271C0: _emalloc (zend_alloc.c:2537)
==423829== by 0x840E4B: zend_string_alloc (zend_string.h:144)
==423829== by 0x840EBE: zend_string_init (zend_string.h:166)
==423829== by 0x841BCB: zend_new_interned_string_request (zend_string.c:245)
==423829== by 0x72B5E9: zval_make_interned_string (zend_compile.c:514)
==423829== by 0x72B664: zend_insert_literal (zend_compile.c:526)
==423829== by 0x72B7EC: zend_add_literal (zend_compile.c:547)
==423829== by 0x72FC82: zend_emit_op (zend_compile.c:2054)
==423829== by 0x73380D: zend_compile_args (zend_compile.c:3516)
==423829==
==423829== Invalid read of size 1
==423829== at 0x591BF5: BF_crypt (crypt_blowfish.c:765)
==423829== by 0x591DAD: php_crypt_blowfish_rn (crypt_blowfish.c:830)
==423829== by 0x5D76BC: php_crypt (crypt.c:143)
==423829== by 0x68CC98: php_password_bcrypt_verify (password.c:157)
==423829== by 0x68DD3C: zif_password_verify (password.c:635)
==423829== by 0x7A58C1: ZEND_DO_ICALL_SPEC_RETVAL_USED_HANDLER (zend_vm_execute.h:1295)
==423829== by 0x81F2AF: execute_ex (zend_vm_execute.h:55163)
==423829== by 0x824B54: zend_execute (zend_vm_execute.h:59523)
==423829== by 0x769201: zend_execute_scripts (zend.c:1694)
==423829== by 0x6B0B56: php_execute_script (main.c:2545)
==423829== by 0x869719: do_cli (php_cli.c:949)
==423829== by 0x86A950: main (php_cli.c:1337)
==423829== Address 0x704da94 is 4 bytes after a block of size 48 alloc'd
==423829== at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==423829== by 0x7286E6: __zend_malloc (zend_alloc.c:3056)
==423829== by 0x726FFF: _malloc_custom (zend_alloc.c:2418)
==423829== by 0x7271C0: _emalloc (zend_alloc.c:2537)
==423829== by 0x840E4B: zend_string_alloc (zend_string.h:144)
==423829== by 0x840EBE: zend_string_init (zend_string.h:166)
==423829== by 0x841BCB: zend_new_interned_string_request (zend_string.c:245)
==423829== by 0x72B5E9: zval_make_interned_string (zend_compile.c:514)
==423829== by 0x72B664: zend_insert_literal (zend_compile.c:526)
==423829== by 0x72B7EC: zend_add_literal (zend_compile.c:547)
==423829== by 0x72FC82: zend_emit_op (zend_compile.c:2054)
==423829== by 0x73380D: zend_compile_args (zend_compile.c:3516)
==423829==
bool(true)
==423829==
==423829== HEAP SUMMARY:
==423829== in use at exit: 1,094 bytes in 20 blocks
==423829== total heap usage: 15,723 allocs, 15,703 frees, 2,766,336 bytes allocated
==423829==
==423829== LEAK SUMMARY:
==423829== definitely lost: 0 bytes in 0 blocks
==423829== indirectly lost: 0 bytes in 0 blocks
==423829== possibly lost: 0 bytes in 0 blocks
==423829== still reachable: 1,094 bytes in 20 blocks
==423829== suppressed: 0 bytes in 0 blocks
==423829== Rerun with --leak-check=full to see details of leaked memory
==423829==
==423829== For lists of detected and suppressed errors, rerun with: -s
==423829== ERROR SUMMARY: 3 errors from 2 contexts (suppressed: 0 from 0)
Impact
Applications that validate passwords with malformed or untrusted hashes might be affected by returning every password as valid for affected hashes.
TimWolla Analyst
Summary
Malformatted BCrypt hashes that include a
$within their salt part will trigger a buffer overread and may erroneously validate any password as valid.Details
Example:
This issue is caused by a PHP specific modification to the crypt_blowfish implementation that is fittingly named “PHP hack”:
php-src/ext/standard/crypt_blowfish.c
Line 374 in 2740920
The “hack” will allow a salt that is cut short by
$to be detected as valid and allowing it to proceed with the algorithm. At the end the originalsetting, which is properly NUL terminated, will be copied into the output buffer:php-src/ext/standard/crypt_blowfish.c
Line 763 in 2740920
which is then used to initialize the returned
zend_stringby leveragingstrlen()and thus cutting the string short after thesetting:php-src/ext/standard/crypt.c
Line 137 in 2740920
… thus returning the original input.
Furthermore a buffer overread of the
settinginput may be triggered when copying thesettinginto the output buffer, because thememcpyuses fixed sizes, even if thesettingmight be too short:php-src/ext/standard/crypt_blowfish.c
Line 763 in 2740920
Suggested Fix
The suggested fix would be removing the “PHP Hack” and thus the differences between PHP's crypt_blowfish and Openwall’s implementation which is the basis of PHP’s implementation.
The “PHP Hack” exists since the very first version of PHP’s own crypt_blowfish implementation and no clear reasoning is given for its existence in the commentary or commit history. In any case such a hash is not a valid BCrypt hash and it is not generated by
password_hash(), which is the recommended password hashing API in PHP.While this technically might break backwards compatibility with existing users, the fact that these hashes are never generated by
password_hash(), are not accepted by other implementations of BCrypt and introduce possible security vulnerabilities in applications, they should be rejected in every supported PHP version.PoC
Running the following in valgrind:
results in:
Impact
Applications that validate passwords with malformed or untrusted hashes might be affected by returning every password as valid for affected hashes.