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/PhabricatorAuthTOTPKeyTemporaryTokenType.php b/src/applications/auth/factor/PhabricatorAuthMFASyncTemporaryTokenType.php rename from src/applications/auth/factor/PhabricatorAuthTOTPKeyTemporaryTokenType.php rename to src/applications/auth/factor/PhabricatorAuthMFASyncTemporaryTokenType.php --- a/src/applications/auth/factor/PhabricatorAuthTOTPKeyTemporaryTokenType.php +++ b/src/applications/auth/factor/PhabricatorAuthMFASyncTemporaryTokenType.php @@ -1,17 +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 { @@ -104,9 +60,6 @@ } } - $form->addHiddenInput('totp', true); - $form->addHiddenInput('totpkey', $key); - $form->appendRemarkupInstructions( pht( 'First, download an authenticator application on your phone. Two '. @@ -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( @@ -61,6 +62,15 @@ return $this->sessionEngine; } + public function setMFASyncToken(PhabricatorAuthTemporaryToken $token) { + $this->mfaSyncToken = $token; + return $this; + } + + public function getMFASyncToken() { + return $this->mfaSyncToken; + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ 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(),