From df428b76ac498110bde0bfec1ad726cf24c21cfa Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Tue, 28 Apr 2015 17:29:10 +0200 Subject: [PATCH 1/3] skip update of encryption keys if file is not encrypted --- apps/encryption/appinfo/application.php | 3 +- apps/encryption/lib/crypto/encryption.php | 45 ++++++++++++----- .../tests/lib/crypto/encryptionTest.php | 49 ++++++++++++++++++- 3 files changed, 81 insertions(+), 16 deletions(-) diff --git a/apps/encryption/appinfo/application.php b/apps/encryption/appinfo/application.php index fa620992c8..0c9dcb76fb 100644 --- a/apps/encryption/appinfo/application.php +++ b/apps/encryption/appinfo/application.php @@ -101,7 +101,8 @@ class Application extends \OCP\AppFramework\App { return new Encryption( $container->query('Crypt'), $container->query('KeyManager'), - $container->query('Util') + $container->query('Util'), + $container->getServer()->getLogger() ); }); } diff --git a/apps/encryption/lib/crypto/encryption.php b/apps/encryption/lib/crypto/encryption.php index 3f29848168..cc61f04ef0 100644 --- a/apps/encryption/lib/crypto/encryption.php +++ b/apps/encryption/lib/crypto/encryption.php @@ -28,6 +28,7 @@ namespace OCA\Encryption\Crypto; use OCA\Encryption\Util; use OCP\Encryption\IEncryptionModule; use OCA\Encryption\KeyManager; +use OCP\ILogger; class Encryption implements IEncryptionModule { @@ -66,16 +67,24 @@ class Encryption implements IEncryptionModule { /** @var Util */ private $util; + /** @var ILogger */ + private $logger; + /** * - * @param \OCA\Encryption\Crypto\Crypt $crypt + * @param Crypt $crypt * @param KeyManager $keyManager * @param Util $util + * @param ILogger $logger */ - public function __construct(Crypt $crypt, KeyManager $keyManager, Util $util) { + public function __construct(Crypt $crypt, + KeyManager $keyManager, + Util $util, + ILogger $logger) { $this->crypt = $crypt; $this->keyManager = $keyManager; $this->util = $util; + $this->logger = $logger; } /** @@ -257,19 +266,29 @@ class Encryption implements IEncryptionModule { */ public function update($path, $uid, array $accessList) { $fileKey = $this->keyManager->getFileKey($path, $uid); - $publicKeys = array(); - foreach ($accessList['users'] as $user) { - $publicKeys[$user] = $this->keyManager->getPublicKey($user); + + if (!empty($fileKey)) { + + $publicKeys = array(); + foreach ($accessList['users'] as $user) { + $publicKeys[$user] = $this->keyManager->getPublicKey($user); + } + + $publicKeys = $this->keyManager->addSystemKeys($accessList, $publicKeys); + + $encryptedFileKey = $this->crypt->multiKeyEncrypt($fileKey, $publicKeys); + + $this->keyManager->deleteAllFileKeys($path); + + $this->keyManager->setAllFileKeys($path, $encryptedFileKey); + + } else { + $this->logger->debug('no file key found, we assume that the file "{file}" is not encrypted', + array('file' => $path, 'app' => 'encryption')); + + return false; } - $publicKeys = $this->keyManager->addSystemKeys($accessList, $publicKeys); - - $encryptedFileKey = $this->crypt->multiKeyEncrypt($fileKey, $publicKeys); - - $this->keyManager->deleteAllFileKeys($path); - - $this->keyManager->setAllFileKeys($path, $encryptedFileKey); - return true; } diff --git a/apps/encryption/tests/lib/crypto/encryptionTest.php b/apps/encryption/tests/lib/crypto/encryptionTest.php index 500433c77d..2fbc7a111d 100644 --- a/apps/encryption/tests/lib/crypto/encryptionTest.php +++ b/apps/encryption/tests/lib/crypto/encryptionTest.php @@ -38,6 +38,9 @@ class EncryptionTest extends TestCase { /** @var \PHPUnit_Framework_MockObject_MockObject */ private $utilMock; + /** @var \PHPUnit_Framework_MockObject_MockObject */ + private $loggerMock; + public function setUp() { parent::setUp(); @@ -50,8 +53,16 @@ class EncryptionTest extends TestCase { $this->keyManagerMock = $this->getMockBuilder('OCA\Encryption\KeyManager') ->disableOriginalConstructor() ->getMock(); + $this->loggerMock = $this->getMockBuilder('OCP\ILogger') + ->disableOriginalConstructor() + ->getMock(); - $this->instance = new Encryption($this->cryptMock, $this->keyManagerMock, $this->utilMock); + $this->instance = new Encryption( + $this->cryptMock, + $this->keyManagerMock, + $this->utilMock, + $this->loggerMock + ); } /** @@ -83,6 +94,9 @@ class EncryptionTest extends TestCase { $this->cryptMock->expects($this->any()) ->method('getLegacyCipher') ->willReturn($legacyCipher); + $this->cryptMock->expects($this->any()) + ->method('generateFileKey') + ->willReturn('fileKey'); $result = $this->instance->begin('/user/files/foo.txt', 'user', $mode, $header, []); @@ -99,5 +113,36 @@ class EncryptionTest extends TestCase { ); } + /** + * @dataProvider dataTestUpdate + * + * @param string $fileKey + * @param boolean $expected + */ + public function testUpdate($fileKey, $expected) { + $this->keyManagerMock->expects($this->once()) + ->method('getFileKey')->willReturn($fileKey); -} \ No newline at end of file + $this->keyManagerMock->expects($this->any()) + ->method('getPublicKey')->willReturn('publicKey'); + + $this->keyManagerMock->expects($this->any()) + ->method('addSystemKeys') + ->willReturnCallback(function($accessList, $publicKeys) { + return $publicKeys; + }); + + $this->assertSame($expected, + $this->instance->update('path', 'user1', ['users' => ['user1']]) + ); + + } + + public function dataTestUpdate() { + return array( + array('', false), + array('fileKey', true) + ); + } + +} From d5cbb66b667e5850bd6ed45d7a211d332d1c93c8 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Tue, 28 Apr 2015 17:31:03 +0200 Subject: [PATCH 2/3] also create encryption keys for empty files --- apps/encryption/lib/crypto/encryption.php | 39 +++++++++++-------- .../tests/lib/crypto/encryptionTest.php | 30 ++++++++++---- 2 files changed, 44 insertions(+), 25 deletions(-) diff --git a/apps/encryption/lib/crypto/encryption.php b/apps/encryption/lib/crypto/encryption.php index cc61f04ef0..4e181b0712 100644 --- a/apps/encryption/lib/crypto/encryption.php +++ b/apps/encryption/lib/crypto/encryption.php @@ -120,26 +120,35 @@ class Encryption implements IEncryptionModule { */ public function begin($path, $user, $mode, array $header, array $accessList) { - if (isset($header['cipher'])) { - $this->cipher = $header['cipher']; - } else if ( + $this->path = $this->getPathToRealFile($path); + $this->accessList = $accessList; + $this->user = $user; + $this->isWriteOperation = false; + $this->writeCache = ''; + + $this->fileKey = $this->keyManager->getFileKey($this->path, $this->user); + + if ( $mode === 'w' || $mode === 'w+' || $mode === 'wb' || $mode === 'wb+' ) { - $this->cipher = $this->crypt->getCipher(); - } else { - $this->cipher = $this->crypt->getLegacyCipher(); + $this->isWriteOperation = true; + if (empty($this->fileKey)) { + $this->fileKey = $this->crypt->generateFileKey(); + } } - $this->path = $this->getPathToRealFile($path); - $this->accessList = $accessList; - $this->user = $user; - $this->writeCache = ''; - $this->isWriteOperation = false; - - $this->fileKey = $this->keyManager->getFileKey($this->path, $this->user); + if (isset($header['cipher'])) { + $this->cipher = $header['cipher']; + } elseif ($this->isWriteOperation) { + $this->cipher = $this->crypt->getCipher(); + } else { + // if we read a file without a header we fall-back to the legacy cipher + // which was used in <=oC6 + $this->cipher = $this->crypt->getLegacyCipher(); + } return array('cipher' => $this->cipher); } @@ -180,10 +189,6 @@ class Encryption implements IEncryptionModule { * @return mixed encrypted data */ public function encrypt($data) { - $this->isWriteOperation = true; - if (empty($this->fileKey)) { - $this->fileKey = $this->crypt->generateFileKey(); - } // If extra data is left over from the last round, make sure it // is integrated into the next 6126 / 8192 block diff --git a/apps/encryption/tests/lib/crypto/encryptionTest.php b/apps/encryption/tests/lib/crypto/encryptionTest.php index 2fbc7a111d..cb4ca2d3a1 100644 --- a/apps/encryption/tests/lib/crypto/encryptionTest.php +++ b/apps/encryption/tests/lib/crypto/encryptionTest.php @@ -86,7 +86,7 @@ class EncryptionTest extends TestCase { /** * @dataProvider dataTestBegin */ - public function testBegin($mode, $header, $legacyCipher, $defaultCipher, $expected) { + public function testBegin($mode, $header, $legacyCipher, $defaultCipher, $fileKey, $expected) { $this->cryptMock->expects($this->any()) ->method('getCipher') @@ -94,22 +94,36 @@ class EncryptionTest extends TestCase { $this->cryptMock->expects($this->any()) ->method('getLegacyCipher') ->willReturn($legacyCipher); - $this->cryptMock->expects($this->any()) - ->method('generateFileKey') - ->willReturn('fileKey'); + if (empty($fileKey)) { + $this->cryptMock->expects($this->once()) + ->method('generateFileKey') + ->willReturn('fileKey'); + } else { + $this->cryptMock->expects($this->never()) + ->method('generateFileKey'); + } + + $this->keyManagerMock->expects($this->once()) + ->method('getFileKey') + ->willReturn($fileKey); $result = $this->instance->begin('/user/files/foo.txt', 'user', $mode, $header, []); $this->assertArrayHasKey('cipher', $result); $this->assertSame($expected, $result['cipher']); + if ($mode === 'w') { + $this->assertTrue(\Test_Helper::invokePrivate($this->instance, 'isWriteOperation')); + } else { + $this->assertFalse(\Test_Helper::invokePrivate($this->instance, 'isWriteOperation')); + } } public function dataTestBegin() { return array( - array('w', ['cipher' => 'myCipher'], 'legacyCipher', 'defaultCipher', 'myCipher'), - array('r', ['cipher' => 'myCipher'], 'legacyCipher', 'defaultCipher', 'myCipher'), - array('w', [], 'legacyCipher', 'defaultCipher', 'defaultCipher'), - array('r', [], 'legacyCipher', 'defaultCipher', 'legacyCipher'), + array('w', ['cipher' => 'myCipher'], 'legacyCipher', 'defaultCipher', 'fileKey', 'myCipher'), + array('r', ['cipher' => 'myCipher'], 'legacyCipher', 'defaultCipher', 'fileKey', 'myCipher'), + array('w', [], 'legacyCipher', 'defaultCipher', '', 'defaultCipher'), + array('r', [], 'legacyCipher', 'defaultCipher', 'file_key', 'legacyCipher'), ); } From 29bcfb2fdbfd922b6918cd8665d4f31858d7e08e Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Tue, 28 Apr 2015 18:06:46 +0200 Subject: [PATCH 3/3] method shouldn't be static --- apps/encryption/lib/crypto/crypt.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/encryption/lib/crypto/crypt.php b/apps/encryption/lib/crypto/crypt.php index 782dbbe5a3..65af3a93d0 100644 --- a/apps/encryption/lib/crypto/crypt.php +++ b/apps/encryption/lib/crypto/crypt.php @@ -410,7 +410,7 @@ class Crypt { * @return string * @throws \Exception */ - public static function generateFileKey() { + public function generateFileKey() { // Generate key $key = base64_encode(openssl_random_pseudo_bytes(32, $strong)); if (!$key || !$strong) {