Merge pull request #4405 from marc1706/ticket/14733

[ticket/14733] Support increasing hashing cost factor

* marc1706/ticket/14733:
  [ticket/14733] Make sure detect_algorithm() works correctly and add tests
  [ticket/14733] Extend passwords driver_interface in rehashable_driver_interface
  [ticket/14733] Use new interface to preserve backwards compatibility
  [ticket/14733] Use default cost factor in bcrypt constructor
  [ticket/14733] Support increasing hashing cost factor

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 }
 

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;
 
 	/**
@@ -52,6 +52,14 @@ abstract class base implements driver_interface
 		return false;
 	}
 
+	/**
+	 * {@inheritdoc}
+	 */
+	public function needs_rehash($hash)
+	{
+		return false;
+	}
+
 	/**
 	* {@inheritdoc}
 	*/

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}
 	*/
@@ -25,6 +43,18 @@ class bcrypt extends base
 		return self::PREFIX;
 	}
 
+	/**
+	 * {@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}
 	*/
@@ -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);

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);
+}

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$

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);
+	}
 }

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);
+	}
 }