From f3340c633562218b7d37cd4f9e61abe3fc9de154 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 23 Jan 2019 16:17:05 +0000 Subject: [PATCH] Allow different MFA factor types (SMS, TOTP, Duo, ...) to share "sync" tokens when enrolling new factors Summary: Depends on D20019. Ref T13222. Currently, TOTP uses a temporary token to make sure you've set up the app on your phone properly and that you're providing an answer to a secret which we generated (not an attacker-generated secret). However, most factor types need some kind of sync token. SMS needs to send you a code; Duo needs to store a transaction ID. Turn this "TOTP" token into an "MFA Sync" token and lift the implementation up to the base class. Also, slightly simplify some of the HTTP form gymnastics. Test Plan: - Hit the TOTP enroll screen. - Reloaded it, got new secrets. - Reloaded it more than 10 times, got told to stop generating new challenges. - Answered a challenge properly, got a new TOTP factor. - Grepped for removed class name. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13222 Differential Revision: https://secure.phabricator.com/D20020 --- src/__phutil_library_map__.php | 4 ++-- src/applications/auth/factor/PhabricatorAuthFactor.php | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/applications/auth/factor/PhabricatorAuthMFASyncTemporaryTokenType.php | 18 ++++++++++++++++++ src/applications/auth/factor/PhabricatorAuthTOTPKeyTemporaryTokenType.php | 17 ----------------- src/applications/auth/factor/PhabricatorTOTPAuthFactor.php | 76 ++++++++++++++++++---------------------------------------------------------- src/applications/auth/storage/PhabricatorAuthFactorConfig.php | 10 ++++++++++ src/applications/auth/storage/PhabricatorAuthTemporaryToken.php | 12 +++++++++++- src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php | 7 +++++++ 8 file(s) changed, 166 insertion(s)(+), 78 deletion(s)(-) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2265,6 +2265,7 @@ 'PhabricatorAuthLoginMessageType' => 'applications/auth/message/PhabricatorAuthLoginMessageType.php', 'PhabricatorAuthLogoutConduitAPIMethod' => 'applications/auth/conduit/PhabricatorAuthLogoutConduitAPIMethod.php', 'PhabricatorAuthMFAEditEngineExtension' => 'applications/auth/engineextension/PhabricatorAuthMFAEditEngineExtension.php', + 'PhabricatorAuthMFASyncTemporaryTokenType' => 'applications/auth/factor/PhabricatorAuthMFASyncTemporaryTokenType.php', 'PhabricatorAuthMainMenuBarExtension' => 'applications/auth/extension/PhabricatorAuthMainMenuBarExtension.php', 'PhabricatorAuthManagementCachePKCS8Workflow' => 'applications/auth/management/PhabricatorAuthManagementCachePKCS8Workflow.php', 'PhabricatorAuthManagementLDAPWorkflow' => 'applications/auth/management/PhabricatorAuthManagementLDAPWorkflow.php', @@ -2359,7 +2360,6 @@ 'PhabricatorAuthSetPasswordController' => 'applications/auth/controller/PhabricatorAuthSetPasswordController.php', 'PhabricatorAuthSetupCheck' => 'applications/config/check/PhabricatorAuthSetupCheck.php', 'PhabricatorAuthStartController' => 'applications/auth/controller/PhabricatorAuthStartController.php', - 'PhabricatorAuthTOTPKeyTemporaryTokenType' => 'applications/auth/factor/PhabricatorAuthTOTPKeyTemporaryTokenType.php', 'PhabricatorAuthTemporaryToken' => 'applications/auth/storage/PhabricatorAuthTemporaryToken.php', 'PhabricatorAuthTemporaryTokenGarbageCollector' => 'applications/auth/garbagecollector/PhabricatorAuthTemporaryTokenGarbageCollector.php', 'PhabricatorAuthTemporaryTokenQuery' => 'applications/auth/query/PhabricatorAuthTemporaryTokenQuery.php', @@ -7986,6 +7986,7 @@ 'PhabricatorAuthLoginMessageType' => 'PhabricatorAuthMessageType', 'PhabricatorAuthLogoutConduitAPIMethod' => 'PhabricatorAuthConduitAPIMethod', 'PhabricatorAuthMFAEditEngineExtension' => 'PhabricatorEditEngineExtension', + 'PhabricatorAuthMFASyncTemporaryTokenType' => 'PhabricatorAuthTemporaryTokenType', 'PhabricatorAuthMainMenuBarExtension' => 'PhabricatorMainMenuBarExtension', 'PhabricatorAuthManagementCachePKCS8Workflow' => 'PhabricatorAuthManagementWorkflow', 'PhabricatorAuthManagementLDAPWorkflow' => 'PhabricatorAuthManagementWorkflow', @@ -8101,7 +8102,6 @@ 'PhabricatorAuthSetPasswordController' => 'PhabricatorAuthController', 'PhabricatorAuthSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorAuthStartController' => 'PhabricatorAuthController', - 'PhabricatorAuthTOTPKeyTemporaryTokenType' => 'PhabricatorAuthTemporaryTokenType', 'PhabricatorAuthTemporaryToken' => array( 'PhabricatorAuthDAO', 'PhabricatorPolicyInterface', diff --git a/src/applications/auth/factor/PhabricatorAuthFactor.php b/src/applications/auth/factor/PhabricatorAuthFactor.php --- a/src/applications/auth/factor/PhabricatorAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorAuthFactor.php @@ -245,4 +245,104 @@ } +/* -( Synchronizing New Factors )------------------------------------------ */ + + + final protected function loadMFASyncToken( + AphrontRequest $request, + AphrontFormView $form, + PhabricatorUser $user) { + + // If the form included a synchronization key, load the corresponding + // token. The user must synchronize to a key we generated because this + // raises the barrier to theoretical attacks where an attacker might + // provide a known key for factors like TOTP. + + // (We store and verify the hash of the key, not the key itself, to limit + // how useful the data in the table is to an attacker.) + + $sync_type = PhabricatorAuthMFASyncTemporaryTokenType::TOKENTYPE; + $sync_token = null; + + $sync_key = $request->getStr($this->getMFASyncTokenFormKey()); + if (strlen($sync_key)) { + $sync_key_digest = PhabricatorHash::digestWithNamedKey( + $sync_key, + PhabricatorAuthMFASyncTemporaryTokenType::DIGEST_KEY); + + $sync_token = id(new PhabricatorAuthTemporaryTokenQuery()) + ->setViewer($user) + ->withTokenResources(array($user->getPHID())) + ->withTokenTypes(array($sync_type)) + ->withExpired(false) + ->withTokenCodes(array($sync_key_digest)) + ->executeOne(); + } + + if (!$sync_token) { + + // Don't generate a new sync token if there are too many outstanding + // tokens already. This is mostly relevant for push factors like SMS, + // where generating a token has the side effect of sending a user a + // message. + + $outstanding_limit = 10; + $outstanding_tokens = id(new PhabricatorAuthTemporaryTokenQuery()) + ->setViewer($user) + ->withTokenResources(array($user->getPHID())) + ->withTokenTypes(array($sync_type)) + ->withExpired(false) + ->execute(); + if (count($outstanding_tokens) > $outstanding_limit) { + throw new Exception( + pht( + 'Your account has too many outstanding, incomplete MFA '. + 'synchronization attempts. Wait an hour and try again.')); + } + + $now = PhabricatorTime::getNow(); + + $sync_key = Filesystem::readRandomCharacters(32); + $sync_key_digest = PhabricatorHash::digestWithNamedKey( + $sync_key, + PhabricatorAuthMFASyncTemporaryTokenType::DIGEST_KEY); + $sync_ttl = $this->getMFASyncTokenTTL(); + + $sync_token = id(new PhabricatorAuthTemporaryToken()) + ->setIsNewTemporaryToken(true) + ->setTokenResource($user->getPHID()) + ->setTokenType($sync_type) + ->setTokenCode($sync_key_digest) + ->setTokenExpires($now + $sync_ttl); + + // Note that property generation is unguarded, since factors that push + // a challenge generally need to perform a write there. + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + $properties = $this->newMFASyncTokenProperties($user); + + foreach ($properties as $key => $value) { + $sync_token->setTemporaryTokenProperty($key, $value); + } + + $sync_token->save(); + unset($unguarded); + } + + $form->addHiddenInput($this->getMFASyncTokenFormKey(), $sync_key); + + return $sync_token; + } + + protected function newMFASyncTokenProperties(PhabricatorUser $user) { + return array(); + } + + private function getMFASyncTokenFormKey() { + return 'sync.key'; + } + + private function getMFASyncTokenTTL() { + return phutil_units('1 hour in seconds'); + } + } diff --git a/src/applications/auth/factor/PhabricatorAuthMFASyncTemporaryTokenType.php b/src/applications/auth/factor/PhabricatorAuthMFASyncTemporaryTokenType.php new file mode 100644 --- /dev/null +++ b/src/applications/auth/factor/PhabricatorAuthMFASyncTemporaryTokenType.php @@ -0,0 +1,18 @@ +getStr('totpkey'); - if (strlen($key)) { - // If the user is providing a key, make sure it's a key we generated. - // This raises the barrier to theoretical attacks where an attacker might - // provide a known key (such attacks are already prevented by CSRF, but - // this is a second barrier to overcome). - - // (We store and verify the hash of the key, not the key itself, to limit - // how useful the data in the table is to an attacker.) - - $token_code = PhabricatorHash::digestWithNamedKey( - $key, - self::DIGEST_TEMPORARY_KEY); - - $temporary_token = id(new PhabricatorAuthTemporaryTokenQuery()) - ->setViewer($user) - ->withTokenResources(array($user->getPHID())) - ->withTokenTypes(array($totp_token_type)) - ->withExpired(false) - ->withTokenCodes(array($token_code)) - ->executeOne(); - if (!$temporary_token) { - // If we don't have a matching token, regenerate the key below. - $key = null; - } - } - - if (!strlen($key)) { - $key = self::generateNewTOTPKey(); - - // Mark this key as one we generated, so the user is allowed to submit - // a response for it. - - $token_code = PhabricatorHash::digestWithNamedKey( - $key, - self::DIGEST_TEMPORARY_KEY); - - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - id(new PhabricatorAuthTemporaryToken()) - ->setTokenResource($user->getPHID()) - ->setTokenType($totp_token_type) - ->setTokenExpires(time() + phutil_units('1 hour in seconds')) - ->setTokenCode($token_code) - ->save(); - unset($unguarded); - } + $sync_token = $this->loadMFASyncToken( + $request, + $form, + $user); + $secret = $sync_token->getTemporaryTokenProperty('secret'); $code = $request->getStr('totpcode'); $e_code = true; - if ($request->getExists('totp')) { + if (!$sync_token->getIsNewTemporaryToken()) { $okay = (bool)$this->getTimestepAtWhichResponseIsValid( $this->getAllowedTimesteps($this->getCurrentTimestep()), - new PhutilOpaqueEnvelope($key), + new PhutilOpaqueEnvelope($secret), $code); if ($okay) { $config = $this->newConfigForUser($user) ->setFactorName(pht('Mobile App (TOTP)')) - ->setFactorSecret($key); + ->setFactorSecret($secret) + ->setMFASyncToken($sync_token); return $config; } else { @@ -103,9 +59,6 @@ } } } - - $form->addHiddenInput('totp', true); - $form->addHiddenInput('totpkey', $key); $form->appendRemarkupInstructions( pht( @@ -126,7 +79,7 @@ 'otpauth://totp/%s:%s?secret=%s&issuer=%s', $issuer, $user->getUsername(), - $key, + $secret, $issuer); $qrcode = $this->renderQRCode($uri); @@ -135,7 +88,7 @@ $form->appendChild( id(new AphrontFormStaticControl()) ->setLabel(pht('Key')) - ->setValue(phutil_tag('strong', array(), $key))); + ->setValue(phutil_tag('strong', array(), $secret))); $form->appendInstructions( pht( @@ -526,4 +479,11 @@ return $value; } + + protected function newMFASyncTokenProperties(PhabricatorUser $user) { + return array( + 'secret' => self::generateNewTOTPKey(), + ); + } + } diff --git a/src/applications/auth/storage/PhabricatorAuthFactorConfig.php b/src/applications/auth/storage/PhabricatorAuthFactorConfig.php --- a/src/applications/auth/storage/PhabricatorAuthFactorConfig.php +++ b/src/applications/auth/storage/PhabricatorAuthFactorConfig.php @@ -15,6 +15,7 @@ private $sessionEngine; private $factorProvider = self::ATTACHABLE; + private $mfaSyncToken; protected function getConfiguration() { return array( @@ -59,6 +60,15 @@ } return $this->sessionEngine; + } + + public function setMFASyncToken(PhabricatorAuthTemporaryToken $token) { + $this->mfaSyncToken = $token; + return $this; + } + + public function getMFASyncToken() { + return $this->mfaSyncToken; } diff --git a/src/applications/auth/storage/PhabricatorAuthTemporaryToken.php b/src/applications/auth/storage/PhabricatorAuthTemporaryToken.php --- a/src/applications/auth/storage/PhabricatorAuthTemporaryToken.php +++ b/src/applications/auth/storage/PhabricatorAuthTemporaryToken.php @@ -10,7 +10,9 @@ protected $tokenExpires; protected $tokenCode; protected $userPHID; - protected $properties; + protected $properties = array(); + + private $isNew = false; protected function getConfiguration() { return array( @@ -114,6 +116,14 @@ return $this->getTemporaryTokenProperty('force-full-session', false); } + public function setIsNewTemporaryToken($is_new) { + $this->isNew = $is_new; + return $this; + } + + public function getIsNewTemporaryToken() { + return $this->isNew; + } /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php --- a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php @@ -266,6 +266,13 @@ $config->save(); + // If we used a temporary token to handle synchronizing the factor, + // revoke it now. + $sync_token = $config->getMFASyncToken(); + if ($sync_token) { + $sync_token->revokeToken(); + } + $log = PhabricatorUserLog::initializeNewLog( $viewer, $user->getPHID(), -- tangled.sh