diff --git a/resources/sql/autopatches/20181213.auth.06.challenge.sql b/resources/sql/autopatches/20181213.auth.06.challenge.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20181213.auth.06.challenge.sql @@ -0,0 +1,12 @@ +CREATE TABLE {$NAMESPACE}_auth.auth_challenge ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + phid VARBINARY(64) NOT NULL, + userPHID VARBINARY(64) NOT NULL, + factorPHID VARBINARY(64) NOT NULL, + sessionPHID VARBINARY(64) NOT NULL, + challengeKey VARCHAR(255) NOT NULL COLLATE {$COLLATE_TEXT}, + challengeTTL INT UNSIGNED NOT NULL, + properties LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; 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 @@ -2187,6 +2187,9 @@ 'PhabricatorAuthApplication' => 'applications/auth/application/PhabricatorAuthApplication.php', 'PhabricatorAuthAuthFactorPHIDType' => 'applications/auth/phid/PhabricatorAuthAuthFactorPHIDType.php', 'PhabricatorAuthAuthProviderPHIDType' => 'applications/auth/phid/PhabricatorAuthAuthProviderPHIDType.php', + 'PhabricatorAuthChallenge' => 'applications/auth/storage/PhabricatorAuthChallenge.php', + 'PhabricatorAuthChallengePHIDType' => 'applications/auth/phid/PhabricatorAuthChallengePHIDType.php', + 'PhabricatorAuthChallengeQuery' => 'applications/auth/query/PhabricatorAuthChallengeQuery.php', 'PhabricatorAuthChangePasswordAction' => 'applications/auth/action/PhabricatorAuthChangePasswordAction.php', 'PhabricatorAuthConduitAPIMethod' => 'applications/auth/conduit/PhabricatorAuthConduitAPIMethod.php', 'PhabricatorAuthConduitTokenRevoker' => 'applications/auth/revoker/PhabricatorAuthConduitTokenRevoker.php', @@ -7822,6 +7825,12 @@ 'PhabricatorAuthApplication' => 'PhabricatorApplication', 'PhabricatorAuthAuthFactorPHIDType' => 'PhabricatorPHIDType', 'PhabricatorAuthAuthProviderPHIDType' => 'PhabricatorPHIDType', + 'PhabricatorAuthChallenge' => array( + 'PhabricatorAuthDAO', + 'PhabricatorPolicyInterface', + ), + 'PhabricatorAuthChallengePHIDType' => 'PhabricatorPHIDType', + 'PhabricatorAuthChallengeQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorAuthChangePasswordAction' => 'PhabricatorSystemAction', 'PhabricatorAuthConduitAPIMethod' => 'ConduitAPIMethod', 'PhabricatorAuthConduitTokenRevoker' => 'PhabricatorAuthRevoker', diff --git a/src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php b/src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php --- a/src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php +++ b/src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php @@ -29,13 +29,28 @@ $throwable) { $viewer = $this->getViewer($request); + $results = $throwable->getFactorValidationResults(); $form = id(new PhabricatorAuthSessionEngine())->renderHighSecurityForm( $throwable->getFactors(), - $throwable->getFactorValidationResults(), + $results, $viewer, $request); + $is_wait = false; + foreach ($results as $result) { + if ($result->getIsWait()) { + $is_wait = true; + break; + } + } + + if ($is_wait) { + $submit = pht('Wait Patiently'); + } else { + $submit = pht('Enter High Security'); + } + $dialog = id(new AphrontDialogView()) ->setUser($viewer) ->setTitle(pht('Entering High Security')) @@ -62,7 +77,7 @@ 'actions, you should leave high security.')) ->setSubmitURI($request->getPath()) ->addCancelButton($throwable->getCancelURI()) - ->addSubmitButton(pht('Enter High Security')); + ->addSubmitButton($submit); $request_parameters = $request->getPassthroughRequestParameters( $respect_quicksand = true); diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php --- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -480,7 +480,59 @@ new PhabricatorAuthTryFactorAction(), 0); + $now = PhabricatorTime::getNow(); + + // We need to do challenge validation first, since this happens whether you + // submitted responses or not. You can't get a "bad response" error before + // you actually submit a response, but you can get a "wait, we can't + // issue a challenge yet" response. Load all issued challenges which are + // currently valid. + $challenges = id(new PhabricatorAuthChallengeQuery()) + ->setViewer($viewer) + ->withFactorPHIDs(mpull($factors, 'getPHID')) + ->withUserPHIDs(array($viewer->getPHID())) + ->withChallengeTTLBetween($now, null) + ->execute(); + $challenge_map = mgroup($challenges, 'getFactorPHID'); + $validation_results = array(); + $ok = true; + + // Validate each factor against issued challenges. For example, this + // prevents you from receiving or responding to a TOTP challenge if another + // challenge was recently issued to a different session. + foreach ($factors as $factor) { + $factor_phid = $factor->getPHID(); + $issued_challenges = idx($challenge_map, $factor_phid, array()); + $impl = $factor->requireImplementation(); + + $new_challenges = $impl->getNewIssuedChallenges( + $factor, + $viewer, + $issued_challenges); + + foreach ($new_challenges as $new_challenge) { + $issued_challenges[] = $new_challenge; + } + $challenge_map[$factor_phid] = $issued_challenges; + + if (!$issued_challenges) { + continue; + } + + $result = $impl->getResultFromIssuedChallenges( + $factor, + $viewer, + $issued_challenges); + + if (!$result) { + continue; + } + + $ok = false; + $validation_results[$factor_phid] = $result; + } + if ($request->isHTTPPost()) { $request->validateCSRF(); if ($request->getExists(AphrontRequest::TYPE_HISEC)) { @@ -491,30 +543,28 @@ new PhabricatorAuthTryFactorAction(), 1); - $ok = true; foreach ($factors as $factor) { - $id = $factor->getID(); + $factor_phid = $factor->getPHID(); + + // If we already have a validation result from previously issued + // challenges, skip validating this factor. + if (isset($validation_results[$factor_phid])) { + continue; + } + $impl = $factor->requireImplementation(); - $validation_result = $impl->processValidateFactorForm( + $validation_result = $impl->getResultFromChallengeResponse( $factor, $viewer, - $request); - - if (!($validation_result instanceof PhabricatorAuthFactorResult)) { - throw new Exception( - pht( - 'Expected "processValidateFactorForm()" to return an object '. - 'of class "%s"; got something else (from "%s").', - 'PhabricatorAuthFactorResult', - get_class($impl))); - } + $request, + $issued_challenges); if (!$validation_result->getIsValid()) { $ok = false; } - $validation_results[$id] = $validation_result; + $validation_results[$factor_phid] = $validation_result; } if ($ok) { @@ -566,6 +616,18 @@ return $token; } + // If we don't have a validation result for some factors yet, fill them + // in with an empty result so form rendering doesn't have to care if the + // results exist or not. This happens when you first load the form and have + // not submitted any responses yet. + foreach ($factors as $factor) { + $factor_phid = $factor->getPHID(); + if (isset($validation_results[$factor_phid])) { + continue; + } + $validation_results[$factor_phid] = new PhabricatorAuthFactorResult(); + } + throw id(new PhabricatorAuthHighSecurityRequiredException()) ->setCancelURI($cancel_uri) ->setFactors($factors) @@ -613,7 +675,7 @@ ->appendRemarkupInstructions(''); foreach ($factors as $factor) { - $result = idx($validation_results, $factor->getID()); + $result = $validation_results[$factor->getPHID()]; $factor->requireImplementation()->renderValidateFactorForm( $factor, 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 @@ -14,12 +14,7 @@ PhabricatorAuthFactorConfig $config, AphrontFormView $form, PhabricatorUser $viewer, - PhabricatorAuthFactorResult $validation_result = null); - - abstract public function processValidateFactorForm( - PhabricatorAuthFactorConfig $config, - PhabricatorUser $viewer, - AphrontRequest $request); + PhabricatorAuthFactorResult $validation_result); public function getParameterName( PhabricatorAuthFactorConfig $config, @@ -40,4 +35,131 @@ ->setFactorKey($this->getFactorKey()); } + protected function newResult() { + return new PhabricatorAuthFactorResult(); + } + + protected function newChallenge( + PhabricatorAuthFactorConfig $config, + PhabricatorUser $viewer) { + + return id(new PhabricatorAuthChallenge()) + ->setUserPHID($viewer->getPHID()) + ->setSessionPHID($viewer->getSession()->getPHID()) + ->setFactorPHID($config->getPHID()); + } + + final public function getNewIssuedChallenges( + PhabricatorAuthFactorConfig $config, + PhabricatorUser $viewer, + array $challenges) { + assert_instances_of($challenges, 'PhabricatorAuthChallenge'); + + $now = PhabricatorTime::getNow(); + + $new_challenges = $this->newIssuedChallenges( + $config, + $viewer, + $challenges); + + assert_instances_of($new_challenges, 'PhabricatorAuthChallenge'); + + foreach ($new_challenges as $new_challenge) { + $ttl = $new_challenge->getChallengeTTL(); + if (!$ttl) { + throw new Exception( + pht('Newly issued MFA challenges must have a valid TTL!')); + } + + if ($ttl < $now) { + throw new Exception( + pht( + 'Newly issued MFA challenges must have a future TTL. This '. + 'factor issued a bad TTL ("%s"). (Did you use a relative '. + 'time instead of an epoch?)', + $ttl)); + } + } + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + foreach ($new_challenges as $challenge) { + $challenge->save(); + } + unset($unguarded); + + return $new_challenges; + } + + abstract protected function newIssuedChallenges( + PhabricatorAuthFactorConfig $config, + PhabricatorUser $viewer, + array $challenges); + + final public function getResultFromIssuedChallenges( + PhabricatorAuthFactorConfig $config, + PhabricatorUser $viewer, + array $challenges) { + assert_instances_of($challenges, 'PhabricatorAuthChallenge'); + + $result = $this->newResultFromIssuedChallenges( + $config, + $viewer, + $challenges); + + if ($result === null) { + return $result; + } + + if (!($result instanceof PhabricatorAuthFactorResult)) { + throw new Exception( + pht( + 'Expected "newResultFromIssuedChallenges()" to return null or '. + 'an object of class "%s"; got something else (in "%s").', + 'PhabricatorAuthFactorResult', + get_class($this))); + } + + $result->setIssuedChallenges($challenges); + + return $result; + } + + abstract protected function newResultFromIssuedChallenges( + PhabricatorAuthFactorConfig $config, + PhabricatorUser $viewer, + array $challenges); + + final public function getResultFromChallengeResponse( + PhabricatorAuthFactorConfig $config, + PhabricatorUser $viewer, + AphrontRequest $request, + array $challenges) { + assert_instances_of($challenges, 'PhabricatorAuthChallenge'); + + $result = $this->newResultFromChallengeResponse( + $config, + $viewer, + $request, + $challenges); + + if (!($result instanceof PhabricatorAuthFactorResult)) { + throw new Exception( + pht( + 'Expected "newResultFromChallengeResponse()" to return an object '. + 'of class "%s"; got something else (in "%s").', + 'PhabricatorAuthFactorResult', + get_class($this))); + } + + $result->setIssuedChallenges($challenges); + + return $result; + } + + abstract protected function newResultFromChallengeResponse( + PhabricatorAuthFactorConfig $config, + PhabricatorUser $viewer, + AphrontRequest $request, + array $challenges); + } diff --git a/src/applications/auth/factor/PhabricatorAuthFactorResult.php b/src/applications/auth/factor/PhabricatorAuthFactorResult.php --- a/src/applications/auth/factor/PhabricatorAuthFactorResult.php +++ b/src/applications/auth/factor/PhabricatorAuthFactorResult.php @@ -4,8 +4,10 @@ extends Phobject { private $isValid = false; - private $hint; + private $isWait = false; + private $errorMessage; private $value; + private $issuedChallenges = array(); public function setIsValid($is_valid) { $this->isValid = $is_valid; @@ -16,13 +18,22 @@ return $this->isValid; } - public function setHint($hint) { - $this->hint = $hint; + public function setIsWait($is_wait) { + $this->isWait = $is_wait; return $this; } - public function getHint() { - return $this->hint; + public function getIsWait() { + return $this->isWait; + } + + public function setErrorMessage($error_message) { + $this->errorMessage = $error_message; + return $this; + } + + public function getErrorMessage() { + return $this->errorMessage; } public function setValue($value) { @@ -34,4 +45,14 @@ return $this->value; } + public function setIssuedChallenges(array $issued_challenges) { + assert_instances_of($issued_challenges, 'PhabricatorAuthChallenge'); + $this->issuedChallenges = $issued_challenges; + return $this; + } + + public function getIssuedChallenges() { + return $this->issuedChallenges; + } + } diff --git a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php --- a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php @@ -77,7 +77,7 @@ $e_code = true; if ($request->getExists('totp')) { - $okay = self::verifyTOTPCode( + $okay = $this->verifyTOTPCode( $user, new PhutilOpaqueEnvelope($key), $code); @@ -150,50 +150,131 @@ } + protected function newIssuedChallenges( + PhabricatorAuthFactorConfig $config, + PhabricatorUser $viewer, + array $challenges) { + + $now = $this->getCurrentTimestep(); + + // If we already issued a valid challenge, don't issue a new one. + if ($challenges) { + return array(); + } + + // Otherwise, generate a new challenge for the current timestep. It TTLs + // after it would fall off the bottom of the window. + $timesteps = $this->getAllowedTimesteps(); + $min_step = min($timesteps); + + $step_duration = $this->getTimestepDuration(); + $ttl_steps = ($now - $min_step) + 1; + $ttl_seconds = ($ttl_steps * $step_duration); + + return array( + $this->newChallenge($config, $viewer) + ->setChallengeKey($now) + ->setChallengeTTL(PhabricatorTime::getNow() + $ttl_seconds), + ); + } + public function renderValidateFactorForm( PhabricatorAuthFactorConfig $config, AphrontFormView $form, PhabricatorUser $viewer, - PhabricatorAuthFactorResult $validation_result = null) { + PhabricatorAuthFactorResult $result) { - if ($validation_result) { - $value = $validation_result->getValue(); - $hint = $validation_result->getHint(); - } else { - $value = null; - $hint = true; - } + $value = $result->getValue(); + $error = $result->getErrorMessage(); + $is_wait = $result->getIsWait(); - $form->appendChild( - id(new PHUIFormNumberControl()) + if ($is_wait) { + $control = id(new AphrontFormMarkupControl()) + ->setValue($error) + ->setError(pht('Wait')); + } else { + $control = id(new PHUIFormNumberControl()) ->setName($this->getParameterName($config, 'totpcode')) - ->setLabel(pht('App Code')) ->setDisableAutocomplete(true) - ->setCaption(pht('Factor Name: %s', $config->getFactorName())) ->setValue($value) - ->setError($hint)); + ->setError($error); + } + + $control + ->setLabel(pht('App Code')) + ->setCaption(pht('Factor Name: %s', $config->getFactorName())); + + $form->appendChild($control); + } + + protected function newResultFromIssuedChallenges( + PhabricatorAuthFactorConfig $config, + PhabricatorUser $viewer, + array $challenges) { + + // If we've already issued a challenge at the current timestep or any + // nearby timestep, require that it was issued to the current session. + // This is defusing attacks where you (broadly) look at someone's phone + // and type the code in more quickly than they do. + + $step_duration = $this->getTimestepDuration(); + $now = $this->getCurrentTimestep(); + $timesteps = $this->getAllowedTimesteps(); + $timesteps = array_fuse($timesteps); + $min_step = min($timesteps); + + $session_phid = $viewer->getSession()->getPHID(); + + foreach ($challenges as $challenge) { + $challenge_timestep = (int)$challenge->getChallengeKey(); + + // This challenge isn't for one of the timesteps you'd be able to respond + // to if you submitted the form right now, so we're good to keep going. + if (!isset($timesteps[$challenge_timestep])) { + continue; + } + + // This is the number of timesteps you need to wait for the problem + // timestep to leave the window, rounded up. + $wait_steps = ($challenge_timestep - $min_step) + 1; + $wait_duration = ($wait_steps * $step_duration); + + if ($challenge->getSessionPHID() !== $session_phid) { + return $this->newResult() + ->setIsWait(true) + ->setErrorMessage( + pht( + 'This factor recently issued a challenge to a different login '. + 'session. Wait %s seconds for the code to cycle, then try '. + 'again.', + new PhutilNumber($wait_duration))); + } + } + + return null; } - public function processValidateFactorForm( + protected function newResultFromChallengeResponse( PhabricatorAuthFactorConfig $config, PhabricatorUser $viewer, - AphrontRequest $request) { + AphrontRequest $request, + array $challenges) { $code = $request->getStr($this->getParameterName($config, 'totpcode')); $key = new PhutilOpaqueEnvelope($config->getFactorSecret()); - $result = id(new PhabricatorAuthFactorResult()) + $result = $this->newResult() ->setValue($code); - if (self::verifyTOTPCode($viewer, $key, $code)) { + if ($this->verifyTOTPCode($viewer, $key, (string)$code)) { $result->setIsValid(true); } else { if (strlen($code)) { - $hint = pht('Invalid'); + $error_message = pht('Invalid'); } else { - $hint = pht('Required'); + $error_message = pht('Required'); } - $result->setHint($hint); + $result->setErrorMessage($error_message); } return $result; @@ -203,7 +284,7 @@ return strtoupper(Filesystem::readRandomCharacters(32)); } - public static function verifyTOTPCode( + private function verifyTOTPCode( PhabricatorUser $user, PhutilOpaqueEnvelope $key, $code) { @@ -318,4 +399,19 @@ $rows); } + private function getTimestepDuration() { + return 30; + } + + private function getCurrentTimestep() { + $duration = $this->getTimestepDuration(); + return (int)(PhabricatorTime::getNow() / $duration); + } + + private function getAllowedTimesteps() { + $now = $this->getCurrentTimestep(); + return range($now - 2, $now + 2); + } + + } diff --git a/src/applications/auth/phid/PhabricatorAuthChallengePHIDType.php b/src/applications/auth/phid/PhabricatorAuthChallengePHIDType.php new file mode 100644 --- /dev/null +++ b/src/applications/auth/phid/PhabricatorAuthChallengePHIDType.php @@ -0,0 +1,32 @@ +ids = $ids; + return $this; + } + + public function withPHIDs(array $phids) { + $this->phids = $phids; + return $this; + } + + public function withUserPHIDs(array $user_phids) { + $this->userPHIDs = $user_phids; + return $this; + } + + public function withFactorPHIDs(array $factor_phids) { + $this->factorPHIDs = $factor_phids; + return $this; + } + + public function withChallengeTTLBetween($challenge_min, $challenge_max) { + $this->challengeTTLMin = $challenge_min; + $this->challengeTTLMax = $challenge_max; + return $this; + } + + public function newResultObject() { + return new PhabricatorAuthChallenge(); + } + + protected function loadPage() { + return $this->loadStandardPage($this->newResultObject()); + } + + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); + + if ($this->ids !== null) { + $where[] = qsprintf( + $conn, + 'id IN (%Ld)', + $this->ids); + } + + if ($this->phids !== null) { + $where[] = qsprintf( + $conn, + 'phid IN (%Ls)', + $this->phids); + } + + if ($this->userPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'userPHID IN (%Ls)', + $this->userPHIDs); + } + + if ($this->factorPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'factorPHID IN (%Ls)', + $this->factorPHIDs); + } + + if ($this->challengeTTLMin !== null) { + $where[] = qsprintf( + $conn, + 'challengeTTL >= %d', + $this->challengeTTLMin); + } + + if ($this->challengeTTLMax !== null) { + $where[] = qsprintf( + $conn, + 'challengeTTL <= %d', + $this->challengeTTLMax); + } + + return $where; + } + + public function getQueryApplicationClass() { + return 'PhabricatorAuthApplication'; + } + +} diff --git a/src/applications/auth/storage/PhabricatorAuthChallenge.php b/src/applications/auth/storage/PhabricatorAuthChallenge.php new file mode 100644 --- /dev/null +++ b/src/applications/auth/storage/PhabricatorAuthChallenge.php @@ -0,0 +1,54 @@ + array( + 'properties' => self::SERIALIZATION_JSON, + ), + self::CONFIG_AUX_PHID => true, + self::CONFIG_COLUMN_SCHEMA => array( + 'challengeKey' => 'text255', + 'challengeTTL' => 'epoch', + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_issued' => array( + 'columns' => array('userPHID', 'challengeTTL'), + ), + ), + ) + parent::getConfiguration(); + } + + public function getPHIDType() { + return PhabricatorAuthChallengePHIDType::TYPECONST; + } + + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + + + public function getCapabilities() { + return array( + PhabricatorPolicyCapability::CAN_VIEW, + ); + } + + public function getPolicy($capability) { + return PhabricatorPolicies::POLICY_NOONE; + } + + public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { + return ($viewer->getPHID() === $this->getUserPHID()); + } + +} diff --git a/src/applications/auth/storage/PhabricatorAuthFactorChallenge.php b/src/applications/auth/storage/PhabricatorAuthFactorChallenge.php deleted file mode 100644