From 3da9844564cf7f93916e420ecae64a4faf15a2d7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 18 Dec 2018 05:55:17 -0800 Subject: [PATCH] Tighten some MFA/TOTP parameters to improve resistance to brute force attacks Summary: Depends on D19897. Ref T13222. See some discussion in D19890. - Only rate limit users if they're actually answering a challenge, not if they're just clicking "Wait Patiently". - Reduce the number of allowed attempts per hour from 100 back to 10. - Reduce the TOTP window from +/- 2 timesteps (allowing ~60 seconds of skew) to +/- 1 timestep (allowing ~30 seconds of skew). - Change the window where a TOTP response remains valid to a flat 60 seconds instead of a calculation based on windows and timesteps. Test Plan: - Hit an MFA prompt. - Without typing in any codes, mashed "submit" as much as I wanted (>>10 times / hour). - Answered prompt correctly. - Mashed "Wait Patiently" as much as I wanted (>>10 times / hour). - Guessed random numbers, was rate limited after 10 attempts. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13222 Differential Revision: https://secure.phabricator.com/D19898 --- .../action/PhabricatorAuthTryFactorAction.php | 2 +- .../engine/PhabricatorAuthSessionEngine.php | 29 +++++++++---- .../auth/factor/PhabricatorAuthFactor.php | 4 ++ .../auth/factor/PhabricatorTOTPAuthFactor.php | 42 +++++++++++++++---- 4 files changed, 59 insertions(+), 18 deletions(-) diff --git a/src/applications/auth/action/PhabricatorAuthTryFactorAction.php b/src/applications/auth/action/PhabricatorAuthTryFactorAction.php index 40e7c858d1..246298567b 100644 --- a/src/applications/auth/action/PhabricatorAuthTryFactorAction.php +++ b/src/applications/auth/action/PhabricatorAuthTryFactorAction.php @@ -9,7 +9,7 @@ final class PhabricatorAuthTryFactorAction extends PhabricatorSystemAction { } public function getScoreThreshold() { - return 100 / phutil_units('1 hour in seconds'); + return 10 / phutil_units('1 hour in seconds'); } public function getLimitExplanation() { diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php index c129d898f2..d29530dbfd 100644 --- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -567,10 +567,21 @@ final class PhabricatorAuthSessionEngine extends Phobject { if ($request->getExists(AphrontRequest::TYPE_HISEC)) { // Limit factor verification rates to prevent brute force attacks. - PhabricatorSystemActionEngine::willTakeAction( - array($viewer->getPHID()), - new PhabricatorAuthTryFactorAction(), - 1); + $any_attempt = false; + foreach ($factors as $factor) { + $impl = $factor->requireImplementation(); + if ($impl->getRequestHasChallengeResponse($factor, $request)) { + $any_attempt = true; + break; + } + } + + if ($any_attempt) { + PhabricatorSystemActionEngine::willTakeAction( + array($viewer->getPHID()), + new PhabricatorAuthTryFactorAction(), + 1); + } foreach ($factors as $factor) { $factor_phid = $factor->getPHID(); @@ -610,10 +621,12 @@ final class PhabricatorAuthSessionEngine extends Phobject { } // Give the user a credit back for a successful factor verification. - PhabricatorSystemActionEngine::willTakeAction( - array($viewer->getPHID()), - new PhabricatorAuthTryFactorAction(), - -1); + if ($any_attempt) { + PhabricatorSystemActionEngine::willTakeAction( + array($viewer->getPHID()), + new PhabricatorAuthTryFactorAction(), + -1); + } if ($session->getIsPartial() && !$jump_into_hisec) { // If we have a partial session and are not jumping directly into diff --git a/src/applications/auth/factor/PhabricatorAuthFactor.php b/src/applications/auth/factor/PhabricatorAuthFactor.php index a9483a9809..5dccbe1653 100644 --- a/src/applications/auth/factor/PhabricatorAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorAuthFactor.php @@ -52,6 +52,10 @@ abstract class PhabricatorAuthFactor extends Phobject { ->setWorkflowKey($engine->getWorkflowKey()); } + abstract public function getRequestHasChallengeResponse( + PhabricatorAuthFactorConfig $config, + AphrontRequest $response); + final public function getNewIssuedChallenges( PhabricatorAuthFactorConfig $config, PhabricatorUser $viewer, diff --git a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php index 2a8999f2e4..33bb961691 100644 --- a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php @@ -80,7 +80,7 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor { $okay = (bool)$this->getTimestepAtWhichResponseIsValid( $this->getAllowedTimesteps($this->getCurrentTimestep()), new PhutilOpaqueEnvelope($key), - (string)$code); + $code); if ($okay) { $config = $this->newConfigForUser($user) @@ -206,9 +206,10 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor { if (!$control) { $value = $result->getValue(); $error = $result->getErrorMessage(); + $name = $this->getChallengeResponseParameterName($config); $control = id(new PHUIFormNumberControl()) - ->setName($this->getParameterName($config, 'totpcode')) + ->setName($name) ->setDisableAutocomplete(true) ->setValue($value) ->setError($error); @@ -221,6 +222,15 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor { $form->appendChild($control); } + public function getRequestHasChallengeResponse( + PhabricatorAuthFactorConfig $config, + AphrontRequest $request) { + + $value = $this->getChallengeResponseFromRequest($config, $request); + return (bool)strlen($value); + } + + protected function newResultFromIssuedChallenges( PhabricatorAuthFactorConfig $config, PhabricatorUser $viewer, @@ -301,7 +311,9 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor { AphrontRequest $request, array $challenges) { - $code = $request->getStr($this->getParameterName($config, 'totpcode')); + $code = $this->getChallengeResponseFromRequest( + $config, + $request); $result = $this->newResult() ->setValue($code); @@ -335,13 +347,10 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor { $valid_timestep = $this->getTimestepAtWhichResponseIsValid( array_intersect_key($challenge_timesteps, $current_timesteps), new PhutilOpaqueEnvelope($config->getFactorSecret()), - (string)$code); + $code); if ($valid_timestep) { - $now = PhabricatorTime::getNow(); - $step_duration = $this->getTimestepDuration(); - $step_window = $this->getTimestepWindowSize(); - $ttl = $now + ($step_duration * $step_window); + $ttl = PhabricatorTime::getNow() + 60; $challenge ->setProperty('totp.timestep', $valid_timestep) @@ -475,7 +484,7 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor { // The user is allowed to provide a code from the recent past or the // near future to account for minor clock skew between the client // and server, and the time it takes to actually enter a code. - return 2; + return 1; } private function getTimestepAtWhichResponseIsValid( @@ -493,6 +502,21 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor { return null; } + private function getChallengeResponseParameterName( + PhabricatorAuthFactorConfig $config) { + return $this->getParameterName($config, 'totpcode'); + } + + private function getChallengeResponseFromRequest( + PhabricatorAuthFactorConfig $config, + AphrontRequest $request) { + $name = $this->getChallengeResponseParameterName($config); + $value = $request->getStr($name); + $value = (string)$value; + $value = trim($value); + + return $value; + } } -- 2.51.2