From 80d724be9172eb3c9511033ff81a520e04847dfe Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Wed, 21 Aug 2024 16:34:37 +0200 Subject: [PATCH 1/4] fix(session): Make session encryption more robust [skipci] Signed-off-by: Christoph Wurst --- lib/base.php | 6 +- lib/composer/composer/autoload_classmap.php | 3 +- lib/composer/composer/autoload_static.php | 3 +- lib/private/Session/CryptoSessionHandler.php | 144 ++++++++++++++++++ lib/private/Session/CryptoWrapper.php | 4 +- ...onData.php => LegacyCryptoSessionData.php} | 103 ++++++------- tests/lib/Session/CryptoSessionDataTest.php | 4 +- tests/lib/Session/CryptoWrappingTest.php | 6 +- 8 files changed, 205 insertions(+), 68 deletions(-) create mode 100644 lib/private/Session/CryptoSessionHandler.php rename lib/private/Session/{CryptoSessionData.php => LegacyCryptoSessionData.php} (70%) diff --git a/lib/base.php b/lib/base.php index 010fed1ea9f8f..7723468826cf0 100644 --- a/lib/base.php +++ b/lib/base.php @@ -7,6 +7,7 @@ * SPDX-License-Identifier: AGPL-3.0-only */ use OC\Encryption\HookManager; +use OC\Session\CryptoSessionHandler; use OC\Share20\Hooks; use OCP\EventDispatcher\IEventDispatcher; use OCP\Group\Events\UserRemovedEvent; @@ -361,6 +362,9 @@ private static function printUpgradePage(\OC\SystemConfig $systemConfig): void { public static function initSession(): void { $request = Server::get(IRequest::class); + $cryptoHandler = Server::get(CryptoSessionHandler::class); + session_set_save_handler($cryptoHandler, true); + // TODO: Temporary disabled again to solve issues with CalDAV/CardDAV clients like DAVx5 that use cookies // TODO: See https://github.com/nextcloud/server/issues/37277#issuecomment-1476366147 and the other comments // TODO: for further information. @@ -658,7 +662,7 @@ public static function init(): void { if (!function_exists('simplexml_load_file')) { throw new \OCP\HintException('The PHP SimpleXML/PHP-XML extension is not installed.', 'Install the extension or make sure it is enabled.'); } - + OC_App::loadApps(['session']); if (!self::$CLI) { self::initSession(); diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 1a3a86fd20776..459bb482f7fc5 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1871,9 +1871,10 @@ 'OC\\ServerContainer' => $baseDir . '/lib/private/ServerContainer.php', 'OC\\ServerNotAvailableException' => $baseDir . '/lib/private/ServerNotAvailableException.php', 'OC\\ServiceUnavailableException' => $baseDir . '/lib/private/ServiceUnavailableException.php', - 'OC\\Session\\CryptoSessionData' => $baseDir . '/lib/private/Session/CryptoSessionData.php', + 'OC\\Session\\CryptoSessionHandler' => $baseDir . '/lib/private/Session/CryptoSessionHandler.php', 'OC\\Session\\CryptoWrapper' => $baseDir . '/lib/private/Session/CryptoWrapper.php', 'OC\\Session\\Internal' => $baseDir . '/lib/private/Session/Internal.php', + 'OC\\Session\\LegacyCryptoSessionData' => $baseDir . '/lib/private/Session/LegacyCryptoSessionData.php', 'OC\\Session\\Memory' => $baseDir . '/lib/private/Session/Memory.php', 'OC\\Session\\Session' => $baseDir . '/lib/private/Session/Session.php', 'OC\\Settings\\AuthorizedGroup' => $baseDir . '/lib/private/Settings/AuthorizedGroup.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index c636ba769f935..df38c56cae5d5 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1904,9 +1904,10 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\ServerContainer' => __DIR__ . '/../../..' . '/lib/private/ServerContainer.php', 'OC\\ServerNotAvailableException' => __DIR__ . '/../../..' . '/lib/private/ServerNotAvailableException.php', 'OC\\ServiceUnavailableException' => __DIR__ . '/../../..' . '/lib/private/ServiceUnavailableException.php', - 'OC\\Session\\CryptoSessionData' => __DIR__ . '/../../..' . '/lib/private/Session/CryptoSessionData.php', + 'OC\\Session\\CryptoSessionHandler' => __DIR__ . '/../../..' . '/lib/private/Session/CryptoSessionHandler.php', 'OC\\Session\\CryptoWrapper' => __DIR__ . '/../../..' . '/lib/private/Session/CryptoWrapper.php', 'OC\\Session\\Internal' => __DIR__ . '/../../..' . '/lib/private/Session/Internal.php', + 'OC\\Session\\LegacyCryptoSessionData' => __DIR__ . '/../../..' . '/lib/private/Session/LegacyCryptoSessionData.php', 'OC\\Session\\Memory' => __DIR__ . '/../../..' . '/lib/private/Session/Memory.php', 'OC\\Session\\Session' => __DIR__ . '/../../..' . '/lib/private/Session/Session.php', 'OC\\Settings\\AuthorizedGroup' => __DIR__ . '/../../..' . '/lib/private/Settings/AuthorizedGroup.php', diff --git a/lib/private/Session/CryptoSessionHandler.php b/lib/private/Session/CryptoSessionHandler.php new file mode 100644 index 0000000000000..594a9ca35adec --- /dev/null +++ b/lib/private/Session/CryptoSessionHandler.php @@ -0,0 +1,144 @@ +secureRandom->generate(128); + return implode('|', [$id, $passphrase]); + } + + /** + * Read and decrypt session data + * + * The decryption passphrase is encoded in the id since Nextcloud 31. For + * backwards compatibility after upgrade we also read the pass phrase from + * the old cookie and try to restore session from the legacy format. + * + * @param string $id + * + * @return false|string + */ + public function read(string $id): false|string { + [$sessionId, $passphrase] = $this->parseId($id); + + /** + * Legacy handling + * + * @TODO remove in Nextcloud 32 + */ + if ($passphrase === null) { + if (($passphrase = $this->request->getCookie(CryptoWrapper::COOKIE_NAME)) === false) { + // No passphrase in the ID or an old cookie. Time to give up. + return false; + } + + // Read the encrypted and encoded data + $serializedData = parent::read($sessionId); + if ($serializedData === '') { + // Nothing to decode or decrypt + return ''; + } + // Back up the superglobal before we decode/restore the legacy session data + // This is necessary because session_decode populates the superglobals + // We restore the old state end the end (the final block, also run in + // case of an error) + $globalBackup = $_SESSION; + try { + if (!session_decode($serializedData)) { + // Bail if we can't decode the data + return false; + } + $encryptedData = $_SESSION['encrypted_session_data']; + try { + $sessionValues = json_decode( + $this->crypto->decrypt($encryptedData, $passphrase), + true, + 512, + JSON_THROW_ON_ERROR, + ); + foreach ($sessionValues as $key => $value) { + $_SESSION[$key] = $value; + } + } catch (Exception $e) { + logger('core')->critical('Could not decrypt or decode encrypted legacy session data', [ + 'exception' => $e, + 'sessionId' => $sessionId, + ]); + } + return session_encode(); + } finally { + foreach (array_keys($_SESSION) as $key) { + unset($_SESSION[$key]); + } + foreach ($globalBackup as $key => $value) { + $_SESSION[$key] = $value; + } + $_SESSION = $globalBackup; + } + } + + $read = parent::read($sessionId); + return $read; + } + + /** + * Encrypt and write session data + * + * @param string $id + * @param string $data + * + * @return bool + */ + public function write(string $id, string $data): bool { + [$sessionId, $passphrase] = $this->parseId($id); + + if ($passphrase === null) { + $passphrase = $this->request->getCookie(CryptoWrapper::COOKIE_NAME); + // return false; + } + + return parent::write($sessionId, $data); + } + + public function close(): bool { + parent::close(); + } + +} diff --git a/lib/private/Session/CryptoWrapper.php b/lib/private/Session/CryptoWrapper.php index aceb387ea741f..3d538a8dc6aa8 100644 --- a/lib/private/Session/CryptoWrapper.php +++ b/lib/private/Session/CryptoWrapper.php @@ -90,8 +90,8 @@ public function __construct(IConfig $config, * @return ISession */ public function wrapSession(ISession $session) { - if (!($session instanceof CryptoSessionData)) { - return new CryptoSessionData($session, $this->crypto, $this->passphrase); + if (!($session instanceof LegacyCryptoSessionData)) { + return new LegacyCryptoSessionData($session, $this->crypto, $this->passphrase); } return $session; diff --git a/lib/private/Session/CryptoSessionData.php b/lib/private/Session/LegacyCryptoSessionData.php similarity index 70% rename from lib/private/Session/CryptoSessionData.php rename to lib/private/Session/LegacyCryptoSessionData.php index 323253af534c7..9cf115f79e47c 100644 --- a/lib/private/Session/CryptoSessionData.php +++ b/lib/private/Session/LegacyCryptoSessionData.php @@ -15,28 +15,24 @@ use function OCP\Log\logger; /** - * Class CryptoSessionData - * * @package OC\Session * @template-implements \ArrayAccess + * @deprecated 31.0.0 */ -class CryptoSessionData implements \ArrayAccess, ISession { +class LegacyCryptoSessionData implements \ArrayAccess, ISession { /** @var ISession */ protected $session; - /** @var \OCP\Security\ICrypto */ + /** @var ICrypto */ protected $crypto; /** @var string */ protected $passphrase; - /** @var array */ - protected $sessionValues; - /** @var bool */ - protected $isModified = false; public const encryptedSessionName = 'encrypted_session_data'; /** * @param ISession $session * @param ICrypto $crypto * @param string $passphrase + * @deprecated 31.0.0 */ public function __construct(ISession $session, ICrypto $crypto, @@ -49,6 +45,7 @@ public function __construct(ISession $session, /** * Close session if class gets destructed + * @deprecated 31.0.0 */ public function __destruct() { try { @@ -59,26 +56,31 @@ public function __destruct() { } } + /** + * @deprecated 31.0.0 + */ protected function initializeSession() { $encryptedSessionData = $this->session->get(self::encryptedSessionName) ?: ''; if ($encryptedSessionData === '') { // Nothing to decrypt - $this->sessionValues = []; - } else { - try { - $this->sessionValues = json_decode( - $this->crypto->decrypt($encryptedSessionData, $this->passphrase), - true, - 512, - JSON_THROW_ON_ERROR, - ); - } catch (\Exception $e) { - logger('core')->critical('Could not decrypt or decode encrypted session data', [ - 'exception' => $e, - ]); - $this->sessionValues = []; - $this->regenerateId(true, false); + return; + } + try { + $sessionValues = json_decode( + $this->crypto->decrypt($encryptedSessionData, $this->passphrase), + true, + 512, + JSON_THROW_ON_ERROR, + ); + foreach ($sessionValues as $key => $value) { + $this->session->set($key, $value); } + $this->session->remove(self::encryptedSessionName); + } catch (\Exception $e) { + logger('core')->critical('Could not decrypt or decode encrypted session data', [ + 'exception' => $e, + ]); + $this->regenerateId(true, false); } } @@ -87,19 +89,10 @@ protected function initializeSession() { * * @param string $key * @param mixed $value + * @deprecated 31.0.0 */ public function set(string $key, $value) { - if ($this->get($key) === $value) { - // Do not write the session if the value hasn't changed to avoid reopening - return; - } - - $reopened = $this->reopen(); - $this->sessionValues[$key] = $value; - $this->isModified = true; - if ($reopened) { - $this->close(); - } + $this->session->set($key, $value); } /** @@ -107,13 +100,10 @@ public function set(string $key, $value) { * * @param string $key * @return string|null Either the value or null + * @deprecated 31.0.0 */ public function get(string $key) { - if (isset($this->sessionValues[$key])) { - return $this->sessionValues[$key]; - } - - return null; + return $this->session->get($key); } /** @@ -121,42 +111,37 @@ public function get(string $key) { * * @param string $key * @return bool + * @deprecated 31.0.0 */ public function exists(string $key): bool { - return isset($this->sessionValues[$key]); + return $this->session->exists($key); } /** * Remove a $key/$value pair from the session * * @param string $key + * @deprecated 31.0.0 */ public function remove(string $key) { - $reopened = $this->reopen(); - $this->isModified = true; - unset($this->sessionValues[$key]); - if ($reopened) { - $this->close(); - } + $this->session->remove($key); } /** * Reset and recreate the session + * @deprecated 31.0.0 */ public function clear() { - $reopened = $this->reopen(); $requesttoken = $this->get('requesttoken'); - $this->sessionValues = []; + $this->session->clear(); if ($requesttoken !== null) { $this->set('requesttoken', $requesttoken); } - $this->isModified = true; - $this->session->clear(); - if ($reopened) { - $this->close(); - } } + /** + * @deprecated 31.0.0 + */ public function reopen(): bool { $reopened = $this->session->reopen(); if ($reopened) { @@ -171,6 +156,7 @@ public function reopen(): bool { * @param bool $deleteOldSession Whether to delete the old associated session file or not. * @param bool $updateToken Wheater to update the associated auth token * @return void + * @deprecated 31.0.0 */ public function regenerateId(bool $deleteOldSession = true, bool $updateToken = false) { $this->session->regenerateId($deleteOldSession, $updateToken); @@ -182,6 +168,7 @@ public function regenerateId(bool $deleteOldSession = true, bool $updateToken = * @return string * @throws SessionNotAvailableException * @since 9.1.0 + * @deprecated 31.0.0 */ public function getId(): string { return $this->session->getId(); @@ -189,19 +176,16 @@ public function getId(): string { /** * Close the session and release the lock, also writes all changed data in batch + * @deprecated 31.0.0 */ public function close() { - if ($this->isModified) { - $encryptedValue = $this->crypto->encrypt(json_encode($this->sessionValues), $this->passphrase); - $this->session->set(self::encryptedSessionName, $encryptedValue); - $this->isModified = false; - } $this->session->close(); } /** * @param mixed $offset * @return bool + * @deprecated 31.0.0 */ public function offsetExists($offset): bool { return $this->exists($offset); @@ -210,6 +194,7 @@ public function offsetExists($offset): bool { /** * @param mixed $offset * @return mixed + * @deprecated 31.0.0 */ #[\ReturnTypeWillChange] public function offsetGet($offset) { @@ -219,6 +204,7 @@ public function offsetGet($offset) { /** * @param mixed $offset * @param mixed $value + * @deprecated 31.0.0 */ public function offsetSet($offset, $value): void { $this->set($offset, $value); @@ -226,6 +212,7 @@ public function offsetSet($offset, $value): void { /** * @param mixed $offset + * @deprecated 31.0.0 */ public function offsetUnset($offset): void { $this->remove($offset); diff --git a/tests/lib/Session/CryptoSessionDataTest.php b/tests/lib/Session/CryptoSessionDataTest.php index 6c1536f47698c..4d95ffe2f0229 100644 --- a/tests/lib/Session/CryptoSessionDataTest.php +++ b/tests/lib/Session/CryptoSessionDataTest.php @@ -7,7 +7,7 @@ namespace Test\Session; -use OC\Session\CryptoSessionData; +use OC\Session\LegacyCryptoSessionData; use OCP\Security\ICrypto; class CryptoSessionDataTest extends Session { @@ -36,6 +36,6 @@ protected function setUp(): void { return substr($input, 1, -1); }); - $this->instance = new CryptoSessionData($this->wrappedSession, $this->crypto, 'PASS'); + $this->instance = new LegacyCryptoSessionData($this->wrappedSession, $this->crypto, 'PASS'); } } diff --git a/tests/lib/Session/CryptoWrappingTest.php b/tests/lib/Session/CryptoWrappingTest.php index 093f2214929ee..9217eb464a7c5 100644 --- a/tests/lib/Session/CryptoWrappingTest.php +++ b/tests/lib/Session/CryptoWrappingTest.php @@ -7,7 +7,7 @@ namespace Test\Session; -use OC\Session\CryptoSessionData; +use OC\Session\LegacyCryptoSessionData; use OCP\ISession; use Test\TestCase; @@ -18,7 +18,7 @@ class CryptoWrappingTest extends TestCase { /** @var \PHPUnit\Framework\MockObject\MockObject|\OCP\ISession */ protected $wrappedSession; - /** @var \OC\Session\CryptoSessionData */ + /** @var \OC\Session\LegacyCryptoSessionData */ protected $instance; protected function setUp(): void { @@ -44,7 +44,7 @@ protected function setUp(): void { return substr($input, 1, -1); }); - $this->instance = new CryptoSessionData($this->wrappedSession, $this->crypto, 'PASS'); + $this->instance = new LegacyCryptoSessionData($this->wrappedSession, $this->crypto, 'PASS'); } public function testUnwrappingGet() { From 7a3f5cdced194a27ee234327e76246cd13e3c2ab Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Tue, 27 Aug 2024 09:26:55 +0200 Subject: [PATCH 2/4] fixup! fix(session): Make session encryption more robust Signed-off-by: Christoph Wurst --- lib/private/Session/CryptoSessionHandler.php | 100 +++++------------- lib/private/Session/Internal.php | 3 +- .../Session/LegacyCryptoSessionData.php | 2 +- 3 files changed, 29 insertions(+), 76 deletions(-) diff --git a/lib/private/Session/CryptoSessionHandler.php b/lib/private/Session/CryptoSessionHandler.php index 594a9ca35adec..d89a5b67c3bc0 100644 --- a/lib/private/Session/CryptoSessionHandler.php +++ b/lib/private/Session/CryptoSessionHandler.php @@ -28,16 +28,6 @@ public function __construct(private ISecureRandom $secureRandom, private IRequest $request) { } - /** - * @param string $id - * - * @return array{0: string, 1: ?string} - */ - private function parseId(string $id): array { - $parts = explode('|', $id); - return [$parts[0], $parts[1]]; - } - public function create_sid(): string { $id = parent::create_sid(); $passphrase = $this->secureRandom->generate(128); @@ -47,75 +37,23 @@ public function create_sid(): string { /** * Read and decrypt session data * - * The decryption passphrase is encoded in the id since Nextcloud 31. For - * backwards compatibility after upgrade we also read the pass phrase from - * the old cookie and try to restore session from the legacy format. - * * @param string $id * * @return false|string */ public function read(string $id): false|string { - [$sessionId, $passphrase] = $this->parseId($id); - - /** - * Legacy handling - * - * @TODO remove in Nextcloud 32 - */ + [$sessionId, $passphrase] = self::parseId($id); if ($passphrase === null) { - if (($passphrase = $this->request->getCookie(CryptoWrapper::COOKIE_NAME)) === false) { - // No passphrase in the ID or an old cookie. Time to give up. - return false; - } + return parent::read($sessionId); + } - // Read the encrypted and encoded data - $serializedData = parent::read($sessionId); - if ($serializedData === '') { - // Nothing to decode or decrypt - return ''; - } - // Back up the superglobal before we decode/restore the legacy session data - // This is necessary because session_decode populates the superglobals - // We restore the old state end the end (the final block, also run in - // case of an error) - $globalBackup = $_SESSION; - try { - if (!session_decode($serializedData)) { - // Bail if we can't decode the data - return false; - } - $encryptedData = $_SESSION['encrypted_session_data']; - try { - $sessionValues = json_decode( - $this->crypto->decrypt($encryptedData, $passphrase), - true, - 512, - JSON_THROW_ON_ERROR, - ); - foreach ($sessionValues as $key => $value) { - $_SESSION[$key] = $value; - } - } catch (Exception $e) { - logger('core')->critical('Could not decrypt or decode encrypted legacy session data', [ - 'exception' => $e, - 'sessionId' => $sessionId, - ]); - } - return session_encode(); - } finally { - foreach (array_keys($_SESSION) as $key) { - unset($_SESSION[$key]); - } - foreach ($globalBackup as $key => $value) { - $_SESSION[$key] = $value; - } - $_SESSION = $globalBackup; - } + $encryptedData = parent::read($sessionId); + if ($encryptedData === '') { + return ''; } + $data = $this->crypto->decrypt($encryptedData, $passphrase); - $read = parent::read($sessionId); - return $read; + return $data; } /** @@ -127,18 +65,32 @@ public function read(string $id): false|string { * @return bool */ public function write(string $id, string $data): bool { - [$sessionId, $passphrase] = $this->parseId($id); + [$sessionId, $passphrase] = self::parseId($id); if ($passphrase === null) { $passphrase = $this->request->getCookie(CryptoWrapper::COOKIE_NAME); - // return false; + if ($passphrase === null) { + return false; + } } - return parent::write($sessionId, $data); + $encryptedData = $this->crypto->encrypt($data, $passphrase); + + return parent::write($sessionId, $encryptedData); } public function close(): bool { - parent::close(); + return parent::close(); + } + + /** + * @param string $id + * + * @return array{0: string, 1: ?string} + */ + public static function parseId(string $id): array { + $parts = explode('|', $id); + return [$parts[0], $parts[1]]; } } diff --git a/lib/private/Session/Internal.php b/lib/private/Session/Internal.php index 5398dc710af9c..a6c7a83f47eb0 100644 --- a/lib/private/Session/Internal.php +++ b/lib/private/Session/Internal.php @@ -153,7 +153,8 @@ public function getId(): string { if ($id === '') { throw new SessionNotAvailableException(); } - return $id; + // Only return the ID part, not the passphrase + return CryptoSessionHandler::parseId($id)[0]; } /** diff --git a/lib/private/Session/LegacyCryptoSessionData.php b/lib/private/Session/LegacyCryptoSessionData.php index 9cf115f79e47c..bd8ac940a106c 100644 --- a/lib/private/Session/LegacyCryptoSessionData.php +++ b/lib/private/Session/LegacyCryptoSessionData.php @@ -77,7 +77,7 @@ protected function initializeSession() { } $this->session->remove(self::encryptedSessionName); } catch (\Exception $e) { - logger('core')->critical('Could not decrypt or decode encrypted session data', [ + logger('core')->critical('Could not decrypt or decode encrypted legacy session data', [ 'exception' => $e, ]); $this->regenerateId(true, false); From fec99721ac97df43da05e6ba209fe9a82f4f20eb Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Tue, 27 Aug 2024 10:57:53 +0200 Subject: [PATCH 3/4] fixup! fix(session): Make session encryption more robust Signed-off-by: Christoph Wurst --- lib/private/Session/CryptoSessionHandler.php | 23 +++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/lib/private/Session/CryptoSessionHandler.php b/lib/private/Session/CryptoSessionHandler.php index d89a5b67c3bc0..838096c20a4fd 100644 --- a/lib/private/Session/CryptoSessionHandler.php +++ b/lib/private/Session/CryptoSessionHandler.php @@ -13,6 +13,7 @@ use OCP\IRequest; use OCP\Security\ICrypto; use OCP\Security\ISecureRandom; +use Psr\Log\LoggerInterface; use SessionHandler; use function explode; use function implode; @@ -20,11 +21,13 @@ use function OCP\Log\logger; use function session_decode; use function session_encode; +use function strlen; class CryptoSessionHandler extends SessionHandler { public function __construct(private ISecureRandom $secureRandom, private ICrypto $crypto, + private LoggerInterface $logger, private IRequest $request) { } @@ -44,16 +47,20 @@ public function create_sid(): string { public function read(string $id): false|string { [$sessionId, $passphrase] = self::parseId($id); if ($passphrase === null) { - return parent::read($sessionId); + $passphrase = $this->request->getCookie(CryptoWrapper::COOKIE_NAME); + if ($passphrase === null) { + $this->logger->debug('Reading unencrypted session data', [ + 'sessionId' => $id, + ]); + return parent::read($sessionId); + } } $encryptedData = parent::read($sessionId); if ($encryptedData === '') { return ''; } - $data = $this->crypto->decrypt($encryptedData, $passphrase); - - return $data; + return $this->crypto->decrypt($encryptedData, $passphrase); } /** @@ -70,6 +77,10 @@ public function write(string $id, string $data): bool { if ($passphrase === null) { $passphrase = $this->request->getCookie(CryptoWrapper::COOKIE_NAME); if ($passphrase === null) { + $this->logger->warning('Can not write session because there is no passphrase', [ + 'sessionId' => $id, + 'dataLength' => strlen($data), + ]); return false; } } @@ -89,8 +100,8 @@ public function close(): bool { * @return array{0: string, 1: ?string} */ public static function parseId(string $id): array { - $parts = explode('|', $id); - return [$parts[0], $parts[1]]; + $parts = explode('|', $id, 2); + return [$parts[0], $parts[1] ?? null]; } } From aee7a7daac748689dbaaad74dae8d2b2fc1ee342 Mon Sep 17 00:00:00 2001 From: Christoph Wurst Date: Wed, 28 Aug 2024 10:13:43 +0200 Subject: [PATCH 4/4] fixup! fix(session): Make session encryption more robust Signed-off-by: Christoph Wurst --- lib/private/Session/CryptoSessionHandler.php | 14 +++++--- lib/private/Session/CryptoWrapper.php | 27 ++++----------- lib/private/Session/Internal.php | 1 + tests/lib/Session/CryptoSessionDataTest.php | 3 ++ .../lib/Session/CryptoSessionHandlerTest.php | 33 +++++++++++++++++++ 5 files changed, 52 insertions(+), 26 deletions(-) create mode 100644 tests/lib/Session/CryptoSessionHandlerTest.php diff --git a/lib/private/Session/CryptoSessionHandler.php b/lib/private/Session/CryptoSessionHandler.php index 838096c20a4fd..5d6b2f43ab955 100644 --- a/lib/private/Session/CryptoSessionHandler.php +++ b/lib/private/Session/CryptoSessionHandler.php @@ -9,7 +9,6 @@ namespace OC\Session; -use Exception; use OCP\IRequest; use OCP\Security\ICrypto; use OCP\Security\ISecureRandom; @@ -17,10 +16,6 @@ use SessionHandler; use function explode; use function implode; -use function json_decode; -use function OCP\Log\logger; -use function session_decode; -use function session_encode; use function strlen; class CryptoSessionHandler extends SessionHandler { @@ -43,6 +38,8 @@ public function create_sid(): string { * @param string $id * * @return false|string + * + * @codeCoverageIgnore */ public function read(string $id): false|string { [$sessionId, $passphrase] = self::parseId($id); @@ -70,6 +67,8 @@ public function read(string $id): false|string { * @param string $data * * @return bool + * + * @codeCoverageIgnore */ public function write(string $id, string $data): bool { [$sessionId, $passphrase] = self::parseId($id); @@ -90,6 +89,11 @@ public function write(string $id, string $data): bool { return parent::write($sessionId, $encryptedData); } + /** + * @return bool + * + * @codeCoverageIgnore + */ public function close(): bool { return parent::close(); } diff --git a/lib/private/Session/CryptoWrapper.php b/lib/private/Session/CryptoWrapper.php index 3d538a8dc6aa8..e69c09764b0c6 100644 --- a/lib/private/Session/CryptoWrapper.php +++ b/lib/private/Session/CryptoWrapper.php @@ -1,5 +1,7 @@ passphrase = $request->getCookie(self::COOKIE_NAME); } else { $this->passphrase = $this->random->generate(128); - $secureCookie = $request->getServerProtocol() === 'https'; - // FIXME: Required for CI - if (!defined('PHPUNIT_RUN')) { - $webRoot = \OC::$WEBROOT; - if ($webRoot === '') { - $webRoot = '/'; - } - - setcookie( - self::COOKIE_NAME, - $this->passphrase, - [ - 'expires' => 0, - 'path' => $webRoot, - 'domain' => '', - 'secure' => $secureCookie, - 'httponly' => true, - 'samesite' => 'Lax', - ] - ); - } } } /** * @param ISession $session * @return ISession + * @deprecated 31.0.0 */ public function wrapSession(ISession $session) { if (!($session instanceof LegacyCryptoSessionData)) { diff --git a/lib/private/Session/Internal.php b/lib/private/Session/Internal.php index a6c7a83f47eb0..afc24a1e913e7 100644 --- a/lib/private/Session/Internal.php +++ b/lib/private/Session/Internal.php @@ -11,6 +11,7 @@ use OC\Authentication\Token\IProvider; use OCP\Authentication\Exceptions\InvalidTokenException; +use OCP\IConfig; use OCP\ILogger; use OCP\Session\Exceptions\SessionNotAvailableException; use Psr\Log\LoggerInterface; diff --git a/tests/lib/Session/CryptoSessionDataTest.php b/tests/lib/Session/CryptoSessionDataTest.php index 4d95ffe2f0229..80e1aa967c687 100644 --- a/tests/lib/Session/CryptoSessionDataTest.php +++ b/tests/lib/Session/CryptoSessionDataTest.php @@ -1,4 +1,7 @@