From ce953ea4479052c7e671c8c75af6833252ebefd4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 17 Dec 2018 08:40:57 -0800 Subject: [PATCH] Explicitly mark MFA challenges as "answered" and "completed" Summary: Depends on D19893. Ref T13222. See PHI873. A challenge is "answered" if you provide a valid response. A challenge is "completed" if we let you through the MFA check and do whatever actual action the check is protecting. If you only have one MFA factor, challenges will be "completed" immediately after they are "answered". However, if you have two or more factors, it's possible to "answer" one or more prompts, but fewer than all of the prompts, and end up with "answered" challenges that are not "completed". In the future, it may also be possible to answer all the challenges but then have an error occur before they are marked "completed" (for example, a unique key collision in the transaction code). For now, nothing interesting happens between "answered" and "completed". This would take the form of the caller explicitly providing flags like "wait to mark the challenges as completed until I do something" and "okay, mark the challenges as completed now". This change prevents all token reuse, even on the same workflow. Future changes will let the answered challenges "stick" to the client form so you don't have to re-answer challenges for a short period of time if you hit a unique key collision. Test Plan: - Used a token to get through an MFA gate. - Tried to go through another gate, was told to wait for a long time for the next challenge window. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13222 Differential Revision: https://secure.phabricator.com/D19894 --- .../autopatches/20181217.auth.01.digest.sql | 2 + .../sql/autopatches/20181217.auth.02.ttl.sql | 2 + .../20181217.auth.03.completed.sql | 2 + .../engine/PhabricatorAuthSessionEngine.php | 12 ++ .../auth/factor/PhabricatorAuthFactor.php | 2 +- .../factor/PhabricatorAuthFactorResult.php | 25 +++- .../auth/factor/PhabricatorTOTPAuthFactor.php | 22 +++- .../auth/storage/PhabricatorAuthChallenge.php | 113 ++++++++++++++++++ 8 files changed, 174 insertions(+), 6 deletions(-) create mode 100644 resources/sql/autopatches/20181217.auth.01.digest.sql create mode 100644 resources/sql/autopatches/20181217.auth.02.ttl.sql create mode 100644 resources/sql/autopatches/20181217.auth.03.completed.sql diff --git a/resources/sql/autopatches/20181217.auth.01.digest.sql b/resources/sql/autopatches/20181217.auth.01.digest.sql new file mode 100644 index 0000000000..8e30143e8f --- /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 index 0000000000..c8e883dbea --- /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 index 0000000000..22ca6e21ff --- /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 index f3814b949d..74183f4ffa 100644 --- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -576,6 +576,8 @@ final class PhabricatorAuthSessionEngine extends Phobject { continue; } + $issued_challenges = idx($challenge_map, $factor_phid, array()); + $impl = $factor->requireImplementation(); $validation_result = $impl->getResultFromChallengeResponse( @@ -592,6 +594,16 @@ final class PhabricatorAuthSessionEngine extends Phobject { } 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 index be99df9c79..8cd61f089d 100644 --- a/src/applications/auth/factor/PhabricatorAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorAuthFactor.php @@ -45,7 +45,7 @@ abstract class PhabricatorAuthFactor extends Phobject { $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 index d75480747d..faa25b4f42 100644 --- 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 index e2f0dc74b3..0984347197 100644 --- a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php @@ -283,6 +283,17 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor { '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 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor { (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 index 63d2092e49..89ffd67ef7 100644 --- a/src/applications/auth/storage/PhabricatorAuthChallenge.php +++ b/src/applications/auth/storage/PhabricatorAuthChallenge.php @@ -10,8 +10,20 @@ final class PhabricatorAuthChallenge 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 @@ final class PhabricatorAuthChallenge 'challengeKey' => 'text255', 'challengeTTL' => 'epoch', 'workflowKey' => 'text255', + 'responseDigest' => 'text255?', + 'responseTTL' => 'epoch?', + 'isCompleted' => 'bool', ), self::CONFIG_KEY_SCHEMA => array( 'key_issued' => array( @@ -38,6 +53,104 @@ final class PhabricatorAuthChallenge 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 )----------------------------------------- */