diff --git a/phpBB/config/password.yml b/phpBB/config/password.yml index 09e935016e..cb45ec3d42 100644 --- a/phpBB/config/password.yml +++ b/phpBB/config/password.yml @@ -101,6 +101,7 @@ services: arguments: - @request - @passwords.driver.salted_md5 + - @passwords.driver_helper - %core.root_path% - %core.php_ext% tags: diff --git a/phpBB/phpbb/passwords/driver/bcrypt.php b/phpBB/phpbb/passwords/driver/bcrypt.php index 23add37a56..eab1c3d569 100644 --- a/phpBB/phpbb/passwords/driver/bcrypt.php +++ b/phpBB/phpbb/passwords/driver/bcrypt.php @@ -68,7 +68,7 @@ class bcrypt extends base return false; } - if ($hash == $this->hash($password, $salt)) + if ($this->helper->string_compare($hash, $this->hash($password, $salt))) { return true; } diff --git a/phpBB/phpbb/passwords/driver/bcrypt_wcf2.php b/phpBB/phpbb/passwords/driver/bcrypt_wcf2.php index 2d6f897a7b..0eee98d7b7 100644 --- a/phpBB/phpbb/passwords/driver/bcrypt_wcf2.php +++ b/phpBB/phpbb/passwords/driver/bcrypt_wcf2.php @@ -78,7 +78,7 @@ class bcrypt_wcf2 extends base return false; } // Works for standard WCF 2.x, i.e. WBB4 and similar - return $hash === $this->bcrypt->hash($this->bcrypt->hash($password, $salt), $salt); + return $this->helper->string_compare($hash, $this->bcrypt->hash($this->bcrypt->hash($password, $salt), $salt)); } } } diff --git a/phpBB/phpbb/passwords/driver/helper.php b/phpBB/phpbb/passwords/driver/helper.php index 2b3ebce53a..caa65080ac 100644 --- a/phpBB/phpbb/passwords/driver/helper.php +++ b/phpBB/phpbb/passwords/driver/helper.php @@ -142,4 +142,24 @@ class helper } return $random; } + + /** + * Compare two strings byte by byte + * + * @param string $string_a The first string + * @param string $string_b The second string + * + * @return bool True if strings are the same, false if not + */ + public function string_compare($string_a, $string_b) + { + $difference = strlen($string_a) != strlen($string_b); + + for ($i = 0; $i < strlen($string_a) && $i < strlen($string_b); $i++) + { + $difference |= $string_a[$i] != $string_b[$i]; + } + + return $difference === 0; + } } diff --git a/phpBB/phpbb/passwords/driver/md5_mybb.php b/phpBB/phpbb/passwords/driver/md5_mybb.php index 61ea8dafd8..f631ceae78 100644 --- a/phpBB/phpbb/passwords/driver/md5_mybb.php +++ b/phpBB/phpbb/passwords/driver/md5_mybb.php @@ -54,7 +54,7 @@ class md5_mybb extends base else { // Works for myBB 1.1.x, 1.2.x, 1.4.x, 1.6.x - return $hash === md5(md5($user_row['user_passwd_salt']) . md5($password)); + return $this->helper->string_compare($hash, md5(md5($user_row['user_passwd_salt']) . md5($password))); } } } diff --git a/phpBB/phpbb/passwords/driver/md5_phpbb2.php b/phpBB/phpbb/passwords/driver/md5_phpbb2.php index 86a4b62ea5..bd8cc51e5a 100644 --- a/phpBB/phpbb/passwords/driver/md5_phpbb2.php +++ b/phpBB/phpbb/passwords/driver/md5_phpbb2.php @@ -23,6 +23,9 @@ class md5_phpbb2 extends base /** @var \phpbb\passwords\driver\salted_md5 */ protected $salted_md5; + /** @var \phpbb\passwords\driver\helper */ + protected $helper; + /** @var string phpBB root path */ protected $phpbb_root_path; @@ -34,13 +37,15 @@ class md5_phpbb2 extends base * * @param \phpbb\request\request $request phpBB request object * @param \phpbb\passwords\driver\salted_md5 $salted_md5 Salted md5 driver + * @param \phpbb\passwords\driver\helper $helper Driver helper * @param string $phpbb_root_path phpBB root path * @param string $php_ext PHP file extension */ - public function __construct($request, \phpbb\passwords\driver\salted_md5 $salted_md5, $phpbb_root_path, $php_ext) + public function __construct($request, salted_md5 $salted_md5, helper $helper, $phpbb_root_path, $php_ext) { $this->request = $request; $this->salted_md5 = $salted_md5; + $this->helper = $helper; $this->phpbb_root_path = $phpbb_root_path; $this->php_ext = $php_ext; } @@ -105,7 +110,7 @@ class md5_phpbb2 extends base include($this->phpbb_root_path . 'includes/utf/data/recode_basic.' . $this->php_ext); } - if (md5($password_old_format) === $hash || md5(\utf8_to_cp1252($password_old_format)) === $hash + if ($this->helper->string_compare(md5($password_old_format), $hash) || $this->helper->string_compare(md5(\utf8_to_cp1252($password_old_format)), $hash) || $this->salted_md5->check(md5($password_old_format), $hash) === true || $this->salted_md5->check(md5(\utf8_to_cp1252($password_old_format)), $hash) === true) { diff --git a/phpBB/phpbb/passwords/driver/md5_vb.php b/phpBB/phpbb/passwords/driver/md5_vb.php index c83c32a596..280b7114c7 100644 --- a/phpBB/phpbb/passwords/driver/md5_vb.php +++ b/phpBB/phpbb/passwords/driver/md5_vb.php @@ -54,7 +54,7 @@ class md5_vb extends base else { // Works for vB 3.8.x, 4.x.x, 5.0.x - return $hash === md5(md5($password) . $user_row['user_passwd_salt']); + return $this->helper->string_compare($hash, md5(md5($password) . $user_row['user_passwd_salt'])); } } } diff --git a/phpBB/phpbb/passwords/driver/salted_md5.php b/phpBB/phpbb/passwords/driver/salted_md5.php index 97a2b9154b..81ac010785 100644 --- a/phpBB/phpbb/passwords/driver/salted_md5.php +++ b/phpBB/phpbb/passwords/driver/salted_md5.php @@ -107,7 +107,7 @@ class salted_md5 extends base return md5($password) === $hash; } - return $hash === $this->hash($password, $hash); + return $this->helper->string_compare($hash, $this->hash($password, $hash)); } /** diff --git a/phpBB/phpbb/passwords/driver/sha1.php b/phpBB/phpbb/passwords/driver/sha1.php index 0852fd32fc..1abead42cd 100644 --- a/phpBB/phpbb/passwords/driver/sha1.php +++ b/phpBB/phpbb/passwords/driver/sha1.php @@ -47,6 +47,6 @@ class sha1 extends base */ public function check($password, $hash, $user_row = array()) { - return (strlen($hash) == 40) ? $hash === sha1($password) : false; + return (strlen($hash) == 40) ? $this->helper->string_compare($hash, sha1($password)) : false; } } diff --git a/phpBB/phpbb/passwords/driver/sha1_smf.php b/phpBB/phpbb/passwords/driver/sha1_smf.php index ec64bd6afb..b30d87265e 100644 --- a/phpBB/phpbb/passwords/driver/sha1_smf.php +++ b/phpBB/phpbb/passwords/driver/sha1_smf.php @@ -46,6 +46,6 @@ class sha1_smf extends base */ public function check($password, $hash, $user_row = array()) { - return (strlen($hash) == 40) ? $hash === $this->hash($password, $user_row) : false; + return (strlen($hash) == 40) ? $this->helper->string_compare($hash, $this->hash($password, $user_row)) : false; } } diff --git a/phpBB/phpbb/passwords/driver/sha1_wcf1.php b/phpBB/phpbb/passwords/driver/sha1_wcf1.php index 919fa2bb71..68006486c4 100644 --- a/phpBB/phpbb/passwords/driver/sha1_wcf1.php +++ b/phpBB/phpbb/passwords/driver/sha1_wcf1.php @@ -54,7 +54,7 @@ class sha1_wcf1 extends base else { // Works for standard WCF 1.x, i.e. WBB3 and similar - return $hash === sha1($user_row['user_passwd_salt'] . sha1($user_row['user_passwd_salt'] . sha1($password))); + return $this->helper->string_compare($hash, sha1($user_row['user_passwd_salt'] . sha1($user_row['user_passwd_salt'] . sha1($password)))); } } } diff --git a/phpBB/phpbb/passwords/driver/sha_xf1.php b/phpBB/phpbb/passwords/driver/sha_xf1.php index 7a1ea1450a..9d8f01796e 100644 --- a/phpBB/phpbb/passwords/driver/sha_xf1.php +++ b/phpBB/phpbb/passwords/driver/sha_xf1.php @@ -54,8 +54,8 @@ class sha_xf1 extends base else { // Works for xenforo 1.0, 1.1 - if ($hash === sha1(sha1($password) . $user_row['user_passwd_salt']) - || $hash === hash('sha256', hash('sha256', $password) . $user_row['user_passwd_salt'])) + if ($this->helper->string_compare($hash, sha1(sha1($password) . $user_row['user_passwd_salt'])) + || $this->helper->string_compare($hash, hash('sha256', hash('sha256', $password) . $user_row['user_passwd_salt']))) { return true; } diff --git a/tests/passwords/drivers_test.php b/tests/passwords/drivers_test.php index ccfb05c40f..5f9fd523c9 100644 --- a/tests/passwords/drivers_test.php +++ b/tests/passwords/drivers_test.php @@ -35,7 +35,7 @@ class phpbb_passwords_helper_test extends \phpbb_test_case 'passwords.driver.md5_vb' => new \phpbb\passwords\driver\md5_vb($config, $this->driver_helper), 'passwords.driver.sha_xf1' => new \phpbb\passwords\driver\sha_xf1($config, $this->driver_helper), ); - $this->passwords_drivers['passwords.driver.md5_phpbb2'] = new \phpbb\passwords\driver\md5_phpbb2($request, $this->passwords_drivers['passwords.driver.salted_md5'], $phpbb_root_path, $php_ext); + $this->passwords_drivers['passwords.driver.md5_phpbb2'] = new \phpbb\passwords\driver\md5_phpbb2($request, $this->passwords_drivers['passwords.driver.salted_md5'], $this->driver_helper, $phpbb_root_path, $php_ext); $this->passwords_drivers['passwords.driver.bcrypt_wcf2'] = new \phpbb\passwords\driver\bcrypt_wcf2($this->passwords_drivers['passwords.driver.bcrypt'], $this->driver_helper); } diff --git a/tests/passwords/manager_test.php b/tests/passwords/manager_test.php index e46cf820f2..333834ee07 100644 --- a/tests/passwords/manager_test.php +++ b/tests/passwords/manager_test.php @@ -41,7 +41,7 @@ class phpbb_passwords_manager_test extends \phpbb_test_case 'passwords.driver.md5_vb' => new \phpbb\passwords\driver\md5_vb($config, $this->driver_helper), 'passwords.driver.sha_xf1' => new \phpbb\passwords\driver\sha_xf1($config, $this->driver_helper), ); - $this->passwords_drivers['passwords.driver.md5_phpbb2'] = new \phpbb\passwords\driver\md5_phpbb2($request, $this->passwords_drivers['passwords.driver.salted_md5'], $phpbb_root_path, $php_ext); + $this->passwords_drivers['passwords.driver.md5_phpbb2'] = new \phpbb\passwords\driver\md5_phpbb2($request, $this->passwords_drivers['passwords.driver.salted_md5'], $this->driver_helper, $phpbb_root_path, $php_ext); $this->passwords_drivers['passwords.driver.bcrypt_wcf2'] = new \phpbb\passwords\driver\bcrypt_wcf2($this->passwords_drivers['passwords.driver.bcrypt'], $this->driver_helper); $this->helper = new \phpbb\passwords\helper; @@ -326,4 +326,22 @@ class phpbb_passwords_manager_test extends \phpbb_test_case $this->assertFalse($this->manager->hash(str_repeat('a', 1024 * 1024 * 16))); $this->assertLessThanOrEqual(5, time() - $start_time); } + + public function data_test_string_compare() + { + return array( + array('foo', 'bar', false), + array(1, '1', false), + array('one', 'one', true), + array('foobar', 'foobaf', false), + ); + } + + /** + * @dataProvider data_test_string_compare + */ + public function test_string_compare($a, $b, $expected) + { + $this->assertSame($expected, $this->driver_helper->string_compare($a, $b)); + } }