diff --git a/lib/base.php b/lib/base.php index b0334ecd729de..2873d4c5a9129 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; @@ -363,6 +364,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); + // Do not initialize sessions for 'status.php' requests // Monitoring endpoints can quickly flood session handlers // and 'status.php' doesn't require sessions anyway diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index e515e3eff07e4..35fabcb3d7c4d 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1946,9 +1946,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 cf2883c3070c5..c0ca08ce18eda 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1987,9 +1987,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..168a7eaebba75 --- /dev/null +++ b/lib/private/Session/CryptoSessionHandler.php @@ -0,0 +1,124 @@ +secureRandom->generate(128); + return implode('|', [$id, $passphrase]); + } + + /** + * Read and decrypt session data + * + * @param string $id + * + * @return false|string + * + * @codeCoverageIgnore + */ + public function read(string $id): false|string { + [$sessionId, $passphrase] = self::parseId($id); + if ($passphrase === null) { + $passphrase = $this->request->getCookie(CryptoWrapper::COOKIE_NAME); + if ($passphrase === null) { + $this->logger->debug('Reading unencrypted session data', [ + 'sessionId' => $id, + ]); + return parent::read($sessionId); + } + } + + $possiblyEncryptedData = parent::read($sessionId); + if ($possiblyEncryptedData === '') { + return ''; + } + if (str_contains($possiblyEncryptedData, 'encrypted_session_data')) { + // This data is not encrypted + return $possiblyEncryptedData; + } + try { + return $this->crypto->decrypt($possiblyEncryptedData, $passphrase); + } catch (Exception $e) { + $this->logger->error('Failed to decrypt session data', [ + 'exception' => $e, + 'sessionId' => $sessionId, + ]); + return ''; + } + } + + /** + * Encrypt and write session data + * + * @param string $id + * @param string $data + * + * @return bool + * + * @codeCoverageIgnore + */ + public function write(string $id, string $data): bool { + [$sessionId, $passphrase] = self::parseId($id); + + 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; + } + } + + $encryptedData = $this->crypto->encrypt($data, $passphrase); + + return parent::write($sessionId, $encryptedData); + } + + /** + * @return bool + * + * @codeCoverageIgnore + */ + public function close(): bool { + return parent::close(); + } + + /** + * @param string $id + * + * @return array{0: string, 1: ?string} + */ + public static function parseId(string $id): array { + $parts = explode('|', $id, 2); + return [$parts[0], $parts[1] ?? null]; + } + +} diff --git a/lib/private/Session/CryptoWrapper.php b/lib/private/Session/CryptoWrapper.php index aceb387ea741f..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 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/Internal.php b/lib/private/Session/Internal.php index b465bcd3edadb..d342244576ec2 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; @@ -82,18 +83,23 @@ public function exists(string $key): bool { * @param string $key */ public function remove(string $key) { - if (isset($_SESSION[$key])) { - unset($_SESSION[$key]); + $reopened = $this->reopen(); + unset($_SESSION[$key]); + if ($reopened) { + $this->close(); } } public function clear() { - $this->reopen(); + $reopened = $this->reopen(); $this->invoke('session_unset'); $this->regenerateId(); $this->invoke('session_write_close'); $this->startSession(true); $_SESSION = []; + if ($reopened) { + $this->close(); + } } public function close() { @@ -155,7 +161,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/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..bd8ac940a106c 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 legacy 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..80e1aa967c687 100644 --- a/tests/lib/Session/CryptoSessionDataTest.php +++ b/tests/lib/Session/CryptoSessionDataTest.php @@ -1,4 +1,7 @@ instance = new CryptoSessionData($this->wrappedSession, $this->crypto, 'PASS'); + $this->instance = new LegacyCryptoSessionData($this->wrappedSession, $this->crypto, 'PASS'); } } diff --git a/tests/lib/Session/CryptoSessionHandlerTest.php b/tests/lib/Session/CryptoSessionHandlerTest.php new file mode 100644 index 0000000000000..af4adaecb2605 --- /dev/null +++ b/tests/lib/Session/CryptoSessionHandlerTest.php @@ -0,0 +1,33 @@ +instance = new CryptoSessionData($this->wrappedSession, $this->crypto, 'PASS'); + $this->instance = new LegacyCryptoSessionData($this->wrappedSession, $this->crypto, 'PASS'); } public function testUnwrappingGet(): void {