Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion cron.php
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,10 @@
\OC::$server->getSession()->close();

// initialize a dummy memory session
\OC::$server->setSession(new \OC\Session\Memory(''));
$session = new \OC\Session\Memory('');
$cryptoWrapper = \OC::$server->getSessionCryptoWrapper();
$session = $cryptoWrapper->wrapSession($session);
\OC::$server->setSession($session);

$logger = \OC::$server->getLogger();

Expand Down
12 changes: 7 additions & 5 deletions lib/base.php
Original file line number Diff line number Diff line change
Expand Up @@ -448,13 +448,15 @@ public static function initSession() {
$useCustomSession = false;
$session = self::$server->getSession();
OC_Hook::emit('OC', 'initSession', array('session' => &$session, 'sessionName' => &$sessionName, 'useCustomSession' => &$useCustomSession));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nickvergessen I'd like to see this implementation reusing this initSession hook to setup this special encrypted session handler - if possible - THX

Well the question is shall this be on top of the existing session, or a replacement. When we use the hook, what should happen, if another app already defined a session, or if another app that is loaded later, defines a session (and doesn't wrap it)?

If we scrap the config, doing it after the hook sounds better. The other idea would be to init the session, before the hook, then the wrapper can just wrap it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if($useCustomSession) {
// use the session reference as the new Session
self::$server->setSession($session);
} else {
if (!$useCustomSession) {
// set the session name to the instance id - which is unique
self::$server->setSession(new \OC\Session\Internal($sessionName));
$session = new \OC\Session\Internal($sessionName);
}

$cryptoWrapper = \OC::$server->getSessionCryptoWrapper();
$session = $cryptoWrapper->wrapSession($session);
self::$server->setSession($session);

// if session cant be started break with http 500 error
} catch (Exception $e) {
\OCP\Util::logException('base', $e);
Expand Down
22 changes: 21 additions & 1 deletion lib/private/server.php
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
use OC\Security\Hasher;
use OC\Security\SecureRandom;
use OC\Security\TrustedDomainHelper;
use OC\Session\CryptoWrapper;
use OC\Tagging\TagMapper;
use OCP\IServerContainer;

Expand Down Expand Up @@ -156,7 +157,12 @@ public function __construct($webRoot) {
});
$this->registerService('UserSession', function (Server $c) {
$manager = $c->getUserManager();
$userSession = new \OC\User\Session($manager, new \OC\Session\Memory(''));

$session = new \OC\Session\Memory('');
$cryptoWrapper = $c->getSessionCryptoWrapper();
$session = $cryptoWrapper->wrapSession($session);

$userSession = new \OC\User\Session($manager, $session);
$userSession->listen('\OC\User', 'preCreateUser', function ($uid, $password) {
\OC_Hook::emit('OC_User', 'pre_createUser', array('run' => true, 'uid' => $uid, 'password' => $password));
});
Expand Down Expand Up @@ -443,6 +449,13 @@ public function __construct($webRoot) {
$this->registerService('MountManager', function () {
return new \OC\Files\Mount\Manager();
});
$this->registerService('CryptoWrapper', function (Server $c) {
return new CryptoWrapper(
$c->getConfig(),
$c->getCrypto(),
$c->getSecureRandom()
);
});
}

/**
Expand Down Expand Up @@ -930,4 +943,11 @@ public function getLockingProvider() {
function getMountManager() {
return $this->query('MountManager');
}

/**
* @return \OC\Session\CryptoWrapper
*/
public function getSessionCryptoWrapper() {
return $this->query('CryptoWrapper');
}
}
140 changes: 140 additions & 0 deletions lib/private/session/cryptosessiondata.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
<?php
/**
* @author Joas Schilling <[email protected]>
*
* @copyright Copyright (c) 2015, ownCloud, Inc.
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/

namespace OC\Session;


use OCP\ISession;
use OCP\Security\ICrypto;

class CryptoSessionData implements \ArrayAccess, ISession {
/** @var ISession */
protected $session;

/** @var \OCP\Security\ICrypto */
protected $crypto;

/** @var string */
protected $passphrase;

/**
* @param ISession $session
* @param ICrypto $crypto
* @param string $passphrase
*/
public function __construct(ISession $session, ICrypto $crypto, $passphrase) {
$this->crypto = $crypto;
$this->session = $session;
$this->passphrase = $passphrase;
}

/**
* Set a value in the session
*
* @param string $key
* @param mixed $value
*/
public function set($key, $value) {
$encryptedValue = $this->crypto->encrypt($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
*/
public function get($key) {
$encryptedValue = $this->session->get($key);
if ($encryptedValue === null) {
return null;
}

$value = $this->crypto->decrypt($encryptedValue, $this->passphrase);
return $value;
}

/**
* Check if a named key exists in the session
*
* @param string $key
* @return bool
*/
public function exists($key) {
return $this->session->exists($key);
}

/**
* Remove a $key/$value pair from the session
*
* @param string $key
*/
public function remove($key) {
$this->session->remove($key);
}

/**
* Reset and recreate the session
*/
public function clear() {
$this->session->clear();
}

/**
* Close the session and release the lock
*/
public function close() {
$this->session->close();
}

/**
* @param mixed $offset
* @return bool
*/
public function offsetExists($offset) {
return $this->exists($offset);
}

/**
* @param mixed $offset
* @return mixed
*/
public function offsetGet($offset) {
return $this->get($offset);
}

/**
* @param mixed $offset
* @param mixed $value
*/
public function offsetSet($offset, $value) {
$this->set($offset, $value);
}

/**
* @param mixed $offset
*/
public function offsetUnset($offset) {
$this->remove($offset);
}
}
81 changes: 81 additions & 0 deletions lib/private/session/cryptowrapper.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
<?php
/**
* @author Joas Schilling <[email protected]>
*
* @copyright Copyright (c) 2015, ownCloud, Inc.
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/

namespace OC\Session;

use OCP\IConfig;
use OCP\ISession;
use OCP\Security\ICrypto;
use OCP\Security\ISecureRandom;

class CryptoWrapper {
const COOKIE_NAME = 'oc_sessionPassphrase';

/** @var ISession */
protected $session;

/** @var \OCP\Security\ICrypto */
protected $crypto;

/** @var ISecureRandom */
protected $random;

/**
* @param IConfig $config
* @param ICrypto $crypto
* @param ISecureRandom $random
*/
public function __construct(IConfig $config, ICrypto $crypto, ISecureRandom $random) {
$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];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copied to keep the discussion from @LukasReschke

This will make everything explode if a client does not resend the cookies - are we all REALLY aware of this?
This WILL break stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executable samples would be nice, so I can test this.

} 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these occurrences of time() in code makes me wonder if we should add some mockable replacement globally? Anyways, other topic. If this is something we might want to have somebody with more knowledge shall file an issue 🙊

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just create a function to set a cookie, which is fed with the seconds until expire, instead of doing it manually all the time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a time factory in appframework


if (!defined('PHPUNIT_RUN')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting a cookie makes the tests fail somehow

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙊 - add a todo?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or fixme

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or try to fix before merge ;)

setcookie(self::COOKIE_NAME, $this->passphrase, $expires, \OC::$WEBROOT, '', $secureCookie);
}
}
}

/**
* @param ISession $session
* @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);
}

return $session;
}
}
1 change: 1 addition & 0 deletions tests/lib/server.php
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ public function dataTestQuery() {
['ContactsManager', '\OCP\Contacts\IManager'],
['Crypto', '\OC\Security\Crypto'],
['Crypto', '\OCP\Security\ICrypto'],
['CryptoWrapper', '\OC\Session\CryptoWrapper'],

['DatabaseConnection', '\OC\DB\Connection'],
['DatabaseConnection', '\OCP\IDBConnection'],
Expand Down
53 changes: 53 additions & 0 deletions tests/lib/session/cryptosessiondatatest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<?php
/**
* @author Joas Schilling <[email protected]>
*
* @copyright Copyright (c) 2015, ownCloud, Inc.
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/

namespace Test\Session;

use OC\Session\CryptoSessionData;

class CryptoSessionDataTest extends Session {
/** @var \PHPUnit_Framework_MockObject_MockObject|\OCP\Security\ICrypto */
protected $crypto;

/** @var \OCP\ISession */
protected $wrappedSession;

protected function setUp() {
parent::setUp();

$this->wrappedSession = new \OC\Session\Memory($this->getUniqueID());
$this->crypto = $this->getMockBuilder('OCP\Security\ICrypto')
->disableOriginalConstructor()
->getMock();
$this->crypto->expects($this->any())
->method('encrypt')
->willReturnCallback(function ($input) {
return '#' . $input . '#';
});
$this->crypto->expects($this->any())
->method('decrypt')
->willReturnCallback(function ($input) {
return substr($input, 1, -1);
});

$this->instance = new CryptoSessionData($this->wrappedSession, $this->crypto, 'PASS');
}
}
Loading