Page MenuHomePhabricator
Files F15430567

D19883.id.diff
No OneTemporary
Actions

D19883.id.diff
View Options

diff --git a/resources/sql/autopatches/20181213.auth.04.longerhashes.sql b/resources/sql/autopatches/20181213.auth.04.longerhashes.sql
new file mode 100644
--- /dev/null
+++ b/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;
diff --git a/resources/sql/autopatches/20181213.auth.05.longerloghashes.sql b/resources/sql/autopatches/20181213.auth.05.longerloghashes.sql
new file mode 100644
--- /dev/null
+++ b/resources/sql/autopatches/20181213.auth.05.longerloghashes.sql
@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_user.user_log
+ CHANGE session session VARBINARY(64);
diff --git a/resources/sql/patches/20130530.sessionhash.php b/resources/sql/patches/20130530.sessionhash.php
--- a/resources/sql/patches/20130530.sessionhash.php
+++ b/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.
diff --git a/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php b/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php
--- a/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php
+++ b/src/applications/auth/controller/PhabricatorAuthTerminateSessionController.php
@@ -16,8 +16,9 @@
$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) {
diff --git a/src/applications/auth/controller/PhabricatorAuthUnlinkController.php b/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
--- a/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
+++ b/src/applications/auth/controller/PhabricatorAuthUnlinkController.php
@@ -56,7 +56,8 @@
id(new PhabricatorAuthSessionEngine())->terminateLoginSessions(
$viewer,
- $request->getCookie(PhabricatorCookies::COOKIE_SESSION));
+ new PhutilOpaqueEnvelope(
+ $request->getCookie(PhabricatorCookies::COOKIE_SESSION)));
return id(new AphrontRedirectResponse())->setURI($this->getDoneURI());
}
diff --git a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
--- a/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
+++ b/src/applications/auth/engine/PhabricatorAuthSessionEngine.php
@@ -109,14 +109,19 @@
$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 @@
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 @@
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 @@
// 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 @@
*/
public function terminateLoginSessions(
PhabricatorUser $user,
- $except_session = null) {
+ PhutilOpaqueEnvelope $except_session = null) {
$sessions = id(new PhabricatorAuthSessionQuery())
->setViewer($user)
@@ -307,7 +333,8 @@
->execute();
if ($except_session !== null) {
- $except_session = PhabricatorHash::weakDigest($except_session);
+ $except_session = PhabricatorAuthSession::newSessionDigest(
+ $except_session);
}
foreach ($sessions as $key => $session) {
diff --git a/src/applications/auth/query/PhabricatorAuthSessionQuery.php b/src/applications/auth/query/PhabricatorAuthSessionQuery.php
--- a/src/applications/auth/query/PhabricatorAuthSessionQuery.php
+++ b/src/applications/auth/query/PhabricatorAuthSessionQuery.php
@@ -91,7 +91,8 @@
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,
diff --git a/src/applications/auth/storage/PhabricatorAuthSession.php b/src/applications/auth/storage/PhabricatorAuthSession.php
--- a/src/applications/auth/storage/PhabricatorAuthSession.php
+++ b/src/applications/auth/storage/PhabricatorAuthSession.php
@@ -6,6 +6,8 @@
const TYPE_WEB = 'web';
const TYPE_CONDUIT = 'conduit';
+ const SESSION_DIGEST_KEY = 'session.digest';
+
protected $userPHID;
protected $type;
protected $sessionKey;
@@ -17,13 +19,19 @@
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?',
diff --git a/src/applications/people/storage/PhabricatorUserLog.php b/src/applications/people/storage/PhabricatorUserLog.php
--- a/src/applications/people/storage/PhabricatorUserLog.php
+++ b/src/applications/people/storage/PhabricatorUserLog.php
@@ -150,7 +150,7 @@
'actorPHID' => 'phid?',
'action' => 'text64',
'remoteAddr' => 'text64',
- 'session' => 'bytes40?',
+ 'session' => 'text64?',
),
self::CONFIG_KEY_SCHEMA => array(
'actorPHID' => array(
diff --git a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
--- a/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorMultiFactorSettingsPanel.php
@@ -193,7 +193,8 @@
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()));
diff --git a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
--- a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php
@@ -121,7 +121,8 @@
id(new PhabricatorAuthSessionEngine())->terminateLoginSessions(
$user,
- $request->getCookie(PhabricatorCookies::COOKIE_SESSION));
+ new PhutilOpaqueEnvelope(
+ $request->getCookie(PhabricatorCookies::COOKIE_SESSION)));
return id(new AphrontRedirectResponse())->setURI($next);
}
diff --git a/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php b/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php
--- a/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php
+++ b/src/applications/settings/panel/PhabricatorSessionsSettingsPanel.php
@@ -44,8 +44,9 @@
->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();
diff --git a/src/infrastructure/util/PhabricatorHash.php b/src/infrastructure/util/PhabricatorHash.php
--- a/src/infrastructure/util/PhabricatorHash.php
+++ b/src/infrastructure/util/PhabricatorHash.php
@@ -187,6 +187,16 @@
}
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 @@
$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.'));
}

File Metadata

Mime Type
text/plain
Expires
Tue, Mar 25, 8:12 AM (3 w, 21 m ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7633951
Default Alt Text
D19883.id.diff (12 KB)

Event Timeline

Phabricator · Built by Phacility