From e71564fc75fd366c667124a4cad49e8839875ca0 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sun, 16 Jun 2013 10:19:31 -0700
Subject: [PATCH] Store the digest of the registration key, not the key itslef

Summary: Ref T1536. Like D6080, we don't need to store the registration key itself. This prevents a theoretical attacker who can read the database but not write to it from hijacking registrations.

Test Plan: Registered a new account.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1536

Differential Revision: https://secure.phabricator.com/D6188
---
 .../auth/controller/PhabricatorAuthLoginController.php    | 4 +++-
 .../auth/controller/PhabricatorAuthRegisterController.php | 8 +++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/applications/auth/controller/PhabricatorAuthLoginController.php b/src/applications/auth/controller/PhabricatorAuthLoginController.php
index 01fa1ef159..b11482d36d 100644
--- a/src/applications/auth/controller/PhabricatorAuthLoginController.php
+++ b/src/applications/auth/controller/PhabricatorAuthLoginController.php
@@ -123,7 +123,9 @@ final class PhabricatorAuthLoginController
     // key.
 
     $registration_key = Filesystem::readRandomCharacters(32);
-    $account->setProperty('registrationKey', $registration_key);
+    $account->setProperty(
+      'registrationKey',
+      PhabricatorHash::digest($registration_key));
 
     $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
       $account->save();
diff --git a/src/applications/auth/controller/PhabricatorAuthRegisterController.php b/src/applications/auth/controller/PhabricatorAuthRegisterController.php
index 74cdb1cfe9..062ac4cc4a 100644
--- a/src/applications/auth/controller/PhabricatorAuthRegisterController.php
+++ b/src/applications/auth/controller/PhabricatorAuthRegisterController.php
@@ -359,7 +359,13 @@ final class PhabricatorAuthRegisterController
           'Check that cookies are enabled and try again.'));
     }
 
-    if ($registration_key != $account->getProperty('registrationKey')) {
+    // We store the digest of the key rather than the key itself to prevent a
+    // theoretical attacker with read-only access to the database from
+    // hijacking registration sessions.
+
+    $actual = $account->getProperty('registrationKey');
+    $expect = PhabricatorHash::digest($registration_key);
+    if ($actual !== $expect) {
       return $this->renderError(
         pht(
           'Your browser submitted a different registration key than the one '.