From 7298589c86ec654f6e876e7d9259b5b73425016c Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 7 Aug 2013 16:09:05 -0700
Subject: [PATCH] Proof of concept mitigation of BREACH

Summary: Ref T3684 for discussion. This could be cleaned up a bit (it would be nice to draw entropy once per request, for instance, and maybe respect CSRF_TOKEN_LENGTH more closely) but should effectively mitigate BREACH.

Test Plan: Submitted forms; submitted forms after mucking with CSRF and observed CSRF error. Verified that source now has "B@..." tokens.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3684

Differential Revision: https://secure.phabricator.com/D6686
---
 .../people/storage/PhabricatorUser.php        | 56 +++++++++++++++++--
 src/infrastructure/util/PhabricatorHash.php   |  7 ++-
 support/PhabricatorStartup.php                |  2 +
 3 files changed, 58 insertions(+), 7 deletions(-)

diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php
index b3e8aece53..ab9f4d332f 100644
--- a/src/applications/people/storage/PhabricatorUser.php
+++ b/src/applications/people/storage/PhabricatorUser.php
@@ -152,12 +152,14 @@ final class PhabricatorUser
   }
 
   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;
 
-  public function getCSRFToken($offset = 0) {
+  private function getRawCSRFToken($offset = 0) {
     return $this->generateToken(
       time() + (self::CSRF_CYCLE_FREQUENCY * $offset),
       self::CSRF_CYCLE_FREQUENCY,
@@ -165,12 +167,42 @@ final class PhabricatorUser
       self::CSRF_TOKEN_LENGTH);
   }
 
-  public function validateCSRFToken($token) {
+  /**
+   * @phutil-external-symbol class PhabricatorStartup
+   */
+  public function getCSRFToken() {
+    $salt = PhabricatorStartup::getGlobal('csrf.salt');
+    if (!$salt) {
+      $salt = Filesystem::readRandomCharacters(self::CSRF_SALT_LENGTH);
+      PhabricatorStartup::setGlobal('csrf.salt', $salt);
+    }
 
+    // Generate a token hash to mitigate BREACH attacks against SSL. See
+    // discussion in T3684.
+    $token = $this->getRawCSRFToken();
+    $hash = PhabricatorHash::digest($token, $salt);
+    return 'B@'.$salt.substr($hash, 0, self::CSRF_TOKEN_LENGTH);
+  }
+
+  public function validateCSRFToken($token) {
     if (!$this->getPHID()) {
       return true;
     }
 
+    $salt = null;
+
+    $version = 'plain';
+
+    // This is a BREACH-mitigating token. See T3684.
+    $breach_prefix = self::CSRF_BREACH_PREFIX;
+    $breach_prelen = strlen($breach_prefix);
+
+    if (!strncmp($token, $breach_prefix, $breach_prelen)) {
+      $version = 'breach';
+      $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_CYLCE_FREQUENCY seconds) and we accept
     // either the current token, the next token (users can submit a "future"
@@ -199,9 +231,23 @@ final class PhabricatorUser
     $csrf_window = 6;
 
     for ($ii = -$csrf_window; $ii <= 1; $ii++) {
-      $valid = $this->getCSRFToken($ii);
-      if ($token == $valid) {
-        return true;
+      $valid = $this->getRawCSRFToken($ii);
+      switch ($version) {
+        // TODO: We can remove this after the BREACH version has been in the
+        // wild for a while.
+        case 'plain':
+          if ($token == $valid) {
+            return true;
+          }
+          break;
+        case 'breach':
+          $digest = PhabricatorHash::digest($valid, $salt);
+          if (substr($digest, 0, self::CSRF_TOKEN_LENGTH) == $token) {
+            return true;
+          }
+          break;
+        default:
+          throw new Exception("Unknown CSRF token format!");
       }
     }
 
diff --git a/src/infrastructure/util/PhabricatorHash.php b/src/infrastructure/util/PhabricatorHash.php
index 7167f5e792..dc86813b56 100644
--- a/src/infrastructure/util/PhabricatorHash.php
+++ b/src/infrastructure/util/PhabricatorHash.php
@@ -9,8 +9,11 @@ final class PhabricatorHash {
    * @param   string  Input string.
    * @return  string  32-byte hexidecimal SHA1+HMAC hash.
    */
-  public static function digest($string) {
-    $key = PhabricatorEnv::getEnvConfig('security.hmac-key');
+  public static function digest($string, $key = null) {
+    if ($key === null) {
+      $key = PhabricatorEnv::getEnvConfig('security.hmac-key');
+    }
+
     if (!$key) {
       throw new Exception(
         "Set a 'security.hmac-key' in your Phabricator configuration!");
diff --git a/support/PhabricatorStartup.php b/support/PhabricatorStartup.php
index d02d565ab2..79b2131def 100644
--- a/support/PhabricatorStartup.php
+++ b/support/PhabricatorStartup.php
@@ -59,6 +59,7 @@ final class PhabricatorStartup {
     if (!array_key_exists($key, self::$globals)) {
       return $default;
     }
+
     return self::$globals[$key];
   }
 
@@ -370,6 +371,7 @@ final class PhabricatorStartup {
   private static function validateGlobal($key) {
     static $globals = array(
       'log.access' => true,
+      'csrf.salt'  => true,
     );
 
     if (empty($globals[$key])) {