From bfa26db4727cf5cf9a26754b82e65a38ffc98bbd Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Tue, 8 Sep 2015 21:15:08 +0200 Subject: [PATCH 1/3] Use md5 over the version file to prevent cyclyc dependency --- lib/private/server.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/private/server.php b/lib/private/server.php index 393c1840973..346761840d6 100644 --- a/lib/private/server.php +++ b/lib/private/server.php @@ -177,8 +177,6 @@ class Server extends SimpleContainer implements IServerContainer { $manager = $c->getUserManager(); $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) { @@ -252,7 +250,7 @@ class Server extends SimpleContainer implements IServerContainer { if($config->getSystemValue('installed', false) && !(defined('PHPUNIT_RUN') && PHPUNIT_RUN)) { $v = \OC_App::getAppVersions(); - $v['core'] = implode('.', \OC_Util::getVersion()); + $v['core'] = md5(file_get_contents(\OC::$SERVERROOT . '/version.php')); $version = implode(',', $v); $instanceId = \OC_Util::getInstanceId(); $path = \OC::$SERVERROOT; From e579dd62fd4d4622766ecf60473dba41d3fbed9b Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Tue, 8 Sep 2015 21:16:11 +0200 Subject: [PATCH 2/3] Write session data to single key This prevents decrypting values multiple times. --- lib/private/session/cryptosessiondata.php | 46 +++++++++++++++-------- tests/lib/session/cryptowrappingtest.php | 8 ++-- 2 files changed, 34 insertions(+), 20 deletions(-) diff --git a/lib/private/session/cryptosessiondata.php b/lib/private/session/cryptosessiondata.php index 60d22b25e97..a0d180757bc 100644 --- a/lib/private/session/cryptosessiondata.php +++ b/lib/private/session/cryptosessiondata.php @@ -32,22 +32,38 @@ use OCP\Security\ICrypto; class CryptoSessionData implements \ArrayAccess, ISession { /** @var ISession */ protected $session; - /** @var \OCP\Security\ICrypto */ protected $crypto; - /** @var string */ protected $passphrase; + /** @var array */ + protected $sessionValues; + CONST encryptedSessionName = 'encrypted_session_data'; /** * @param ISession $session * @param ICrypto $crypto * @param string $passphrase */ - public function __construct(ISession $session, ICrypto $crypto, $passphrase) { + public function __construct(ISession $session, + ICrypto $crypto, + $passphrase) { $this->crypto = $crypto; $this->session = $session; $this->passphrase = $passphrase; + $this->initializeSession(); + } + + protected function initializeSession() { + $encryptedSessionData = $this->session->get(self::encryptedSessionName); + try { + $this->sessionValues = json_decode( + $this->crypto->decrypt($encryptedSessionData, $this->passphrase), + true + ); + } catch (\Exception $e) { + $this->sessionValues = []; + } } /** @@ -57,8 +73,9 @@ class CryptoSessionData implements \ArrayAccess, ISession { * @param mixed $value */ public function set($key, $value) { - $encryptedValue = $this->crypto->encrypt(json_encode($value), $this->passphrase); - $this->session->set($key, $encryptedValue); + $this->sessionValues[$key] = $value; + $encryptedValue = $this->crypto->encrypt(json_encode($this->sessionValues), $this->passphrase); + $this->session->set(self::encryptedSessionName, $encryptedValue); } /** @@ -68,17 +85,12 @@ class CryptoSessionData implements \ArrayAccess, ISession { * @return string|null Either the value or null */ public function get($key) { - $encryptedValue = $this->session->get($key); - if ($encryptedValue === null) { - return null; + + if(isset($this->sessionValues[$key])) { + return $this->sessionValues[$key]; } - try { - $value = $this->crypto->decrypt($encryptedValue, $this->passphrase); - return json_decode($value); - } catch (\Exception $e) { - return null; - } + return null; } /** @@ -88,7 +100,7 @@ class CryptoSessionData implements \ArrayAccess, ISession { * @return bool */ public function exists($key) { - return $this->session->exists($key); + return isset($this->sessionValues[$key]); } /** @@ -97,13 +109,15 @@ class CryptoSessionData implements \ArrayAccess, ISession { * @param string $key */ public function remove($key) { - $this->session->remove($key); + unset($this->sessionValues[$key]); + $this->session->remove(self::encryptedSessionName); } /** * Reset and recreate the session */ public function clear() { + $this->sessionValues = []; $this->session->clear(); } diff --git a/tests/lib/session/cryptowrappingtest.php b/tests/lib/session/cryptowrappingtest.php index 12b3c905b7f..1cbe60066fe 100644 --- a/tests/lib/session/cryptowrappingtest.php +++ b/tests/lib/session/cryptowrappingtest.php @@ -62,8 +62,8 @@ class CryptoWrappingTest extends TestCase { $this->wrappedSession->expects($this->once()) ->method('set') - ->with('key', $this->crypto->encrypt(json_encode($unencryptedValue))); - $this->instance->set('key', $unencryptedValue); + ->with('encrypted_session_data', $this->crypto->encrypt(json_encode(['encrypted_session_data' => $unencryptedValue]))); + $this->instance->set('encrypted_session_data', $unencryptedValue); } public function testUnwrappingGet() { @@ -72,11 +72,11 @@ class CryptoWrappingTest extends TestCase { $this->wrappedSession->expects($this->once()) ->method('get') - ->with('key') + ->with('encrypted_session_data') ->willReturnCallback(function () use ($encryptedValue) { return $encryptedValue; }); - $this->assertSame($unencryptedValue, $this->wrappedSession->get('key')); + $this->assertSame($unencryptedValue, $this->wrappedSession->get('encrypted_session_data')); } } From 0b910874899b3c7cd839e8b0a0a8416e44e348bc Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Tue, 8 Sep 2015 22:05:36 +0200 Subject: [PATCH 3/3] Write to session in batch at the end of the request --- lib/private/session/cryptosessiondata.php | 22 ++++++++++++++++++---- lib/private/session/internal.php | 8 ++++---- tests/lib/session/cryptowrappingtest.php | 9 --------- 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/lib/private/session/cryptosessiondata.php b/lib/private/session/cryptosessiondata.php index a0d180757bc..6826ede5e33 100644 --- a/lib/private/session/cryptosessiondata.php +++ b/lib/private/session/cryptosessiondata.php @@ -38,6 +38,8 @@ class CryptoSessionData implements \ArrayAccess, ISession { protected $passphrase; /** @var array */ protected $sessionValues; + /** @var bool */ + protected $isModified = false; CONST encryptedSessionName = 'encrypted_session_data'; /** @@ -54,6 +56,13 @@ class CryptoSessionData implements \ArrayAccess, ISession { $this->initializeSession(); } + /** + * Close session if class gets destructed + */ + public function __destruct() { + $this->close(); + } + protected function initializeSession() { $encryptedSessionData = $this->session->get(self::encryptedSessionName); try { @@ -74,8 +83,7 @@ class CryptoSessionData implements \ArrayAccess, ISession { */ public function set($key, $value) { $this->sessionValues[$key] = $value; - $encryptedValue = $this->crypto->encrypt(json_encode($this->sessionValues), $this->passphrase); - $this->session->set(self::encryptedSessionName, $encryptedValue); + $this->isModified = true; } /** @@ -85,7 +93,6 @@ class CryptoSessionData implements \ArrayAccess, ISession { * @return string|null Either the value or null */ public function get($key) { - if(isset($this->sessionValues[$key])) { return $this->sessionValues[$key]; } @@ -109,6 +116,7 @@ class CryptoSessionData implements \ArrayAccess, ISession { * @param string $key */ public function remove($key) { + $this->isModified = true; unset($this->sessionValues[$key]); $this->session->remove(self::encryptedSessionName); } @@ -118,13 +126,19 @@ class CryptoSessionData implements \ArrayAccess, ISession { */ public function clear() { $this->sessionValues = []; + $this->isModified = true; $this->session->clear(); } /** - * Close the session and release the lock + * Close the session and release the lock, also writes all changed data in batch */ public function close() { + if($this->isModified) { + $encryptedValue = $this->crypto->encrypt(json_encode($this->sessionValues), $this->passphrase); + $this->session->set(self::encryptedSessionName, $encryptedValue); + $this->isModified = false; + } $this->session->close(); } diff --git a/lib/private/session/internal.php b/lib/private/session/internal.php index 77197887754..8ee21272104 100644 --- a/lib/private/session/internal.php +++ b/lib/private/session/internal.php @@ -32,6 +32,10 @@ namespace OC\Session; * @package OC\Session */ class Internal extends Session { + /** + * @param string $name + * @throws \Exception + */ public function __construct($name) { session_name($name); set_error_handler(array($this, 'trapError')); @@ -42,10 +46,6 @@ class Internal extends Session { } } - public function __destruct() { - $this->close(); - } - /** * @param string $key * @param integer $value diff --git a/tests/lib/session/cryptowrappingtest.php b/tests/lib/session/cryptowrappingtest.php index 1cbe60066fe..e1fadbf933f 100644 --- a/tests/lib/session/cryptowrappingtest.php +++ b/tests/lib/session/cryptowrappingtest.php @@ -57,15 +57,6 @@ class CryptoWrappingTest extends TestCase { $this->instance = new CryptoSessionData($this->wrappedSession, $this->crypto, 'PASS'); } - public function testWrappingSet() { - $unencryptedValue = 'foobar'; - - $this->wrappedSession->expects($this->once()) - ->method('set') - ->with('encrypted_session_data', $this->crypto->encrypt(json_encode(['encrypted_session_data' => $unencryptedValue]))); - $this->instance->set('encrypted_session_data', $unencryptedValue); - } - public function testUnwrappingGet() { $unencryptedValue = 'foobar'; $encryptedValue = $this->crypto->encrypt($unencryptedValue);