From 1e19d154bad60b11da133d4a88581518d177e6a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Mon, 16 Mar 2020 22:39:49 +0100 Subject: [PATCH 1/3] MDL-68183 auth: Fix the performance of forgotten password user search 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 | 30 ++++++++++++++--- login/tests/lib_test.php | 71 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 5 deletions(-) diff --git a/login/lib.php b/login/lib.php index b391ea7b712..e0644050bdb 100644 --- a/login/lib.php +++ b/login/lib.php @@ -24,6 +24,9 @@ * @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); @@ -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. - // 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. + // + // 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) . ")"; - $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); + $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. diff --git a/login/tests/lib_test.php b/login/tests/lib_test.php index a32941c6390..02bc226104f 100644 --- a/login/tests/lib_test.php +++ b/login/tests/lib_test.php @@ -355,4 +355,75 @@ class core_login_lib_testcase extends advanced_testcase { $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; + } + } From d23fba1deb88a878dce50afb6f2dab6bdaa15b2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Tue, 17 Mar 2020 13:04:07 +0100 Subject: [PATCH 2/3] MDL-68183 auth: Fix the performance of signup_validate_data search query When searching for other users with the same 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 in a subselect, which can use the index. Only then we perform the additional accent-sensitive search on this limited set or records. --- lib/authlib.php | 19 ++++++++++--- lib/tests/authlib_test.php | 58 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 4 deletions(-) diff --git a/lib/authlib.php b/lib/authlib.php index 1d2f972e912..d1604e6a18a 100644 --- a/lib/authlib.php +++ b/lib/authlib.php @@ -1027,14 +1027,25 @@ function signup_validate_data($data, $files) { $errors['email'] = get_string('invalidemail'); } else if (empty($CFG->allowaccountssameemail)) { - // Make a case-insensitive query for the given email address. - $select = $DB->sql_equal('email', ':email', false) . ' AND mnethostid = :mnethostid'; + // Emails in Moodle as case-insensitive and accents-sensitive. Such a combination can lead to very slow queries + // on some DBs such as MySQL. So we first get the list of candidate users in a subselect via more effective + // accent-insensitive query that can make use of the index and only then we search within that limited subset. + $sql = "SELECT 'x' + FROM {user} + WHERE " . $DB->sql_equal('email', ':email1', false, true) . " + AND id IN (SELECT id + FROM {user} + WHERE " . $DB->sql_equal('email', ':email2', false, false) . " + AND mnethostid = :mnethostid)"; + $params = array( - 'email' => $data['email'], + 'email1' => $data['email'], + 'email2' => $data['email'], 'mnethostid' => $CFG->mnet_localhost_id, ); + // If there are other user(s) that already have the same email, show an error. - if ($DB->record_exists_select('user', $select, $params)) { + if ($DB->record_exists_sql($sql, $params)) { $forgotpasswordurl = new moodle_url('/login/forgot_password.php'); $forgotpasswordlink = html_writer::link($forgotpasswordurl, get_string('emailexistshintlink')); $errors['email'] = get_string('emailexists') . ' ' . get_string('emailexistssignuphint', 'moodle', $forgotpasswordlink); diff --git a/lib/tests/authlib_test.php b/lib/tests/authlib_test.php index e59e3833296..ffa7128bc06 100644 --- a/lib/tests/authlib_test.php +++ b/lib/tests/authlib_test.php @@ -345,4 +345,62 @@ class core_authlib_testcase extends advanced_testcase { $this->assertInstanceOf('coding_exception', $e); } } + + /** + * Test the {@link signup_validate_data()} duplicate email validation. + */ + public function test_signup_validate_data_same_email() { + global $CFG; + require_once($CFG->libdir . '/authlib.php'); + require_once($CFG->dirroot . '/user/profile/lib.php'); + + $this->resetAfterTest(); + + $CFG->registerauth = 'email'; + $CFG->passwordpolicy = false; + + // In this test, we want to check accent-sensitive email search. However, accented 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); + }; + + // Check that two users cannot share the same email address if the site is configured so. + // Emails in Moodle are supposed to be case-insensitive (and accent-sensitive but accents are not yet supported). + $CFG->allowaccountssameemail = false; + + $u1 = $this->getDataGenerator()->create_user([ + 'username' => 'abcdef', + 'email' => 'abcdef@example.com', + ]); + + $formdata = [ + 'username' => 'newuser', + 'firstname' => 'First', + 'lastname' => 'Last', + 'password' => 'weak', + 'email' => 'ABCDEF@example.com', + ]; + + $errors = signup_validate_data($formdata, []); + $this->assertContains('This email address is already registered.', $errors['email']); + + // Emails are accent-sensitive though so if we change a -> á in the u1's email, it should pass. + // Please note that Moodle does not normally support such emails yet. We test the DB search sensitivity here. + $formdata['email'] = 'ábcdef@example.com'; + $errors = signup_validate_data($formdata, []); + $this->assertArrayNotHasKey('email', $errors); + + // Check that users can share the same email if the site is configured so. + $CFG->allowaccountssameemail = true; + $formdata['email'] = 'abcdef@example.com'; + $errors = signup_validate_data($formdata, []); + $this->assertArrayNotHasKey('email', $errors); + + // Restore the original email address validator. + moodle_phpmailer::$validator = $defaultvalidator; + } } From bee8448520ff5b5548a480a7a8f3492a34bc1c02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Mudr=C3=A1k?= Date: Tue, 17 Mar 2020 14:44:50 +0100 Subject: [PATCH 3/3] MDL-68183 auth: Fix the performance of get_complete_user_data search When searching for the user by a case-insensitive field (email address is the only one supported now), the performance may be very poor as the DB cannot use the index due to the LOWER() operation and the full sequential scan of all the user records is performed. On some DBs such as MySQL, this can be significantly improved by pre-filtering the users with accent-insensitive search. So we first perform accent-insensitive search for potential candidates in a subselect, which can use the index. Only then we perform the additional accent-sensitive search on this limited set or records. --- lib/moodlelib.php | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/moodlelib.php b/lib/moodlelib.php index 71dcf51dac9..e5ee465dc53 100644 --- a/lib/moodlelib.php +++ b/lib/moodlelib.php @@ -4877,11 +4877,15 @@ function get_complete_user_data($field, $value, $mnethostid = null, $throwexcept // Build the WHERE clause for an SQL query. $params = array('fieldval' => $value); - // Do a case-insensitive query, if necessary. + // Do a case-insensitive query, if necessary. These are generally very expensive. The performance can be improved on some DBs + // such as MySQL by pre-filtering users with accent-insensitive subselect. if (in_array($field, $caseinsensitivefields)) { $fieldselect = $DB->sql_equal($field, ':fieldval', false); + $idsubselect = $DB->sql_equal($field, ':fieldval2', false, false); + $params['fieldval2'] = $value; } else { $fieldselect = "$field = :fieldval"; + $idsubselect = ''; } $constraints = "$fieldselect AND deleted <> 1"; @@ -4898,6 +4902,10 @@ function get_complete_user_data($field, $value, $mnethostid = null, $throwexcept $constraints .= " AND mnethostid = :mnethostid"; } + if ($idsubselect) { + $constraints .= " AND id IN (SELECT id FROM {user} WHERE {$idsubselect})"; + } + // Get all the basic user data. try { // Make sure that there's only a single record that matches our query.