From 32f91557f8985c3532e0e40ecd9e25c12b2517d2 Mon Sep 17 00:00:00 2001
From: Jakub Vrana <jakub@vrana.cz>
Date: Thu, 30 May 2013 17:30:06 -0700
Subject: [PATCH] Store hash of session key

Summary:
This prevents security by obscurity.
If I have read-only access to the database then I can pretend to be any logged-in user.

I've used `PhabricatorHash::digest()` (even though we don't need salt as the hashed string is random) to be compatible with user log.

Test Plan:
Applied patch.
Verified I'm still logged in.
Logged out.
Logged in.

  $ arc tasks

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D6080
---
 .../sql/patches/20130530.sessionhash.php      | 22 +++++++++++++++++++
 .../base/controller/PhabricatorController.php |  2 +-
 .../PhabricatorConduitAPIController.php       |  2 +-
 .../people/storage/PhabricatorUser.php        |  8 +++----
 .../patch/PhabricatorBuiltinPatchList.php     |  4 ++++
 5 files changed, 32 insertions(+), 6 deletions(-)
 create mode 100644 resources/sql/patches/20130530.sessionhash.php

diff --git a/resources/sql/patches/20130530.sessionhash.php b/resources/sql/patches/20130530.sessionhash.php
new file mode 100644
index 0000000000..4efbe5feec
--- /dev/null
+++ b/resources/sql/patches/20130530.sessionhash.php
@@ -0,0 +1,22 @@
+<?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::digest($session['sessionKey']),
+    $session['userPHID'],
+    $session['type']);
+}
+
+$table->saveTransaction();
diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php
index 70073813d1..6b40fbd90d 100644
--- a/src/applications/base/controller/PhabricatorController.php
+++ b/src/applications/base/controller/PhabricatorController.php
@@ -52,7 +52,7 @@ abstract class PhabricatorController extends AphrontController {
         $user->getTableName(),
         'phabricator_session',
         'web-',
-        $phsid);
+        PhabricatorHash::digest($phsid));
       if ($info) {
         $user->loadFromArray($info);
       }
diff --git a/src/applications/conduit/controller/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/PhabricatorConduitAPIController.php
index 83908d73d5..290a356627 100644
--- a/src/applications/conduit/controller/PhabricatorConduitAPIController.php
+++ b/src/applications/conduit/controller/PhabricatorConduitAPIController.php
@@ -283,7 +283,7 @@ final class PhabricatorConduitAPIController
       id(new PhabricatorUser())->establishConnection('r'),
       'SELECT * FROM %T WHERE sessionKey = %s',
       PhabricatorUser::SESSION_TABLE,
-      $session_key);
+      PhabricatorHash::digest($session_key));
     if (!$session) {
       return array(
         'ERR-INVALID-SESSION',
diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php
index c34f246859..b69c3211fe 100644
--- a/src/applications/people/storage/PhabricatorUser.php
+++ b/src/applications/people/storage/PhabricatorUser.php
@@ -290,7 +290,7 @@ final class PhabricatorUser extends PhabricatorUserDAO implements PhutilPerson {
         $try_type = $session_type.'-'.$ii;
         if (!in_array($try_type, $existing_sessions)) {
           $establish_type = $try_type;
-          $expect_key = $session_key;
+          $expect_key = PhabricatorHash::digest($session_key);
           $existing_sessions[] = $try_type;
 
           // Ensure the row exists so we can issue an update below. We don't
@@ -302,7 +302,7 @@ final class PhabricatorUser extends PhabricatorUserDAO implements PhutilPerson {
             self::SESSION_TABLE,
             $this->getPHID(),
             $establish_type,
-            $session_key);
+            PhabricatorHash::digest($session_key));
           break;
         }
       }
@@ -325,7 +325,7 @@ final class PhabricatorUser extends PhabricatorUserDAO implements PhutilPerson {
         'UPDATE %T SET sessionKey = %s, sessionStart = UNIX_TIMESTAMP()
           WHERE userPHID = %s AND type = %s AND sessionKey = %s',
         self::SESSION_TABLE,
-        $session_key,
+        PhabricatorHash::digest($session_key),
         $this->getPHID(),
         $establish_type,
         $expect_key);
@@ -365,7 +365,7 @@ final class PhabricatorUser extends PhabricatorUserDAO implements PhutilPerson {
       'DELETE FROM %T WHERE userPHID = %s AND sessionKey = %s',
       self::SESSION_TABLE,
       $this->getPHID(),
-      $session_key);
+      PhabricatorHash::digest($session_key));
   }
 
   private function generateEmailToken(
diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php
index 1c8336acec..8fbbe9dfee 100644
--- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php
+++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php
@@ -1326,6 +1326,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList {
         'type' => 'php',
         'name' => $this->getPatchPath('20130529.macroauthormig.php'),
       ),
+      '20130530.sessionhash.php' => array(
+        'type' => 'php',
+        'name' => $this->getPatchPath('20130530.sessionhash.php'),
+      ),
     );
   }
 }