Commit 21d1b479 authored by catch's avatar catch
Browse files

Issue #2515050 by alexpott, pwolanin, YesCT, David_Rothstein, prasad_gogate: A...

Issue #2515050 by alexpott, pwolanin, YesCT, David_Rothstein, prasad_gogate: A valid one-time login link may be leaked by the referer header to 3rd parties
parent 7fd06936
......@@ -6,10 +6,13 @@
use Drupal\Component\Utility\Xss;
use Drupal\Core\Controller\ControllerBase;
use Drupal\Core\Datetime\DateFormatterInterface;
use Drupal\user\Form\UserPasswordResetForm;
use Drupal\user\UserDataInterface;
use Drupal\user\UserInterface;
use Drupal\user\UserStorageInterface;
use Psr\Log\LoggerInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
/**
......@@ -38,6 +41,13 @@ class UserController extends ControllerBase {
*/
protected $userData;
/**
* A logger instance.
*
* @var \Psr\Log\LoggerInterface
*/
protected $logger;
/**
* Constructs a UserController object.
*
......@@ -47,11 +57,14 @@ class UserController extends ControllerBase {
* The user storage.
* @param \Drupal\user\UserDataInterface $user_data
* The user data service.
* @param \Psr\Log\LoggerInterface $logger
* A logger instance.
*/
public function __construct(DateFormatterInterface $date_formatter, UserStorageInterface $user_storage, UserDataInterface $user_data) {
public function __construct(DateFormatterInterface $date_formatter, UserStorageInterface $user_storage, UserDataInterface $user_data, LoggerInterface $logger) {
$this->dateFormatter = $date_formatter;
$this->userStorage = $user_storage;
$this->userData = $user_data;
$this->logger = $logger;
}
/**
......@@ -61,38 +74,51 @@ public static function create(ContainerInterface $container) {
return new static(
$container->get('date.formatter'),
$container->get('entity.manager')->getStorage('user'),
$container->get('user.data')
$container->get('user.data'),
$container->get('logger.factory')->get('user')
);
}
/**
* Returns the user password reset page.
* Redirects to the user password reset form.
*
* In order to never disclose a reset link via a referrer header this
* controller must always return a redirect response.
*
* @param \Symfony\Component\HttpFoundation\Request $request
* The request.
* @param int $uid
* UID of user requesting reset.
* User ID of the user requesting reset.
* @param int $timestamp
* The current timestamp.
* @param string $hash
* Login link hash.
*
* @return array|\Symfony\Component\HttpFoundation\RedirectResponse
* The form structure or a redirect response.
*
* @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
* If the login link is for a blocked user or invalid user ID.
* @return \Symfony\Component\HttpFoundation\RedirectResponse
* The redirect response.
*/
public function resetPass($uid, $timestamp, $hash) {
public function resetPass(Request $request, $uid, $timestamp, $hash) {
$account = $this->currentUser();
$config = $this->config('user.settings');
// When processing the one-time login link, we have to make sure that a user
// isn't already logged in.
if ($account->isAuthenticated()) {
// The current user is already logged in.
if ($account->id() == $uid) {
user_logout();
// We need to begin the redirect process again because logging out will
// destroy the session.
return $this->redirect(
'user.reset',
[
'uid' => $uid,
'timestamp' => $timestamp,
'hash' => $hash,
]
);
}
// A different user is already logged in on the computer.
else {
/** @var \Drupal\user\UserInterface $reset_link_user */
if ($reset_link_user = $this->userStorage->load($uid)) {
drupal_set_message($this->t('Another user (%other_user) is already logged into the site on this computer, but you tried to use a one-time link for user %resetting_user. Please <a href=":logout">log out</a> and try using the link again.',
array('%other_user' => $account->getUsername(), '%resetting_user' => $reset_link_user->getUsername(), ':logout' => $this->url('user.logout'))), 'warning');
......@@ -104,33 +130,117 @@ public function resetPass($uid, $timestamp, $hash) {
return $this->redirect('<front>');
}
}
// The current user is not logged in, so check the parameters.
$session = $request->getSession();
$session->set('pass_reset_hash', $hash);
$session->set('pass_reset_timeout', $timestamp);
return $this->redirect(
'user.reset.form',
['uid' => $uid]
);
}
/**
* Returns the user password reset form.
*
* @param \Symfony\Component\HttpFoundation\Request $request
* The request.
* @param int $uid
* User ID of the user requesting reset.
*
* @return array|\Symfony\Component\HttpFoundation\RedirectResponse
* The form structure or a redirect response.
*
* @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
* If the pass_reset_timeout or pass_reset_hash are not available in the
* session. Or if $uid is for a blocked user or invalid user ID.
*/
public function getResetPassForm(Request $request, $uid) {
$session = $request->getSession();
$timestamp = $session->get('pass_reset_timeout');
$hash = $session->get('pass_reset_hash');
// As soon as the session variables are used they are removed to prevent the
// hash and timestamp from being leaked unexpectedly. This could occur if
// the user does not click on the log in button on the form.
$session->remove('pass_reset_timeout');
$session->remove('pass_reset_hash');
if (!$hash || !$timestamp) {
throw new AccessDeniedHttpException();
}
/** @var \Drupal\user\UserInterface $user */
$user = $this->userStorage->load($uid);
if ($user === NULL || !$user->isActive()) {
// Blocked or invalid user ID, so deny access. The parameters will be in
// the watchdog's URL for the administrator to check.
throw new AccessDeniedHttpException();
}
// Time out, in seconds, until login URL expires.
$timeout = $config->get('password_reset_timeout');
$current = REQUEST_TIME;
$timeout = $this->config('user.settings')->get('password_reset_timeout');
$expiration_date = $user->getLastLoginTime() ? $this->dateFormatter->format($timestamp + $timeout) : NULL;
return $this->formBuilder()->getForm(UserPasswordResetForm::class, $user, $expiration_date, $timestamp, $hash);
}
/* @var \Drupal\user\UserInterface $user */
/**
* Validates user, hash, and timestamp; logs the user in if correct.
*
* @param int $uid
* User ID of the user requesting reset.
* @param int $timestamp
* The current timestamp.
* @param string $hash
* Login link hash.
*
* @return \Symfony\Component\HttpFoundation\RedirectResponse
* Returns a redirect to the user edit form if the information is correct.
* If the information is incorrect redirects to 'user.pass' route with a
* message for the user.
*
* @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
* If $uid is for a blocked user or invalid user ID.
*/
public function resetPassLogin($uid, $timestamp, $hash) {
// The current user is not logged in, so check the parameters.
$current = REQUEST_TIME;
/** @var \Drupal\user\UserInterface $user */
$user = $this->userStorage->load($uid);
// Verify that the user exists and is active.
if ($user && $user->isActive()) {
// No time out for first time login.
if ($user->getLastLoginTime() && $current - $timestamp > $timeout) {
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.'), 'error');
return $this->redirect('user.pass');
}
elseif ($user->isAuthenticated() && ($timestamp >= $user->getLastLoginTime()) && ($timestamp <= $current) && Crypt::hashEquals($hash, user_pass_rehash($user, $timestamp))) {
$expiration_date = $user->getLastLoginTime() ? $this->dateFormatter->format($timestamp + $timeout) : NULL;
return $this->formBuilder()->getForm('Drupal\user\Form\UserPasswordResetForm', $user, $expiration_date, $timestamp, $hash);
}
else {
drupal_set_message($this->t('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.'), 'error');
return $this->redirect('user.pass');
}
if ($user === NULL || !$user->isActive()) {
// Blocked or invalid user ID, so deny access. The parameters will be in
// the watchdog's URL for the administrator to check.
throw new AccessDeniedHttpException();
}
// Blocked or invalid user ID, so deny access. The parameters will be in the
// watchdog's URL for the administrator to check.
throw new AccessDeniedHttpException();
// Time out, in seconds, until login URL expires.
$timeout = $this->config('user.settings')->get('password_reset_timeout');
// No time out for first time login.
if ($user->getLastLoginTime() && $current - $timestamp > $timeout) {
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.'), 'error');
return $this->redirect('user.pass');
}
elseif ($user->isAuthenticated() && ($timestamp >= $user->getLastLoginTime()) && ($timestamp <= $current) && Crypt::hashEquals($hash, user_pass_rehash($user, $timestamp))) {
user_login_finalize($user);
$this->logger->notice('User %name used one-time login link at time %timestamp.', ['%name' => $user->getDisplayName(), '%timestamp' => $timestamp]);
drupal_set_message($this->t('You have just used your one-time login link. It is no longer necessary to use this link to log in. Please change your password.'));
// Let the user's password be changed without the current password
// check.
$token = Crypt::randomBytesBase64(55);
$_SESSION['pass_reset_' . $user->id()] = $token;
return $this->redirect(
'entity.user.edit_form',
['user' => $user->id()],
[
'query' => ['pass-reset-token' => $token],
'absolute' => TRUE,
]
);
}
drupal_set_message($this->t('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.'), 'error');
return $this->redirect('user.pass');
}
/**
......
......@@ -4,42 +4,14 @@
use Drupal\Core\Form\FormStateInterface;
use Drupal\Core\Session\AccountInterface;
use Drupal\Component\Utility\Crypt;
use Drupal\Core\Form\FormBase;
use Psr\Log\LoggerInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Drupal\Core\Url;
/**
* Form controller for the user password forms.
*/
class UserPasswordResetForm extends FormBase {
/**
* A logger instance.
*
* @var \Psr\Log\LoggerInterface
*/
protected $logger;
/**
* Constructs a new UserPasswordResetForm.
*
* @param \Psr\Log\LoggerInterface $logger
* A logger instance.
*/
public function __construct(LoggerInterface $logger) {
$this->logger = $logger;
}
/**
* {@inheritdoc}
*/
public static function create(ContainerInterface $container) {
return new static(
$container->get('logger.factory')->get('user')
);
}
/**
* {@inheritdoc}
*/
......@@ -75,20 +47,17 @@ public function buildForm(array $form, FormStateInterface $form_state, AccountIn
$form['#title'] = $this->t('Set password');
}
$form['user'] = array(
'#type' => 'value',
'#value' => $user,
);
$form['timestamp'] = array(
'#type' => 'value',
'#value' => $timestamp,
);
$form['help'] = array('#markup' => '<p>' . $this->t('This login can be used only once.') . '</p>');
$form['actions'] = array('#type' => 'actions');
$form['actions']['submit'] = array(
'#type' => 'submit',
'#value' => $this->t('Log in'),
);
$form['#action'] = Url::fromRoute('user.reset.login', [
'uid' => $user->id(),
'timestamp' => $timestamp,
'hash' => $hash,
])->toString();
return $form;
}
......@@ -96,22 +65,8 @@ public function buildForm(array $form, FormStateInterface $form_state, AccountIn
* {@inheritdoc}
*/
public function submitForm(array &$form, FormStateInterface $form_state) {
/** @var $user \Drupal\user\UserInterface */
$user = $form_state->getValue('user');
user_login_finalize($user);
$this->logger->notice('User %name used one-time login link at time %timestamp.', array('%name' => $user->getUsername(), '%timestamp' => $form_state->getValue('timestamp')));
drupal_set_message($this->t('You have just used your one-time login link. It is no longer necessary to use this link to log in. Please change your password.'));
// Let the user's password be changed without the current password check.
$token = Crypt::randomBytesBase64(55);
$_SESSION['pass_reset_' . $user->id()] = $token;
$form_state->setRedirect(
'entity.user.edit_form',
array('user' => $user->id()),
array(
'query' => array('pass-reset-token' => $token),
'absolute' => TRUE,
)
);
// This form works by submitting the hash and timestamp to the user.reset
// route with a 'login' action.
}
}
......@@ -2,6 +2,8 @@
namespace Drupal\user\Tests;
use Drupal\Component\Render\FormattableMarkup;
use Drupal\Core\Url;
use Drupal\system\Tests\Cache\PageCacheTagsTestBase;
use Drupal\user\Entity\User;
......@@ -68,6 +70,11 @@ protected function setUp() {
* Tests password reset functionality.
*/
function testUserPasswordReset() {
// Verify that accessing the password reset form without having the session
// variables set results in an access denied message.
$this->drupalGet(Url::fromRoute('user.reset.form', ['uid' => $this->account->id()]));
$this->assertResponse(403);
// Try to reset the password for an invalid account.
$this->drupalGet('user/password');
......@@ -88,6 +95,9 @@ function testUserPasswordReset() {
$resetURL = $this->getResetURL();
$this->drupalGet($resetURL);
// Ensure that the current url does not contain the hash and timestamp.
$this->assertUrl(Url::fromRoute('user.reset.form', ['uid' => $this->account->id()]));
$this->assertFalse($this->drupalGetHeader('X-Drupal-Cache'));
// Ensure the password reset URL is not cached.
......@@ -125,6 +135,7 @@ function testUserPasswordReset() {
// Log out, and try to log in again using the same one-time link.
$this->drupalLogout();
$this->drupalGet($resetURL);
$this->drupalPostForm(NULL, NULL, t('Log in'));
$this->assertText(t('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.'), 'One-time link is no longer valid.');
// Request a new password again, this time using the email address.
......@@ -149,6 +160,7 @@ function testUserPasswordReset() {
$bogus_timestamp = REQUEST_TIME - $timeout - 60;
$_uid = $this->account->id();
$this->drupalGet("user/reset/$_uid/$bogus_timestamp/" . user_pass_rehash($this->account, $bogus_timestamp));
$this->drupalPostForm(NULL, NULL, t('Log in'));
$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.
......@@ -175,7 +187,31 @@ function testUserPasswordReset() {
$this->account->setEmail("1" . $this->account->getEmail());
$this->account->save();
$this->drupalGet($old_email_reset_link);
$this->drupalPostForm(NULL, NULL, t('Log in'));
$this->assertText(t('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.'), 'One-time link is no longer valid.');
// Verify a password reset link will automatically log a user when /login is
// appended.
$this->drupalGet('user/password');
$edit = array('name' => $this->account->getUsername());
$this->drupalPostForm(NULL, $edit, t('Submit'));
$reset_url = $this->getResetURL();
$this->drupalGet($reset_url . '/login');
$this->assertLink(t('Log out'));
$this->assertTitle(t('@name | @site', array('@name' => $this->account->getUsername(), '@site' => $this->config('system.site')->get('name'))), 'Logged in using password reset link.');
// Ensure blocked and deleted accounts can't access the user.reset.login
// route.
$this->drupalLogout();
$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, $timestamp) . '/login');
$this->assertResponse(403);
$blocked_account->delete();
$this->drupalGet("user/reset/" . $blocked_account->id() . "/$timestamp/" . user_pass_rehash($blocked_account, $timestamp) . '/login');
$this->assertResponse(403);
}
/**
......@@ -195,6 +231,25 @@ public function getResetURL() {
* Test user password reset while logged in.
*/
public function testUserPasswordResetLoggedIn() {
$another_account = $this->drupalCreateUser();
$this->drupalLogin($another_account);
$this->drupalGet('user/password');
$this->drupalPostForm(NULL, NULL, t('Submit'));
// Click the reset URL while logged and change our password.
$resetURL = $this->getResetURL();
// Log in as a different user.
$this->drupalLogin($this->account);
$this->drupalGet($resetURL);
$this->assertRaw(new FormattableMarkup(
'Another user (%other_user) is already logged into the site on this computer, but you tried to use a one-time link for user %resetting_user. Please <a href=":logout">log out</a> and try using the link again.',
['%other_user' => $this->account->getUsername(), '%resetting_user' => $another_account->getUsername(), ':logout' => Url::fromRoute('user.logout')->toString()]
));
$another_account->delete();
$this->drupalGet($resetURL);
$this->assertText('The one-time login link you clicked is invalid.');
// Log in.
$this->drupalLogin($this->account);
......@@ -212,6 +267,14 @@ public function testUserPasswordResetLoggedIn() {
$edit = array('pass[pass1]' => $password, 'pass[pass2]' => $password);
$this->drupalPostForm(NULL, $edit, t('Save'));
$this->assertText(t('The changes have been saved.'), 'Password changed.');
// Logged in users should not be able to access the user.reset.login or the
// user.reset.form routes.
$timestamp = REQUEST_TIME - 1;
$this->drupalGet("user/reset/" . $this->account->id() . "/$timestamp/" . user_pass_rehash($this->account, $timestamp) . '/login');
$this->assertResponse(403);
$this->drupalGet("user/reset/" . $this->account->id());
$this->assertResponse(403);
}
/**
......@@ -265,6 +328,7 @@ function testResetImpersonation() {
$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->drupalPostForm(NULL, NULL, t('Log in'));
$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.');
......
......@@ -168,6 +168,17 @@ user.cancel_confirm:
_entity_access: 'user.delete'
user: \d+
user.reset.login:
path: '/user/reset/{uid}/{timestamp}/{hash}/login'
defaults:
_controller: '\Drupal\user\Controller\UserController::resetPassLogin'
_title: 'Reset password'
requirements:
_user_is_logged_in: 'FALSE'
options:
_maintenance_access: TRUE
no_cache: TRUE
user.reset:
path: '/user/reset/{uid}/{timestamp}/{hash}'
defaults:
......@@ -178,3 +189,14 @@ user.reset:
options:
_maintenance_access: TRUE
no_cache: TRUE
user.reset.form:
path: '/user/reset/{uid}'
defaults:
_controller: '\Drupal\user\Controller\UserController::getResetPassForm'
_title: 'Reset password'
requirements:
_user_is_logged_in: 'FALSE'
options:
_maintenance_access: TRUE
no_cache: TRUE
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment