From 63cf33d8252edc81888be4dbb9b7542e4b289ca2 Mon Sep 17 00:00:00 2001 From: Tyler Romeo Date: Fri, 26 Dec 2014 11:29:15 -0500 Subject: [PATCH] SECURITY: Set maximal password length for DoS Prevent DoS attacks caused by the amount of time it takes to hash long passwords by setting a limit on password length. Slightly restructures the behavior of User::checkPasswordValidity in order to accommodate for the difference between passwords the user should be able to log in with and passwords they should not. Bug: T64685 Change-Id: I24f33474c6f934fb8d94bb054dc23093abfebd5e --- includes/DefaultSettings.php | 12 ++++++++ includes/User.php | 40 +++++++++++++++++--------- includes/specials/SpecialUserlogin.php | 13 ++++----- languages/i18n/en.json | 1 + languages/i18n/qqq.json | 1 + tests/phpunit/includes/UserTest.php | 14 ++++++++- 6 files changed, 58 insertions(+), 23 deletions(-) diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index 5ab557eb8a..84dc3aa7b8 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -4226,6 +4226,18 @@ $wgPasswordSalt = true; */ $wgMinimalPasswordLength = 1; +/** + * Specifies the maximal length of a user password (T64685). + * + * It is not recommended to make this greater than the default, as it can + * allow DoS attacks by users setting really long passwords. In addition, + * this should not be lowered too much, as it enforces weak passwords. + * + * @warning Unlike other password settings, user with passwords greater than + * the maximum will not be able to log in. + */ +$wgMaximalPasswordLength = 4096; + /** * Specifies if users should be sent to a password-reset form on login, if their * password doesn't meet the requirements of User::isValidPassword(). diff --git a/includes/User.php b/includes/User.php index 89ff299f3e..2e88978110 100644 --- a/includes/User.php +++ b/includes/User.php @@ -826,15 +826,24 @@ class User implements IDBAccessObject { } /** - * Check if this is a valid password for this user. Status will be good if - * the password is valid, or have an array of error messages if not. + * Check if this is a valid password for this user + * + * Create a Status object based on the password's validity. + * The Status should be set to fatal if the user should not + * be allowed to log in, and should have any errors that + * would block changing the password. + * + * If the return value of this is not OK, the password + * should not be checked. If the return value is not Good, + * the password can be checked, but the user should not be + * able to set their password to this. * * @param string $password Desired password * @return Status * @since 1.23 */ public function checkPasswordValidity( $password ) { - global $wgMinimalPasswordLength, $wgContLang; + global $wgMinimalPasswordLength, $wgMaximalPasswordLength, $wgContLang; static $blockedLogins = array( 'Useruser' => 'Passpass', 'Useruser1' => 'Passpass1', # r75589 @@ -854,6 +863,10 @@ class User implements IDBAccessObject { if ( strlen( $password ) < $wgMinimalPasswordLength ) { $status->error( 'passwordtooshort', $wgMinimalPasswordLength ); return $status; + } elseif ( strlen( $password ) > $wgMaximalPasswordLength ) { + // T64685: Password too long, might cause DoS attack + $status->fatal( 'passwordtoolong', $wgMaximalPasswordLength ); + return $status; } elseif ( $wgContLang->lc( $password ) == $wgContLang->lc( $this->mName ) ) { $status->error( 'password-name-match' ); return $status; @@ -2382,17 +2395,9 @@ class User implements IDBAccessObject { throw new PasswordError( wfMessage( 'password-change-forbidden' )->text() ); } - if ( !$this->isValidPassword( $str ) ) { - global $wgMinimalPasswordLength; - $valid = $this->getPasswordValidity( $str ); - if ( is_array( $valid ) ) { - $message = array_shift( $valid ); - $params = $valid; - } else { - $message = $valid; - $params = array( $wgMinimalPasswordLength ); - } - throw new PasswordError( wfMessage( $message, $params )->text() ); + $status = $this->checkPasswordValidity( $str ); + if ( !$status->isGood() ) { + throw new PasswordError( $status->getMessage()->text() ); } } @@ -3900,6 +3905,13 @@ class User implements IDBAccessObject { $this->loadPasswords(); + // Some passwords will give a fatal Status, which means there is + // some sort of technical or security reason for this password to + // be completely invalid and should never be checked (e.g., T64685) + if ( !$this->checkPasswordValidity( $password )->isOK() ) { + return false; + } + // Certain authentication plugins do NOT want to save // domain passwords in a mysql database, so we should // check this (in case $wgAuth->strict() is false). diff --git a/includes/specials/SpecialUserlogin.php b/includes/specials/SpecialUserlogin.php index 44240d8ddd..d6634a83c0 100644 --- a/includes/specials/SpecialUserlogin.php +++ b/includes/specials/SpecialUserlogin.php @@ -545,14 +545,11 @@ class LoginForm extends SpecialPage { return Status::newFatal( 'badretype' ); } - # check for minimal password length - $valid = $u->getPasswordValidity( $this->mPassword ); - if ( $valid !== true ) { - if ( !is_array( $valid ) ) { - $valid = array( $valid, $wgMinimalPasswordLength ); - } - - return call_user_func_array( 'Status::newFatal', $valid ); + # check for password validity, return a fatal Status if invalid + $validity = $u->checkPasswordValidity( $this->mPassword ); + if ( !$validity->isGood() ) { + $validity->ok = false; // make sure this Status is fatal + return $validity; } } diff --git a/languages/i18n/en.json b/languages/i18n/en.json index f81567fdeb..fb7056c39e 100644 --- a/languages/i18n/en.json +++ b/languages/i18n/en.json @@ -458,6 +458,7 @@ "wrongpassword": "Incorrect password entered.\nPlease try again.", "wrongpasswordempty": "Password entered was blank.\nPlease try again.", "passwordtooshort": "Passwords must be at least {{PLURAL:$1|1 character|$1 characters}}.", + "passwordtoolong": "Passwords cannot be longer than {{PLURAL:$1|1 character|$1 characters}}.", "password-name-match": "Your password must be different from your username.", "password-login-forbidden": "The use of this username and password has been forbidden.", "mailmypassword": "Reset password", diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json index fef9ac8b2e..b7c31fc7cf 100644 --- a/languages/i18n/qqq.json +++ b/languages/i18n/qqq.json @@ -623,6 +623,7 @@ "wrongpassword": "Used as error message when the provided password is wrong.\nThis message is used in html.\n{{Identical|Please try again}}", "wrongpasswordempty": "Error message displayed when entering a blank password.\n{{Identical|Please try again}}", "passwordtooshort": "This message is shown in [[Special:Preferences]] and [[Special:CreateAccount]].\n\nParameters:\n* $1 - the minimum number of characters in the password", + "passwordtoolong": "This message is shown in [[Special:Preferences]], [[Special:CreateAccount]], and [[Special:Userlogin]].\n\nParameters:\n* $1 - the maximum number of characters in the password", "password-name-match": "Used as error message when password validity check failed.", "password-login-forbidden": "Error message shown when the user has tried to log in using one of the special username/password combinations used for MediaWiki testing. (See [[mwr:75589]], [[mwr:75605]].)", "mailmypassword": "Used as label for Submit button in [[Special:PasswordReset]].\n{{Identical|Reset password}}", diff --git a/tests/phpunit/includes/UserTest.php b/tests/phpunit/includes/UserTest.php index 860529e152..73e50518f8 100644 --- a/tests/phpunit/includes/UserTest.php +++ b/tests/phpunit/includes/UserTest.php @@ -306,7 +306,10 @@ class UserTest extends MediaWikiTestCase { * @covers User::isValidPassword() */ public function testCheckPasswordValidity() { - $this->setMwGlobals( 'wgMinimalPasswordLength', 6 ); + $this->setMwGlobals( array( + 'wgMinimalPasswordLength' => 6, + 'wgMaximalPasswordLength' => 30, + ) ); $user = User::newFromName( 'Useruser' ); // Sanity $this->assertTrue( $user->isValidPassword( 'Password1234' ) ); @@ -314,10 +317,19 @@ class UserTest extends MediaWikiTestCase { // Minimum length $this->assertFalse( $user->isValidPassword( 'a' ) ); $this->assertFalse( $user->checkPasswordValidity( 'a' )->isGood() ); + $this->assertTrue( $user->checkPasswordValidity( 'a' )->isOK() ); $this->assertEquals( 'passwordtooshort', $user->getPasswordValidity( 'a' ) ); + // Maximum length + $longPass = str_repeat( 'a', 31 ); + $this->assertFalse( $user->isValidPassword( $longPass ) ); + $this->assertFalse( $user->checkPasswordValidity( $longPass )->isGood() ); + $this->assertFalse( $user->checkPasswordValidity( $longPass )->isOK() ); + $this->assertEquals( 'passwordtoolong', $user->getPasswordValidity( $longPass ) ); + // Matches username $this->assertFalse( $user->checkPasswordValidity( 'Useruser' )->isGood() ); + $this->assertTrue( $user->checkPasswordValidity( 'Useruser' )->isOK() ); $this->assertEquals( 'password-name-match', $user->getPasswordValidity( 'Useruser' ) ); // On the forbidden list -- 2.20.1