Merge branch 'develop-ascraeus' into develop

* develop-ascraeus:
  [ticket/13203] Fix passwords manager tests
  [ticket/13203] Use string_compare method in passwords drivers
  [ticket/13203] Add method for byte by byte comparison to drivers helper

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:

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

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

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

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

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

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']));
 		}
 	}
 }

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));
 	}
 
 	/**

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

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

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

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'])
+			if ($this->helper->string_compare($hash, sha1(sha1($password) . $user_row['user_passwd_salt']))
-				|| $hash === hash('sha256', hash('sha256', $password) . $user_row['user_passwd_salt']))
+				|| $this->helper->string_compare($hash, hash('sha256', hash('sha256', $password) . $user_row['user_passwd_salt'])))
 			{
 				return true;
 			}

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

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