Password: replace equals() with verify()

So far, our key derivation code assumed that it has control over
the salt used by the derivation routines, however I want to add Argon2
support and it doesn't work this way: password_hash() generates the
salt itself, and the only way to verify a password is by using
password_verify(). Current way the things are done doesn't support it
because it relies on the result of password hashing with parameters we
provide to be deterministic.

Therefore, I'm deprecating Password::equals(), as well as whole concept
of comparing Password objects - it's used only in tests anyway. It's
getting replaced with verify() that only accepts password strings.
Uses of old function are fixed with exception of a few calls in tests
that will be addressed in my Argon2 patch.

Change-Id: I2b2be9a422ee0f773490eac316ad81505c3f8571

diff --git a/RELEASE-NOTES-1.33 b/RELEASE-NOTES-1.33
index 03a4a67..7f81f9f 100644 (file)
--- a/RELEASE-NOTES-1.33
+++ b/RELEASE-NOTES-1.33
@@ -215,6 +215,7 @@ because of Phabricator reports.
 * The class WebInstallerOutput is now marked as @private.
 * (T209699) The jquery.async module has been deprecated. JavaScript code that
   needs asynchronous behaviour should use Promises.
+* Password::equals() is deprecated, use verify().
 
 === Other changes in 1.33 ===
 * (T208871) The hard-coded Google search form on the database error page was

diff --git a/includes/auth/LocalPasswordPrimaryAuthenticationProvider.php b/includes/auth/LocalPasswordPrimaryAuthenticationProvider.php
index c538ee7..e9adb7e 100644 (file)
--- a/includes/auth/LocalPasswordPrimaryAuthenticationProvider.php
+++ b/includes/auth/LocalPasswordPrimaryAuthenticationProvider.php
@@ -120,12 +120,12 @@ class LocalPasswordPrimaryAuthenticationProvider
                }
 
                $pwhash = $this->getPassword( $row->user_password );
-               if ( !$pwhash->equals( $req->password ) ) {
+               if ( !$pwhash->verify( $req->password ) ) {
                        if ( $this->config->get( 'LegacyEncoding' ) ) {
                                // Some wikis were converted from ISO 8859-1 to UTF-8, the passwords can't be converted
                                // Check for this with iconv
                                $cp1252Password = iconv( 'UTF-8', 'WINDOWS-1252//TRANSLIT', $req->password );
-                               if ( $cp1252Password === $req->password || !$pwhash->equals( $cp1252Password ) ) {
+                               if ( $cp1252Password === $req->password || !$pwhash->verify( $cp1252Password ) ) {
                                        return $this->failResponse( $req );
                                }
                        } else {

diff --git a/includes/auth/TemporaryPasswordPrimaryAuthenticationProvider.php b/includes/auth/TemporaryPasswordPrimaryAuthenticationProvider.php
index 0ef13b3..045f791 100644 (file)
--- a/includes/auth/TemporaryPasswordPrimaryAuthenticationProvider.php
+++ b/includes/auth/TemporaryPasswordPrimaryAuthenticationProvider.php
@@ -146,7 +146,7 @@ class TemporaryPasswordPrimaryAuthenticationProvider
                }
 
                $pwhash = $this->getPassword( $row->user_newpassword );
-               if ( !$pwhash->equals( $req->password ) ) {
+               if ( !$pwhash->verify( $req->password ) ) {
                        return $this->failResponse( $req );
                }
 

diff --git a/includes/password/InvalidPassword.php b/includes/password/InvalidPassword.php
index e45b774..978118f 100644 (file)
--- a/includes/password/InvalidPassword.php
+++ b/includes/password/InvalidPassword.php
@@ -41,6 +41,10 @@ class InvalidPassword extends Password {
                return false;
        }
 
+       public function verify( $password ) {
+               return false;
+       }
+
        public function needsUpdate() {
                return false;
        }

diff --git a/includes/password/Password.php b/includes/password/Password.php
index c8a0267..f167f95 100644 (file)
--- a/includes/password/Password.php
+++ b/includes/password/Password.php
@@ -20,6 +20,8 @@
  * @file
  */
 
+use Wikimedia\Assert\Assert;
+
 /**
  * Represents a password hash for use in authentication
  *
@@ -147,21 +149,38 @@ abstract class Password {
         * Password::toString() for each object. This can be overridden to do
         * custom comparison, but it is not recommended unless necessary.
         *
+        * @deprecated since 1.33, use verify()
+        *
         * @param Password|string $other The other password
         * @return bool True if equal, false otherwise
         */
        public function equals( $other ) {
-               if ( !$other instanceof self ) {
-                       // No need to use the factory because we're definitely making
-                       // an object of the same type.
-                       $obj = clone $this;
-                       $obj->crypt( $other );
-                       $other = $obj;
+               if ( is_string( $other ) ) {
+                       return $this->verify( $other );
                }
 
                return hash_equals( $this->toString(), $other->toString() );
        }
 
+       /**
+        * Checks whether the given password matches the hash stored in this object.
+        *
+        * @param string $password Password to check
+        * @return bool
+        */
+       public function verify( $password ) {
+               Assert::parameter( is_string( $password ),
+                       '$password', 'must be string, actual: ' . gettype( $password )
+               );
+
+               // No need to use the factory because we're definitely making
+               // an object of the same type.
+               $obj = clone $this;
+               $obj->crypt( $password );
+
+               return hash_equals( $this->toString(), $obj->toString() );
+       }
+
        /**
         * Convert this hash to a string that can be stored in the database
         *

diff --git a/includes/user/BotPassword.php b/includes/user/BotPassword.php
index b83c209..e8cd94a 100644 (file)
--- a/includes/user/BotPassword.php
+++ b/includes/user/BotPassword.php
@@ -506,7 +506,7 @@ class BotPassword implements IDBAccessObject {
                        return self::loginHook( $user, $bp,
                                Status::newFatal( 'botpasswords-needs-reset', $name, $appId ) );
                }
-               if ( !$passwordObj->equals( $password ) ) {
+               if ( !$passwordObj->verify( $password ) ) {
                        return self::loginHook( $user, $bp, Status::newFatal( 'wrongpassword' ) );
                }
 

diff --git a/tests/phpunit/includes/TestUser.php b/tests/phpunit/includes/TestUser.php
index 952a662..773bd51 100644 (file)
--- a/tests/phpunit/includes/TestUser.php
+++ b/tests/phpunit/includes/TestUser.php
@@ -143,7 +143,7 @@ class TestUser {
                }
 
                $passwordFactory = MediaWikiServices::getInstance()->getPasswordFactory();
-               if ( !$passwordFactory->newFromCiphertext( $row->user_password )->equals( $password ) ) {
+               if ( !$passwordFactory->newFromCiphertext( $row->user_password )->verify( $password ) ) {
                        $passwordHash = $passwordFactory->newFromPlaintext( $password );
                        $dbw->update(
                                'user',

diff --git a/tests/phpunit/includes/password/LayeredParameterizedPasswordTest.php b/tests/phpunit/includes/password/LayeredParameterizedPasswordTest.php
index 6a965a0..0f848ab 100644 (file)
--- a/tests/phpunit/includes/password/LayeredParameterizedPasswordTest.php
+++ b/tests/phpunit/includes/password/LayeredParameterizedPasswordTest.php
@@ -58,6 +58,6 @@ class LayeredParameterizedPasswordTest extends PasswordTestCase {
                $totalPassword = $this->passwordFactory->newFromType( 'testLargeLayeredTop' );
                $totalPassword->partialCrypt( $partialPassword );
 
-               $this->assertTrue( $totalPassword->equals( 'testPassword123' ) );
+               $this->assertTrue( $totalPassword->verify( 'testPassword123' ) );
        }
 }

diff --git a/tests/phpunit/includes/password/PasswordTestCase.php b/tests/phpunit/includes/password/PasswordTestCase.php
index 7afdd0a..384db5f 100644 (file)
--- a/tests/phpunit/includes/password/PasswordTestCase.php
+++ b/tests/phpunit/includes/password/PasswordTestCase.php
@@ -60,9 +60,9 @@ abstract class PasswordTestCase extends MediaWikiTestCase {
         * @dataProvider providePasswordTests
         */
        public function testHashing( $shouldMatch, $hash, $password ) {
-               $hash = $this->passwordFactory->newFromCiphertext( $hash );
-               $password = $this->passwordFactory->newFromPlaintext( $password, $hash );
-               $this->assertSame( $shouldMatch, $hash->equals( $password ) );
+               $fromHash = $this->passwordFactory->newFromCiphertext( $hash );
+               $fromPassword = $this->passwordFactory->newFromPlaintext( $password, $fromHash );
+               $this->assertSame( $shouldMatch, $fromHash->equals( $fromPassword ) );
        }
 
        /**
@@ -72,7 +72,7 @@ abstract class PasswordTestCase extends MediaWikiTestCase {
                $hashObj = $this->passwordFactory->newFromCiphertext( $hash );
                $serialized = $hashObj->toString();
                $unserialized = $this->passwordFactory->newFromCiphertext( $serialized );
-               $this->assertTrue( $hashObj->equals( $unserialized ) );
+               $this->assertEquals( $hashObj->toString(), $unserialized->toString() );
        }
 
        /**