fix(session): Make session encryption more robust

[skipci]

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>

lib/base.php

@@ -7,6 +7,7 @@ declare(strict_types=1);
  * 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 @@ class OC {
 	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 @@ class OC {
 		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();

lib/composer/composer/autoload_classmap.php

@@ -1871,9 +1871,10 @@ return array(
     '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',

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',

lib/private/Session/CryptoSessionHandler.php

@@ -0,0 +1,144 @@
+<?php
+
+declare(strict_types=1);
+
+/**
+ * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-only
+ */
+
+namespace OC\Session;
+
+use Exception;
+use OCP\IRequest;
+use OCP\Security\ICrypto;
+use OCP\Security\ISecureRandom;
+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;
+
+class CryptoSessionHandler extends SessionHandler {
+
+	public function __construct(private ISecureRandom $secureRandom,
+		private ICrypto $crypto,
+		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);
+		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();
+	}
+
+}

lib/private/Session/CryptoWrapper.php

@@ -90,8 +90,8 @@ class CryptoWrapper {
 	 * @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;

lib/private/Session/LegacyCryptoSessionData.php

@@ -15,28 +15,24 @@ use function json_decode;
 use function OCP\Log\logger;
 
 /**
- * Class CryptoSessionData
- *
  * @package OC\Session
  * @template-implements \ArrayAccess<string,mixed>
+ * @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 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 
 	/**
 	 * Close session if class gets destructed
+	 * @deprecated 31.0.0
 	 */
 	public function __destruct() {
 		try {
@@ -59,26 +56,31 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 		}
 	}
 
+	/**
+	 * @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 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 	 *
 	 * @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 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 	 *
 	 * @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 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 	 *
 	 * @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 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 	 * @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 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 	 * @return string
 	 * @throws SessionNotAvailableException
 	 * @since 9.1.0
+	 * @deprecated 31.0.0
 	 */
 	public function getId(): string {
 		return $this->session->getId();
@@ -189,19 +176,16 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 
 	/**
 	 * 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 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 	/**
 	 * @param mixed $offset
 	 * @return mixed
+	 * @deprecated 31.0.0
 	 */
 	#[\ReturnTypeWillChange]
 	public function offsetGet($offset) {
@@ -219,6 +204,7 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 	/**
 	 * @param mixed $offset
 	 * @param mixed $value
+	 * @deprecated 31.0.0
 	 */
 	public function offsetSet($offset, $value): void {
 		$this->set($offset, $value);
@@ -226,6 +212,7 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 
 	/**
 	 * @param mixed $offset
+	 * @deprecated 31.0.0
 	 */
 	public function offsetUnset($offset): void {
 		$this->remove($offset);

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 @@ class CryptoSessionDataTest extends Session {
 				return substr($input, 1, -1);
 			});
 
-		$this->instance = new CryptoSessionData($this->wrappedSession, $this->crypto, 'PASS');
+		$this->instance = new LegacyCryptoSessionData($this->wrappedSession, $this->crypto, 'PASS');
 	}
 }

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 @@ class CryptoWrappingTest extends TestCase {
 				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() {