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

diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php
index 5ab557e..84dc3aa 100644 (file)
--- 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 89ff299..2e88978 100644 (file)
--- 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 44240d8..d6634a8 100644 (file)
--- 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 f81567f..fb7056c 100644 (file)
--- 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 fef9ac8..b7c31fc 100644 (file)
--- 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 860529e..73e5051 100644 (file)
--- 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