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

epriestley 7 years ago 46052878 b6999c7e

+66 -1

7 changed files

expand all

resources

sql

autopatches

20181214.auth.01.workflowkey.sql

src

applications

auth

engine

PhabricatorAuthSessionEngine.php

factor

PhabricatorAuthFactor.php

PhabricatorTOTPAuthFactor.php

storage

PhabricatorAuthChallenge.php

PhabricatorAuthFactorConfig.php

legalpad

controller

LegalpadDocumentSignController.php

resources/sql/autopatches/20181214.auth.01.workflowkey.sql

··· 1 + ALTER TABLE {$NAMESPACE}_auth.auth_challenge 2 + ADD workflowKey VARCHAR(255) NOT NULL COLLATE {$COLLATE_TEXT};

+24

src/applications/auth/engine/PhabricatorAuthSessionEngine.php

··· 46 46 const ONETIME_USERNAME = 'rename'; 47 47 48 48 49 + private $workflowKey; 50 + 51 + public function setWorkflowKey($workflow_key) { 52 + $this->workflowKey = $workflow_key; 53 + return $this; 54 + } 55 + 56 + public function getWorkflowKey() { 57 + 58 + // TODO: A workflow key should become required in order to issue an MFA 59 + // challenge, but allow things to keep working for now until we can update 60 + // callsites. 61 + if ($this->workflowKey === null) { 62 + return 'legacy'; 63 + } 64 + 65 + return $this->workflowKey; 66 + } 67 + 68 + 49 69 /** 50 70 * Get the session kind (e.g., anonymous, user, external account) from a 51 71 * session token. Returns a `KIND_` constant. ··· 471 491 // since they never actually got marked as hisec. 472 492 if (!$factors) { 473 493 return $this->issueHighSecurityToken($session, true); 494 + } 495 + 496 + foreach ($factors as $factor) { 497 + $factor->setSessionEngine($this); 474 498 } 475 499 476 500 // Check for a rate limit without awarding points, so the user doesn't

+4 -1

src/applications/auth/factor/PhabricatorAuthFactor.php

··· 43 43 PhabricatorAuthFactorConfig $config, 44 44 PhabricatorUser $viewer) { 45 45 46 + $engine = $config->getSessionEngine(); 47 + 46 48 return id(new PhabricatorAuthChallenge()) 47 49 ->setUserPHID($viewer->getPHID()) 48 50 ->setSessionPHID($viewer->getSession()->getPHID()) 49 - ->setFactorPHID($config->getPHID()); 51 + ->setFactorPHID($config->getPHID()) 52 + ->setWorkflowKey($engine->getWorkflowKey()); 50 53 } 51 54 52 55 final public function getNewIssuedChallenges(

+14

src/applications/auth/factor/PhabricatorTOTPAuthFactor.php

··· 225 225 226 226 $session_phid = $viewer->getSession()->getPHID(); 227 227 228 + $engine = $config->getSessionEngine(); 229 + $workflow_key = $engine->getWorkflowKey(); 230 + 228 231 foreach ($challenges as $challenge) { 229 232 $challenge_timestep = (int)$challenge->getChallengeKey(); 230 233 ··· 246 249 pht( 247 250 'This factor recently issued a challenge to a different login '. 248 251 'session. Wait %s seconds for the code to cycle, then try '. 252 + 'again.', 253 + new PhutilNumber($wait_duration))); 254 + } 255 + 256 + if ($challenge->getWorkflowKey() !== $workflow_key) { 257 + return $this->newResult() 258 + ->setIsWait(true) 259 + ->setErrorMessage( 260 + pht( 261 + 'This factor recently issued a challenge for a different '. 262 + 'workflow. Wait %s seconds for the code to cycle, then try '. 249 263 'again.', 250 264 new PhutilNumber($wait_duration))); 251 265 }

src/applications/auth/storage/PhabricatorAuthChallenge.php

··· 7 7 protected $userPHID; 8 8 protected $factorPHID; 9 9 protected $sessionPHID; 10 + protected $workflowKey; 10 11 protected $challengeKey; 11 12 protected $challengeTTL; 12 13 protected $properties = array(); ··· 20 21 self::CONFIG_COLUMN_SCHEMA => array( 21 22 'challengeKey' => 'text255', 22 23 'challengeTTL' => 'epoch', 24 + 'workflowKey' => 'text255', 23 25 ), 24 26 self::CONFIG_KEY_SCHEMA => array( 25 27 'key_issued' => array(

+15

src/applications/auth/storage/PhabricatorAuthFactorConfig.php

··· 8 8 protected $factorSecret; 9 9 protected $properties = array(); 10 10 11 + private $sessionEngine; 12 + 11 13 protected function getConfiguration() { 12 14 return array( 13 15 self::CONFIG_SERIALIZATION => array( ··· 47 49 } 48 50 49 51 return $impl; 52 + } 53 + 54 + public function setSessionEngine(PhabricatorAuthSessionEngine $engine) { 55 + $this->sessionEngine = $engine; 56 + return $this; 57 + } 58 + 59 + public function getSessionEngine() { 60 + if (!$this->sessionEngine) { 61 + throw new PhutilInvalidStateException('setSessionEngine'); 62 + } 63 + 64 + return $this->sessionEngine; 50 65 } 51 66 52 67 }

src/applications/legalpad/controller/LegalpadDocumentSignController.php

··· 154 154 155 155 // Require two-factor auth to sign legal documents. 156 156 if ($viewer->isLoggedIn()) { 157 + $workflow_key = sprintf( 158 + 'legalpad.sign(%s)', 159 + $document->getPHID()); 160 + 157 161 $hisec_token = id(new PhabricatorAuthSessionEngine()) 162 + ->setWorkflowKey($workflow_key) 158 163 ->requireHighSecurityToken( 159 164 $viewer, 160 165 $request,

Configure Feed

Configure Feed