Remove "phabricator.csrf-key" and upgrade CSRF hashing to SHA256

Summary:
Ref T12509.

  - Remove the "phabricator.csrf-key" configuration option in favor of automatically generating an HMAC key.
  - Upgrade two hasher callsites (one in CSRF itself, one in providing a CSRF secret for logged-out users) to SHA256.
  - Extract the CSRF logic from `PhabricatorUser` to a standalone engine.

I was originally going to do this as two changes (extract logic, then upgrade hashes) but the logic had a couple of very silly pieces to it that made faithful extraction a little silly.

For example, it computed `time_block = (epoch + (offset * cycle_frequency)) / cycle_frequency` instead of `time_block = (epoch / cycle_frequency) + offset`. These are equivalent but the former was kind of silly.

It also computed `substr(hmac(substr(hmac(secret)).salt))` instead of `substr(hmac(secret.salt))`. These have the same overall effect but the former is, again, kind of silly (and a little bit materially worse, in this case).

This will cause a one-time compatibility break: pages loaded before the upgrade won't be able to submit contained forms after the upgrade, unless they're open for long enough for the Javascript to refresh the CSRF token (an hour, I think?). I'll note this in the changelog.

Test Plan:
  - As a logged-in user, submitted forms normally (worked).
  - As a logged-in user, submitted forms with a bad CSRF value (error, as expected).
  - As a logged-out user, hit the success and error cases.
  - Visually inspected tokens for correct format.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12509

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

src/__phutil_library_map__.php

@@ -2190,6 +2190,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthApplication' => 'applications/auth/application/PhabricatorAuthApplication.php',
     'PhabricatorAuthAuthFactorPHIDType' => 'applications/auth/phid/PhabricatorAuthAuthFactorPHIDType.php',
     'PhabricatorAuthAuthProviderPHIDType' => 'applications/auth/phid/PhabricatorAuthAuthProviderPHIDType.php',
+    'PhabricatorAuthCSRFEngine' => 'applications/auth/engine/PhabricatorAuthCSRFEngine.php',
     'PhabricatorAuthChallenge' => 'applications/auth/storage/PhabricatorAuthChallenge.php',
     'PhabricatorAuthChallengeGarbageCollector' => 'applications/auth/garbagecollector/PhabricatorAuthChallengeGarbageCollector.php',
     'PhabricatorAuthChallengePHIDType' => 'applications/auth/phid/PhabricatorAuthChallengePHIDType.php',
@@ -7828,6 +7829,7 @@ phutil_register_library_map(array(
     'PhabricatorAuthApplication' => 'PhabricatorApplication',
     'PhabricatorAuthAuthFactorPHIDType' => 'PhabricatorPHIDType',
     'PhabricatorAuthAuthProviderPHIDType' => 'PhabricatorPHIDType',
+    'PhabricatorAuthCSRFEngine' => 'Phobject',
     'PhabricatorAuthChallenge' => array(
       'PhabricatorAuthDAO',
       'PhabricatorPolicyInterface',

src/applications/auth/engine/PhabricatorAuthCSRFEngine.php

@@ -0,0 +1,119 @@
+<?php
+
+final class PhabricatorAuthCSRFEngine extends Phobject {
+
+  private $salt;
+  private $secret;
+
+  public function setSalt($salt) {
+    $this->salt = $salt;
+    return $this;
+  }
+
+  public function getSalt() {
+    return $this->salt;
+  }
+
+  public function setSecret(PhutilOpaqueEnvelope $secret) {
+    $this->secret = $secret;
+    return $this;
+  }
+
+  public function getSecret() {
+    return $this->secret;
+  }
+
+  public function newSalt() {
+    $salt_length = $this->getSaltLength();
+    return Filesystem::readRandomCharacters($salt_length);
+  }
+
+  public function newToken() {
+    $salt = $this->getSalt();
+
+    if (!$salt) {
+      throw new PhutilInvalidStateException('setSalt');
+    }
+
+    $token = $this->newRawToken($salt);
+    $prefix = $this->getBREACHPrefix();
+
+    return sprintf('%s%s%s', $prefix, $salt, $token);
+  }
+
+  public function isValidToken($token) {
+    $salt_length = $this->getSaltLength();
+
+    // We expect a BREACH-mitigating token. See T3684.
+    $breach_prefix = $this->getBREACHPrefix();
+    $breach_prelen = strlen($breach_prefix);
+    if (strncmp($token, $breach_prefix, $breach_prelen) !== 0) {
+      return false;
+    }
+
+    $salt = substr($token, $breach_prelen, $salt_length);
+    $token = substr($token, $breach_prelen + $salt_length);
+
+    foreach ($this->getWindowOffsets() as $offset) {
+      $expect_token = $this->newRawToken($salt, $offset);
+      if (phutil_hashes_are_identical($expect_token, $token)) {
+        return true;
+      }
+    }
+
+    return false;
+  }
+
+  private function newRawToken($salt, $offset = 0) {
+    $now = PhabricatorTime::getNow();
+    $cycle_frequency = $this->getCycleFrequency();
+
+    $time_block = (int)floor($now / $cycle_frequency);
+    $time_block = $time_block + $offset;
+
+    $secret = $this->getSecret();
+    if (!$secret) {
+      throw new PhutilInvalidStateException('setSecret');
+    }
+    $secret = $secret->openEnvelope();
+
+    $hash = PhabricatorHash::digestWithNamedKey(
+      $secret.$time_block.$salt,
+      'csrf');
+
+    return substr($hash, 0, $this->getTokenLength());
+  }
+
+  private function getBREACHPrefix() {
+    return 'B@';
+  }
+
+  private function getSaltLength() {
+    return 8;
+  }
+
+  private function getTokenLength() {
+    return 16;
+  }
+
+  private function getCycleFrequency() {
+    return phutil_units('1 hour in seconds');
+  }
+
+  private function getWindowOffsets() {
+    // We accept some tokens from the recent past and near future. Users may
+    // have older tokens if they close their laptop and open it up again
+    // later. Users may have newer tokens if there are multiple web hosts with
+    // a bit of clock skew.
+
+    // Javascript on the client tries to keep CSRF tokens up to date, but
+    // it may fail, and it doesn't run if the user closes their laptop.
+
+    // The window during which our tokens remain valid is generally more
+    // conservative than other platforms. For example, Rails uses "session
+    // duration" and Django uses "forever".
+
+    return range(-6, 1);
+  }
+
+}

src/applications/base/controller/PhabricatorController.php

@@ -98,7 +98,8 @@ abstract class PhabricatorController extends AphrontController {
 
 
       if (!$user->isLoggedIn()) {
-        $user->attachAlternateCSRFString(PhabricatorHash::weakDigest($phsid));
+        $csrf = PhabricatorHash::digestWithNamedKey($phsid, 'csrf.alternate');
+        $user->attachAlternateCSRFString($csrf);
       }
 
       $request->setUser($user);

src/applications/config/check/PhabricatorExtraConfigSetupCheck.php

@@ -388,6 +388,9 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck {
 
       'metamta.mail-key' => pht(
         'Mail object address hash keys are now generated automatically.'),
+
+      'phabricator.csrf-key' => pht(
+        'CSRF HMAC keys are now managed automatically.'),
     );
 
     return $ancient_config;

src/applications/config/option/PhabricatorSecurityConfigOptions.php

@@ -154,21 +154,6 @@ EOTEXT
             pht('Multi-Factor Required'),
             pht('Multi-Factor Optional'),
           )),
-      $this->newOption(
-        'phabricator.csrf-key',
-        'string',
-        '0b7ec0592e0a2829d8b71df2fa269b2c6172eca3')
-        ->setHidden(true)
-        ->setSummary(
-          pht('Hashed with other inputs to generate CSRF tokens.'))
-        ->setDescription(
-          pht(
-            'This is hashed with other inputs to generate CSRF tokens. If '.
-            'you want, you can change it to some other string which is '.
-            'unique to your install. This will make your install more secure '.
-            'in a vague, mostly theoretical way. But it will take you like 3 '.
-            'seconds of mashing on your keyboard to set it up so you might '.
-            'as well.')),
        $this->newOption(
         'uri.allowed-protocols',
         'set',

src/applications/people/storage/PhabricatorUser.php

@@ -318,112 +318,9 @@ final class PhabricatorUser
     return Filesystem::readRandomCharacters(255);
   }
 
-  const CSRF_CYCLE_FREQUENCY  = 3600;
-  const CSRF_SALT_LENGTH      = 8;
-  const CSRF_TOKEN_LENGTH     = 16;
-  const CSRF_BREACH_PREFIX    = 'B@';
-
   const EMAIL_CYCLE_FREQUENCY = 86400;
   const EMAIL_TOKEN_LENGTH    = 24;
 
-  private function getRawCSRFToken($offset = 0) {
-    return $this->generateToken(
-      time() + (self::CSRF_CYCLE_FREQUENCY * $offset),
-      self::CSRF_CYCLE_FREQUENCY,
-      PhabricatorEnv::getEnvConfig('phabricator.csrf-key'),
-      self::CSRF_TOKEN_LENGTH);
-  }
-
-  public function getCSRFToken() {
-    if ($this->isOmnipotent()) {
-      // We may end up here when called from the daemons. The omnipotent user
-      // has no meaningful CSRF token, so just return `null`.
-      return null;
-    }
-
-    if ($this->csrfSalt === null) {
-      $this->csrfSalt = Filesystem::readRandomCharacters(
-        self::CSRF_SALT_LENGTH);
-    }
-
-    $salt = $this->csrfSalt;
-
-    // Generate a token hash to mitigate BREACH attacks against SSL. See
-    // discussion in T3684.
-    $token = $this->getRawCSRFToken();
-    $hash = PhabricatorHash::weakDigest($token, $salt);
-    return self::CSRF_BREACH_PREFIX.$salt.substr(
-        $hash, 0, self::CSRF_TOKEN_LENGTH);
-  }
-
-  public function validateCSRFToken($token) {
-    // We expect a BREACH-mitigating token. See T3684.
-    $breach_prefix = self::CSRF_BREACH_PREFIX;
-    $breach_prelen = strlen($breach_prefix);
-    if (strncmp($token, $breach_prefix, $breach_prelen) !== 0) {
-      return false;
-    }
-
-    $salt = substr($token, $breach_prelen, self::CSRF_SALT_LENGTH);
-    $token = substr($token, $breach_prelen + self::CSRF_SALT_LENGTH);
-
-    // When the user posts a form, we check that it contains a valid CSRF token.
-    // Tokens cycle each hour (every CSRF_CYCLE_FREQUENCY seconds) and we accept
-    // either the current token, the next token (users can submit a "future"
-    // token if you have two web frontends that have some clock skew) or any of
-    // the last 6 tokens. This means that pages are valid for up to 7 hours.
-    // There is also some Javascript which periodically refreshes the CSRF
-    // tokens on each page, so theoretically pages should be valid indefinitely.
-    // However, this code may fail to run (if the user loses their internet
-    // connection, or there's a JS problem, or they don't have JS enabled).
-    // Choosing the size of the window in which we accept old CSRF tokens is
-    // an issue of balancing concerns between security and usability. We could
-    // choose a very narrow (e.g., 1-hour) window to reduce vulnerability to
-    // attacks using captured CSRF tokens, but it's also more likely that real
-    // users will be affected by this, e.g. if they close their laptop for an
-    // hour, open it back up, and try to submit a form before the CSRF refresh
-    // can kick in. Since the user experience of submitting a form with expired
-    // CSRF is often quite bad (you basically lose data, or it's a big pain to
-    // recover at least) and I believe we gain little additional protection
-    // by keeping the window very short (the overwhelming value here is in
-    // preventing blind attacks, and most attacks which can capture CSRF tokens
-    // can also just capture authentication information [sniffing networks]
-    // or act as the user [xss]) the 7 hour default seems like a reasonable
-    // balance. Other major platforms have much longer CSRF token lifetimes,
-    // like Rails (session duration) and Django (forever), which suggests this
-    // is a reasonable analysis.
-    $csrf_window = 6;
-
-    for ($ii = -$csrf_window; $ii <= 1; $ii++) {
-      $valid = $this->getRawCSRFToken($ii);
-
-      $digest = PhabricatorHash::weakDigest($valid, $salt);
-      $digest = substr($digest, 0, self::CSRF_TOKEN_LENGTH);
-      if (phutil_hashes_are_identical($digest, $token)) {
-        return true;
-      }
-    }
-
-    return false;
-  }
-
-  private function generateToken($epoch, $frequency, $key, $len) {
-    if ($this->getPHID()) {
-      $vec = $this->getPHID().$this->getAccountSecret();
-    } else {
-      $vec = $this->getAlternateCSRFString();
-    }
-
-    if ($this->hasSession()) {
-      $vec = $vec.$this->getSession()->getSessionKey();
-    }
-
-    $time_block = floor($epoch / $frequency);
-    $vec = $vec.$key.$time_block;
-
-    return substr(PhabricatorHash::weakDigest($vec), 0, $len);
-  }
-
   public function getUserProfile() {
     return $this->assertAttached($this->profile);
   }
@@ -623,15 +520,6 @@ final class PhabricatorUser
     return (string)$uri;
   }
 
-  public function getAlternateCSRFString() {
-    return $this->assertAttached($this->alternateCSRFString);
-  }
-
-  public function attachAlternateCSRFString($string) {
-    $this->alternateCSRFString = $string;
-    return $this;
-  }
-
   /**
    * Populate the nametoken table, which used to fetch typeahead results. When
    * a user types "linc", we want to match "Abraham Lincoln" from on-demand
@@ -1216,6 +1104,58 @@ final class PhabricatorUser
     return $this->assertAttached($this->badgePHIDs);
   }
 
+/* -(  CSRF  )--------------------------------------------------------------- */
+
+
+  public function getCSRFToken() {
+    if ($this->isOmnipotent()) {
+      // We may end up here when called from the daemons. The omnipotent user
+      // has no meaningful CSRF token, so just return `null`.
+      return null;
+    }
+
+    return $this->newCSRFEngine()
+      ->newToken();
+  }
+
+  public function validateCSRFToken($token) {
+    return $this->newCSRFengine()
+      ->isValidToken($token);
+  }
+
+  public function getAlternateCSRFString() {
+    return $this->assertAttached($this->alternateCSRFString);
+  }
+
+  public function attachAlternateCSRFString($string) {
+    $this->alternateCSRFString = $string;
+    return $this;
+  }
+
+  private function newCSRFEngine() {
+    if ($this->getPHID()) {
+      $vec = $this->getPHID().$this->getAccountSecret();
+    } else {
+      $vec = $this->getAlternateCSRFString();
+    }
+
+    if ($this->hasSession()) {
+      $vec = $vec.$this->getSession()->getSessionKey();
+    }
+
+    $engine = new PhabricatorAuthCSRFEngine();
+
+    if ($this->csrfSalt === null) {
+      $this->csrfSalt = $engine->newSalt();
+    }
+
+    $engine
+      ->setSalt($this->csrfSalt)
+      ->setSecret(new PhutilOpaqueEnvelope($vec));
+
+    return $engine;
+  }
+
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */