Upgrade sessions digests to HMAC256, retaining compatibility with old digests

Summary:
Ref T13222. Ref T13225. We store a digest of the session key in the session table (not the session key itself) so that users with access to this table can't easily steal sessions by just setting their cookies to values from the table.

Users with access to the database can //probably// do plenty of other bad stuff (e.g., T13134 mentions digesting Conduit tokens) but there's very little cost to storing digests instead of live tokens.

We currently digest session keys with HMAC-SHA1. This is fine, but HMAC-SHA256 is better. Upgrade:

  - Always write new digests.
  - We still match sessions with either digest.
  - When we read a session with an old digest, upgrade it to a new digest.

In a few months we can throw away the old code. When we do, installs that skip upgrades for a long time may suffer a one-time logout, but I'll note this in the changelog.

We could avoid this by storing `hmac256(hmac1(key))` instead and re-hashing in a migration, but I think the cost of a one-time logout for some tiny subset of users is very low, and worth keeping things simpler in the long run.

Test Plan:
  - Hit a page with an old session, got a session upgrade.
  - Reviewed sessions in Settings.
  - Reviewed user logs.
  - Logged out.
  - Logged in.
  - Terminated other sessions individually.
  - Terminated all other sessions.
  - Spot checked session table for general sanity.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13225, T13222

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

resources/sql/autopatches/20181213.auth.04.longerhashes.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_user.phabricator_session
+  CHANGE sessionKey sessionKey VARBINARY(64) NOT NULL;

resources/sql/autopatches/20181213.auth.05.longerloghashes.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_user.user_log
+  CHANGE session session VARBINARY(64);

resources/sql/patches/20130530.sessionhash.php

@@ -1,22 +1,7 @@
 <?php
 
-$table = new PhabricatorUser();
-$table->openTransaction();
-$conn = $table->establishConnection('w');
-
-$sessions = queryfx_all(
-  $conn,
-  'SELECT userPHID, type, sessionKey FROM %T FOR UPDATE',
-  PhabricatorUser::SESSION_TABLE);
-
-foreach ($sessions as $session) {
-  queryfx(
-    $conn,
-    'UPDATE %T SET sessionKey = %s WHERE userPHID = %s AND type = %s',
-    PhabricatorUser::SESSION_TABLE,
-    PhabricatorHash::weakDigest($session['sessionKey']),
-    $session['userPHID'],
-    $session['type']);
-}
-
-$table->saveTransaction();
+// See T13225. Long ago, this upgraded session key storage from unhashed to
+// HMAC-SHA1 here. We later upgraded storage to HMAC-SHA256, so this is initial
+// upgrade is now fairly pointless. Dropping this migration entirely only logs
+// users out of installs that waited more than 5 years to upgrade, which seems
+// like a reasonable behavior.

src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php

@@ -16,8 +16,9 @@ final class PhabricatorAuthTerminateSessionController
       $query->withIDs(array($id));
     }
 
-    $current_key = PhabricatorHash::weakDigest(
-      $request->getCookie(PhabricatorCookies::COOKIE_SESSION));
+    $current_key = PhabricatorAuthSession::newSessionDigest(
+      new PhutilOpaqueEnvelope(
+        $request->getCookie(PhabricatorCookies::COOKIE_SESSION)));
 
     $sessions = $query->execute();
     foreach ($sessions as $key => $session) {

src/applications/auth/controller/PhabricatorAuthUnlinkController.php

@@ -56,7 +56,8 @@ final class PhabricatorAuthUnlinkController
 
       id(new PhabricatorAuthSessionEngine())->terminateLoginSessions(
         $viewer,
-        $request->getCookie(PhabricatorCookies::COOKIE_SESSION));
+        new PhutilOpaqueEnvelope(
+          $request->getCookie(PhabricatorCookies::COOKIE_SESSION)));
 
       return id(new AphrontRedirectResponse())->setURI($this->getDoneURI());
     }

src/applications/auth/engine/PhabricatorAuthSessionEngine.php

@@ -109,14 +109,19 @@ final class PhabricatorAuthSessionEngine extends Phobject {
 
     $session_table = new PhabricatorAuthSession();
     $user_table = new PhabricatorUser();
-    $conn_r = $session_table->establishConnection('r');
-    $session_key = PhabricatorHash::weakDigest($session_token);
+    $conn = $session_table->establishConnection('r');
 
-    $cache_parts = $this->getUserCacheQueryParts($conn_r);
+    // TODO: See T13225. We're moving sessions to a more modern digest
+    // algorithm, but still accept older cookies for compatibility.
+    $session_key = PhabricatorAuthSession::newSessionDigest(
+      new PhutilOpaqueEnvelope($session_token));
+    $weak_key = PhabricatorHash::weakDigest($session_token);
+
+    $cache_parts = $this->getUserCacheQueryParts($conn);
     list($cache_selects, $cache_joins, $cache_map, $types_map) = $cache_parts;
 
     $info = queryfx_one(
-      $conn_r,
+      $conn,
       'SELECT
           s.id AS s_id,
           s.phid AS s_phid,
@@ -125,21 +130,28 @@ final class PhabricatorAuthSessionEngine extends Phobject {
           s.highSecurityUntil AS s_highSecurityUntil,
           s.isPartial AS s_isPartial,
           s.signedLegalpadDocuments as s_signedLegalpadDocuments,
+          IF(s.sessionKey = %P, 1, 0) as s_weak,
           u.*
           %Q
-        FROM %T u JOIN %T s ON u.phid = s.userPHID
-        AND s.type = %s AND s.sessionKey = %P %Q',
+        FROM %R u JOIN %R s ON u.phid = s.userPHID
+        AND s.type = %s AND s.sessionKey IN (%P, %P) %Q',
+      new PhutilOpaqueEnvelope($weak_key),
       $cache_selects,
-      $user_table->getTableName(),
-      $session_table->getTableName(),
+      $user_table,
+      $session_table,
       $session_type,
       new PhutilOpaqueEnvelope($session_key),
+      new PhutilOpaqueEnvelope($weak_key),
       $cache_joins);
 
     if (!$info) {
       return null;
     }
 
+    // TODO: Remove this, see T13225.
+    $is_weak = (bool)$info['s_weak'];
+    unset($info['s_weak']);
+
     $session_dict = array(
       'userPHID' => $info['phid'],
       'sessionKey' => $session_key,
@@ -202,6 +214,19 @@ final class PhabricatorAuthSessionEngine extends Phobject {
       unset($unguarded);
     }
 
+    // TODO: Remove this, see T13225.
+    if ($is_weak) {
+      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+        $conn_w = $session_table->establishConnection('w');
+        queryfx(
+          $conn_w,
+          'UPDATE %T SET sessionKey = %P WHERE id = %d',
+          $session->getTableName(),
+          new PhutilOpaqueEnvelope($session_key),
+          $session->getID());
+      unset($unguarded);
+    }
+
     $user->attachSession($session);
     return $user;
   }
@@ -241,7 +266,8 @@ final class PhabricatorAuthSessionEngine extends Phobject {
     // This has a side effect of validating the session type.
     $session_ttl = PhabricatorAuthSession::getSessionTypeTTL($session_type);
 
-    $digest_key = PhabricatorHash::weakDigest($session_key);
+    $digest_key = PhabricatorAuthSession::newSessionDigest(
+      new PhutilOpaqueEnvelope($session_key));
 
     // Logging-in users don't have CSRF stuff yet, so we have to unguard this
     // write.
@@ -299,7 +325,7 @@ final class PhabricatorAuthSessionEngine extends Phobject {
    */
   public function terminateLoginSessions(
     PhabricatorUser $user,
-    $except_session = null) {
+    PhutilOpaqueEnvelope $except_session = null) {
 
     $sessions = id(new PhabricatorAuthSessionQuery())
       ->setViewer($user)
@@ -307,7 +333,8 @@ final class PhabricatorAuthSessionEngine extends Phobject {
       ->execute();
 
     if ($except_session !== null) {
-      $except_session = PhabricatorHash::weakDigest($except_session);
+      $except_session = PhabricatorAuthSession::newSessionDigest(
+        $except_session);
     }
 
     foreach ($sessions as $key => $session) {

src/applications/auth/query/PhabricatorAuthSessionQuery.php

@@ -91,7 +91,8 @@ final class PhabricatorAuthSessionQuery
     if ($this->sessionKeys !== null) {
       $hashes = array();
       foreach ($this->sessionKeys as $session_key) {
-        $hashes[] = PhabricatorHash::weakDigest($session_key);
+        $hashes[] = PhabricatorAuthSession::newSessionDigest(
+          new PhutilOpaqueEnvelope($session_key));
       }
       $where[] = qsprintf(
         $conn,

src/applications/auth/storage/PhabricatorAuthSession.php

@@ -6,6 +6,8 @@ final class PhabricatorAuthSession extends PhabricatorAuthDAO
   const TYPE_WEB      = 'web';
   const TYPE_CONDUIT  = 'conduit';
 
+  const SESSION_DIGEST_KEY = 'session.digest';
+
   protected $userPHID;
   protected $type;
   protected $sessionKey;
@@ -17,13 +19,19 @@ final class PhabricatorAuthSession extends PhabricatorAuthDAO
 
   private $identityObject = self::ATTACHABLE;
 
+  public static function newSessionDigest(PhutilOpaqueEnvelope $session_token) {
+    return PhabricatorHash::digestWithNamedKey(
+      $session_token->openEnvelope(),
+      self::SESSION_DIGEST_KEY);
+  }
+
   protected function getConfiguration() {
     return array(
       self::CONFIG_TIMESTAMPS => false,
       self::CONFIG_AUX_PHID => true,
       self::CONFIG_COLUMN_SCHEMA => array(
         'type' => 'text32',
-        'sessionKey' => 'bytes40',
+        'sessionKey' => 'text64',
         'sessionStart' => 'epoch',
         'sessionExpires' => 'epoch',
         'highSecurityUntil' => 'epoch?',

src/applications/people/storage/PhabricatorUserLog.php

@@ -150,7 +150,7 @@ final class PhabricatorUserLog extends PhabricatorUserDAO
         'actorPHID' => 'phid?',
         'action' => 'text64',
         'remoteAddr' => 'text64',
-        'session' => 'bytes40?',
+        'session' => 'text64?',
       ),
       self::CONFIG_KEY_SCHEMA => array(
         'actorPHID' => array(

src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php

@@ -193,7 +193,8 @@ final class PhabricatorMultiFactorSettingsPanel
 
         id(new PhabricatorAuthSessionEngine())->terminateLoginSessions(
           $user,
-          $request->getCookie(PhabricatorCookies::COOKIE_SESSION));
+          new PhutilOpaqueEnvelope(
+            $request->getCookie(PhabricatorCookies::COOKIE_SESSION)));
 
         return id(new AphrontRedirectResponse())
           ->setURI($this->getPanelURI('?id='.$config->getID()));

src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php

@@ -121,7 +121,8 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel {
 
         id(new PhabricatorAuthSessionEngine())->terminateLoginSessions(
           $user,
-          $request->getCookie(PhabricatorCookies::COOKIE_SESSION));
+          new PhutilOpaqueEnvelope(
+            $request->getCookie(PhabricatorCookies::COOKIE_SESSION)));
 
         return id(new AphrontRedirectResponse())->setURI($next);
       }

src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php

@@ -44,8 +44,9 @@ final class PhabricatorSessionsSettingsPanel extends PhabricatorSettingsPanel {
       ->withPHIDs($identity_phids)
       ->execute();
 
-    $current_key = PhabricatorHash::weakDigest(
-      $request->getCookie(PhabricatorCookies::COOKIE_SESSION));
+    $current_key = PhabricatorAuthSession::newSessionDigest(
+      new PhutilOpaqueEnvelope(
+        $request->getCookie(PhabricatorCookies::COOKIE_SESSION)));
 
     $rows = array();
     $rowc = array();

src/infrastructure/util/PhabricatorHash.php

@@ -187,6 +187,16 @@ final class PhabricatorHash extends Phobject {
   }
 
   public static function digestHMACSHA256($message, $key) {
+    if (!is_string($message)) {
+      throw new Exception(
+        pht('HMAC-SHA256 can only digest strings.'));
+    }
+
+    if (!is_string($key)) {
+      throw new Exception(
+        pht('HMAC-SHA256 keys must be strings.'));
+    }
+
     if (!strlen($key)) {
       throw new Exception(
         pht('HMAC-SHA256 requires a nonempty key.'));
@@ -194,7 +204,9 @@ final class PhabricatorHash extends Phobject {
 
     $result = hash_hmac('sha256', $message, $key, $raw_output = false);
 
-    if ($result === false) {
+    // Although "hash_hmac()" is documented as returning `false` when it fails,
+    // it can also return `null` if you pass an object as the "$message".
+    if ($result === false || $result === null) {
       throw new Exception(
         pht('Unable to compute HMAC-SHA256 digest of message.'));
     }