Commit 13964141 authored by Oliver Hader's avatar Oliver Hader Committed by Oliver Hader
Browse files

[SECURITY] Protect persisted session IDs from being used directly

Instead of storing session IDs with their corresponding storage
backends in plain text, their HMAC-SHA256 (Redis) or HMAC-MD5 (DB)
is being used. HMAC-MD5 had to be chosen to avoid breaking changes
for limited field size in database fields (32 characters currently).

This change also allows a fallback to non-hashed-session values,
meaning that
* set() and update() will create new session records with the hashed
  identifier
* get() contains a fallback to the non-hashed-version when no session
  with a hashed identifier is found

Resolves: #91854
Releases: master, 10.4, 9.5
Change-Id: Ia57acc5e0d0cf71088af1aaff1ab894bd1d4e3dd
Security-Bulletin: TYPO3-CORE-SA-2020-011
Security-References: CVE-2020-26228
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/66664

Tested-by: Oliver Hader's avatarOliver Hader <oliver.hader@typo3.org>
Reviewed-by: Oliver Hader's avatarOliver Hader <oliver.hader@typo3.org>
parent 6a972407
......@@ -310,6 +310,7 @@ class BackendUserController extends ActionController
*/
protected function terminateBackendUserSessionAction(BackendUser $backendUser, $sessionId)
{
// terminating value of persisted session ID (probably hashed value)
$sessionBackend = $this->getSessionBackend();
$success = $sessionBackend->remove($sessionId);
......@@ -347,8 +348,7 @@ class BackendUserController extends ActionController
]
);
$sessionBackend = $this->getSessionBackend();
$sessionBackend->update(
$this->getSessionBackend()->update(
$this->getBackendUserAuthentication()->getSessionId(),
[
'ses_userid' => (int)$targetUser['uid'],
......
......@@ -32,6 +32,7 @@ use TYPO3\CMS\Core\Database\Query\Restriction\StartTimeRestriction;
use TYPO3\CMS\Core\Exception;
use TYPO3\CMS\Core\Http\CookieHeaderTrait;
use TYPO3\CMS\Core\Session\Backend\Exception\SessionNotFoundException;
use TYPO3\CMS\Core\Session\Backend\HashableSessionBackendInterface;
use TYPO3\CMS\Core\Session\Backend\SessionBackendInterface;
use TYPO3\CMS\Core\Session\SessionManager;
use TYPO3\CMS\Core\SysLog\Action\Login as SystemLogLoginAction;
......@@ -692,6 +693,17 @@ abstract class AbstractUserAuthentication implements LoggerAwareInterface
]);
}
} elseif ($haveSession) {
// Validate the session ID and promote it
// This check can be removed in TYPO3 v11
if ($this->getSessionBackend() instanceof HashableSessionBackendInterface && !empty($authInfo['userSession']['ses_id'] ?? '')) {
// The session is stored in plaintext, promote it
if ($authInfo['userSession']['ses_id'] === $this->id) {
$authInfo['userSession'] = $this->getSessionBackend()->update(
$this->id,
['ses_data' => $authInfo['userSession']['ses_data']]
);
}
}
// if we come here the current session is for sure not anonymous as this is a pre-condition for $authenticated = true
$this->user = $authInfo['userSession'];
}
......
......@@ -32,7 +32,7 @@ use TYPO3\CMS\Core\Utility\GeneralUtility;
* This session backend requires the 'table' configuration option. If the backend is used to holds non-authenticated
* sessions (default if 'TYPO3_MODE' is 'FE'), the 'ses_anonymous' configuration option must be set to true.
*/
class DatabaseSessionBackend implements SessionBackendInterface
class DatabaseSessionBackend implements SessionBackendInterface, HashableSessionBackendInterface
{
/**
* @var array
......@@ -75,6 +75,14 @@ class DatabaseSessionBackend implements SessionBackendInterface
return true;
}
public function hash(string $sessionId): string
{
// The sha1 hash ensures we have good length for the key.
$key = sha1($GLOBALS['TYPO3_CONF_VARS']['SYS']['encryptionKey'] . 'core-session-backend');
// @todo md5 is used as be_sessions.ses_id field only supports 32 characters in stable branches
return hash_hmac('md5', $sessionId, $key);
}
/**
* Read session data
*
......@@ -88,15 +96,25 @@ class DatabaseSessionBackend implements SessionBackendInterface
$query->select('*')
->from($this->configuration['table'])
->where($query->expr()->eq('ses_id', $query->createNamedParameter($sessionId, \PDO::PARAM_STR)));
->where($query->expr()->eq('ses_id', $query->createNamedParameter($this->hash($sessionId), \PDO::PARAM_STR)));
$result = $query->execute()->fetch();
if (!is_array($result)) {
throw new SessionNotFoundException(
'The session with identifier ' . $sessionId . ' was not found ',
1481885483
);
// Check for a non-hashed-version, will be removed in TYPO3 v11
$query = $this->getQueryBuilder();
$result = $query->select('*')
->from($this->configuration['table'])
->where($query->expr()->eq('ses_id', $query->createNamedParameter($sessionId, \PDO::PARAM_STR)))
->execute()
->fetch();
if (!is_array($result)) {
throw new SessionNotFoundException(
'The session with identifier ' . $sessionId . ' was not found ',
1481885483
);
}
}
return $result;
}
......@@ -111,7 +129,12 @@ class DatabaseSessionBackend implements SessionBackendInterface
{
$query = $this->getQueryBuilder();
$query->delete($this->configuration['table'])
->where($query->expr()->eq('ses_id', $query->createNamedParameter($sessionId, \PDO::PARAM_STR)));
->where(
$query->expr()->orX(
$query->expr()->eq('ses_id', $query->createNamedParameter($this->hash($sessionId), \PDO::PARAM_STR)),
$query->expr()->eq('ses_id', $query->createNamedParameter($sessionId, \PDO::PARAM_STR))
)
);
return (bool)$query->execute();
}
......@@ -128,6 +151,7 @@ class DatabaseSessionBackend implements SessionBackendInterface
*/
public function set(string $sessionId, array $sessionData): array
{
$sessionId = $this->hash($sessionId);
$sessionData['ses_id'] = $sessionId;
$sessionData['ses_tstamp'] = $GLOBALS['EXEC_TIME'] ?? time();
......@@ -160,11 +184,19 @@ class DatabaseSessionBackend implements SessionBackendInterface
*/
public function update(string $sessionId, array $sessionData): array
{
$sessionData['ses_id'] = $sessionId;
$hashedSessionId = $this->hash($sessionId);
$sessionData['ses_id'] = $hashedSessionId;
$sessionData['ses_tstamp'] = $GLOBALS['EXEC_TIME'] ?? time();
try {
// allow 0 records to be affected, happens when no columns where changed
$this->getConnection()->update(
$this->configuration['table'],
$sessionData,
['ses_id' => $hashedSessionId],
['ses_data' => \PDO::PARAM_LOB]
);
// Migrate old session data as well to remove old entries and promote them to migrated entries
$this->getConnection()->update(
$this->configuration['table'],
$sessionData,
......
<?php
declare(strict_types=1);
/*
* This file is part of the TYPO3 CMS project.
*
* It is free software; you can redistribute it and/or modify it under
* the terms of the GNU General Public License, either version 2
* of the License, or any later version.
*
* For the full copyright and license information, please read the
* LICENSE.txt file that was distributed with this source code.
*
* The TYPO3 project - inspiring people to share!
*/
namespace TYPO3\CMS\Core\Session\Backend;
interface HashableSessionBackendInterface
{
public function hash(string $sessionId): string;
}
......@@ -29,7 +29,7 @@ use TYPO3\CMS\Core\Session\Backend\Exception\SessionNotUpdatedException;
* This session backend takes these optional configuration options: 'hostname' (default '127.0.0.1'),
* 'database' (default 0), 'port' (default 3679) and 'password' (no default value).
*/
class RedisSessionBackend implements SessionBackendInterface, LoggerAwareInterface
class RedisSessionBackend implements SessionBackendInterface, HashableSessionBackendInterface, LoggerAwareInterface
{
use LoggerAwareTrait;
......@@ -116,6 +116,13 @@ class RedisSessionBackend implements SessionBackendInterface, LoggerAwareInterfa
}
}
public function hash(string $sessionId): string
{
// The sha1 hash ensures we have good length for the key.
$key = sha1($GLOBALS['TYPO3_CONF_VARS']['SYS']['encryptionKey'] . 'core-session-backend');
return hash_hmac('sha256', $sessionId, $key);
}
/**
* Read session data
*
......@@ -127,9 +134,16 @@ class RedisSessionBackend implements SessionBackendInterface, LoggerAwareInterfa
{
$this->initializeConnection();
$key = $this->getSessionKeyName($sessionId);
$rawData = $this->redis->get($key);
$hashedSessionId = $this->hash($sessionId);
$rawData = $this->redis->get($this->getSessionKeyName($hashedSessionId));
if ($rawData !== false) {
$decodedValue = json_decode($rawData, true);
if (is_array($decodedValue)) {
return $decodedValue;
}
}
// Fallback to the non-hashed-value, will be removed in TYPO3 v11
$rawData = $this->redis->get($this->getSessionKeyName($sessionId));
if ($rawData !== false) {
$decodedValue = json_decode($rawData, true);
if (is_array($decodedValue)) {
......@@ -149,8 +163,11 @@ class RedisSessionBackend implements SessionBackendInterface, LoggerAwareInterfa
public function remove(string $sessionId): bool
{
$this->initializeConnection();
$status = $this->redis->del($this->getSessionKeyName($this->hash($sessionId))) >= 1;
// Checking for non-hashed-identifier, will be removed in TYPO3 v11
$statusLegacy = $this->redis->del($this->getSessionKeyName($sessionId)) >= 1;
return $this->redis->del($this->getSessionKeyName($sessionId)) >= 1;
return $status || $statusLegacy;
}
/**
......@@ -166,15 +183,15 @@ class RedisSessionBackend implements SessionBackendInterface, LoggerAwareInterfa
public function set(string $sessionId, array $sessionData): array
{
$this->initializeConnection();
$sessionData['ses_id'] = $sessionId;
$sessionData['ses_tstamp'] = $GLOBALS['EXEC_TIME'] ?? time();
$key = $this->getSessionKeyName($sessionId);
$hashedSessionId = $this->hash($sessionId);
$sessionData['ses_id'] = $hashedSessionId;
$sessionData['ses_tstamp'] = $GLOBALS['EXEC_TIME'] ?? time();
// nx will not allow overwriting existing keys
$jsonString = json_encode($sessionData);
$wasSet = is_string($jsonString) && $this->redis->set(
$key,
$this->getSessionKeyName($hashedSessionId),
$jsonString,
['nx']
);
......@@ -198,15 +215,16 @@ class RedisSessionBackend implements SessionBackendInterface, LoggerAwareInterfa
*/
public function update(string $sessionId, array $sessionData): array
{
$hashedSessionId = $this->hash($sessionId);
try {
$sessionData = array_merge($this->get($sessionId), $sessionData);
$sessionData = array_merge($this->get($hashedSessionId), $sessionData);
} catch (SessionNotFoundException $e) {
throw new SessionNotUpdatedException('Cannot update non-existing record', 1484389971, $e);
}
$sessionData['ses_id'] = $sessionId;
$sessionData['ses_id'] = $hashedSessionId;
$sessionData['ses_tstamp'] = $GLOBALS['EXEC_TIME'] ?? time();
$key = $this->getSessionKeyName($sessionId);
$key = $this->getSessionKeyName($hashedSessionId);
$jsonString = json_encode($sessionData);
$wasSet = is_string($jsonString) && $this->redis->set($key, $jsonString);
......
......@@ -18,6 +18,7 @@ declare(strict_types=1);
namespace TYPO3\CMS\Core\Session;
use TYPO3\CMS\Core\Authentication\AbstractUserAuthentication;
use TYPO3\CMS\Core\Session\Backend\HashableSessionBackendInterface;
use TYPO3\CMS\Core\Session\Backend\SessionBackendInterface;
use TYPO3\CMS\Core\SingletonInterface;
use TYPO3\CMS\Core\Utility\GeneralUtility;
......@@ -75,15 +76,21 @@ class SessionManager implements SingletonInterface
public function invalidateAllSessionsByUserId(SessionBackendInterface $backend, int $userId, AbstractUserAuthentication $userAuthentication = null)
{
$sessionToRenew = '';
$hashedSessionToRenew = '';
// Prevent destroying the session of the current user session, but renew session id
if ($userAuthentication !== null && (int)$userAuthentication->user['uid'] === $userId) {
$sessionToRenew = $userAuthentication->getSessionId();
}
if ($sessionToRenew !== '' && $backend instanceof HashableSessionBackendInterface) {
$hashedSessionToRenew = $backend->hash($sessionToRenew);
}
foreach ($backend->getAll() as $session) {
if ($userAuthentication !== null && $session['ses_id'] === $sessionToRenew) {
$userAuthentication->enforceNewSessionId();
continue;
if ($userAuthentication !== null) {
if ($session['ses_id'] === $sessionToRenew || $session['ses_id'] === $hashedSessionToRenew) {
$userAuthentication->enforceNewSessionId();
continue;
}
}
if ((int)$session['ses_userid'] === $userId) {
$backend->remove($session['ses_id']);
......
<?xml version="1.0" encoding="utf-8"?>
<dataset>
<be_sessions>
<!-- hash_hmac('md5', '886526ce72b86870739cc41991144ec1', sha1('iAmInvalid' . 'core-session-backend')) -->
<ses_id>a7475832dbc0aa7ed07bb1f800520d16</ses_id>
<ses_iplock>[DISABLED]</ses_iplock>
<ses_userid>1</ses_userid>
<ses_tstamp>1777777777</ses_tstamp>
<ses_data></ses_data>
<ses_backuserid>0</ses_backuserid>
</be_sessions>
<be_sessions>
<!-- hash_hmac('md5', 'ff83dfd81e20b34c27d3e97771a4525a', sha1('iAmInvalid' . 'core-session-backend')) -->
<ses_id>b99a7e54850ef064b7181d0de7d67900</ses_id>
<ses_iplock>[DISABLED]</ses_iplock>
<ses_userid>2</ses_userid>
<ses_tstamp>1777777777</ses_tstamp>
<ses_data></ses_data>
<ses_backuserid>0</ses_backuserid>
</be_sessions>
</dataset>
......@@ -58,7 +58,7 @@ class BackendCoreEnvironment extends BackendEnvironment
],
'xmlDatabaseFixtures' => [
'PACKAGE:typo3/testing-framework/Resources/Core/Acceptance/Fixtures/be_users.xml',
'PACKAGE:typo3/testing-framework/Resources/Core/Acceptance/Fixtures/be_sessions.xml',
'typo3/sysext/core/Tests/Acceptance/Fixtures/be_sessions.xml',
'PACKAGE:typo3/testing-framework/Resources/Core/Acceptance/Fixtures/be_groups.xml',
'PACKAGE:typo3/testing-framework/Resources/Core/Acceptance/Fixtures/sys_category.xml',
'PACKAGE:typo3/testing-framework/Resources/Core/Acceptance/Fixtures/tx_extensionmanager_domain_model_extension.xml',
......
......@@ -56,7 +56,7 @@ class PageTreeCoreEnvironment extends BackendEnvironment
'xmlDatabaseFixtures' => [
'typo3/sysext/core/Tests/Acceptance/Fixtures/pages.xml',
'PACKAGE:typo3/testing-framework/Resources/Core/Acceptance/Fixtures/be_users.xml',
'PACKAGE:typo3/testing-framework/Resources/Core/Acceptance/Fixtures/be_sessions.xml',
'typo3/sysext/core/Tests/Acceptance/Fixtures/be_sessions.xml',
'PACKAGE:typo3/testing-framework/Resources/Core/Acceptance/Fixtures/be_groups.xml',
'PACKAGE:typo3/testing-framework/Resources/Core/Acceptance/Fixtures/sys_category.xml',
],
......
......@@ -36,7 +36,8 @@ class DatabaseSessionBackendTest extends FunctionalTestCase
* @var array
*/
protected $testSessionRecord = [
'ses_id' => 'randomSessionId',
// DatabaseSessionBackend::hash('randomSessionId') with encryption key 12345
'ses_id' => '76898588caa1baee7984f4dc8adfed3b',
'ses_userid' => 1,
// serialize(['foo' => 'bar', 'boo' => 'far'])
'ses_data' => 'a:2:{s:3:"foo";s:3:"bar";s:3:"boo";s:3:"far";}',
......@@ -48,6 +49,7 @@ class DatabaseSessionBackendTest extends FunctionalTestCase
protected function setUp(): void
{
parent::setUp();
$GLOBALS['TYPO3_CONF_VARS']['SYS']['encryptionKey'] = '12345';
$this->subject = new DatabaseSessionBackend();
$this->subject->initialize('default', [
......
......@@ -39,7 +39,8 @@ class RedisSessionBackendTest extends FunctionalTestCase
* @var array
*/
protected $testSessionRecord = [
'ses_id' => 'randomSessionId',
// RedisSessionBackend::hash('randomSessionId') with encryption key 12345
'ses_id' => '21c0e911565a67315cdc384889c470fd291feafbfa62e31ecf7409430640bc7a',
'ses_userid' => 1,
// serialize(['foo' => 'bar', 'boo' => 'far'])
'ses_data' => 'a:2:{s:3:"foo";s:3:"bar";s:3:"boo";s:3:"far";}',
......@@ -51,6 +52,7 @@ class RedisSessionBackendTest extends FunctionalTestCase
protected function setUp(): void
{
parent::setUp();
$GLOBALS['TYPO3_CONF_VARS']['SYS']['encryptionKey'] = '12345';
if (!getenv('typo3TestingRedisHost')) {
self::markTestSkipped('environment variable "typo3TestingRedisHost" must be set to run this test');
......
......@@ -31,16 +31,19 @@ class SessionManagerTest extends FunctionalTestCase
* @var array
*/
protected $testSessionRecords = [
[
'ses_id' => 'randomSessionId1',
'randomSessionId1' => [
// DatabaseSessionBackend::hash('randomSessionId1') with encryption key 12345
'ses_id' => 'e1ad65e4bad3c29e12c754c8e9f5927e',
'ses_userid' => 1,
],
[
'ses_id' => 'randomSessionId2',
'randomSessionId2' => [
// DatabaseSessionBackend::hash('randomSessionId2') with encryption key 12345
'ses_id' => '72b1cf1fccc010ddb760c6db03f668db',
'ses_userid' => 1,
],
[
'ses_id' => 'randomSessionId3',
'randomSessionId3' => [
// DatabaseSessionBackend::hash('randomSessionId3') with encryption key 12345
'ses_id' => '7ee0836849b95d884108486c4a8973f3',
'ses_userid' => 2,
]
];
......@@ -51,14 +54,16 @@ class SessionManagerTest extends FunctionalTestCase
protected function setUp(): void
{
parent::setUp();
$GLOBALS['TYPO3_CONF_VARS']['SYS']['encryptionKey'] = '12345';
$this->subject = new SessionManager();
$frontendSessionBackend = $this->subject->getSessionBackend('FE');
foreach ($this->testSessionRecords as $testSessionRecord) {
$frontendSessionBackend->set($testSessionRecord['ses_id'], $testSessionRecord);
foreach ($this->testSessionRecords as $sessionId => $testSessionRecord) {
$frontendSessionBackend->set($sessionId, $testSessionRecord);
}
$backendSessionBackend = $this->subject->getSessionBackend('BE');
foreach ($this->testSessionRecords as $testSessionRecord) {
$backendSessionBackend->set($testSessionRecord['ses_id'], $testSessionRecord);
foreach ($this->testSessionRecords as $sessionId => $testSessionRecord) {
$backendSessionBackend->set($sessionId, $testSessionRecord);
}
}
......@@ -73,7 +78,7 @@ class SessionManagerTest extends FunctionalTestCase
$this->subject->invalidateAllSessionsByUserId($backendSessionBackend, 1);
$allActiveSessions = $backendSessionBackend->getAll();
self::assertCount(1, $allActiveSessions);
self::assertSame('randomSessionId3', $allActiveSessions[0]['ses_id']);
self::assertSame($this->testSessionRecords['randomSessionId3']['ses_id'], $allActiveSessions[0]['ses_id']);
self::assertSame(2, (int)$allActiveSessions[0]['ses_userid']);
}
......@@ -88,7 +93,7 @@ class SessionManagerTest extends FunctionalTestCase
$this->subject->invalidateAllSessionsByUserId($frontendSessionBackend, 1);
$allActiveSessions = $frontendSessionBackend->getAll();
self::assertCount(1, $allActiveSessions);
self::assertSame('randomSessionId3', $allActiveSessions[0]['ses_id']);
self::assertSame($this->testSessionRecords['randomSessionId3']['ses_id'], $allActiveSessions[0]['ses_id']);
self::assertSame(2, (int)$allActiveSessions[0]['ses_userid']);
}
}
......@@ -41,6 +41,8 @@ use TYPO3\TestingFramework\Core\Unit\UnitTestCase;
*/
class FrontendUserAuthenticationTest extends UnitTestCase
{
private const NOT_CHECKED_INDICATOR = '--not-checked--';
/**
* @var bool Reset singletons created by subject
*/
......@@ -70,7 +72,7 @@ class FrontendUserAuthenticationTest extends UnitTestCase
// Main session backend setup
$sessionBackendProphecy = $this->prophesize(SessionBackendInterface::class);
$sessionRecord = [
'ses_id' => $uniqueSessionId,
'ses_id' => $uniqueSessionId . self::NOT_CHECKED_INDICATOR,
'ses_data' => serialize(['foo' => 'bar']),
'ses_anonymous' => true,
'ses_iplock' => '[DISABLED]',
......@@ -111,6 +113,7 @@ class FrontendUserAuthenticationTest extends UnitTestCase
$sessionManagerProphecy = $this->prophesize(SessionManager::class);
GeneralUtility::setSingletonInstance(SessionManager::class, $sessionManagerProphecy->reveal());
$sessionManagerProphecy->getSessionBackend('FE')->willReturn($sessionBackendProphecy->reveal());
// @todo Session handling is not used/evaluated at all in this test
// Verify new session id is generated
$randomProphecy = $this->prophesize(Random::class);
......@@ -153,7 +156,7 @@ class FrontendUserAuthenticationTest extends UnitTestCase
// Main session backend setup
$sessionBackendProphecy = $this->prophesize(SessionBackendInterface::class);
$sessionRecord = [
'ses_id' => $uniqueSessionId,
'ses_id' => $uniqueSessionId . self::NOT_CHECKED_INDICATOR,
'ses_data' => serialize(['foo' => 'bar']),
'ses_anonymous' => true,
'ses_iplock' => '[DISABLED]',
......@@ -277,7 +280,7 @@ class FrontendUserAuthenticationTest extends UnitTestCase
// a valid session is returned
$sessionBackendProphecy->get($uniqueSessionId)->shouldBeCalled()->willReturn(
[
'ses_id' => $uniqueSessionId,
'ses_id' => $uniqueSessionId . self::NOT_CHECKED_INDICATOR,
'ses_userid' => 1,
'ses_iplock' => '[DISABLED]',
'ses_tstamp' => $currentTime,
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment