MDL-68183 auth: Fix the performance of forgotten password user search
authorDavid Mudrák <david@moodle.com>
Mon, 16 Mar 2020 21:39:49 +0000 (22:39 +0100)
committerDavid Mudrák <david@moodle.com>
Tue, 17 Mar 2020 13:52:32 +0000 (14:52 +0100)
When searching for the user matching the given email address, we perform
the case-insensitive and accent-sensitive search. That may be expensive
as some DBs such as MySQL cannot use the index in that case. Instead,
sequential scan of all the user records is performed and the comparison
uses the LOWER function to filter the matching records. This leads to
significant performance heavy queries which in turn represent a surface
for DoS attacks.

For that reason, we first perform accent-insensitive search for
potential candidates, which can use the index. Only then we perform the
additional accent-sensitive search on this limited set or records.

login/lib.php
login/tests/lib_test.php

index b391ea7..e064405 100644 (file)
@@ -24,6 +24,9 @@
  * @copyright  Peter Bulmer
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
  * @copyright  Peter Bulmer
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
+
+defined('MOODLE_INTERNAL') || die();
+
 define('PWRESET_STATUS_NOEMAILSENT', 1);
 define('PWRESET_STATUS_TOKENSENT', 2);
 define('PWRESET_STATUS_OTHEREMAILSENT', 3);
 define('PWRESET_STATUS_NOEMAILSENT', 1);
 define('PWRESET_STATUS_TOKENSENT', 2);
 define('PWRESET_STATUS_OTHEREMAILSENT', 3);
@@ -93,14 +96,31 @@ function core_login_process_password_reset($username, $email) {
         $user = $DB->get_record('user', $userparams);
     } else {
         // Try to load the user record based on email address.
         $user = $DB->get_record('user', $userparams);
     } else {
         // Try to load the user record based on email address.
-        // this is tricky because
+        // This is tricky because:
         // 1/ the email is not guaranteed to be unique - TODO: send email with all usernames to select the account for pw reset
         // 2/ mailbox may be case sensitive, the email domain is case insensitive - let's pretend it is all case-insensitive.
         // 1/ the email is not guaranteed to be unique - TODO: send email with all usernames to select the account for pw reset
         // 2/ mailbox may be case sensitive, the email domain is case insensitive - let's pretend it is all case-insensitive.
-
-        $select = $DB->sql_like('email', ':email', false, true, false, '|') .
-                " AND mnethostid = :mnethostid AND deleted=0 AND suspended=0";
-        $params = array('email' => $DB->sql_like_escape($email, '|'), 'mnethostid' => $CFG->mnet_localhost_id);
-        $user = $DB->get_record_select('user', $select, $params, '*', IGNORE_MULTIPLE);
+        //
+        // The case-insensitive + accent-sensitive search may be expensive as some DBs such as MySQL cannot use the
+        // index in that case. For that reason, we first perform accent-insensitive search in a subselect for potential
+        // candidates (which can use the index) and only then perform the additional accent-sensitive search on this
+        // limited set of records in the outer select.
+        $sql = "SELECT *
+                  FROM {user}
+                 WHERE " . $DB->sql_equal('email', ':email1', false, true) . "
+                   AND id IN (SELECT id
+                                FROM {user}
+                               WHERE mnethostid = :mnethostid
+                                 AND deleted = 0
+                                 AND suspended = 0
+                                 AND " . $DB->sql_equal('email', ':email2', false, false) . ")";
+
+        $params = array(
+            'email1' => $email,
+            'email2' => $email,
+            'mnethostid' => $CFG->mnet_localhost_id,
+        );
+
+        $user = $DB->get_record_sql($sql, $params, IGNORE_MULTIPLE);
     }
 
     // Target user details have now been identified, or we know that there is no such account.
     }
 
     // Target user details have now been identified, or we know that there is no such account.
index a32941c..02bc226 100644 (file)
@@ -355,4 +355,75 @@ class core_login_lib_testcase extends advanced_testcase {
             $this->assertArrayNotHasKey('email', $validationerrors);
         }
     }
             $this->assertArrayNotHasKey('email', $validationerrors);
         }
     }
+
+    /**
+     * Test searching for the user record by matching the provided email address when resetting password.
+     *
+     * Email addresses should be handled as case-insensitive but accent sensitive.
+     */
+    public function test_core_login_process_password_reset_email_sensitivity() {
+        global $CFG;
+        require_once($CFG->libdir.'/phpmailer/moodle_phpmailer.php');
+
+        $this->resetAfterTest();
+        $sink = $this->redirectEmails();
+        $CFG->protectusernames = 0;
+
+        // In this test, we need to mock sending emails on non-ASCII email addresses. However, such email addresses do
+        // not pass the default `validate_email()` and Moodle does not yet provide a CFG switch to allow such emails.
+        // So we inject our own validation method here and revert it back once we are done. This custom validator method
+        // is identical to the default 'php' validator with the only difference: it has the FILTER_FLAG_EMAIL_UNICODE
+        // set so that it allows to use non-ASCII characters in email addresses.
+        $defaultvalidator = moodle_phpmailer::$validator;
+        moodle_phpmailer::$validator = function($address) {
+            return (bool) filter_var($address, FILTER_VALIDATE_EMAIL, FILTER_FLAG_EMAIL_UNICODE);
+        };
+
+        // Emails are treated as case-insensitive when searching for the matching user account.
+        $u1 = $this->getDataGenerator()->create_user(['email' => 'priliszlutouckykunupeldabelskeody@example.com']);
+
+        list($status, $notice, $url) = core_login_process_password_reset(null, 'PrIlIsZlUtOuCkYKuNupELdAbElSkEoDy@eXaMpLe.CoM');
+
+        $this->assertSame('emailresetconfirmsent', $status);
+        $emails = $sink->get_messages();
+        $this->assertCount(1, $emails);
+        $email = reset($emails);
+        $this->assertSame($u1->email, $email->to);
+        $sink->clear();
+
+        // There may exist two users with same emails.
+        $u2 = $this->getDataGenerator()->create_user(['email' => 'PRILISZLUTOUCKYKUNUPELDABELSKEODY@example.com']);
+
+        list($status, $notice, $url) = core_login_process_password_reset(null, 'PrIlIsZlUtOuCkYKuNupELdAbElSkEoDy@eXaMpLe.CoM');
+
+        $this->assertSame('emailresetconfirmsent', $status);
+        $emails = $sink->get_messages();
+        $this->assertCount(1, $emails);
+        $email = reset($emails);
+        $this->assertSame(core_text::strtolower($u2->email), core_text::strtolower($email->to));
+        $sink->clear();
+
+        // However, emails are accent sensitive - note this is the u1's email with a single character a -> á changed.
+        list($status, $notice, $url) = core_login_process_password_reset(null, 'priliszlutouckykunupeldábelskeody@example.com');
+
+        $this->assertSame('emailpasswordconfirmnotsent', $status);
+        $emails = $sink->get_messages();
+        $this->assertCount(0, $emails);
+        $sink->clear();
+
+        $u3 = $this->getDataGenerator()->create_user(['email' => 'PřílišŽluťoučkýKůňÚpělĎálebskéÓdy@example.com']);
+
+        list($status, $notice, $url) = core_login_process_password_reset(null, 'pŘÍLIŠžLuŤOuČkÝkŮŇúPĚLďÁLEBSKÉóDY@eXaMpLe.CoM');
+
+        $this->assertSame('emailresetconfirmsent', $status);
+        $emails = $sink->get_messages();
+        $this->assertCount(1, $emails);
+        $email = reset($emails);
+        $this->assertSame($u3->email, $email->to);
+        $sink->clear();
+
+        // Restore the original email address validator.
+        moodle_phpmailer::$validator = $defaultvalidator;
+    }
+
 }
 }