From 6232e9676cd4acb67a747d5463ce9a8092eb802f Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 11 Aug 2014 12:13:09 -0700 Subject: [PATCH] Don't send reset links to unverified addresses on accounts with verified addresses Summary: Via HackerOne. If a user adds an email address and typos it, entering `alinculne@gmailo.com`, and it happens to be a valid address which an evil user controls, the evil user can request a password reset and compromise the account. This strains the imagination, but we can implement a better behavior cheaply. - If an account has any verified addresses, only send to verified addresses. - If an account has no verified addresses (e.g., is a new account), send to any address. We've also received several reports about reset links not being destroyed as aggressively as researchers expect. While there's no specific scenario where this does any harm, revoke all outstanding reset tokens when a reset link is used to improve the signal/noise ratio of the reporting channel. Test Plan: - Tried to send a reset link to an unverified address on an account with a verified address (got new error). - Tried to send a reset link to a verified adddress on an account with a verified address (got email). - Tried to send a reset link to an invalid address (got old error). - Tried to send a reset link to an unverified address on an account with only unverified addresses -- a new user (got email). - Requested several reset links, used one, verified all the others were revoked. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Differential Revision: https://secure.phabricator.com/D10206 --- .../PhabricatorAuthOneTimeLoginController.php | 13 ++++++++++-- .../PhabricatorEmailLoginController.php | 20 +++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php index 12decda80d..a40ab02d89 100644 --- a/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php +++ b/src/applications/auth/controller/PhabricatorAuthOneTimeLoginController.php @@ -98,8 +98,17 @@ final class PhabricatorAuthOneTimeLoginController // to go through a second round of email verification. $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - // Nuke the token so that this URI is one-time only. - $token->delete(); + // Nuke the token and all other outstanding password reset tokens. + // There is no particular security benefit to destroying them all, but + // it should reduce HackerOne reports of nebulous harm. + + PhabricatorAuthTemporaryToken::revokeTokens( + $target_user, + array($target_user->getPHID()), + array( + PhabricatorAuthSessionEngine::ONETIME_TEMPORARY_TOKEN_TYPE, + PhabricatorAuthSessionEngine::PASSWORD_TEMPORARY_TOKEN_TYPE, + )); if ($target_email) { id(new PhabricatorUserEditor()) diff --git a/src/applications/auth/controller/PhabricatorEmailLoginController.php b/src/applications/auth/controller/PhabricatorEmailLoginController.php index 7ec9ae97b1..8b5a6b19c5 100644 --- a/src/applications/auth/controller/PhabricatorEmailLoginController.php +++ b/src/applications/auth/controller/PhabricatorEmailLoginController.php @@ -58,6 +58,26 @@ final class PhabricatorEmailLoginController $e_email = pht('Invalid'); } + // If this address is unverified, only send a reset link to it if + // the account has no verified addresses. This prevents an opportunistic + // attacker from compromising an account if a user adds an email + // address but mistypes it and doesn't notice. + + // (For a newly created account, all the addresses may be unverified, + // which is why we'll send to an unverified address in that case.) + + if ($target_email && !$target_email->getIsVerified()) { + $verified_addresses = id(new PhabricatorUserEmail())->loadAllWhere( + 'userPHID = %s AND isVerified = 1', + $target_email->getUserPHID()); + if ($verified_addresses) { + $errors[] = pht( + 'That email addess is not verified. You can only send '. + 'password reset links to a verified address.'); + $e_email = pht('Unverified'); + } + } + if (!$errors) { $engine = new PhabricatorAuthSessionEngine(); $uri = $engine->getOneTimeLoginURI(