From 46052878b1de10f33feab0895c8c90d2e87c20ec Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 14 Dec 2018 05:39:53 -0800 Subject: [PATCH] Bind MFA challenges to particular workflows, like signing a specific Legalpad document Summary: Depends on D19888. Ref T13222. When we issue an MFA challenge, prevent the user from responding to it in the context of a different workflow: if you ask for MFA to do something minor (award a token) you can't use the same challenge to do something more serious (launch nukes). This defuses highly-hypothetical attacks where the attacker: - already controls the user's session (since the challenge is already bound to the session); and - can observe MFA codes. One version of this attack is the "spill coffee on the victim when the code is shown on their phone, then grab their phone" attack. This whole vector really strains the bounds of plausibility, but it's easy to lock challenges to a workflow and it's possible that there's some more clever version of the "spill coffee" attack available to more sophisticated social engineers or with future MFA factors which we don't yet support. The "spill coffee" attack, in detail, is: - Go over to the victim's desk. - Ask them to do something safe and nonsuspicious that requires MFA (sign `L123 Best Friendship Agreement`). - When they unlock their phone, spill coffee all over them. - Urge them to go to the bathroom to clean up immediately, leaving their phone and computer in your custody. - Type the MFA code shown on the phone into a dangerous MFA prompt (sign `L345 Eternal Declaration of War`). - When they return, they may not suspect anything (it would be normal for the MFA token to have expired), or you can spill more coffee on their computer now to destroy it, and blame it on the earlier spill. Test Plan: - Triggered signatures for two different documents. - Got prompted in one, got a "wait" in the other. - Backed out of the good prompt, returned, still prompted. - Answered the good prompt. - Waited for the bad prompt to expire. - Went through the bad prompt again, got an actual prompt this time. Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13222 Differential Revision: https://secure.phabricator.com/D19889 --- .../20181214.auth.01.workflowkey.sql | 2 ++ .../engine/PhabricatorAuthSessionEngine.php | 24 +++++++++++++++++++ .../auth/factor/PhabricatorAuthFactor.php | 5 +++- .../auth/factor/PhabricatorTOTPAuthFactor.php | 14 +++++++++++ .../auth/storage/PhabricatorAuthChallenge.php | 2 ++ .../storage/PhabricatorAuthFactorConfig.php | 15 ++++++++++++ .../LegalpadDocumentSignController.php | 5 ++++ 7 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 resources/sql/autopatches/20181214.auth.01.workflowkey.sql diff --git a/resources/sql/autopatches/20181214.auth.01.workflowkey.sql b/resources/sql/autopatches/20181214.auth.01.workflowkey.sql new file mode 100644 index 0000000000..538778e218 --- /dev/null +++ b/resources/sql/autopatches/20181214.auth.01.workflowkey.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_auth.auth_challenge + ADD workflowKey VARCHAR(255) NOT NULL COLLATE {$COLLATE_TEXT}; diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php index acd16f690f..f3814b949d 100644 --- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php +++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php @@ -46,6 +46,26 @@ final class PhabricatorAuthSessionEngine extends Phobject { const ONETIME_USERNAME = 'rename'; + private $workflowKey; + + public function setWorkflowKey($workflow_key) { + $this->workflowKey = $workflow_key; + return $this; + } + + public function getWorkflowKey() { + + // TODO: A workflow key should become required in order to issue an MFA + // challenge, but allow things to keep working for now until we can update + // callsites. + if ($this->workflowKey === null) { + return 'legacy'; + } + + return $this->workflowKey; + } + + /** * Get the session kind (e.g., anonymous, user, external account) from a * session token. Returns a `KIND_` constant. @@ -473,6 +493,10 @@ final class PhabricatorAuthSessionEngine extends Phobject { return $this->issueHighSecurityToken($session, true); } + foreach ($factors as $factor) { + $factor->setSessionEngine($this); + } + // Check for a rate limit without awarding points, so the user doesn't // get partway through the workflow only to get blocked. PhabricatorSystemActionEngine::willTakeAction( diff --git a/src/applications/auth/factor/PhabricatorAuthFactor.php b/src/applications/auth/factor/PhabricatorAuthFactor.php index 2b8ec486e2..be99df9c79 100644 --- a/src/applications/auth/factor/PhabricatorAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorAuthFactor.php @@ -43,10 +43,13 @@ abstract class PhabricatorAuthFactor extends Phobject { PhabricatorAuthFactorConfig $config, PhabricatorUser $viewer) { + $engine = $config->getSessionEngine(); + return id(new PhabricatorAuthChallenge()) ->setUserPHID($viewer->getPHID()) ->setSessionPHID($viewer->getSession()->getPHID()) - ->setFactorPHID($config->getPHID()); + ->setFactorPHID($config->getPHID()) + ->setWorkflowKey($engine->getWorkflowKey()); } final public function getNewIssuedChallenges( diff --git a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php index 7f426d0138..373745e244 100644 --- a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php @@ -225,6 +225,9 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor { $session_phid = $viewer->getSession()->getPHID(); + $engine = $config->getSessionEngine(); + $workflow_key = $engine->getWorkflowKey(); + foreach ($challenges as $challenge) { $challenge_timestep = (int)$challenge->getChallengeKey(); @@ -249,6 +252,17 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor { 'again.', new PhutilNumber($wait_duration))); } + + if ($challenge->getWorkflowKey() !== $workflow_key) { + return $this->newResult() + ->setIsWait(true) + ->setErrorMessage( + pht( + 'This factor recently issued a challenge for a different '. + 'workflow. Wait %s seconds for the code to cycle, then try '. + 'again.', + new PhutilNumber($wait_duration))); + } } return null; diff --git a/src/applications/auth/storage/PhabricatorAuthChallenge.php b/src/applications/auth/storage/PhabricatorAuthChallenge.php index 4ef2a7054f..63d2092e49 100644 --- a/src/applications/auth/storage/PhabricatorAuthChallenge.php +++ b/src/applications/auth/storage/PhabricatorAuthChallenge.php @@ -7,6 +7,7 @@ final class PhabricatorAuthChallenge protected $userPHID; protected $factorPHID; protected $sessionPHID; + protected $workflowKey; protected $challengeKey; protected $challengeTTL; protected $properties = array(); @@ -20,6 +21,7 @@ final class PhabricatorAuthChallenge self::CONFIG_COLUMN_SCHEMA => array( 'challengeKey' => 'text255', 'challengeTTL' => 'epoch', + 'workflowKey' => 'text255', ), self::CONFIG_KEY_SCHEMA => array( 'key_issued' => array( diff --git a/src/applications/auth/storage/PhabricatorAuthFactorConfig.php b/src/applications/auth/storage/PhabricatorAuthFactorConfig.php index 8420ea9ba7..2bed939402 100644 --- a/src/applications/auth/storage/PhabricatorAuthFactorConfig.php +++ b/src/applications/auth/storage/PhabricatorAuthFactorConfig.php @@ -8,6 +8,8 @@ final class PhabricatorAuthFactorConfig extends PhabricatorAuthDAO { protected $factorSecret; protected $properties = array(); + private $sessionEngine; + protected function getConfiguration() { return array( self::CONFIG_SERIALIZATION => array( @@ -49,4 +51,17 @@ final class PhabricatorAuthFactorConfig extends PhabricatorAuthDAO { return $impl; } + public function setSessionEngine(PhabricatorAuthSessionEngine $engine) { + $this->sessionEngine = $engine; + return $this; + } + + public function getSessionEngine() { + if (!$this->sessionEngine) { + throw new PhutilInvalidStateException('setSessionEngine'); + } + + return $this->sessionEngine; + } + } diff --git a/src/applications/legalpad/controller/LegalpadDocumentSignController.php b/src/applications/legalpad/controller/LegalpadDocumentSignController.php index 8ef35e3493..27769432c2 100644 --- a/src/applications/legalpad/controller/LegalpadDocumentSignController.php +++ b/src/applications/legalpad/controller/LegalpadDocumentSignController.php @@ -154,7 +154,12 @@ final class LegalpadDocumentSignController extends LegalpadController { // Require two-factor auth to sign legal documents. if ($viewer->isLoggedIn()) { + $workflow_key = sprintf( + 'legalpad.sign(%s)', + $document->getPHID()); + $hisec_token = id(new PhabricatorAuthSessionEngine()) + ->setWorkflowKey($workflow_key) ->requireHighSecurityToken( $viewer, $request,