From 8e54eca05a65c6231b02510e1917af0c9191e549 Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Thu, 19 Mar 2015 12:40:07 +0000 Subject: [PATCH] Issue #2455079 by klausi, David_Rothstein, pwolanin, benjy, Berdir: Password reset URL access bypass fixes from SA-CORE-2015-001 need to be ported to Drupal 8 --- .../user/src/Controller/UserController.php | 4 +- .../modules/user/src/Tests/UserCancelTest.php | 14 +++--- .../user/src/Tests/UserPasswordResetTest.php | 45 ++++++++++++++++++- core/modules/user/user.module | 12 ++--- 4 files changed, 59 insertions(+), 16 deletions(-) diff --git a/core/modules/user/src/Controller/UserController.php b/core/modules/user/src/Controller/UserController.php index a34c30bf471..0243c33690a 100644 --- a/core/modules/user/src/Controller/UserController.php +++ b/core/modules/user/src/Controller/UserController.php @@ -122,7 +122,7 @@ class UserController extends ControllerBase { drupal_set_message($this->t('You have tried to use a one-time login link that has expired. Please request a new one using the form below.')); return $this->redirect('user.pass'); } - elseif ($user->isAuthenticated() && ($timestamp >= $user->getLastLoginTime()) && ($timestamp <= $current) && ($hash === user_pass_rehash($user->getPassword(), $timestamp, $user->getLastLoginTime()))) { + elseif ($user->isAuthenticated() && ($timestamp >= $user->getLastLoginTime()) && ($timestamp <= $current) && ($hash === user_pass_rehash($user->getPassword(), $timestamp, $user->getLastLoginTime(), $user->id()))) { $expiration_date = $user->getLastLoginTime() ? $this->dateFormatter->format($timestamp + $timeout) : NULL; return $this->formBuilder()->getForm('Drupal\user\Form\UserPasswordResetForm', $user, $expiration_date, $timestamp, $hash); } @@ -197,7 +197,7 @@ class UserController extends ControllerBase { $account_data = $this->userData->get('user', $user->id()); if (isset($account_data['cancel_method']) && !empty($timestamp) && !empty($hashed_pass)) { // Validate expiration and hashed password/login. - if ($timestamp <= $current && $current - $timestamp < $timeout && $user->id() && $timestamp >= $user->getLastLoginTime() && $hashed_pass == user_pass_rehash($user->getPassword(), $timestamp, $user->getLastLoginTime())) { + if ($timestamp <= $current && $current - $timestamp < $timeout && $user->id() && $timestamp >= $user->getLastLoginTime() && $hashed_pass == user_pass_rehash($user->getPassword(), $timestamp, $user->getLastLoginTime(), $user->id())) { $edit = array( 'user_cancel_notify' => isset($account_data['cancel_notify']) ? $account_data['cancel_notify'] : $this->config('user.settings')->get('notify.status_canceled'), ); diff --git a/core/modules/user/src/Tests/UserCancelTest.php b/core/modules/user/src/Tests/UserCancelTest.php index 029a66c0a96..267f1d22163 100644 --- a/core/modules/user/src/Tests/UserCancelTest.php +++ b/core/modules/user/src/Tests/UserCancelTest.php @@ -56,7 +56,7 @@ class UserCancelTest extends WebTestBase { // Attempt bogus account cancellation request confirmation. $timestamp = $account->getLastLoginTime(); - $this->drupalGet("user/" . $account->id() . "/cancel/confirm/$timestamp/" . user_pass_rehash($account->getPassword(), $timestamp, $account->getLastLoginTime())); + $this->drupalGet("user/" . $account->id() . "/cancel/confirm/$timestamp/" . user_pass_rehash($account->getPassword(), $timestamp, $account->getLastLoginTime(), $account->id())); $this->assertResponse(403, 'Bogus cancelling request rejected.'); $account = user_load($account->id()); $this->assertTrue($account->isActive(), 'User account was not canceled.'); @@ -153,14 +153,14 @@ class UserCancelTest extends WebTestBase { // Attempt bogus account cancellation request confirmation. $bogus_timestamp = $timestamp + 60; - $this->drupalGet("user/" . $account->id() . "/cancel/confirm/$bogus_timestamp/" . user_pass_rehash($account->getPassword(), $bogus_timestamp, $account->getLastLoginTime())); + $this->drupalGet("user/" . $account->id() . "/cancel/confirm/$bogus_timestamp/" . user_pass_rehash($account->getPassword(), $bogus_timestamp, $account->getLastLoginTime(), $account->id())); $this->assertText(t('You have tried to use an account cancellation link that has expired. Please request a new one using the form below.'), 'Bogus cancelling request rejected.'); $account = user_load($account->id()); $this->assertTrue($account->isActive(), 'User account was not canceled.'); // Attempt expired account cancellation request confirmation. $bogus_timestamp = $timestamp - 86400 - 60; - $this->drupalGet("user/" . $account->id() . "/cancel/confirm/$bogus_timestamp/" . user_pass_rehash($account->getPassword(), $bogus_timestamp, $account->getLastLoginTime())); + $this->drupalGet("user/" . $account->id() . "/cancel/confirm/$bogus_timestamp/" . user_pass_rehash($account->getPassword(), $bogus_timestamp, $account->getLastLoginTime(), $account->id())); $this->assertText(t('You have tried to use an account cancellation link that has expired. Please request a new one using the form below.'), 'Expired cancel account request rejected.'); $account = user_load($account->id(), TRUE); $this->assertTrue($account->isActive(), 'User account was not canceled.'); @@ -198,7 +198,7 @@ class UserCancelTest extends WebTestBase { $this->assertText(t('A confirmation request to cancel your account has been sent to your email address.'), 'Account cancellation request mailed message displayed.'); // Confirm account cancellation request. - $this->drupalGet("user/" . $account->id() . "/cancel/confirm/$timestamp/" . user_pass_rehash($account->getPassword(), $timestamp, $account->getLastLoginTime())); + $this->drupalGet("user/" . $account->id() . "/cancel/confirm/$timestamp/" . user_pass_rehash($account->getPassword(), $timestamp, $account->getLastLoginTime(), $account->id())); $account = user_load($account->id(), TRUE); $this->assertTrue($account->isBlocked(), 'User has been blocked.'); @@ -253,7 +253,7 @@ class UserCancelTest extends WebTestBase { $this->assertText(t('A confirmation request to cancel your account has been sent to your email address.'), 'Account cancellation request mailed message displayed.'); // Confirm account cancellation request. - $this->drupalGet("user/" . $account->id() . "/cancel/confirm/$timestamp/" . user_pass_rehash($account->getPassword(), $timestamp, $account->getLastLoginTime())); + $this->drupalGet("user/" . $account->id() . "/cancel/confirm/$timestamp/" . user_pass_rehash($account->getPassword(), $timestamp, $account->getLastLoginTime(), $account->id())); $account = user_load($account->id(), TRUE); $this->assertTrue($account->isBlocked(), 'User has been blocked.'); @@ -310,7 +310,7 @@ class UserCancelTest extends WebTestBase { $this->assertText(t('A confirmation request to cancel your account has been sent to your email address.'), 'Account cancellation request mailed message displayed.'); // Confirm account cancellation request. - $this->drupalGet("user/" . $account->id() . "/cancel/confirm/$timestamp/" . user_pass_rehash($account->getPassword(), $timestamp, $account->getLastLoginTime())); + $this->drupalGet("user/" . $account->id() . "/cancel/confirm/$timestamp/" . user_pass_rehash($account->getPassword(), $timestamp, $account->getLastLoginTime(), $account->id())); $this->assertFalse(user_load($account->id(), TRUE), 'User is not found in the database.'); // Confirm that user's content has been attributed to anonymous user. @@ -379,7 +379,7 @@ class UserCancelTest extends WebTestBase { $this->assertText(t('A confirmation request to cancel your account has been sent to your email address.'), 'Account cancellation request mailed message displayed.'); // Confirm account cancellation request. - $this->drupalGet("user/" . $account->id() . "/cancel/confirm/$timestamp/" . user_pass_rehash($account->getPassword(), $timestamp, $account->getLastLoginTime())); + $this->drupalGet("user/" . $account->id() . "/cancel/confirm/$timestamp/" . user_pass_rehash($account->getPassword(), $timestamp, $account->getLastLoginTime(), $account->id())); $this->assertFalse(user_load($account->id(), TRUE), 'User is not found in the database.'); // Confirm that user's content has been deleted. diff --git a/core/modules/user/src/Tests/UserPasswordResetTest.php b/core/modules/user/src/Tests/UserPasswordResetTest.php index 535ef4c991e..5b61fda76f6 100644 --- a/core/modules/user/src/Tests/UserPasswordResetTest.php +++ b/core/modules/user/src/Tests/UserPasswordResetTest.php @@ -8,6 +8,7 @@ namespace Drupal\user\Tests; use Drupal\simpletest\WebTestBase; +use Drupal\user\Entity\User; /** * Ensure that password reset methods work as expected. @@ -137,14 +138,14 @@ class UserPasswordResetTest extends WebTestBase { $timeout = $this->config('user.settings')->get('password_reset_timeout'); $bogus_timestamp = REQUEST_TIME - $timeout - 60; $_uid = $this->account->id(); - $this->drupalGet("user/reset/$_uid/$bogus_timestamp/" . user_pass_rehash($this->account->getPassword(), $bogus_timestamp, $this->account->getLastLoginTime())); + $this->drupalGet("user/reset/$_uid/$bogus_timestamp/" . user_pass_rehash($this->account->getPassword(), $bogus_timestamp, $this->account->getLastLoginTime(), $this->account->id())); $this->assertText(t('You have tried to use a one-time login link that has expired. Please request a new one using the form below.'), 'Expired password reset request rejected.'); // Create a user, block the account, and verify that a login link is denied. $timestamp = REQUEST_TIME - 1; $blocked_account = $this->drupalCreateUser()->block(); $blocked_account->save(); - $this->drupalGet("user/reset/" . $blocked_account->id() . "/$timestamp/" . user_pass_rehash($blocked_account->getPassword(), $timestamp, $blocked_account->getLastLoginTime())); + $this->drupalGet("user/reset/" . $blocked_account->id() . "/$timestamp/" . user_pass_rehash($blocked_account->getPassword(), $timestamp, $blocked_account->getLastLoginTime(), $this->account->id())); $this->assertResponse(403); } @@ -177,4 +178,44 @@ class UserPasswordResetTest extends WebTestBase { $this->drupalGet('user/password', array('query' => array('name' => $edit['name']))); $this->assertFieldByName('name', $edit['name'], 'User name found.'); } + + /** + * Make sure that users cannot forge password reset URLs of other users. + */ + function testResetImpersonation() { + // Create two identical user accounts except for the user name. They must + // have the same empty password, so we can't use $this->drupalCreateUser(). + $edit = array(); + $edit['name'] = $this->randomMachineName(); + $edit['mail'] = $edit['name'] . '@example.com'; + $edit['status'] = 1; + $user1 = User::create($edit); + $user1->save(); + + $edit['name'] = $this->randomMachineName(); + $user2 = User::create($edit); + $user2->save(); + + // Unique password hashes are automatically generated, the only way to + // change that is to update it directly in the database. + db_update('users_field_data') + ->fields(['pass' => NULL]) + ->condition('uid', [$user1->id(), $user2->id()], 'IN') + ->execute(); + \Drupal::entityManager()->getStorage('user')->resetCache(); + $user1 = User::load($user1->id()); + $user2 = User::load($user2->id()); + + $this->assertEqual($user1->getPassword(), $user2->getPassword(), 'Both users have the same password hash.'); + + // The password reset URL must not be valid for the second user when only + // the user ID is changed in the URL. + $reset_url = user_pass_reset_url($user1); + $attack_reset_url = str_replace("user/reset/{$user1->id()}", "user/reset/{$user2->id()}", $reset_url); + $this->drupalGet($attack_reset_url); + $this->assertNoText($user2->getUsername(), 'The invalid password reset page does not show the user name.'); + $this->assertUrl('user/password', array(), 'The user is redirected to the password reset request page.'); + $this->assertText('You have tried to use a one-time login link that has either been used or is no longer valid. Please request a new one using the form below.'); + } + } diff --git a/core/modules/user/user.module b/core/modules/user/user.module index fbc900cf996..665b43d2b10 100644 --- a/core/modules/user/user.module +++ b/core/modules/user/user.module @@ -635,7 +635,7 @@ function user_pass_reset_url($account, $options = array()) { array( 'uid' => $account->id(), 'timestamp' => $timestamp, - 'hash' => user_pass_rehash($account->getPassword(), $timestamp, $account->getLastLoginTime()), + 'hash' => user_pass_rehash($account->getPassword(), $timestamp, $account->getLastLoginTime(), $account->id()), ), array( 'absolute' => TRUE, @@ -672,7 +672,7 @@ function user_cancel_url($account, $options = array()) { return \Drupal::url('user.cancel_confirm', [ 'user' => $account->id(), 'timestamp' => $timestamp, - 'hashed_pass' => user_pass_rehash($account->getPassword(), $timestamp, $account->getLastLoginTime()) + 'hashed_pass' => user_pass_rehash($account->getPassword(), $timestamp, $account->getLastLoginTime(), $account->id()) ], $url_options); } @@ -694,12 +694,14 @@ function user_cancel_url($account, $options = array()) { * A UNIX timestamp, typically REQUEST_TIME. * @param int $login * The UNIX timestamp of the user's last login. + * @param int $uid + * The user ID. * - * @return + * @return string * A string that is safe for use in URLs and SQL statements. */ -function user_pass_rehash($password, $timestamp, $login) { - return Crypt::hmacBase64($timestamp . $login, Settings::getHashSalt() . $password); +function user_pass_rehash($password, $timestamp, $login, $uid) { + return Crypt::hmacBase64($timestamp . $login . $uid, Settings::getHashSalt() . $password); } /**