1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-23 22:10:55 +01:00

Add a rate limit for guessing old passwords when changing passwords

Summary:
Depends on D18904. Ref T13043. If an attacker compromises a victim's session and bypasses their MFA, they can try to guess the user's current account password by making repeated requests to change it: if they guess the right "Old Password", they get a different error than if they don't.

I don't think this is really a very serious concern (the attacker already got a session and MFA, if configured, somehow; many installs don't use passwords anyway) but we get occasional reports about it from HackerOne. Technically, it's better policy to rate limit it, and this should reduce the reports we receive.

Test Plan: Tried to change password over and over again, eventually got rated limited. Used `bin/auth unlimit` to clear the limit, changed password normally without issues.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18906
This commit is contained in:
epriestley 2018-01-21 17:44:00 -08:00
parent cab2bba6f2
commit 026ec11b9d
3 changed files with 45 additions and 5 deletions

View file

@ -2033,6 +2033,7 @@ phutil_register_library_map(array(
'PhabricatorAuthApplication' => 'applications/auth/application/PhabricatorAuthApplication.php', 'PhabricatorAuthApplication' => 'applications/auth/application/PhabricatorAuthApplication.php',
'PhabricatorAuthAuthFactorPHIDType' => 'applications/auth/phid/PhabricatorAuthAuthFactorPHIDType.php', 'PhabricatorAuthAuthFactorPHIDType' => 'applications/auth/phid/PhabricatorAuthAuthFactorPHIDType.php',
'PhabricatorAuthAuthProviderPHIDType' => 'applications/auth/phid/PhabricatorAuthAuthProviderPHIDType.php', 'PhabricatorAuthAuthProviderPHIDType' => 'applications/auth/phid/PhabricatorAuthAuthProviderPHIDType.php',
'PhabricatorAuthChangePasswordAction' => 'applications/auth/action/PhabricatorAuthChangePasswordAction.php',
'PhabricatorAuthConduitAPIMethod' => 'applications/auth/conduit/PhabricatorAuthConduitAPIMethod.php', 'PhabricatorAuthConduitAPIMethod' => 'applications/auth/conduit/PhabricatorAuthConduitAPIMethod.php',
'PhabricatorAuthConduitTokenRevoker' => 'applications/auth/revoker/PhabricatorAuthConduitTokenRevoker.php', 'PhabricatorAuthConduitTokenRevoker' => 'applications/auth/revoker/PhabricatorAuthConduitTokenRevoker.php',
'PhabricatorAuthConfirmLinkController' => 'applications/auth/controller/PhabricatorAuthConfirmLinkController.php', 'PhabricatorAuthConfirmLinkController' => 'applications/auth/controller/PhabricatorAuthConfirmLinkController.php',
@ -7321,6 +7322,7 @@ phutil_register_library_map(array(
'PhabricatorAuthApplication' => 'PhabricatorApplication', 'PhabricatorAuthApplication' => 'PhabricatorApplication',
'PhabricatorAuthAuthFactorPHIDType' => 'PhabricatorPHIDType', 'PhabricatorAuthAuthFactorPHIDType' => 'PhabricatorPHIDType',
'PhabricatorAuthAuthProviderPHIDType' => 'PhabricatorPHIDType', 'PhabricatorAuthAuthProviderPHIDType' => 'PhabricatorPHIDType',
'PhabricatorAuthChangePasswordAction' => 'PhabricatorSystemAction',
'PhabricatorAuthConduitAPIMethod' => 'ConduitAPIMethod', 'PhabricatorAuthConduitAPIMethod' => 'ConduitAPIMethod',
'PhabricatorAuthConduitTokenRevoker' => 'PhabricatorAuthRevoker', 'PhabricatorAuthConduitTokenRevoker' => 'PhabricatorAuthRevoker',
'PhabricatorAuthConfirmLinkController' => 'PhabricatorAuthController', 'PhabricatorAuthConfirmLinkController' => 'PhabricatorAuthController',

View file

@ -0,0 +1,22 @@
<?php
final class PhabricatorAuthChangePasswordAction
extends PhabricatorSystemAction {
const TYPECONST = 'auth.password';
public function getActionConstant() {
return self::TYPECONST;
}
public function getScoreThreshold() {
return 20 / phutil_units('1 hour in seconds');
}
public function getLimitExplanation() {
return pht(
'You have failed to enter the correct account password too often in '.
'a short period of time.');
}
}

View file

@ -25,11 +25,13 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
} }
public function processRequest(AphrontRequest $request) { public function processRequest(AphrontRequest $request) {
$user = $request->getUser(); $viewer = $request->getUser();
$user = $this->getUser();
$content_source = PhabricatorContentSource::newFromRequest($request); $content_source = PhabricatorContentSource::newFromRequest($request);
$token = id(new PhabricatorAuthSessionEngine())->requireHighSecuritySession( $token = id(new PhabricatorAuthSessionEngine())->requireHighSecuritySession(
$user, $viewer,
$request, $request,
'/settings/'); '/settings/');
@ -44,7 +46,7 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
$account_type = PhabricatorAuthPassword::PASSWORD_TYPE_ACCOUNT; $account_type = PhabricatorAuthPassword::PASSWORD_TYPE_ACCOUNT;
$password_objects = id(new PhabricatorAuthPasswordQuery()) $password_objects = id(new PhabricatorAuthPasswordQuery())
->setViewer($user) ->setViewer($viewer)
->withObjectPHIDs(array($user->getPHID())) ->withObjectPHIDs(array($user->getPHID()))
->withPasswordTypes(array($account_type)) ->withPasswordTypes(array($account_type))
->withIsRevoked(false) ->withIsRevoked(false)
@ -63,10 +65,18 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
$errors = array(); $errors = array();
if ($request->isFormPost()) { if ($request->isFormPost()) {
// Rate limit guesses about the old password. This page requires MFA and
// session compromise already, so this is mostly just to stop researchers
// from reporting this as a vulnerability.
PhabricatorSystemActionEngine::willTakeAction(
array($viewer->getPHID()),
new PhabricatorAuthChangePasswordAction(),
1);
$envelope = new PhutilOpaqueEnvelope($request->getStr('old_pw')); $envelope = new PhutilOpaqueEnvelope($request->getStr('old_pw'));
$engine = id(new PhabricatorAuthPasswordEngine()) $engine = id(new PhabricatorAuthPasswordEngine())
->setViewer($user) ->setViewer($viewer)
->setContentSource($content_source) ->setContentSource($content_source)
->setPasswordType($account_type) ->setPasswordType($account_type)
->setObject($user); ->setObject($user);
@ -79,6 +89,12 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
$e_old = pht('Invalid'); $e_old = pht('Invalid');
} else { } else {
$e_old = null; $e_old = null;
// Refund the user an action credit for getting the password right.
PhabricatorSystemActionEngine::willTakeAction(
array($viewer->getPHID()),
new PhabricatorAuthChangePasswordAction(),
-1);
} }
$pass = $request->getStr('new_pw'); $pass = $request->getStr('new_pw');
@ -142,7 +158,7 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
} }
$form = id(new AphrontFormView()) $form = id(new AphrontFormView())
->setViewer($user) ->setViewer($viewer)
->appendChild( ->appendChild(
id(new AphrontFormPasswordControl()) id(new AphrontFormPasswordControl())
->setLabel(pht('Old Password')) ->setLabel(pht('Old Password'))