Handle failures gracefully, remove switch

2 files changed, 43 insertions, 21 deletions

diff --git a/lib/private/session/cryptosessiondata.php b/lib/private/session/cryptosessiondata.php
index 9a3cd928826..60d22b25e97 100644
--- a/lib/private/session/cryptosessiondata.php
+++ b/lib/private/session/cryptosessiondata.php
@@ -21,10 +21,14 @@
 
 namespace OC\Session;
 
-
 use OCP\ISession;
 use OCP\Security\ICrypto;
 
+/**
+ * Class CryptoSessionData
+ *
+ * @package OC\Session
+ */
 class CryptoSessionData implements \ArrayAccess, ISession {
 	/** @var ISession */
 	protected $session;
@@ -53,7 +57,7 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 	 * @param mixed $value
 	 */
 	public function set($key, $value) {
-		$encryptedValue = $this->crypto->encrypt($value, $this->passphrase);
+		$encryptedValue = $this->crypto->encrypt(json_encode($value), $this->passphrase);
 		$this->session->set($key, $encryptedValue);
 	}
 
@@ -61,8 +65,7 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 	 * Get a value from the session
 	 *
 	 * @param string $key
-	 * @return mixed should return null if $key does not exist
-	 * @throws \Exception when the data could not be decrypted
+	 * @return string|null Either the value or null
 	 */
 	public function get($key) {
 		$encryptedValue = $this->session->get($key);
@@ -70,8 +73,12 @@ class CryptoSessionData implements \ArrayAccess, ISession {
 			return null;
 		}
 
-		$value = $this->crypto->decrypt($encryptedValue, $this->passphrase);
-		return $value;
+		try {
+			$value = $this->crypto->decrypt($encryptedValue, $this->passphrase);
+			return json_decode($value);
+		} catch (\Exception $e) {
+			return null;
+		}
 	}
 
 	/**
diff --git a/lib/private/session/cryptowrapper.php b/lib/private/session/cryptowrapper.php
index 8bc41ac1242..62bdcbfb719 100644
--- a/lib/private/session/cryptowrapper.php
+++ b/lib/private/session/cryptowrapper.php
@@ -21,11 +21,29 @@
 
 namespace OC\Session;
 
+use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\IConfig;
+use OCP\IRequest;
 use OCP\ISession;
 use OCP\Security\ICrypto;
 use OCP\Security\ISecureRandom;
 
+/**
+ * Class CryptoWrapper provides some rough basic level of additional security by
+ * storing the session data in an encrypted form.
+ *
+ * The content of the session is encrypted using another cookie sent by the browser.
+ * One should note that an adversary with access to the source code or the system
+ * memory is still able to read the original session ID from the users' request.
+ * This thus can not be considered a strong security measure one should consider
+ * it as an additional small security obfuscation layer to comply with compliance
+ * guidelines.
+ *
+ * TODO: Remove this in a future relase with an approach such as
+ * https://github.com/owncloud/core/pull/17866
+ *
+ * @package OC\Session
+ */
 class CryptoWrapper {
 	const COOKIE_NAME = 'oc_sessionPassphrase';
 
@@ -42,27 +60,24 @@ class CryptoWrapper {
 	 * @param IConfig $config
 	 * @param ICrypto $crypto
 	 * @param ISecureRandom $random
+	 * @param IRequest $request
 	 */
-	public function __construct(IConfig $config, ICrypto $crypto, ISecureRandom $random) {
+	public function __construct(IConfig $config,
+								ICrypto $crypto,
+								ISecureRandom $random,
+								IRequest $request) {
 		$this->crypto = $crypto;
 		$this->config = $config;
 		$this->random = $random;
 
-		if (isset($_COOKIE[self::COOKIE_NAME])) {
-			// TODO circular dependency
-//			$request = \OC::$server->getRequest();
-//			$this->passphrase = $request->getCookie(self::COOKIE_NAME);
-			$this->passphrase = $_COOKIE[self::COOKIE_NAME];
+		if (!is_null($request->getCookie(self::COOKIE_NAME))) {
+			$this->passphrase = $request->getCookie(self::COOKIE_NAME);
 		} else {
 			$this->passphrase = $this->random->getMediumStrengthGenerator()->generate(128);
-
-			// TODO circular dependency
-			// $secureCookie = \OC::$server->getRequest()->getServerProtocol() === 'https';
-			$secureCookie = false;
-			$expires = time() + $this->config->getSystemValue('remember_login_cookie_lifetime', 60 * 60 * 24 * 15);
-
+			$secureCookie = $request->getServerProtocol() === 'https';
+			// FIXME: Required for CI
 			if (!defined('PHPUNIT_RUN')) {
-				setcookie(self::COOKIE_NAME, $this->passphrase, $expires, \OC::$WEBROOT, '', $secureCookie);
+				setcookie(self::COOKIE_NAME, $this->passphrase, 0, \OC::$WEBROOT, '', $secureCookie, true);
 			}
 		}
 	}
@@ -72,8 +87,8 @@ class CryptoWrapper {
 	 * @return ISession
 	 */
 	public function wrapSession(ISession $session) {
-		if (!($session instanceof CryptoSessionData) &&  $this->config->getSystemValue('encrypt.session', false)) {
-			return new \OC\Session\CryptoSessionData($session, $this->crypto, $this->passphrase);
+		if (!($session instanceof CryptoSessionData)) {
+			return new CryptoSessionData($session, $this->crypto, $this->passphrase);
 		}
 
 		return $session;