From 292e13edc58af5e712c00da68709a6a76cb5f9f9 Mon Sep 17 00:00:00 2001 From: Max Semenik Date: Mon, 3 Dec 2018 23:46:38 -0800 Subject: [PATCH] Add support for Argon2 password hashing So far, everything we had was vulnerable to newest advances in GPU cracking and timing side-channel attacks. Argon2 was designed specifically to address these problems. Unfortunately, PHP support is lagging, with some builds missing Argon2id or even Argon2i. Change-Id: Ifdf648f5d8a734a663e630286724a6d0a87c7510 --- RELEASE-NOTES-1.33 | 3 + autoload.php | 1 + includes/DefaultSettings.php | 18 +++ includes/password/Argon2Password.php | 117 ++++++++++++++++++ includes/password/Password.php | 18 ++- .../includes/password/Argon2PasswordTest.php | 105 ++++++++++++++++ .../password/EncryptedPasswordTest.php | 3 +- .../includes/password/PasswordTest.php | 8 -- .../includes/password/PasswordTestCase.php | 15 ++- 9 files changed, 271 insertions(+), 17 deletions(-) create mode 100644 includes/password/Argon2Password.php create mode 100644 tests/phpunit/includes/password/Argon2PasswordTest.php diff --git a/RELEASE-NOTES-1.33 b/RELEASE-NOTES-1.33 index 299cd94842..55787e77de 100644 --- a/RELEASE-NOTES-1.33 +++ b/RELEASE-NOTES-1.33 @@ -50,6 +50,9 @@ production. pages. * (T214706) LinksUpdate::getAddedExternalLinks() and LinksUpdate::getRemovedExternalLinks() were introduced. +* Argon2 password hashing is now available, can be enabled via + $wgPasswordDefault = 'argon2'. It's designed to resist timing attacks + (requires PHP 7.2+) and GPU hacking (7.3+). === External library changes in 1.33 === diff --git a/autoload.php b/autoload.php index d577272de4..a13763e5fb 100644 --- a/autoload.php +++ b/autoload.php @@ -156,6 +156,7 @@ $wgAutoloadLocalClasses = [ 'ApiValidatePassword' => __DIR__ . '/includes/api/ApiValidatePassword.php', 'ApiWatch' => __DIR__ . '/includes/api/ApiWatch.php', 'ArchivedFile' => __DIR__ . '/includes/filerepo/file/ArchivedFile.php', + 'Argon2Password' => __DIR__ . '/includes/password/Argon2Password.php', 'ArrayDiffFormatter' => __DIR__ . '/includes/diff/ArrayDiffFormatter.php', 'ArrayUtils' => __DIR__ . '/includes/libs/ArrayUtils.php', 'Article' => __DIR__ . '/includes/page/Article.php', diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index dc8f1e86e4..582826e8c1 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -4785,6 +4785,24 @@ $wgPasswordConfig = [ 'cost' => '30000', 'length' => '64', ], + 'argon2' => [ + 'class' => Argon2Password::class, + + // Algorithm used: + // * 'argon2i' is optimized against side-channel attacks (PHP 7.2+) + // * 'argon2id' is optimized against both side-channel and GPU cracking (PHP 7.3+) + // * 'auto' to use best available algorithm. If you're using more than one server, be + // careful when you're mixing PHP versions because newer PHP might generate hashes that + // older versions might would not understand. + 'algo' => 'auto', + + // The parameters below are the same as options accepted by password_hash(). + // Set them to override that function's defaults. + // + // 'memory_cost' => PASSWORD_ARGON2_DEFAULT_MEMORY_COST, + // 'time_cost' => PASSWORD_ARGON2_DEFAULT_TIME_COST, + // 'threads' => PASSWORD_ARGON2_DEFAULT_THREADS, + ], ]; /** diff --git a/includes/password/Argon2Password.php b/includes/password/Argon2Password.php new file mode 100644 index 0000000000..9138c3309c --- /dev/null +++ b/includes/password/Argon2Password.php @@ -0,0 +1,117 @@ + null, + 'time_cost' => null, + 'threads' => null, + ]; + + /** + * @inheritDoc + */ + protected function isSupported() { + // It is actually possible to have a PHP build with Argon2i but not Argon2id + return defined( 'PASSWORD_ARGON2I' ) || defined( 'PASSWORD_ARGON2ID' ); + } + + /** + * @return mixed[] Array of 2nd and third parmeters to password_hash() + */ + private function prepareParams() { + switch ( $this->config['algo'] ) { + case 'argon2i': + $algo = PASSWORD_ARGON2I; + break; + case 'argon2id': + $algo = PASSWORD_ARGON2ID; + break; + case 'auto': + $algo = defined( 'PASSWORD_ARGON2ID' ) ? PASSWORD_ARGON2ID : PASSWORD_ARGON2I; + break; + default: + throw new LogicException( "Unexpected algo: {$this->config['algo']}" ); + + } + + $params = array_intersect_key( $this->config, self::$knownOptions ); + + return [ $algo, $params ]; + } + + /** + * @inheritDoc + */ + public function crypt( $password ) { + list( $algo, $params ) = $this->prepareParams(); + $this->hash = password_hash( $password, $algo, $params ); + } + + /** + * @inheritDoc + */ + public function equals( $other ) { + if ( is_string( $other ) ) { + return $this->verify( $other ); + } + + // Argon2 key derivation is not deterministic, can't pass objects to equals() + return false; + } + + /** + * @inheritDoc + */ + public function verify( $password ) { + Assert::parameterType( 'string', $password, '$password' ); + + return password_verify( $password, $this->hash ); + } + + /** + * @inheritDoc + */ + public function toString() { + $res = ":argon2:{$this->hash}"; + $this->assertIsSafeSize( $res ); + return $res; + } + + /** + * @inheritDoc + */ + public function needsUpdate() { + list( $algo, $params ) = $this->prepareParams(); + return password_needs_rehash( $this->hash, $algo, $params ); + } +} diff --git a/includes/password/Password.php b/includes/password/Password.php index f167f958e6..8f6cb3e65a 100644 --- a/includes/password/Password.php +++ b/includes/password/Password.php @@ -101,8 +101,11 @@ abstract class Password { * @param string|null $hash The raw hash, including the type */ final public function __construct( PasswordFactory $factory, array $config, $hash = null ) { + if ( !$this->isSupported() ) { + throw new Exception( 'PHP support not found for ' . get_class( $this ) ); + } if ( !isset( $config['type'] ) ) { - throw new MWException( 'Password configuration must contain a type name.' ); + throw new Exception( 'Password configuration must contain a type name.' ); } $this->config = $config; $this->factory = $factory; @@ -125,6 +128,15 @@ abstract class Password { return $this->config['type']; } + /** + * Whether current password type is supported on this system. + * + * @return bool + */ + protected function isSupported() { + return true; + } + /** * Perform any parsing necessary on the hash to see if the hash is valid * and/or to perform logic for seeing if the hash needs updating. @@ -169,9 +181,7 @@ abstract class Password { * @return bool */ public function verify( $password ) { - Assert::parameter( is_string( $password ), - '$password', 'must be string, actual: ' . gettype( $password ) - ); + Assert::parameterType( 'string', $password, '$password' ); // No need to use the factory because we're definitely making // an object of the same type. diff --git a/tests/phpunit/includes/password/Argon2PasswordTest.php b/tests/phpunit/includes/password/Argon2PasswordTest.php new file mode 100644 index 0000000000..b518040e87 --- /dev/null +++ b/tests/phpunit/includes/password/Argon2PasswordTest.php @@ -0,0 +1,105 @@ +markTestSkipped( 'Argon2 support not found' ); + } + } + + /** + * Return an array of configs to be used for this class's password type. + * + * @return array[] + */ + protected function getTypeConfigs() { + return [ + 'argon2' => [ + 'class' => Argon2Password::class, + 'algo' => 'argon2i', + 'memory_cost' => 1024, + 'time_cost' => 2, + 'threads' => 2, + ] + ]; + } + + /** + * @return array + */ + public static function providePasswordTests() { + $result = [ + [ + true, + ':argon2:$argon2i$v=19$m=1024,t=2,p=2$RHpGTXJPeFlSV2NDTEswNA$VeW7rumZY4pL8XO4KeQkKD43r5uX3eazVJRtrFN7lNc', + 'password', + ], + [ + true, + ':argon2:$argon2i$v=19$m=2048,t=5,p=3$MHFKSnh6WWZEWkpKa09SUQ$vU92h/8hkByL5VKW1P9amCj054pZILGKznAvKWAivZE', + 'password', + ], + [ + true, + ':argon2:$argon2i$v=19$m=1024,t=2,p=2$bFJ4TzM5RWh2T0VmeFhDTA$AHFUFZRh69aZYBqyxn6tpujpEcf2JP8wgRCPU3nw3W4', + "pass\x00word", + ], + [ + false, + ':argon2:$argon2i$v=19$m=1024,t=2,p=2$UGZqTWJRUkI1alVNTGRUbA$RcASw9XUWjCDO9WNnuVkGkEylURUW/CcNwSffdFwN74', + 'password', + ] + ]; + + if ( defined( 'PASSWORD_ARGON2ID' ) ) { + // @todo: Argon2id cases + $result = array_merge( $result, [] ); + } + + return $result; + } + + /** + * @dataProvider provideNeedsUpdate + */ + public function testNeedsUpdate( $updateExpected, $hash ) { + $password = $this->passwordFactory->newFromCiphertext( $hash ); + $this->assertSame( $updateExpected, $password->needsUpdate() ); + } + + public function provideNeedsUpdate() { + return [ + [ false, ':argon2:$argon2i$v=19$m=1024,t=2,p=2$bFJ4TzM5RWh2T0VmeFhDTA$AHFUFZRh69aZYBqyxn6tpujpEcf2JP8wgRCPU3nw3W4' ], + [ false, ':argon2:$argon2i$v=19$m=1024,t=2,p=2$' ], + [ true, ':argon2:$argon2i$v=19$m=666,t=2,p=2$' ], + [ true, ':argon2:$argon2i$v=19$m=1024,t=666,p=2$' ], + [ true, ':argon2:$argon2i$v=19$m=1024,t=2,p=666$' ], + ]; + } + + public function testPartialConfig() { + $factory = new PasswordFactory(); + $factory->register( 'argon2', [ + 'class' => Argon2Password::class, + 'algo' => 'argon2i', + ] ); + + $partialPassword = $factory->newFromType( 'argon2' ); + $partialPassword->crypt( 'password' ); + $fullPassword = $this->passwordFactory->newFromCiphertext( $partialPassword->toString() ); + + $this->assertFalse( $fullPassword->needsUpdate(), + 'Options not set for a password should fall back to defaults' + ); + } +} diff --git a/tests/phpunit/includes/password/EncryptedPasswordTest.php b/tests/phpunit/includes/password/EncryptedPasswordTest.php index c5d397fbc7..7384310aa2 100644 --- a/tests/phpunit/includes/password/EncryptedPasswordTest.php +++ b/tests/phpunit/includes/password/EncryptedPasswordTest.php @@ -78,6 +78,7 @@ class EncryptedPasswordTest extends PasswordTestCase { $this->assertRegExp( '/^:both:aes-256-cbc:1:/', $serialized ); $fromNewHash = $this->passwordFactory->newFromCiphertext( $serialized ); $fromPlaintext = $this->passwordFactory->newFromPlaintext( 'password', $fromNewHash ); - $this->assertTrue( $fromHash->equals( $fromPlaintext ) ); + $this->assertTrue( $fromPlaintext->verify( 'password' ) ); + $this->assertTrue( $fromHash->verify( 'password' ) ); } } diff --git a/tests/phpunit/includes/password/PasswordTest.php b/tests/phpunit/includes/password/PasswordTest.php index 65c9199310..61a5147277 100644 --- a/tests/phpunit/includes/password/PasswordTest.php +++ b/tests/phpunit/includes/password/PasswordTest.php @@ -24,14 +24,6 @@ * @covers InvalidPassword */ class PasswordTest extends MediaWikiTestCase { - public function testInvalidUnequalInvalid() { - $passwordFactory = new PasswordFactory(); - $invalid1 = $passwordFactory->newFromCiphertext( null ); - $invalid2 = $passwordFactory->newFromCiphertext( null ); - - $this->assertFalse( $invalid1->equals( $invalid2 ) ); - } - public function testInvalidPlaintext() { $passwordFactory = new PasswordFactory(); $invalid = $passwordFactory->newFromPlaintext( null ); diff --git a/tests/phpunit/includes/password/PasswordTestCase.php b/tests/phpunit/includes/password/PasswordTestCase.php index 384db5fa4f..d1e257895e 100644 --- a/tests/phpunit/includes/password/PasswordTestCase.php +++ b/tests/phpunit/includes/password/PasswordTestCase.php @@ -60,9 +60,8 @@ abstract class PasswordTestCase extends MediaWikiTestCase { * @dataProvider providePasswordTests */ public function testHashing( $shouldMatch, $hash, $password ) { - $fromHash = $this->passwordFactory->newFromCiphertext( $hash ); - $fromPassword = $this->passwordFactory->newFromPlaintext( $password, $fromHash ); - $this->assertSame( $shouldMatch, $fromHash->equals( $fromPassword ) ); + $passwordObj = $this->passwordFactory->newFromCiphertext( $hash ); + $this->assertSame( $shouldMatch, $passwordObj->verify( $password ) ); } /** @@ -85,6 +84,7 @@ abstract class PasswordTestCase extends MediaWikiTestCase { $this->assertFalse( $invalid->equals( $normal ) ); $this->assertFalse( $normal->equals( $invalid ) ); + $this->assertFalse( $invalid->verify( $hash ) ); } protected function getValidTypes() { @@ -106,6 +106,13 @@ abstract class PasswordTestCase extends MediaWikiTestCase { $fromType = $this->passwordFactory->newFromType( $type ); $fromType->crypt( 'password' ); $fromPlaintext = $this->passwordFactory->newFromPlaintext( 'password', $fromType ); - $this->assertTrue( $fromType->equals( $fromPlaintext ) ); + $this->assertTrue( $fromType->verify( 'password' ) ); + $this->assertTrue( $fromPlaintext->verify( 'password' ) ); + $this->assertFalse( $fromType->verify( 'different password' ) ); + $this->assertFalse( $fromPlaintext->verify( 'different password' ) ); + $this->assertEquals( get_class( $fromType ), + get_class( $fromPlaintext ), + 'newFromPlaintext() should produce instance of the same class as newFromType()' + ); } } -- 2.20.1