Add CSRF to SMS challenges, and pave the way for more MFA types (including Duo)

Summary:
Depends on D20026. Ref T13222. Ref T13231. The primary change here is that we'll no longer send you an SMS if you hit an MFA gate without CSRF tokens.

Then there's a lot of support for genralizing into Duo (and other push factors, potentially), I'll annotate things inline.

Test Plan: Implemented Duo, elsewhere.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13231, T13222

Differential Revision: https://secure.phabricator.com/D20028

resources/celerity/map.php

@@ -9,7 +9,7 @@ return array(
   'names' => array(
     'conpherence.pkg.css' => '3c8a0668',
     'conpherence.pkg.js' => '020aebcf',
-    'core.pkg.css' => 'a66ea2e7',
+    'core.pkg.css' => 'e0cb8094',
     'core.pkg.js' => '5c737607',
     'differential.pkg.css' => 'b8df73d4',
     'differential.pkg.js' => '67c9ea4c',
@@ -151,7 +151,7 @@ return array(
     'rsrc/css/phui/phui-document.css' => '52b748a5',
     'rsrc/css/phui/phui-feed-story.css' => 'a0c05029',
     'rsrc/css/phui/phui-fontkit.css' => '9b714a5e',
-    'rsrc/css/phui/phui-form-view.css' => '9508671e',
+    'rsrc/css/phui/phui-form-view.css' => '0807e7ac',
     'rsrc/css/phui/phui-form.css' => '159e2d9c',
     'rsrc/css/phui/phui-head-thing.css' => 'd7f293df',
     'rsrc/css/phui/phui-header-view.css' => '93cea4ec',
@@ -159,7 +159,7 @@ return array(
     'rsrc/css/phui/phui-icon-set-selector.css' => '7aa5f3ec',
     'rsrc/css/phui/phui-icon.css' => '281f964d',
     'rsrc/css/phui/phui-image-mask.css' => '62c7f4d2',
-    'rsrc/css/phui/phui-info-view.css' => 'f9464caf',
+    'rsrc/css/phui/phui-info-view.css' => '37b8d9ce',
     'rsrc/css/phui/phui-invisible-character-view.css' => 'c694c4a4',
     'rsrc/css/phui/phui-left-right.css' => '68513c34',
     'rsrc/css/phui/phui-lightbox.css' => '4ebf22da',
@@ -817,7 +817,7 @@ return array(
     'phui-font-icon-base-css' => 'd7994e06',
     'phui-fontkit-css' => '9b714a5e',
     'phui-form-css' => '159e2d9c',
-    'phui-form-view-css' => '9508671e',
+    'phui-form-view-css' => '0807e7ac',
     'phui-head-thing-view-css' => 'd7f293df',
     'phui-header-view-css' => '93cea4ec',
     'phui-hovercard' => '074f0783',
@@ -825,7 +825,7 @@ return array(
     'phui-icon-set-selector-css' => '7aa5f3ec',
     'phui-icon-view-css' => '281f964d',
     'phui-image-mask-css' => '62c7f4d2',
-    'phui-info-view-css' => 'f9464caf',
+    'phui-info-view-css' => '37b8d9ce',
     'phui-inline-comment-view-css' => '48acce5b',
     'phui-invisible-character-view-css' => 'c694c4a4',
     'phui-left-right-css' => '68513c34',

src/__phutil_library_map__.php

@@ -2239,6 +2239,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthFactorProviderTransactionType' => 'applications/auth/xaction/PhabricatorAuthFactorProviderTransactionType.php',
     'PhabricatorAuthFactorProviderViewController' => 'applications/auth/controller/mfa/PhabricatorAuthFactorProviderViewController.php',
     'PhabricatorAuthFactorResult' => 'applications/auth/factor/PhabricatorAuthFactorResult.php',
+    'PhabricatorAuthFactorResultException' => 'applications/auth/exception/PhabricatorAuthFactorResultException.php',
     'PhabricatorAuthFactorTestCase' => 'applications/auth/factor/__tests__/PhabricatorAuthFactorTestCase.php',
     'PhabricatorAuthFinishController' => 'applications/auth/controller/PhabricatorAuthFinishController.php',
     'PhabricatorAuthHMACKey' => 'applications/auth/storage/PhabricatorAuthHMACKey.php',
@@ -7965,6 +7966,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthFactorProviderTransactionType' => 'PhabricatorModularTransactionType',
     'PhabricatorAuthFactorProviderViewController' => 'PhabricatorAuthFactorProviderController',
     'PhabricatorAuthFactorResult' => 'Phobject',
+    'PhabricatorAuthFactorResultException' => 'Exception',
     'PhabricatorAuthFactorTestCase' => 'PhabricatorTestCase',
     'PhabricatorAuthFinishController' => 'PhabricatorAuthController',
     'PhabricatorAuthHMACKey' => 'PhabricatorAuthDAO',

src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php

@@ -38,10 +38,14 @@ final class PhabricatorHighSecurityRequestExceptionHandler
       $request);
 
     $is_wait = false;
+    $is_continue = false;
     foreach ($results as $result) {
       if ($result->getIsWait()) {
         $is_wait = true;
-        break;
+      }
+
+      if ($result->getIsContinue()) {
+        $is_continue = true;
       }
     }
 
@@ -55,7 +59,7 @@ final class PhabricatorHighSecurityRequestExceptionHandler
 
     if ($is_wait) {
       $submit = pht('Wait Patiently');
-    } else if ($is_upgrade) {
+    } else if ($is_upgrade && !$is_continue) {
       $submit = pht('Enter High Security');
     } else {
       $submit = pht('Continue');
@@ -74,19 +78,21 @@ final class PhabricatorHighSecurityRequestExceptionHandler
     $form_layout = $form->buildLayoutView();
 
     if ($is_upgrade) {
-      $dialog
-        ->setErrors(
-          array(
+      $messages = array(
         pht(
           'You are taking an action which requires you to enter '.
           'high security.'),
-          ))
+      );
+
+      $info_view = id(new PHUIInfoView())
+        ->setSeverity(PHUIInfoView::SEVERITY_MFA)
+        ->setErrors($messages);
+
+      $dialog
+        ->appendChild($info_view)
         ->appendParagraph(
           pht(
-            'High security mode helps protect your account from security '.
-            'threats, like session theft or someone messing with your stuff '.
-            'while you\'re grabbing a coffee. To enter high security mode, '.
-            'confirm your credentials.'))
+            'To enter high security mode, confirm your credentials:'))
         ->appendChild($form_layout)
         ->appendParagraph(
           pht(

src/applications/auth/engine/PhabricatorAuthSessionEngine.php

@@ -47,6 +47,7 @@ final class PhabricatorAuthSessionEngine extends Phobject {
 
 
   private $workflowKey;
+  private $request;
 
   public function setWorkflowKey($workflow_key) {
     $this->workflowKey = $workflow_key;
@@ -65,6 +66,10 @@ final class PhabricatorAuthSessionEngine extends Phobject {
     return $this->workflowKey;
   }
 
+  public function getRequest() {
+    return $this->request;
+  }
+
 
   /**
    * Get the session kind (e.g., anonymous, user, external account) from a
@@ -480,6 +485,7 @@ final class PhabricatorAuthSessionEngine extends Phobject {
       return $this->issueHighSecurityToken($session, true);
     }
 
+    $this->request = $request;
     foreach ($factors as $factor) {
       $factor->setSessionEngine($this);
     }
@@ -523,10 +529,17 @@ final class PhabricatorAuthSessionEngine extends Phobject {
       $provider = $factor->getFactorProvider();
       $impl = $provider->getFactor();
 
+      try {
         $new_challenges = $impl->getNewIssuedChallenges(
           $factor,
           $viewer,
           $issued_challenges);
+      } catch (PhabricatorAuthFactorResultException $ex) {
+        $ok = false;
+        $validation_results[$factor_phid] = $ex->getResult();
+        $challenge_map[$factor_phid] = $issued_challenges;
+        continue;
+      }
 
       foreach ($new_challenges as $new_challenge) {
         $issued_challenges[] = $new_challenge;
@@ -546,7 +559,10 @@ final class PhabricatorAuthSessionEngine extends Phobject {
         continue;
       }
 
+      if (!$result->getIsValid()) {
         $ok = false;
+      }
+
       $validation_results[$factor_phid] = $result;
     }
 

src/applications/auth/exception/PhabricatorAuthFactorResultException.php

@@ -0,0 +1,17 @@
+<?php
+
+final class PhabricatorAuthFactorResultException
+  extends Exception {
+
+  private $result;
+
+  public function __construct(PhabricatorAuthFactorResult $result) {
+    $this->result = $result;
+    parent::__construct();
+  }
+
+  public function getResult() {
+    return $this->result;
+  }
+
+}

src/applications/auth/factor/PhabricatorAuthFactor.php

@@ -232,6 +232,16 @@ abstract class PhabricatorAuthFactor extends Phobject {
   final protected function newAutomaticControl(
     PhabricatorAuthFactorResult $result) {
 
+    $is_error = $result->getIsError();
+    if ($is_error) {
+      return $this->newErrorControl($result);
+    }
+
+    $is_continue = $result->getIsContinue();
+    if ($is_continue) {
+      return $this->newContinueControl($result);
+    }
+
     $is_answered = (bool)$result->getAnsweredChallenge();
     if ($is_answered) {
       return $this->newAnsweredControl($result);
@@ -271,6 +281,34 @@ abstract class PhabricatorAuthFactor extends Phobject {
         pht('You responded to this challenge correctly.'));
   }
 
+  private function newErrorControl(
+    PhabricatorAuthFactorResult $result) {
+
+    $error = $result->getErrorMessage();
+
+    $icon = id(new PHUIIconView())
+      ->setIcon('fa-times', 'red');
+
+    return id(new PHUIFormTimerControl())
+      ->setIcon($icon)
+      ->appendChild($error)
+      ->setError(pht('Error'));
+  }
+
+  private function newContinueControl(
+    PhabricatorAuthFactorResult $result) {
+
+    $error = $result->getErrorMessage();
+
+    $icon = id(new PHUIIconView())
+      ->setIcon('fa-commenting', 'green');
+
+    return id(new PHUIFormTimerControl())
+      ->setIcon($icon)
+      ->appendChild($error);
+  }
+
+
 
 /* -(  Synchronizing New Factors  )------------------------------------------ */
 
@@ -400,4 +438,86 @@ abstract class PhabricatorAuthFactor extends Phobject {
     return null;
   }
 
+
+  /**
+   * @phutil-external-symbol class QRcode
+   */
+  final protected function newQRCode($uri) {
+    $root = dirname(phutil_get_library_root('phabricator'));
+    require_once $root.'/externals/phpqrcode/phpqrcode.php';
+
+    $lines = QRcode::text($uri);
+
+    $total_width = 240;
+    $cell_size = floor($total_width / count($lines));
+
+    $rows = array();
+    foreach ($lines as $line) {
+      $cells = array();
+      for ($ii = 0; $ii < strlen($line); $ii++) {
+        if ($line[$ii] == '1') {
+          $color = '#000';
+        } else {
+          $color = '#fff';
+        }
+
+        $cells[] = phutil_tag(
+          'td',
+          array(
+            'width' => $cell_size,
+            'height' => $cell_size,
+            'style' => 'background: '.$color,
+          ),
+          '');
+      }
+      $rows[] = phutil_tag('tr', array(), $cells);
+    }
+
+    return phutil_tag(
+      'table',
+      array(
+        'style' => 'margin: 24px auto;',
+      ),
+      $rows);
+  }
+
+  final protected function throwResult(PhabricatorAuthFactorResult $result) {
+    throw new PhabricatorAuthFactorResultException($result);
+  }
+
+  final protected function getInstallDisplayName() {
+    $uri = PhabricatorEnv::getURI('/');
+    $uri = new PhutilURI($uri);
+    return $uri->getDomain();
+  }
+
+  final protected function getChallengeResponseParameterName(
+    PhabricatorAuthFactorConfig $config) {
+    return $this->getParameterName($config, 'mfa.response');
+  }
+
+  final protected function getChallengeResponseFromRequest(
+    PhabricatorAuthFactorConfig $config,
+    AphrontRequest $request) {
+
+    $name = $this->getChallengeResponseParameterName($config);
+
+    $value = $request->getStr($name);
+    $value = (string)$value;
+    $value = trim($value);
+
+    return $value;
+  }
+
+  final protected function hasCSRF(PhabricatorAuthFactorConfig $config) {
+    $engine = $config->getSessionEngine();
+    $request = $engine->getRequest();
+
+    if (!$request->isHTTPPost()) {
+      return false;
+    }
+
+    return $request->validateCSRF();
+  }
+
 }

src/applications/auth/factor/PhabricatorAuthFactorResult.php

@@ -5,6 +5,8 @@ final class PhabricatorAuthFactorResult
 
   private $answeredChallenge;
   private $isWait = false;
+  private $isError = false;
+  private $isContinue = false;
   private $errorMessage;
   private $value;
   private $issuedChallenges = array();
@@ -44,6 +46,24 @@ final class PhabricatorAuthFactorResult
     return $this->isWait;
   }
 
+  public function setIsError($is_error) {
+    $this->isError = $is_error;
+    return $this;
+  }
+
+  public function getIsError() {
+    return $this->isError;
+  }
+
+  public function setIsContinue($is_continue) {
+    $this->isContinue = $is_continue;
+    return $this;
+  }
+
+  public function getIsContinue() {
+    return $this->isContinue;
+  }
+
   public function setErrorMessage($error_message) {
     $this->errorMessage = $error_message;
     return $this;

src/applications/auth/factor/PhabricatorSMSAuthFactor.php

@@ -171,6 +171,38 @@ final class PhabricatorSMSAuthFactor
       return array();
     }
 
+    if (!$this->loadUserContactNumber($viewer)) {
+      $result = $this->newResult()
+        ->setIsError(true)
+        ->setErrorMessage(
+          pht(
+            'Your account has no primary contact number.'));
+
+      $this->throwResult($result);
+    }
+
+    if (!$this->isSMSMailerConfigured()) {
+      $result = $this->newResult()
+        ->setIsError(true)
+        ->setErrorMessage(
+          pht(
+            'No outbound mailer which can deliver SMS messages is '.
+            'configured.'));
+
+      $this->throwResult($result);
+    }
+
+    if (!$this->hasCSRF($config)) {
+      $result = $this->newResult()
+        ->setIsContinue(true)
+        ->setErrorMessage(
+          pht(
+            'A text message with an authorization code will be sent to your '.
+            'primary contact number.'));
+
+      $this->throwResult($result);
+    }
+
     // Otherwise, issue a new challenge.
 
     $challenge_code = $this->newSMSChallengeCode();
@@ -329,10 +361,6 @@ final class PhabricatorSMSAuthFactor
   private function sendSMSCodeToUser(
     PhutilOpaqueEnvelope $envelope,
     PhabricatorUser $user) {
-
-    $uri = PhabricatorEnv::getURI('/');
-    $uri = new PhutilURI($uri);
-
     return id(new PhabricatorMetaMTAMail())
       ->setMessageType(PhabricatorMailSMSMessage::MESSAGETYPE)
       ->addTos(array($user->getPHID()))
@@ -341,7 +369,7 @@ final class PhabricatorSMSAuthFactor
       ->setBody(
         pht(
           'Phabricator (%s) MFA Code: %s',
-          $uri->getDomain(),
+          $this->getInstallDisplayName(),
           $envelope->openEnvelope()))
       ->save();
   }
@@ -350,22 +378,4 @@ final class PhabricatorSMSAuthFactor
     return trim($code);
   }
 
-  private function getChallengeResponseParameterName(
-    PhabricatorAuthFactorConfig $config) {
-    return $this->getParameterName($config, 'sms.code');
-  }
-
-  private function getChallengeResponseFromRequest(
-    PhabricatorAuthFactorConfig $config,
-    AphrontRequest $request) {
-
-    $name = $this->getChallengeResponseParameterName($config);
-
-    $value = $request->getStr($name);
-    $value = (string)$value;
-    $value = trim($value);
-
-    return $value;
-  }
-
 }

src/applications/auth/factor/PhabricatorTOTPAuthFactor.php

@@ -90,7 +90,7 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
       $secret,
       $issuer);
 
-    $qrcode = $this->renderQRCode($uri);
+    $qrcode = $this->newQRCode($uri);
     $form->appendChild($qrcode);
 
     $form->appendChild(
@@ -390,49 +390,6 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
     return $code;
   }
 
-
-  /**
-   * @phutil-external-symbol class QRcode
-   */
-  private function renderQRCode($uri) {
-    $root = dirname(phutil_get_library_root('phabricator'));
-    require_once $root.'/externals/phpqrcode/phpqrcode.php';
-
-    $lines = QRcode::text($uri);
-
-    $total_width = 240;
-    $cell_size = floor($total_width / count($lines));
-
-    $rows = array();
-    foreach ($lines as $line) {
-      $cells = array();
-      for ($ii = 0; $ii < strlen($line); $ii++) {
-        if ($line[$ii] == '1') {
-          $color = '#000';
-        } else {
-          $color = '#fff';
-        }
-
-        $cells[] = phutil_tag(
-          'td',
-          array(
-            'width' => $cell_size,
-            'height' => $cell_size,
-            'style' => 'background: '.$color,
-          ),
-          '');
-      }
-      $rows[] = phutil_tag('tr', array(), $cells);
-    }
-
-    return phutil_tag(
-      'table',
-      array(
-        'style' => 'margin: 24px auto;',
-      ),
-      $rows);
-  }
-
   private function getTimestepDuration() {
     return 30;
   }
@@ -470,24 +427,6 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor {
     return null;
   }
 
-  private function getChallengeResponseParameterName(
-    PhabricatorAuthFactorConfig $config) {
-    return $this->getParameterName($config, 'totpcode');
-  }
-
-  private function getChallengeResponseFromRequest(
-    PhabricatorAuthFactorConfig $config,
-    AphrontRequest $request) {
-
-    $name = $this->getChallengeResponseParameterName($config);
-
-    $value = $request->getStr($name);
-    $value = (string)$value;
-    $value = trim($value);
-
-    return $value;
-  }
-
   protected function newMFASyncTokenProperties(PhabricatorUser $user) {
     return array(
       'secret' => self::generateNewTOTPKey(),

src/applications/auth/future/PhabricatorDuoFuture.php

@@ -112,6 +112,11 @@ final class PhabricatorDuoFuture
         $this->secretKey->openEnvelope());
       $signature = new PhutilOpaqueEnvelope($signature);
 
+      if ($http_method === 'GET') {
+        $uri->setQueryParams($data);
+        $data = array();
+      }
+
       $future = id(new HTTPSFuture($uri, $data))
         ->setHTTPBasicAuthCredentials($this->integrationKey, $signature)
         ->setMethod($http_method)

src/applications/auth/storage/PhabricatorAuthChallenge.php

@@ -163,10 +163,16 @@ final class PhabricatorAuthChallenge
     $token = Filesystem::readRandomCharacters(32);
     $token = new PhutilOpaqueEnvelope($token);
 
-    return $this
+    $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+
+    $this
       ->setResponseToken($token)
       ->setResponseTTL($ttl)
       ->save();
+
+    unset($unguarded);
+
+    return $this;
   }
 
   public function markChallengeAsCompleted() {

src/applications/auth/storage/PhabricatorAuthFactorConfig.php

@@ -71,6 +71,15 @@ final class PhabricatorAuthFactorConfig
     return $this->mfaSyncToken;
   }
 
+  public function getAuthFactorConfigProperty($key, $default = null) {
+    return idx($this->properties, $key, $default);
+  }
+
+  public function setAuthFactorConfigProperty($key, $value) {
+    $this->properties[$key] = $value;
+    return $this;
+  }
+
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 

src/view/phui/PHUIInfoView.php

@@ -8,6 +8,7 @@ final class PHUIInfoView extends AphrontTagView {
   const SEVERITY_NODATA = 'nodata';
   const SEVERITY_SUCCESS = 'success';
   const SEVERITY_PLAIN = 'plain';
+  const SEVERITY_MFA = 'mfa';
 
   private $title;
   private $errors = array();
@@ -83,10 +84,12 @@ final class PHUIInfoView extends AphrontTagView {
       case self::SEVERITY_PLAIN:
       case self::SEVERITY_NODATA:
         return null;
-      break;
       case self::SEVERITY_SUCCESS:
         $icon = 'fa-check-circle';
         break;
+      case self::SEVERITY_MFA:
+        $icon = 'fa-lock';
+        break;
     }
 
     $icon = id(new PHUIIconView())

webroot/rsrc/css/phui/phui-form-view.css

@@ -574,3 +574,7 @@ properly, and submit values. */
   color: {$darkgreytext};
   vertical-align: middle;
 }
+
+.mfa-form-enroll-button {
+  text-align: center;
+}

webroot/rsrc/css/phui/phui-info-view.css

@@ -93,6 +93,15 @@ h1.phui-info-view-head {
   color: {$red};
 }
 
+.phui-info-severity-mfa {
+  border-color: {$blue};
+  border-left-width: 6px;
+}
+
+.phui-info-severity-mfa .phui-info-icon {
+  color: {$blue};
+}
+
 .phui-info-severity-warning {
   border-color: {$yellow};
   border-left-width: 6px;