Skip to content

Commit

Permalink
Handle failures gracefully, remove switch
Browse files Browse the repository at this point in the history
  • Loading branch information
LukasReschke committed Aug 21, 2015
1 parent 36eef2d commit 6a3fb0d
Show file tree
Hide file tree
Showing 4 changed files with 101 additions and 25 deletions.
55 changes: 54 additions & 1 deletion lib/private/server.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
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;
Expand Down Expand Up @@ -468,10 +469,28 @@ public function __construct($webRoot) {
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
);
});
}
Expand Down Expand Up @@ -962,4 +981,38 @@ function getMountManager() {
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');
}
}
19 changes: 13 additions & 6 deletions lib/private/session/cryptosessiondata.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -53,25 +57,28 @@ public function __construct(ISession $session, ICrypto $crypto, $passphrase) {
* @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);
}

/**
* 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);
if ($encryptedValue === null) {
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;
}
}

/**
Expand Down
45 changes: 30 additions & 15 deletions lib/private/session/cryptowrapper.php
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -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);
}
}
}
Expand All @@ -72,8 +87,8 @@ public function __construct(IConfig $config, ICrypto $crypto, ISecureRandom $ran
* @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;
Expand Down
7 changes: 4 additions & 3 deletions tests/lib/session/cryptowrappingtest.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ protected function setUp() {
$this->crypto->expects($this->any())
->method('encrypt')
->willReturnCallback(function ($input) {
return '#' . $input . '#';
return $input;
});
$this->crypto->expects($this->any())
->method('decrypt')
Expand All @@ -62,7 +62,7 @@ public function testWrappingSet() {

$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);
}

Expand All @@ -76,6 +76,7 @@ public function testUnwrappingGet() {
->willReturnCallback(function () use ($encryptedValue) {
return $encryptedValue;
});
$this->assertSame($unencryptedValue, $this->instance->get('key'));

$this->assertSame($unencryptedValue, $this->wrappedSession->get('key'));
}
}

0 comments on commit 6a3fb0d

Please sign in to comment.