fix(session): Make session encryption more robust

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

lib/composer/composer/autoload_classmap.php

@@ -1963,9 +1963,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',

lib/composer/composer/autoload_static.php

@@ -2004,9 +2004,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,124 @@
+<?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 Psr\Log\LoggerInterface;
+use SessionHandler;
+use function explode;
+use function implode;
+use function strlen;
+
+class CryptoSessionHandler extends SessionHandler {
+
+	public function __construct(private ISecureRandom $secureRandom,
+		private ICrypto $crypto,
+		private LoggerInterface $logger,
+		private IRequest $request) {
+	}
+
+	public function create_sid(): string {
+		$id = parent::create_sid();
+		$passphrase = $this->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];
+	}
+
+}

lib/private/Session/CryptoWrapper.php

@@ -1,5 +1,7 @@
 <?php
 
+declare(strict_types=1);
+
 /**
  * SPDX-FileCopyrightText: 2016-2024 Nextcloud GmbH and Nextcloud contributors
  * SPDX-FileCopyrightText: 2016 ownCloud, Inc.
@@ -28,8 +30,10 @@
  * https://github.com/owncloud/core/pull/17866
  *
  * @package OC\Session
+ * @deprecated
  */
 class CryptoWrapper {
+	/** @deprecated 31.0.0 */
 	public const COOKIE_NAME = 'oc_sessionPassphrase';
 
 	/** @var IConfig */
@@ -48,6 +52,7 @@ class CryptoWrapper {
 	 * @param ICrypto $crypto
 	 * @param ISecureRandom $random
 	 * @param IRequest $request
+	 * @depreacted 31.0.0
 	 */
 	public function __construct(IConfig $config,
 		ICrypto $crypto,
@@ -61,37 +66,17 @@ public function __construct(IConfig $config,
 			$this->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;

lib/private/Session/Internal.php

@@ -11,10 +11,15 @@
 
 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;
 use function call_user_func_array;
+use function is_array;
+use function is_object;
+use function json_decode;
+use function json_encode;
 use function microtime;
 
 /**
@@ -49,11 +54,20 @@ public function __construct(
 
 	/**
 	 * @param string $key
-	 * @param integer $value
+	 * @param mixed $value
 	 */
 	public function set(string $key, $value) {
 		$reopened = $this->reopen();
-		$_SESSION[$key] = $value;
+
+		// The previous mechanism for session encryption json-encoded all values,
+		// which implicitly led to objects convert to arrays or objects if they
+		// implement (json) serializable interfaces.
+		$normalized = match (is_array($value) || is_object($value)) {
+			true => json_decode(json_encode($value, JSON_THROW_ON_ERROR), true, 512, JSON_THROW_ON_ERROR),
+			false => $value,
+		};
+
+		$_SESSION[$key] = $normalized;
 		if ($reopened) {
 			$this->close();
 		}
@@ -82,18 +96,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 +174,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];
 	}
 
 	/**