diff options
-rw-r--r-- | phpBB/config/default/container/services_password.yml | 5 | ||||
-rw-r--r-- | phpBB/phpbb/passwords/driver/base.php | 12 | ||||
-rw-r--r-- | phpBB/phpbb/passwords/driver/bcrypt.php | 32 | ||||
-rw-r--r-- | phpBB/phpbb/passwords/driver/rehashable_driver_interface.php | 25 | ||||
-rw-r--r-- | phpBB/phpbb/passwords/manager.php | 11 | ||||
-rw-r--r-- | tests/passwords/drivers_test.php | 23 | ||||
-rw-r--r-- | tests/passwords/manager_test.php | 54 |
7 files changed, 153 insertions, 9 deletions
diff --git a/phpBB/config/default/container/services_password.yml b/phpBB/config/default/container/services_password.yml index 82850dc2a7..c276365c56 100644 --- a/phpBB/config/default/container/services_password.yml +++ b/phpBB/config/default/container/services_password.yml @@ -1,3 +1,6 @@ +parameters: + passwords.driver.bcrypt_cost: 10 + services: # ----- Password management ----- passwords.manager: @@ -29,6 +32,7 @@ services: arguments: - '@config' - '@passwords.driver_helper' + - '%passwords.driver.bcrypt_cost%' tags: - { name: passwords.driver } @@ -37,6 +41,7 @@ services: arguments: - '@config' - '@passwords.driver_helper' + - '%passwords.driver.bcrypt_cost%' tags: - { name: passwords.driver } diff --git a/phpBB/phpbb/passwords/driver/base.php b/phpBB/phpbb/passwords/driver/base.php index fd07a61bf4..0997b5b700 100644 --- a/phpBB/phpbb/passwords/driver/base.php +++ b/phpBB/phpbb/passwords/driver/base.php @@ -13,7 +13,7 @@ namespace phpbb\passwords\driver; -abstract class base implements driver_interface +abstract class base implements rehashable_driver_interface { /** @var \phpbb\config\config */ protected $config; @@ -21,7 +21,7 @@ abstract class base implements driver_interface /** @var \phpbb\passwords\driver\helper */ protected $helper; - /** @var driver name */ + /** @var string Driver name */ protected $name; /** @@ -53,6 +53,14 @@ abstract class base implements driver_interface } /** + * {@inheritdoc} + */ + public function needs_rehash($hash) + { + return false; + } + + /** * {@inheritdoc} */ public function get_settings_only($hash, $full = false) diff --git a/phpBB/phpbb/passwords/driver/bcrypt.php b/phpBB/phpbb/passwords/driver/bcrypt.php index eab1c3d569..eb1aeeeb76 100644 --- a/phpBB/phpbb/passwords/driver/bcrypt.php +++ b/phpBB/phpbb/passwords/driver/bcrypt.php @@ -17,6 +17,24 @@ class bcrypt extends base { const PREFIX = '$2a$'; + /** @var int Hashing cost factor */ + protected $cost_factor; + + /** + * Constructor of passwords driver object + * + * @param \phpbb\config\config $config phpBB config + * @param \phpbb\passwords\driver\helper $helper Password driver helper + * @param int $cost_factor Hashing cost factor (optional) + */ + public function __construct(\phpbb\config\config $config, helper $helper, $cost_factor = 10) + { + parent::__construct($config, $helper); + + // Don't allow cost factor to be below default setting + $this->cost_factor = max(10, $cost_factor); + } + /** * {@inheritdoc} */ @@ -26,6 +44,18 @@ class bcrypt extends base } /** + * {@inheritdoc} + */ + public function needs_rehash($hash) + { + preg_match('/^' . preg_quote($this->get_prefix()) . '([0-9]+)\$/', $hash, $matches); + + list(, $cost_factor) = $matches; + + return empty($cost_factor) || $this->cost_factor !== intval($cost_factor); + } + + /** * {@inheritdoc} */ public function hash($password, $salt = '') @@ -46,7 +76,7 @@ class bcrypt extends base if ($salt == '') { - $salt = $prefix . '10$' . $this->get_random_salt(); + $salt = $prefix . $this->cost_factor . '$' . $this->get_random_salt(); } $hash = crypt($password, $salt); diff --git a/phpBB/phpbb/passwords/driver/rehashable_driver_interface.php b/phpBB/phpbb/passwords/driver/rehashable_driver_interface.php new file mode 100644 index 0000000000..ca30748502 --- /dev/null +++ b/phpBB/phpbb/passwords/driver/rehashable_driver_interface.php @@ -0,0 +1,25 @@ +<?php +/** +* +* This file is part of the phpBB Forum Software package. +* +* @copyright (c) phpBB Limited <https://www.phpbb.com> +* @license GNU General Public License, version 2 (GPL-2.0) +* +* For full copyright and license information, please see +* the docs/CREDITS.txt file. +* +*/ + +namespace phpbb\passwords\driver; + +interface rehashable_driver_interface extends driver_interface +{ + /** + * Check if password needs to be rehashed + * + * @param string $hash Hash to check for rehash + * @return bool True if password needs to be rehashed, false if not + */ + public function needs_rehash($hash); +} diff --git a/phpBB/phpbb/passwords/manager.php b/phpBB/phpbb/passwords/manager.php index b2caba81f2..fad76a9fe5 100644 --- a/phpBB/phpbb/passwords/manager.php +++ b/phpBB/phpbb/passwords/manager.php @@ -174,7 +174,7 @@ class manager // Be on the lookout for multiple hashing algorithms // 2 is correct: H\2a > 2, H\P > 2 - if (strlen($match[1]) > 2) + if (strlen($match[1]) > 2 && strpos($match[1], '\\') !== false) { $hash_types = explode('\\', $match[1]); $return_ary = array(); @@ -297,7 +297,14 @@ class manager } else { - $this->convert_flag = false; + if ($stored_hash_type instanceof driver\rehashable_driver_interface) + { + $this->convert_flag = $stored_hash_type->needs_rehash($hash); + } + else + { + $this->convert_flag = false; + } } // Check all legacy hash types if prefix is $CP$ diff --git a/tests/passwords/drivers_test.php b/tests/passwords/drivers_test.php index 5f9fd523c9..01c69a38bb 100644 --- a/tests/passwords/drivers_test.php +++ b/tests/passwords/drivers_test.php @@ -23,8 +23,8 @@ class phpbb_passwords_helper_test extends \phpbb_test_case $php_ext = 'php'; $this->passwords_drivers = array( - 'passwords.driver.bcrypt_2y' => new \phpbb\passwords\driver\bcrypt_2y($config, $this->driver_helper), - 'passwords.driver.bcrypt' => new \phpbb\passwords\driver\bcrypt($config, $this->driver_helper), + 'passwords.driver.bcrypt_2y' => new \phpbb\passwords\driver\bcrypt_2y($config, $this->driver_helper, 10), + 'passwords.driver.bcrypt' => new \phpbb\passwords\driver\bcrypt($config, $this->driver_helper, 10), 'passwords.driver.salted_md5' => new \phpbb\passwords\driver\salted_md5($config, $this->driver_helper), 'passwords.driver.phpass' => new \phpbb\passwords\driver\phpass($config, $this->driver_helper), 'passwords.driver.sha1_smf' => new \phpbb\passwords\driver\sha1_smf($config, $this->driver_helper), @@ -413,4 +413,23 @@ class phpbb_passwords_helper_test extends \phpbb_test_case ); return strtr($string, $transform); } + + public function data_needs_rehash() + { + return array( + array('passwords.driver.bcrypt_2y', '$2y$10$somerandomhash', false), + array('passwords.driver.bcrypt', '$2a$10$somerandomhash', false), + array('passwords.driver.salted_md5', 'foobar', false), + array('passwords.driver.bcrypt_2y', '$2y$9$somerandomhash', true), + array('passwords.driver.bcrypt', '$2a$04$somerandomhash', true), + ); + } + + /** + * @dataProvider data_needs_rehash + */ + public function test_needs_rehash($driver, $hash, $expected) + { + $this->assertSame($this->passwords_drivers[$driver]->needs_rehash($hash), $expected); + } } diff --git a/tests/passwords/manager_test.php b/tests/passwords/manager_test.php index 333834ee07..0410d7035f 100644 --- a/tests/passwords/manager_test.php +++ b/tests/passwords/manager_test.php @@ -29,8 +29,8 @@ class phpbb_passwords_manager_test extends \phpbb_test_case $php_ext = 'php'; $this->passwords_drivers = array( - 'passwords.driver.bcrypt_2y' => new \phpbb\passwords\driver\bcrypt_2y($config, $this->driver_helper), - 'passwords.driver.bcrypt' => new \phpbb\passwords\driver\bcrypt($config, $this->driver_helper), + 'passwords.driver.bcrypt_2y' => new \phpbb\passwords\driver\bcrypt_2y($config, $this->driver_helper, 10), + 'passwords.driver.bcrypt' => new \phpbb\passwords\driver\bcrypt($config, $this->driver_helper, 10), 'passwords.driver.salted_md5' => new \phpbb\passwords\driver\salted_md5($config, $this->driver_helper), 'passwords.driver.phpass' => new \phpbb\passwords\driver\phpass($config, $this->driver_helper), 'passwords.driver.convert_password' => new \phpbb\passwords\driver\convert_password($config, $this->driver_helper), @@ -344,4 +344,54 @@ class phpbb_passwords_manager_test extends \phpbb_test_case { $this->assertSame($expected, $this->driver_helper->string_compare($a, $b)); } + + public function data_driver_interface_driver() + { + return array( + array(false, false, false), + array(true, false, false), + array(true, true, true), + ); + } + + /** + * @dataProvider data_driver_interface_driver + */ + public function test_driver_interface_driver($use_new_interface, $needs_rehash, $expected) + { + if ($use_new_interface) + { + $test_driver = $this->getMock('\phpbb\passwords\driver\rehashable_driver_interface', array('needs_rehash', 'get_prefix', 'check', 'is_supported', 'is_legacy', 'hash', 'get_settings_only')); + $test_driver->method('needs_rehash') + ->willReturn($needs_rehash); + } + else + { + $test_driver = $this->getMock('\phpbb\passwords\driver\driver_interface', array('get_prefix', 'check', 'is_supported', 'is_legacy', 'hash', 'get_settings_only')); + } + $config = new \phpbb\config\config(array()); + + $test_driver->method('is_supported') + ->willReturn(true); + $test_driver->method('get_prefix') + ->willReturn('$test$'); + $test_driver->method('check') + ->with($this->anything()) + ->willReturn(true); + $passwords_drivers = array( + 'passwords.driver.foobar' => $test_driver, + 'passwords.driver.bcrypt_2y' => new \phpbb\passwords\driver\bcrypt_2y($config, $this->driver_helper, 10), + ); + // Set up another manager + $foobar_manager = new \phpbb\passwords\manager($config, $passwords_drivers, $this->helper, array('passwords.driver.foobar')); + + $this->assertTrue($foobar_manager->check('foobar', '$test$somerandomstuff')); + $this->assertEquals($expected, $foobar_manager->convert_flag); + + // Should always return true in case a different driver is default + $foobar_manager = new \phpbb\passwords\manager($config, $passwords_drivers, $this->helper, array('passwords.driver.bcrypt_2y', 'passwords.driver.foobar')); + + $this->assertTrue($foobar_manager->check('foobar', '$test$somerandomstuff')); + $this->assertTrue($foobar_manager->convert_flag); + } } |