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 @@ 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.')); }