diff --git a/RELEASE-NOTES-1.40 b/RELEASE-NOTES-1.40 index 4b0e02a07d9..36e8b611330 100644 --- a/RELEASE-NOTES-1.40 +++ b/RELEASE-NOTES-1.40 @@ -19,6 +19,14 @@ Some specific notes for MediaWiki 1.40 upgrades are below: For notes on 1.39.x and older releases, see HISTORY. === Configuration changes for system administrators in 1.40 === +* When computing PBKDF2 password hashes, MediaWiki now detects and uses OpenSSL + support if available, unless $wgPasswordConfig['pbkdf2']['class'] is set in + LocalSettings.php. OpenSSL is more efficient, so if that setting is present, + you should remove it (or set it to 'Pbkdf2PasswordUsingOpenSSL' if possible). + If users get an internal error when trying to log in, you can try setting it + to 'Pbkdf2PasswordUsingHashExtension'. In particular, this would be necessary + if existing PBKDF2 password hashes were computed using a hash algorithm other + than "sha512" or "sha256" (the current and prior defaults). * … ==== New configuration ==== @@ -132,6 +140,8 @@ because of Phabricator reports. * ParserOutput::{get,set}ExternalLinkTarget() and ParserOutput::{get,set}MaxTemplateDepth() have been deprecated and marked for @internal use only. +* The class Pbkdf2Password was renamed to Pbkdf2PasswordUsingHashExtension, + and the old name is now deprecated. * … === Other changes in 1.40 === diff --git a/autoload.php b/autoload.php index f8a9d113eeb..b5e539466a1 100644 --- a/autoload.php +++ b/autoload.php @@ -7,6 +7,7 @@ $wgAutoloadLocalClasses = [ 'APCUBagOStuff' => __DIR__ . '/includes/libs/objectcache/APCUBagOStuff.php', 'AbkhazUppercaseCollation' => __DIR__ . '/includes/collation/AbkhazUppercaseCollation.php', 'AbstractContent' => __DIR__ . '/includes/content/AbstractContent.php', + 'AbstractPbkdf2Password' => __DIR__ . '/includes/password/AbstractPbkdf2Password.php', 'Action' => __DIR__ . '/includes/actions/Action.php', 'ActiveUsersPager' => __DIR__ . '/includes/specials/pagers/ActiveUsersPager.php', 'ActivityUpdateJob' => __DIR__ . '/includes/jobqueue/jobs/ActivityUpdateJob.php', @@ -1215,7 +1216,9 @@ $wgAutoloadLocalClasses = [ 'PathRouter' => __DIR__ . '/includes/PathRouter.php', 'PatrolLog' => __DIR__ . '/includes/logging/PatrolLog.php', 'PatrolLogFormatter' => __DIR__ . '/includes/logging/PatrolLogFormatter.php', - 'Pbkdf2Password' => __DIR__ . '/includes/password/Pbkdf2Password.php', + 'Pbkdf2Password' => __DIR__ . '/includes/password/Pbkdf2PasswordUsingHashExtension.php', + 'Pbkdf2PasswordUsingHashExtension' => __DIR__ . '/includes/password/Pbkdf2PasswordUsingHashExtension.php', + 'Pbkdf2PasswordUsingOpenSSL' => __DIR__ . '/includes/password/Pbkdf2PasswordUsingOpenSSL.php', 'PerRowAugmentor' => __DIR__ . '/includes/search/PerRowAugmentor.php', 'PermissionsError' => __DIR__ . '/includes/exception/PermissionsError.php', 'Pingback' => __DIR__ . '/includes/Pingback.php', diff --git a/docs/config-schema.yaml b/docs/config-schema.yaml index 21fcb97970a..142eaf9338a 100755 --- a/docs/config-schema.yaml +++ b/docs/config-schema.yaml @@ -4511,13 +4511,17 @@ config-schema: pbkdf2-legacyA: { class: LayeredParameterizedPassword, types: [A, pbkdf2] } pbkdf2-legacyB: { class: LayeredParameterizedPassword, types: [B, pbkdf2] } bcrypt: { class: BcryptPassword, cost: 9 } - pbkdf2: { class: Pbkdf2Password, algo: sha512, cost: '30000', length: '64' } + pbkdf2: { factory: [AbstractPbkdf2Password, newInstance], algo: sha512, cost: '30000', length: '64' } argon2: { class: Argon2Password, algo: auto } type: object description: |- - Configuration for built-in password types. Maps the password type - to an array of options. The 'class' option is the Password class to - use. All other options are class-dependent. + Configuration for built-in password types. + Maps the password type to an array of options: + - class: The Password class to use. + - factory (since 1.40): A function that creates and returns a suitable Password object. + This option is intended only for internal use; the function signature is unstable and + subject to change in future versions. + All other options are class-dependent. An advanced example: ``` $wgPasswordConfig['bcrypt-peppered'] = [ diff --git a/includes/MainConfigSchema.php b/includes/MainConfigSchema.php index 86ff37e11c9..c84b60eba73 100644 --- a/includes/MainConfigSchema.php +++ b/includes/MainConfigSchema.php @@ -11,6 +11,7 @@ // phpcs:disable Generic.Files.LineLength.TooLong namespace MediaWiki; +use AbstractPbkdf2Password; use ActivityUpdateJob; use APCUBagOStuff; use Argon2Password; @@ -58,7 +59,6 @@ use MWSaltedPassword; use NamespaceInfo; use NullJob; use PatrolLogFormatter; -use Pbkdf2Password; use ProtectLogFormatter; use PublishStashedFileJob; use RecentChangesUpdateJob; @@ -7076,9 +7076,16 @@ class MainConfigSchema { ]; /** - * Configuration for built-in password types. Maps the password type - * to an array of options. The 'class' option is the Password class to - * use. All other options are class-dependent. + * Configuration for built-in password types. + * + * Maps the password type to an array of options: + * + * - class: The Password class to use. + * - factory (since 1.40): A function that creates and returns a suitable Password object. + * This option is intended only for internal use; the function signature is unstable and + * subject to change in future versions. + * + * All other options are class-dependent. * * An advanced example: * @@ -7122,7 +7129,7 @@ class MainConfigSchema { 'cost' => 9, ], 'pbkdf2' => [ - 'class' => Pbkdf2Password::class, + 'factory' => [ AbstractPbkdf2Password::class, 'newInstance' ], 'algo' => 'sha512', 'cost' => '30000', 'length' => '64', diff --git a/includes/config-schema.php b/includes/config-schema.php index 7090608b795..c226b1b377c 100755 --- a/includes/config-schema.php +++ b/includes/config-schema.php @@ -973,7 +973,10 @@ return [ 'cost' => 9, ], 'pbkdf2' => [ - 'class' => 'Pbkdf2Password', + 'factory' => [ + 0 => 'AbstractPbkdf2Password', + 1 => 'newInstance', + ], 'algo' => 'sha512', 'cost' => '30000', 'length' => '64', diff --git a/includes/password/AbstractPbkdf2Password.php b/includes/password/AbstractPbkdf2Password.php new file mode 100644 index 00000000000..3fe84a4629e --- /dev/null +++ b/includes/password/AbstractPbkdf2Password.php @@ -0,0 +1,148 @@ += 1.0.2) + return function_exists( 'openssl_pbkdf2' ) && OPENSSL_VERSION_NUMBER >= 0x1000106f; + } + + protected function getDefaultParams(): array { + return [ + 'algo' => $this->config['algo'], + 'rounds' => $this->config['cost'], + 'length' => $this->config['length'] + ]; + } + + protected function getDelimiter(): string { + return ':'; + } + + public function crypt( string $password ): void { + if ( count( $this->args ) == 0 ) { + $this->args[] = base64_encode( random_bytes( 16 ) ); + } + + $algo = $this->params['algo']; + $salt = base64_decode( $this->args[0] ); + $rounds = (int)$this->params['rounds']; + $length = (int)$this->params['length']; + + $digestAlgo = $this->getDigestAlgo( $algo ); + if ( $digestAlgo === null ) { + throw new PasswordError( "Unknown or unsupported algo: $algo" ); + } + if ( $rounds <= 0 || $rounds >= 0x7fffffff ) { + throw new PasswordError( 'Invalid number of rounds.' ); + } + if ( $length <= 0 || $length >= 0x7fffffff ) { + throw new PasswordError( 'Invalid length.' ); + } + + $hash = $this->pbkdf2( $digestAlgo, $password, $salt, $rounds, $length ); + $this->hash = base64_encode( $hash ); + } + + /** + * Get the implementation specific name for a hash algorithm. + * + * @param string $algo Algorithm specified in the password hash string + * @return string|null $algo Implementation specific name, or null if unsupported + */ + abstract protected function getDigestAlgo( string $algo ): ?string; + + /** + * Call the PBKDF2 implementation, which hashes the password. + * + * @param string $digestAlgo Implementation specific hash algorithm name + * @param string $password Password to hash + * @param string $salt Salt as a binary string + * @param int $rounds Number of iterations + * @param int $length Length of the hash value in bytes + * @return string Hash value as a binary string + * @throws PasswordError If an internal error occurs in hashing + */ + abstract protected function pbkdf2( + string $digestAlgo, + string $password, + string $salt, + int $rounds, + int $length + ): string; +} diff --git a/includes/password/PasswordFactory.php b/includes/password/PasswordFactory.php index 0a9b234189f..9ce3435b2c5 100644 --- a/includes/password/PasswordFactory.php +++ b/includes/password/PasswordFactory.php @@ -23,6 +23,7 @@ declare( strict_types = 1 ); use MediaWiki\MainConfigNames; +use Wikimedia\ObjectFactory\ObjectFactory; /** * Factory class for creating and checking Password objects @@ -130,7 +131,7 @@ final class PasswordFactory { } /** - * Create a new Hash object from an existing string hash + * Create a new Password object from an existing string hash * * Parse the type of a hash and create a new hash object based on the parsed type. * Pass the raw hash to the constructor of the new object. Use InvalidPassword type @@ -148,34 +149,44 @@ final class PasswordFactory { } $type = substr( $hash, 1, strpos( $hash, ':', 1 ) - 1 ); - if ( !isset( $this->types[$type] ) ) { - throw new PasswordError( "Unrecognized password hash type $type." ); - } - - $config = $this->types[$type]; - - return new $config['class']( $this, $config, $hash ); + return $this->newFromTypeAndHash( $type, $hash ); } /** - * Make a new default password of the given type. + * Create a new Password object of the given type. * * @param string $type Existing type * @return Password - * @throws PasswordError If hash is invalid or type is not recognized + * @throws PasswordError If type is not recognized */ public function newFromType( string $type ): Password { + return $this->newFromTypeAndHash( $type, null ); + } + + /** + * Create a new Password object of the given type, optionally with an existing string hash. + * + * @param string $type Existing type + * @param string|null $hash Existing hash + * @return Password + * @throws PasswordError If hash is invalid or type is not recognized + */ + private function newFromTypeAndHash( string $type, ?string $hash ): Password { if ( !isset( $this->types[$type] ) ) { throw new PasswordError( "Unrecognized password hash type $type." ); } $config = $this->types[$type]; - return new $config['class']( $this, $config ); + // @phan-suppress-next-line PhanTypeInvalidCallableArrayKey + return ObjectFactory::getObjectFromSpec( $config, [ + 'extraArgs' => [ $this, $config, $hash ], + 'assertClass' => Password::class, + ] ); } /** - * Create a new Hash object from a plaintext password + * Create a new Password object from a plaintext password * * If no existing object is given, make a new default object. If one is given, clone that * object. Then pass the plaintext to Password::crypt(). @@ -190,8 +201,7 @@ final class PasswordFactory { } if ( $existing === null ) { - $config = $this->types[$this->default]; - $obj = new $config['class']( $this, $config ); + $obj = $this->newFromType( $this->default ); } else { $obj = clone $existing; } diff --git a/includes/password/Pbkdf2Password.php b/includes/password/Pbkdf2Password.php deleted file mode 100644 index 09fc86f23fd..00000000000 --- a/includes/password/Pbkdf2Password.php +++ /dev/null @@ -1,72 +0,0 @@ - $this->config['algo'], - 'rounds' => $this->config['cost'], - 'length' => $this->config['length'] - ]; - } - - protected function getDelimiter(): string { - return ':'; - } - - public function crypt( string $password ): void { - if ( count( $this->args ) == 0 ) { - $this->args[] = base64_encode( random_bytes( 16 ) ); - } - - try { - $hash = hash_pbkdf2( - $this->params['algo'], - $password, - base64_decode( $this->args[0] ), - (int)$this->params['rounds'], - (int)$this->params['length'], - true - ); - - // PHP < 8 raises a warning in case of an error, such as unknown algorithm... - if ( !is_string( $hash ) ) { - throw new PasswordError( 'Error when hashing password.' ); - } - } catch ( ValueError $e ) { - // ...while PHP 8 throws ValueError - throw new PasswordError( 'Error when hashing password.', 0, $e ); - } - - $this->hash = base64_encode( $hash ); - } -} diff --git a/includes/password/Pbkdf2PasswordUsingHashExtension.php b/includes/password/Pbkdf2PasswordUsingHashExtension.php new file mode 100644 index 00000000000..3c00049664a --- /dev/null +++ b/includes/password/Pbkdf2PasswordUsingHashExtension.php @@ -0,0 +1,62 @@ + + */ + private static $digestAlgos; + + /** + * List of hash algorithms we support and OpenSSL's names for them. + * + * We include only the algorithms that make sense to support, rather than + * all potentially available algorithms. In particular, we do not include: + * + * - Broken algorithms, such as "md5" and "sha1" + * - Algorithms no longer available by default, such as "whirlpool" + * - Algorithms that perform especially poorly on server CPUs relative + * to other available hardware (as of 2022, this includes "sha3-512"; + * see ) + * - Variants for which there is no reason for use, such as "sha384" + * (a truncated "sha512" that starts with a different initial state) + * + * The array keys should match the algorithm names known to hash_pbkdf2(). + */ + private const DIGEST_ALGOS = [ + 'sha256' => 'sha256', + 'sha512' => 'sha512', + ]; + + protected function isSupported(): bool { + return self::canUseOpenSSL(); + } + + protected function getDigestAlgo( string $algo ): ?string { + if ( !isset( self::$digestAlgos ) ) { + self::$digestAlgos = array_intersect( self::DIGEST_ALGOS, openssl_get_md_methods() ); + } + return self::$digestAlgos[$algo] ?? null; + } + + protected function pbkdf2( + string $digestAlgo, + string $password, + string $salt, + int $rounds, + int $length + ): string { + // Clear error string + while ( openssl_error_string() !== false ); + $hash = openssl_pbkdf2( $password, $salt, $length, $rounds, $digestAlgo ); + if ( !is_string( $hash ) ) { + throw new PasswordError( 'Error when hashing password: ' . openssl_error_string() ); + } + return $hash; + } +} diff --git a/tests/common/TestsAutoLoader.php b/tests/common/TestsAutoLoader.php index b226d4715b4..82fe17088c1 100644 --- a/tests/common/TestsAutoLoader.php +++ b/tests/common/TestsAutoLoader.php @@ -260,8 +260,9 @@ $wgAutoloadClasses += [ # tests/phpunit/unit/includes/utils 'UrlUtilsProviders' => "$testDir/phpunit/unit/includes/utils/UrlUtilsProviders.php", - # tests/phpunit/includes/unit/password + # tests/phpunit/unit/includes/password 'PasswordTestCase' => "$testDir/phpunit/unit/includes/password/PasswordTestCase.php", + 'Pbkdf2PasswordTestCase' => "$testDir/phpunit/unit/includes/password/Pbkdf2PasswordTestCase.php", # tests/phpunit/integration/includes/user 'MediaWiki\Tests\User\ActorStoreTestBase' => "$testDir/phpunit/integration/includes/user/ActorStoreTestBase.php", diff --git a/tests/phpunit/unit/includes/password/AbstractPbkdf2PasswordTest.php b/tests/phpunit/unit/includes/password/AbstractPbkdf2PasswordTest.php new file mode 100644 index 00000000000..e77b1bae605 --- /dev/null +++ b/tests/phpunit/unit/includes/password/AbstractPbkdf2PasswordTest.php @@ -0,0 +1,42 @@ +createStub( AbstractPbkdf2Password::class ) ); + $password = AbstractPbkdf2Password::newInstance( + $factory, + [ + 'type' => 'pbkdf2', + 'class' => $class, + 'factory' => [ AbstractPbkdf2Password::class, 'newInstance' ], + 'algo' => 'sha256', + 'cost' => '10000', + 'length' => '128', + ] + ); + $this->assertInstanceOf( $class, $password ); + } + + /** + * @requires function openssl_pbkdf2 + */ + public function testNewInstanceUsesOpenSSLByDefault() { + $factory = new PasswordFactory(); + $password = AbstractPbkdf2Password::newInstance( + $factory, + [ + 'type' => 'pbkdf2', + 'factory' => [ AbstractPbkdf2Password::class, 'newInstance' ], + 'algo' => 'sha256', + 'cost' => '10000', + 'length' => '128', + ] + ); + $this->assertInstanceOf( Pbkdf2PasswordUsingOpenSSL::class, $password ); + } +} diff --git a/tests/phpunit/unit/includes/password/EncryptedPasswordTest.php b/tests/phpunit/unit/includes/password/EncryptedPasswordTest.php index b2ae909149a..f0425c8e488 100644 --- a/tests/phpunit/unit/includes/password/EncryptedPasswordTest.php +++ b/tests/phpunit/unit/includes/password/EncryptedPasswordTest.php @@ -34,7 +34,7 @@ class EncryptedPasswordTest extends PasswordTestCase { 'cipher' => 'aes-256-cbc', ], 'pbkdf2' => [ - 'class' => Pbkdf2Password::class, + 'class' => Pbkdf2PasswordUsingHashExtension::class, 'algo' => 'sha256', 'cost' => '10', 'length' => '64', diff --git a/tests/phpunit/unit/includes/password/LayeredParameterizedPasswordTest.php b/tests/phpunit/unit/includes/password/LayeredParameterizedPasswordTest.php index 2865acadd86..3b720dedfb4 100644 --- a/tests/phpunit/unit/includes/password/LayeredParameterizedPasswordTest.php +++ b/tests/phpunit/unit/includes/password/LayeredParameterizedPasswordTest.php @@ -18,7 +18,7 @@ class LayeredParameterizedPasswordTest extends PasswordTestCase { ], ], 'testLargeLayeredBottom' => [ - 'class' => Pbkdf2Password::class, + 'class' => Pbkdf2PasswordUsingHashExtension::class, 'algo' => 'sha512', 'cost' => 1024, 'length' => 512, diff --git a/tests/phpunit/unit/includes/password/Pbkdf2PasswordFallbackTest.php b/tests/phpunit/unit/includes/password/Pbkdf2PasswordFallbackTest.php deleted file mode 100644 index a58cf2a7919..00000000000 --- a/tests/phpunit/unit/includes/password/Pbkdf2PasswordFallbackTest.php +++ /dev/null @@ -1,27 +0,0 @@ - [ - 'class' => Pbkdf2Password::class, - 'algo' => 'sha256', - 'cost' => '10000', - 'length' => '128', - ], - ]; - } - - public static function providePasswordTests() { - return [ - [ true, ":pbkdf2:sha1:1:20:c2FsdA==:DGDID5YfDnHzqbUkr2ASBi/gN6Y=", 'password' ], - [ true, ":pbkdf2:sha1:2:20:c2FsdA==:6mwBTcctb4zNHtkqzh1B8NjeiVc=", 'password' ], - [ true, ":pbkdf2:sha1:4096:20:c2FsdA==:SwB5AbdlSJq+rUnZJvch0GWkKcE=", 'password' ], - [ true, ":pbkdf2:sha1:4096:16:c2EAbHQ=:Vvpqp1VICZ3MN9fwNCXgww==", "pass\x00word" ], - ]; - } -} diff --git a/tests/phpunit/unit/includes/password/Pbkdf2PasswordTest.php b/tests/phpunit/unit/includes/password/Pbkdf2PasswordTest.php deleted file mode 100644 index b03fe99bdac..00000000000 --- a/tests/phpunit/unit/includes/password/Pbkdf2PasswordTest.php +++ /dev/null @@ -1,44 +0,0 @@ - [ - 'class' => Pbkdf2Password::class, - 'algo' => 'sha256', - 'cost' => '10000', - 'length' => '128', - ] ]; - } - - public static function providePasswordTests() { - return [ - [ true, ":pbkdf2:sha1:1:20:c2FsdA==:DGDID5YfDnHzqbUkr2ASBi/gN6Y=", 'password' ], - [ true, ":pbkdf2:sha1:2:20:c2FsdA==:6mwBTcctb4zNHtkqzh1B8NjeiVc=", 'password' ], - [ true, ":pbkdf2:sha1:4096:20:c2FsdA==:SwB5AbdlSJq+rUnZJvch0GWkKcE=", 'password' ], - [ true, ":pbkdf2:sha1:4096:16:c2EAbHQ=:Vvpqp1VICZ3MN9fwNCXgww==", "pass\x00word" ], - ]; - } - - public function testCryptThrows() { - $factory = new PasswordFactory(); - $password = new Pbkdf2Password( - $factory, - [ - 'type' => 'pbkdf2', - 'algo' => 'fail', - 'cost' => '10000', - 'length' => '128', - ] - ); - $this->expectException( PasswordError::class ); - $this->expectExceptionMessage( 'Error when hashing password.' ); - @$password->crypt( 'whatever' ); - } -} diff --git a/tests/phpunit/unit/includes/password/Pbkdf2PasswordTestCase.php b/tests/phpunit/unit/includes/password/Pbkdf2PasswordTestCase.php new file mode 100644 index 00000000000..4219f238272 --- /dev/null +++ b/tests/phpunit/unit/includes/password/Pbkdf2PasswordTestCase.php @@ -0,0 +1,74 @@ + [ + 'class' => static::getPbkdf2PasswordClass(), + 'algo' => 'sha256', + 'cost' => '10000', + 'length' => '128', + ] ]; + } + + public static function providePasswordTests() { + return [ + [ true, ":pbkdf2:sha512:1:20:c2FsdA==:hn9wzxreAs/zdSWZo6U9xK80x6Y=", 'password' ], + [ true, ":pbkdf2:sha512:2:20:c2FsdA==:4dnBaqaBcIpF9cfE4hXOtm4BGi4=", 'password' ], + [ true, ":pbkdf2:sha512:4096:20:c2FsdA==:0Zexsz2wFD4BixLz0dFHnmzevcw=", 'password' ], + [ true, ":pbkdf2:sha512:4096:16:c2EAbHQ=:nZ6cTNIf5L4k1bgkTHWWZQ==", "pass\x00word" ], + ]; + } + + public function testCryptThrowsOnInvalidAlgo() { + $factory = new PasswordFactory(); + $class = static::getPbkdf2PasswordClass(); + $password = new $class( + $factory, + [ + 'type' => 'pbkdf2', + 'algo' => 'fail', + 'cost' => '10000', + 'length' => '128', + ] + ); + $this->expectException( PasswordError::class ); + $this->expectExceptionMessage( 'Unknown or unsupported algo: fail' ); + $password->crypt( 'whatever' ); + } + + public function testCryptThrowsOnInvalidCost() { + $factory = new PasswordFactory(); + $class = static::getPbkdf2PasswordClass(); + $password = new $class( + $factory, + [ + 'type' => 'pbkdf2', + 'algo' => 'sha256', + 'cost' => '0', + 'length' => '128', + ] + ); + $this->expectException( PasswordError::class ); + $this->expectExceptionMessage( 'Invalid number of rounds.' ); + $password->crypt( 'whatever' ); + } + + public function testCryptThrowsOnInvalidLength() { + $factory = new PasswordFactory(); + $class = static::getPbkdf2PasswordClass(); + $password = new $class( + $factory, + [ + 'type' => 'pbkdf2', + 'algo' => 'sha256', + 'cost' => '10000', + 'length' => '0', + ] + ); + $this->expectException( PasswordError::class ); + $this->expectExceptionMessage( 'Invalid length.' ); + $password->crypt( 'whatever' ); + } +} diff --git a/tests/phpunit/unit/includes/password/Pbkdf2PasswordUsingHashExtensionTest.php b/tests/phpunit/unit/includes/password/Pbkdf2PasswordUsingHashExtensionTest.php new file mode 100644 index 00000000000..08d47f230b3 --- /dev/null +++ b/tests/phpunit/unit/includes/password/Pbkdf2PasswordUsingHashExtensionTest.php @@ -0,0 +1,24 @@ +