Handle failures gracefully, remove switch

10 years ago · 6a3fb0d3b3
4 changed files with 101 additions and 25 deletions
--- a/lib/private/server.php
+++ b/lib/private/server.php
@ -40,6 +40,7 @@ use bantu\IniGetWrapper\IniGetWrapper;
 use OC\AppFramework\Http\Request;
 use OC\AppFramework\Db\Db;
 use OC\AppFramework\Utility\SimpleContainer;
+use OC\AppFramework\Utility\TimeFactory;
 use OC\Command\AsyncBus;
 use OC\Diagnostics\EventLogger;
 use OC\Diagnostics\NullEventLogger;
@ -468,10 +469,28 @@ class Server extends SimpleContainer implements IServerContainer {
 			return new EventDispatcher();
 		});
 		$this->registerService('CryptoWrapper', function (Server $c) {
+			// FIXME: Instantiiated here due to cyclic dependency
+			$request = new Request(
+				[
+					'get' => $_GET,
+					'post' => $_POST,
+					'files' => $_FILES,
+					'server' => $_SERVER,
+					'env' => $_ENV,
+					'cookies' => $_COOKIE,
+					'method' => (isset($_SERVER) && isset($_SERVER['REQUEST_METHOD']))
+						? $_SERVER['REQUEST_METHOD']
+						: null,
+				],
+				new SecureRandom(),
+				$c->getConfig()
+			);
+
 			return new CryptoWrapper(
 				$c->getConfig(),
 				$c->getCrypto(),
-				$c->getSecureRandom()
+				$c->getSecureRandom(),
+				$request
 			);
 		});
 	}
@ -962,4 +981,38 @@ class Server extends SimpleContainer implements IServerContainer {
 		return $this->query('MountManager');
 	}

+	/*
+	 * Get the MimeTypeDetector
+	 *
+	 * @return \OCP\Files\IMimeTypeDetector
+	 */
+	public function getMimeTypeDetector() {
+		return $this->query('MimeTypeDetector');
+	}
+
+	/**
+	 * Get the manager of all the capabilities
+	 *
+	 * @return \OC\CapabilitiesManager
+	 */
+	public function getCapabilitiesManager() {
+		return $this->query('CapabilitiesManager');
+	}
+
+	/**
+	 * Get the EventDispatcher
+	 *
+	 * @return EventDispatcherInterface
+	 * @since 8.2.0
+	 */
+	public function getEventDispatcher() {
+		return $this->query('EventDispatcher');
+	}
+
+	/**
+	 * @return \OC\Session\CryptoWrapper
+	 */
+	public function getSessionCryptoWrapper() {
+		return $this->query('CryptoWrapper');
+	}
 }
--- 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;
+		}
 	}

 	/**
--- 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;
--- a/tests/lib/session/cryptowrappingtest.php
+++ b/tests/lib/session/cryptowrappingtest.php
@ -46,7 +46,7 @@ class CryptoWrappingTest extends TestCase {
 		$this->crypto->expects($this->any())
 			->method('encrypt')
 			->willReturnCallback(function ($input) {
-				return '#' . $input . '#';
+				return $input;
 			});
 		$this->crypto->expects($this->any())
 			->method('decrypt')
@ -62,7 +62,7 @@ class CryptoWrappingTest extends TestCase {

 		$this->wrappedSession->expects($this->once())
 			->method('set')
-			->with('key', $this->crypto->encrypt($unencryptedValue));
+			->with('key', $this->crypto->encrypt(json_encode($unencryptedValue)));
 		$this->instance->set('key', $unencryptedValue);
 	}

@ -76,6 +76,7 @@ class CryptoWrappingTest extends TestCase {
 			->willReturnCallback(function () use ($encryptedValue) {
 				return $encryptedValue;
 			});
-		$this->assertSame($unencryptedValue, $this->instance->get('key'));
+
+		$this->assertSame($unencryptedValue, $this->wrappedSession->get('key'));
 	}
 }