diff --git a/resources/sql/autopatches/20140520.authtemptoken.sql b/resources/sql/autopatches/20140520.authtemptoken.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20140520.authtemptoken.sql @@ -0,0 +1,11 @@ +CREATE TABLE {$NAMESPACE}_auth.auth_temporarytoken ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + objectPHID VARCHAR(64) NOT NULL COLLATE utf8_bin, + tokenType VARCHAR(64) NOT NULL COLLATE utf8_bin, + tokenExpires INT UNSIGNED NOT NULL, + tokenCode VARCHAR(64) NOT NULL COLLATE utf8_bin, + + UNIQUE KEY `key_token` (objectPHID, tokenType, tokenCode), + KEY `key_expires` (tokenExpires) + +) ENGINE=InnoDB, COLLATE utf8_general_ci; 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 @@ -1279,6 +1279,9 @@ 'PhabricatorAuthSessionGarbageCollector' => 'applications/auth/garbagecollector/PhabricatorAuthSessionGarbageCollector.php', 'PhabricatorAuthSessionQuery' => 'applications/auth/query/PhabricatorAuthSessionQuery.php', 'PhabricatorAuthStartController' => 'applications/auth/controller/PhabricatorAuthStartController.php', + 'PhabricatorAuthTemporaryToken' => 'applications/auth/storage/PhabricatorAuthTemporaryToken.php', + 'PhabricatorAuthTemporaryTokenGarbageCollector' => 'applications/auth/garbagecollector/PhabricatorAuthTemporaryTokenGarbageCollector.php', + 'PhabricatorAuthTemporaryTokenQuery' => 'applications/auth/query/PhabricatorAuthTemporaryTokenQuery.php', 'PhabricatorAuthTerminateSessionController' => 'applications/auth/controller/PhabricatorAuthTerminateSessionController.php', 'PhabricatorAuthTryFactorAction' => 'applications/auth/action/PhabricatorAuthTryFactorAction.php', 'PhabricatorAuthUnlinkController' => 'applications/auth/controller/PhabricatorAuthUnlinkController.php', @@ -4044,6 +4047,13 @@ 'PhabricatorAuthSessionGarbageCollector' => 'PhabricatorGarbageCollector', 'PhabricatorAuthSessionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorAuthStartController' => 'PhabricatorAuthController', + 'PhabricatorAuthTemporaryToken' => + array( + 0 => 'PhabricatorAuthDAO', + 1 => 'PhabricatorPolicyInterface', + ), + 'PhabricatorAuthTemporaryTokenGarbageCollector' => 'PhabricatorGarbageCollector', + 'PhabricatorAuthTemporaryTokenQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorAuthTerminateSessionController' => 'PhabricatorAuthController', 'PhabricatorAuthTryFactorAction' => 'PhabricatorSystemAction', 'PhabricatorAuthUnlinkController' => 'PhabricatorAuthController', diff --git a/src/applications/auth/factor/PhabricatorAuthFactorTOTP.php b/src/applications/auth/factor/PhabricatorAuthFactorTOTP.php --- a/src/applications/auth/factor/PhabricatorAuthFactorTOTP.php +++ b/src/applications/auth/factor/PhabricatorAuthFactorTOTP.php @@ -2,6 +2,8 @@ final class PhabricatorAuthFactorTOTP extends PhabricatorAuthFactor { + const TEMPORARY_TOKEN_TYPE = 'mfa:totp:key'; + public function getFactorKey() { return 'totp'; } @@ -23,13 +25,42 @@ PhabricatorUser $user) { $key = $request->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.) + + $temporary_token = id(new PhabricatorAuthTemporaryTokenQuery()) + ->setViewer($user) + ->withObjectPHIDs(array($user->getPHID())) + ->withTokenTypes(array(self::TEMPORARY_TOKEN_TYPE)) + ->withExpired(false) + ->withTokenCodes(array(PhabricatorHash::digest($key))) + ->executeOne(); + if (!$temporary_token) { + // If we don't have a matching token, regenerate the key below. + $key = null; + } + } + if (!strlen($key)) { - // TODO: When the user submits a key, we should require that it be - // one we generated for them, so there's no way an attacker can ever - // force a key they control onto an account. However, it's clumsy to - // do this right now. Once we have one-time tokens for SMS and email, - // we should be able to put it on that infrastructure. $key = self::generateNewTOTPKey(); + + // Mark this key as one we generated, so the user is allowed to submit + // a response for it. + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + id(new PhabricatorAuthTemporaryToken()) + ->setObjectPHID($user->getPHID()) + ->setTokenType(self::TEMPORARY_TOKEN_TYPE) + ->setTokenExpires(time() + phutil_units('1 hour in seconds')) + ->setTokenCode(PhabricatorHash::digest($key)) + ->save(); + unset($unguarded); } $code = $request->getStr('totpcode'); diff --git a/src/applications/auth/garbagecollector/PhabricatorAuthTemporaryTokenGarbageCollector.php b/src/applications/auth/garbagecollector/PhabricatorAuthTemporaryTokenGarbageCollector.php new file mode 100644 --- /dev/null +++ b/src/applications/auth/garbagecollector/PhabricatorAuthTemporaryTokenGarbageCollector.php @@ -0,0 +1,18 @@ +establishConnection('w'); + + queryfx( + $conn_w, + 'DELETE FROM %T WHERE tokenExpires <= UNIX_TIMESTAMP() LIMIT 100', + $session_table->getTableName()); + + return ($conn_w->getAffectedRows() == 100); + } + +} diff --git a/src/applications/auth/query/PhabricatorAuthTemporaryTokenQuery.php b/src/applications/auth/query/PhabricatorAuthTemporaryTokenQuery.php new file mode 100644 --- /dev/null +++ b/src/applications/auth/query/PhabricatorAuthTemporaryTokenQuery.php @@ -0,0 +1,106 @@ +ids = $ids; + return $this; + } + + public function withObjectPHIDs(array $object_phids) { + $this->objectPHIDs = $object_phids; + return $this; + } + + public function withTokenTypes(array $types) { + $this->tokenTypes = $types; + return $this; + } + + public function withExpired($expired) { + $this->expired = $expired; + return $this; + } + + public function withTokenCodes(array $codes) { + $this->tokenCodes = $codes; + return $this; + } + + protected function loadPage() { + $table = new PhabricatorAuthTemporaryToken(); + $conn_r = $table->establishConnection('r'); + + $data = queryfx_all( + $conn_r, + 'SELECT * FROM %T %Q %Q %Q', + $table->getTableName(), + $this->buildWhereClause($conn_r), + $this->buildOrderClause($conn_r), + $this->buildLimitClause($conn_r)); + + return $table->loadAllFromArray($data); + } + + protected function buildWhereClause(AphrontDatabaseConnection $conn_r) { + $where = array(); + + if ($this->ids !== null) { + $where[] = qsprintf( + $conn_r, + 'id IN (%Ld)', + $this->ids); + } + + if ($this->objectPHIDs !== null) { + $where[] = qsprintf( + $conn_r, + 'objectPHID IN (%Ls)', + $this->objectPHIDs); + } + + if ($this->tokenTypes !== null) { + $where[] = qsprintf( + $conn_r, + 'tokenType IN (%Ls)', + $this->tokenTypes); + } + + if ($this->expired !== null) { + if ($this->expired) { + $where[] = qsprintf( + $conn_r, + 'tokenExpires <= %d', + time()); + } else { + $where[] = qsprintf( + $conn_r, + 'tokenExpires > %d', + time()); + } + } + + if ($this->tokenCodes !== null) { + $where[] = qsprintf( + $conn_r, + 'tokenCode IN (%Ls)', + $this->tokenCodes); + } + + $where[] = $this->buildPagingClause($conn_r); + + return $this->formatWhereClause($where); + } + + public function getQueryApplicationClass() { + return 'PhabricatorApplicationAuth'; + } + +} diff --git a/src/applications/auth/storage/PhabricatorAuthTemporaryToken.php b/src/applications/auth/storage/PhabricatorAuthTemporaryToken.php new file mode 100644 --- /dev/null +++ b/src/applications/auth/storage/PhabricatorAuthTemporaryToken.php @@ -0,0 +1,41 @@ + false, + ) + parent::getConfiguration(); + } + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + + + public function getCapabilities() { + return array( + PhabricatorPolicyCapability::CAN_VIEW, + ); + } + + public function getPolicy($capability) { + // We're just implement this interface to get access to the standard + // query infrastructure. + return PhabricatorPolicies::getMostOpenPolicy(); + } + + public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { + return false; + } + + public function describeAutomaticCapability($capability) { + return null; + } + + +}