diff --git a/resources/sql/autopatches/20181217.auth.01.digest.sql b/resources/sql/autopatches/20181217.auth.01.digest.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20181217.auth.01.digest.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_auth.auth_challenge + ADD responseDigest VARCHAR(255) COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20181217.auth.02.ttl.sql b/resources/sql/autopatches/20181217.auth.02.ttl.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20181217.auth.02.ttl.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_auth.auth_challenge + ADD responseTTL INT UNSIGNED; diff --git a/resources/sql/autopatches/20181217.auth.03.completed.sql b/resources/sql/autopatches/20181217.auth.03.completed.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20181217.auth.03.completed.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_auth.auth_challenge + ADD isCompleted BOOL NOT NULL; 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 @@ -576,6 +576,8 @@ continue; } + $issued_challenges = idx($challenge_map, $factor_phid, array()); + $impl = $factor->requireImplementation(); $validation_result = $impl->getResultFromChallengeResponse( @@ -592,6 +594,16 @@ } if ($ok) { + // We're letting you through, so mark all the challenges you + // responded to as completed. These challenges can never be used + // again, even by the same session and workflow: you can't use the + // same response to take two different actions, even if those actions + // are of the same type. + foreach ($validation_results as $validation_result) { + $challenge = $validation_result->getAnsweredChallenge() + ->markChallengeAsCompleted(); + } + // Give the user a credit back for a successful factor verification. PhabricatorSystemActionEngine::willTakeAction( array($viewer->getPHID()), 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 @@ -45,7 +45,7 @@ $engine = $config->getSessionEngine(); - return id(new PhabricatorAuthChallenge()) + return PhabricatorAuthChallenge::initializeNewChallenge() ->setUserPHID($viewer->getPHID()) ->setSessionPHID($viewer->getSession()->getPHID()) ->setFactorPHID($config->getPHID()) 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 @@ -3,19 +3,36 @@ final class PhabricatorAuthFactorResult extends Phobject { - private $isValid = false; + private $answeredChallenge; private $isWait = false; private $errorMessage; private $value; private $issuedChallenges = array(); - public function setIsValid($is_valid) { - $this->isValid = $is_valid; + public function setAnsweredChallenge(PhabricatorAuthChallenge $challenge) { + if (!$challenge->getIsAnsweredChallenge()) { + throw new PhutilInvalidStateException('markChallengeAsAnswered'); + } + + if ($challenge->getIsCompleted()) { + throw new Exception( + pht( + 'A completed challenge was provided as an answered challenge. '. + 'The underlying factor is implemented improperly, challenges '. + 'may not be reused.')); + } + + $this->answeredChallenge = $challenge; + return $this; } + public function getAnsweredChallenge() { + return $this->answeredChallenge; + } + public function getIsValid() { - return $this->isValid; + return (bool)$this->getAnsweredChallenge(); } public function setIsWait($is_wait) { 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 @@ -283,6 +283,17 @@ 'the code to cycle, then try again.', new PhutilNumber($wait_duration))); } + + if ($challenge->getIsReusedChallenge()) { + return $this->newResult() + ->setIsWait(true) + ->setErrorMessage( + pht( + 'You recently provided a response to this factor. Responses '. + 'may not be reused. Wait %s second(s) for the code to cycle, '. + 'then try again.', + new PhutilNumber($wait_duration))); + } } return null; @@ -325,7 +336,16 @@ (string)$code); if ($valid_timestep) { - $result->setIsValid(true); + $now = PhabricatorTime::getNow(); + $step_duration = $this->getTimestepDuration(); + $step_window = $this->getTimestepWindowSize(); + $ttl = $now + ($step_duration * $step_window); + + $challenge + ->setProperty('totp.timestep', $valid_timestep) + ->markChallengeAsAnswered($ttl); + + $result->setAnsweredChallenge($challenge); } else { if (strlen($code)) { $error_message = pht('Invalid'); diff --git a/src/applications/auth/storage/PhabricatorAuthChallenge.php b/src/applications/auth/storage/PhabricatorAuthChallenge.php --- a/src/applications/auth/storage/PhabricatorAuthChallenge.php +++ b/src/applications/auth/storage/PhabricatorAuthChallenge.php @@ -10,8 +10,20 @@ protected $workflowKey; protected $challengeKey; protected $challengeTTL; + protected $responseDigest; + protected $responseTTL; + protected $isCompleted; protected $properties = array(); + private $responseToken; + + const TOKEN_DIGEST_KEY = 'auth.challenge.token'; + + public static function initializeNewChallenge() { + return id(new self()) + ->setIsCompleted(0); + } + protected function getConfiguration() { return array( self::CONFIG_SERIALIZATION => array( @@ -22,6 +34,9 @@ 'challengeKey' => 'text255', 'challengeTTL' => 'epoch', 'workflowKey' => 'text255', + 'responseDigest' => 'text255?', + 'responseTTL' => 'epoch?', + 'isCompleted' => 'bool', ), self::CONFIG_KEY_SCHEMA => array( 'key_issued' => array( @@ -38,6 +53,104 @@ return PhabricatorAuthChallengePHIDType::TYPECONST; } + public function getIsReusedChallenge() { + if ($this->getIsCompleted()) { + return true; + } + + // TODO: A challenge is "reused" if it has been answered previously and + // the request doesn't include proof that the client provided the answer. + // Since we aren't tracking client responses yet, any answered challenge + // is always a reused challenge for now. + + return $this->getIsAnsweredChallenge(); + } + + public function getIsAnsweredChallenge() { + return (bool)$this->getResponseDigest(); + } + + public function markChallengeAsAnswered($ttl) { + $token = Filesystem::readRandomCharacters(32); + $token = new PhutilOpaqueEnvelope($token); + + return $this + ->setResponseToken($token, $ttl) + ->save(); + } + + public function markChallengeAsCompleted() { + return $this + ->setIsCompleted(true) + ->save(); + } + + public function setResponseToken(PhutilOpaqueEnvelope $token, $ttl) { + if (!$this->getUserPHID()) { + throw new PhutilInvalidStateException('setUserPHID'); + } + + if ($this->responseToken) { + throw new Exception( + pht( + 'This challenge already has a response token; you can not '. + 'set a new response token.')); + } + + $now = PhabricatorTime::getNow(); + if ($ttl < $now) { + throw new Exception( + pht( + 'Response TTL is invalid: TTLs must be an epoch timestamp '. + 'coresponding to a future time (did you use a relative TTL by '. + 'mistake?).')); + } + + if (preg_match('/ /', $token->openEnvelope())) { + throw new Exception( + pht( + 'The response token for this challenge is invalid: response '. + 'tokens may not include spaces.')); + } + + $digest = PhabricatorHash::digestWithNamedKey( + $token->openEnvelope(), + self::TOKEN_DIGEST_KEY); + + if ($this->responseDigest !== null) { + if (!phutil_hashes_are_identical($digest, $this->responseDigest)) { + throw new Exception( + pht( + 'Invalid response token for this challenge: token digest does '. + 'not match stored digest.')); + } + } else { + $this->responseDigest = $digest; + } + + $this->responseToken = $token; + $this->responseTTL = $ttl; + + return $this; + } + + public function setResponseDigest($value) { + throw new Exception( + pht( + 'You can not set the response digest for a challenge directly. '. + 'Instead, set a response token. A response digest will be computed '. + 'automatically.')); + } + + public function setProperty($key, $value) { + $this->properties[$key] = $value; + return $this; + } + + public function getProperty($key, $default = null) { + return $this->properties[$key]; + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */