Merge pull request #15919 from owncloud/enc_handle_empty_files

Encryption improve handling of empty and unencrypted files

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

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) {

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;
 	}
 
 	/**
@@ -111,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);
 	}
@@ -171,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
@@ -257,19 +271,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;
 	}
 

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
+		);
 	}
 
 	/**
@@ -75,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')
@@ -83,21 +94,69 @@ class EncryptionTest extends TestCase {
 		$this->cryptMock->expects($this->any())
 			->method('getLegacyCipher')
 			->willReturn($legacyCipher);
+		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'),
 		);
 	}
 
+	/**
+	 * @dataProvider dataTestUpdate
+	 *
+	 * @param string $fileKey
+	 * @param boolean $expected
+	 */
+	public function testUpdate($fileKey, $expected) {
+		$this->keyManagerMock->expects($this->once())
+			->method('getFileKey')->willReturn($fileKey);
 
-}
+		$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)
+		);
+	}
+
+}