Changeset View
Changeset View
Standalone View
Standalone View
src/applications/auth/factor/PhabricatorTOTPAuthFactor.php
| Show First 20 Lines • Show All 71 Lines • ▼ Show 20 Lines | if (!strlen($key)) { | ||||
| ->save(); | ->save(); | ||||
| unset($unguarded); | unset($unguarded); | ||||
| } | } | ||||
| $code = $request->getStr('totpcode'); | $code = $request->getStr('totpcode'); | ||||
| $e_code = true; | $e_code = true; | ||||
| if ($request->getExists('totp')) { | if ($request->getExists('totp')) { | ||||
| $okay = $this->verifyTOTPCode( | $okay = (bool)$this->getValidResponseTimestep( | ||||
| $user, | $this->getAllowedTimesteps($this->getCurrentTimestep()), | ||||
| new PhutilOpaqueEnvelope($key), | new PhutilOpaqueEnvelope($key), | ||||
| $code); | (string)$code); | ||||
| if ($okay) { | if ($okay) { | ||||
| $config = $this->newConfigForUser($user) | $config = $this->newConfigForUser($user) | ||||
| ->setFactorName(pht('Mobile App (TOTP)')) | ->setFactorName(pht('Mobile App (TOTP)')) | ||||
| ->setFactorSecret($key); | ->setFactorSecret($key); | ||||
| return $config; | return $config; | ||||
| } else { | } else { | ||||
| ▲ Show 20 Lines • Show All 143 Lines • ▼ Show 20 Lines | protected function newResultFromIssuedChallenges( | ||||
| // This is defusing attacks where you (broadly) look at someone's phone | // This is defusing attacks where you (broadly) look at someone's phone | ||||
| // and type the code in more quickly than they do. | // and type the code in more quickly than they do. | ||||
| $session_phid = $viewer->getSession()->getPHID(); | $session_phid = $viewer->getSession()->getPHID(); | ||||
| $now = PhabricatorTime::getNow(); | $now = PhabricatorTime::getNow(); | ||||
| $engine = $config->getSessionEngine(); | $engine = $config->getSessionEngine(); | ||||
| $workflow_key = $engine->getWorkflowKey(); | $workflow_key = $engine->getWorkflowKey(); | ||||
| $current_timestep = $this->getCurrentTimestep(); | |||||
| foreach ($challenges as $challenge) { | foreach ($challenges as $challenge) { | ||||
| $challenge_timestep = (int)$challenge->getChallengeKey(); | $challenge_timestep = (int)$challenge->getChallengeKey(); | ||||
| $wait_duration = ($challenge->getChallengeTTL() - $now) + 1; | $wait_duration = ($challenge->getChallengeTTL() - $now) + 1; | ||||
| if ($challenge->getSessionPHID() !== $session_phid) { | if ($challenge->getSessionPHID() !== $session_phid) { | ||||
| return $this->newResult() | return $this->newResult() | ||||
| ->setIsWait(true) | ->setIsWait(true) | ||||
| ->setErrorMessage( | ->setErrorMessage( | ||||
| Show All 9 Lines | foreach ($challenges as $challenge) { | ||||
| ->setIsWait(true) | ->setIsWait(true) | ||||
| ->setErrorMessage( | ->setErrorMessage( | ||||
| pht( | pht( | ||||
| 'This factor recently issued a challenge for a different '. | 'This factor recently issued a challenge for a different '. | ||||
| 'workflow. Wait %s second(s) for the code to cycle, then try '. | 'workflow. Wait %s second(s) for the code to cycle, then try '. | ||||
| 'again.', | 'again.', | ||||
| new PhutilNumber($wait_duration))); | new PhutilNumber($wait_duration))); | ||||
| } | } | ||||
| // If the current realtime timestep isn't a valid response to the current | |||||
| // challenge but the challenge hasn't expired yet, we're locking out | |||||
| // the factor to prevent challenge windows from overlapping. Let the user | |||||
| // know that they should wait for a new challenge. | |||||
| $challenge_timesteps = $this->getAllowedTimesteps($challenge_timestep); | |||||
| if (!isset($challenge_timesteps[$current_timestep])) { | |||||
| return $this->newResult() | |||||
| ->setIsWait(true) | |||||
| ->setErrorMessage( | |||||
| pht( | |||||
| 'This factor recently issued a challenge which has expired. '. | |||||
| 'A new challenge can not be issued yet. Wait %s second(s) for '. | |||||
| 'the code to cycle, then try again.', | |||||
| new PhutilNumber($wait_duration))); | |||||
| } | |||||
| } | } | ||||
| return null; | return null; | ||||
| } | } | ||||
| protected function newResultFromChallengeResponse( | protected function newResultFromChallengeResponse( | ||||
| PhabricatorAuthFactorConfig $config, | PhabricatorAuthFactorConfig $config, | ||||
| PhabricatorUser $viewer, | PhabricatorUser $viewer, | ||||
| AphrontRequest $request, | AphrontRequest $request, | ||||
| array $challenges) { | array $challenges) { | ||||
| $code = $request->getStr($this->getParameterName($config, 'totpcode')); | $code = $request->getStr($this->getParameterName($config, 'totpcode')); | ||||
| $key = new PhutilOpaqueEnvelope($config->getFactorSecret()); | |||||
| $result = $this->newResult() | $result = $this->newResult() | ||||
| ->setValue($code); | ->setValue($code); | ||||
| if ($this->verifyTOTPCode($viewer, $key, (string)$code)) { | // We expect to reach TOTP validation with exactly one valid challenge. | ||||
| if (count($challenges) !== 1) { | |||||
amckinley: Do I remember correctly that you don't believe in using `UNIQUE` keys at the DB level? Even if… | |||||
epriestleyAuthorUnsubmitted Done Inline ActionsI like UNIQUE keys (and we use tons of them). I'm not so hot on FOREIGN keys. I think we can't put either type of key on the database because SMS/push factors can freely have multiple challenges for the same session/workflow with overlapping validity windows. Only TOTP has this restriction. (A lot of this logic is living in the TOTP factor instead of the base class because it's specific to how TOTP works.) We could do transactions here, but then the persistence code would have to live in the factor, and you're only ever racing against yourself in the same session, which seems like a hard race to hit. Or we could maybe add an optional unique key that only gave us uniqueness, and not populate it for SMS/push, but populate it for TOTP. I'd sort of like to see a user hit this first, though. epriestley: I like UNIQUE keys (and we use tons of them). I'm not so hot on FOREIGN keys.
I think we can't… | |||||
| throw new Exception( | |||||
| pht( | |||||
| 'Reached TOTP challenge validation with an unexpected number of '. | |||||
| 'unexpired challenges (%d), expected exactly one.', | |||||
| phutil_count($challenges))); | |||||
| } | |||||
| $challenge = head($challenges); | |||||
| $challenge_timestep = (int)$challenge->getChallengeKey(); | |||||
| $current_timestep = $this->getCurrentTimestep(); | |||||
| $challenge_timesteps = $this->getAllowedTimesteps($challenge_timestep); | |||||
| $current_timesteps = $this->getAllowedTimesteps($current_timestep); | |||||
amckinleyUnsubmitted Not Done Inline ActionsMaybe move this logic into getValidResponseTimestep? amckinley: Maybe move this logic into `getValidResponseTimestep`? | |||||
epriestleyAuthorUnsubmitted Done Inline ActionsIt can't easily move because we array_intersect_key() the two windows. getValidResponseTimestep() could have a signature like array $list_of_timesteps_and_the_response_must_belong_to_all_of_them but that feels less intuitive? epriestley: It can't easily move because we `array_intersect_key()` the two windows. | |||||
| // We require responses be both valid for the challenge and valid for the | |||||
| // current timestep. A longer challenge TTL doesn't let you use older | |||||
amckinleyUnsubmitted Not Done Inline ActionsWhat's the attack where you respond with a response that's valid for the challenge but invalid for the current timestamp? I read the correct response off your phone with my telescope, immediately kill your internet access, and then fix a sandwich before using your now-expired code? amckinley: What's the attack where you respond with a response that's valid for the challenge but invalid… | |||||
epriestleyAuthorUnsubmitted Done Inline ActionsSomething like that, yeah. You somehow interrupt the user and have a relatively longer time to steal the code (up to twice as much time-ish?). This is most useful if their device is also somehow skewed way backward since it means the oldest code we'd accept becomes unacceptable after 30 seconds instead of after 150 seconds if I did the math right. But, really, all the attacks connected to all of this are pretty hard for me to take very seriously. epriestley: Something like that, yeah. You somehow interrupt the user and have a relatively longer time to… | |||||
| // codes for a longer period of time. | |||||
| $valid_timestep = $this->getValidResponseTimestep( | |||||
| array_intersect_key($challenge_timesteps, $current_timesteps), | |||||
| new PhutilOpaqueEnvelope($config->getFactorSecret()), | |||||
| (string)$code); | |||||
| if ($valid_timestep) { | |||||
amckinleyUnsubmitted Not Done Inline Actions$valid_timestep_and_response? Or just $valid? I had to go read getValidResponseTimestep before being sure that this wasn't accidentally skipping one of the two checks. Or maybe just rename the method to getTimestepAndResponseValid. amckinley: `$valid_timestep_and_response`? Or just `$valid`? I had to go read `getValidResponseTimestep`… | |||||
epriestleyAuthorUnsubmitted Done Inline ActionsMaybe getTimestepAtWhichResponseIsValid()? epriestley: Maybe `getTimestepAtWhichResponseIsValid()`? | |||||
| $result->setIsValid(true); | $result->setIsValid(true); | ||||
| } else { | } else { | ||||
| if (strlen($code)) { | if (strlen($code)) { | ||||
| $error_message = pht('Invalid'); | $error_message = pht('Invalid'); | ||||
| } else { | } else { | ||||
| $error_message = pht('Required'); | $error_message = pht('Required'); | ||||
| } | } | ||||
| $result->setErrorMessage($error_message); | $result->setErrorMessage($error_message); | ||||
| } | } | ||||
| return $result; | return $result; | ||||
| } | } | ||||
| public static function generateNewTOTPKey() { | public static function generateNewTOTPKey() { | ||||
| return strtoupper(Filesystem::readRandomCharacters(32)); | return strtoupper(Filesystem::readRandomCharacters(32)); | ||||
| } | } | ||||
| private function verifyTOTPCode( | |||||
| PhabricatorUser $user, | |||||
| PhutilOpaqueEnvelope $key, | |||||
| $code) { | |||||
| $now = (int)(time() / 30); | |||||
| // Allow the user to enter a code a few minutes away on either side, in | |||||
| // case the server or client has some clock skew. | |||||
| for ($offset = -2; $offset <= 2; $offset++) { | |||||
| $real = self::getTOTPCode($key, $now + $offset); | |||||
| if (phutil_hashes_are_identical($real, $code)) { | |||||
| return true; | |||||
| } | |||||
| } | |||||
| // TODO: After validating a code, this should mark it as used and prevent | |||||
| // it from being reused. | |||||
amckinleyUnsubmitted Not Done Inline ActionsThis part of the TODO is still accurate, right? amckinley: This part of the `TODO` is still accurate, right? | |||||
epriestleyAuthorUnsubmitted Done Inline ActionsYes, until the next change (D19894). epriestley: Yes, until the next change (D19894). | |||||
| return false; | |||||
| } | |||||
| public static function base32Decode($buf) { | public static function base32Decode($buf) { | ||||
| $buf = strtoupper($buf); | $buf = strtoupper($buf); | ||||
| $map = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ234567'; | $map = 'ABCDEFGHIJKLMNOPQRSTUVWXYZ234567'; | ||||
| $map = str_split($map); | $map = str_split($map); | ||||
| $map = array_flip($map); | $map = array_flip($map); | ||||
| $out = ''; | $out = ''; | ||||
| ▲ Show 20 Lines • Show All 85 Lines • ▼ Show 20 Lines | private function getTimestepDuration() { | ||||
| return 30; | return 30; | ||||
| } | } | ||||
| private function getCurrentTimestep() { | private function getCurrentTimestep() { | ||||
| $duration = $this->getTimestepDuration(); | $duration = $this->getTimestepDuration(); | ||||
| return (int)(PhabricatorTime::getNow() / $duration); | return (int)(PhabricatorTime::getNow() / $duration); | ||||
| } | } | ||||
| private function getAllowedTimesteps() { | private function getAllowedTimesteps($at_timestep) { | ||||
| $current_step = $this->getCurrentTimestep(); | |||||
| $window = $this->getTimestepWindowSize(); | $window = $this->getTimestepWindowSize(); | ||||
| return range($current_step - $window, $current_step + $window); | $range = range($at_timestep - $window, $at_timestep + $window); | ||||
| return array_fuse($range); | |||||
| } | } | ||||
| private function getTimestepWindowSize() { | private function getTimestepWindowSize() { | ||||
| // The user is allowed to provide a code from the recent past or the | |||||
| // near future to account for minor clock skew between the client | |||||
| // and server, and the time it takes to actually enter a code. | |||||
| return 2; | return 2; | ||||
| } | } | ||||
| private function getValidResponseTimestep( | |||||
| array $timesteps, | |||||
| PhutilOpaqueEnvelope $key, | |||||
| $code) { | |||||
| foreach ($timesteps as $timestep) { | |||||
| $expect_code = self::getTOTPCode($key, $timestep); | |||||
| if (phutil_hashes_are_identical($code, $expect_code)) { | |||||
| return $timestep; | |||||
| } | |||||
| } | |||||
| return null; | |||||
| } | |||||
| } | } | ||||
Do I remember correctly that you don't believe in using UNIQUE keys at the DB level? Even if you don't, it seems like it might be a better idea to have the challenge-writing INSERT happen inside a transaction that first looks for existing challenges matching the session/factor pair. Throwing at this point seems less helpful from a potential future debugging standpoint.