From 7a7976ba7a406b6c7b90e256f26faaa3257b7ade Mon Sep 17 00:00:00 2001 From: Max Semenik Date: Wed, 23 Jan 2019 18:51:21 -0800 Subject: [PATCH] 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 --- RELEASE-NOTES-1.33 | 1 + ...lPasswordPrimaryAuthenticationProvider.php | 4 +-- ...yPasswordPrimaryAuthenticationProvider.php | 2 +- includes/password/InvalidPassword.php | 4 +++ includes/password/Password.php | 31 +++++++++++++++---- includes/user/BotPassword.php | 2 +- tests/phpunit/includes/TestUser.php | 2 +- .../LayeredParameterizedPasswordTest.php | 2 +- .../includes/password/PasswordTestCase.php | 8 ++--- 9 files changed, 40 insertions(+), 16 deletions(-) diff --git a/RELEASE-NOTES-1.33 b/RELEASE-NOTES-1.33 index 03a4a67387..7f81f9f702 100644 --- 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 c538ee7e6d..e9adb7ee32 100644 --- 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 0ef13b34ec..045f7915f3 100644 --- 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 e45b774441..978118f43f 100644 --- 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 c8a0267cb1..f167f958e6 100644 --- 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 b83c209a17..e8cd94a8a2 100644 --- 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 952a662fcd..773bd519ab 100644 --- 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 6a965a0387..0f848ab1ee 100644 --- 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 7afdd0abb8..384db5fa4f 100644 --- 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() ); } /** -- 2.20.1