From f2f5769df7d4c2b33a847e86a71d94d5c689decd Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Mon, 10 Feb 2014 17:23:54 +0100 Subject: [PATCH] catch errors during decryption --- apps/files_encryption/lib/util.php | 38 ++++++++----- apps/files_encryption/tests/util.php | 81 +++++++++++++++++++++++++++- settings/ajax/decryptall.php | 9 +++- 3 files changed, 112 insertions(+), 16 deletions(-) diff --git a/apps/files_encryption/lib/util.php b/apps/files_encryption/lib/util.php index ced4b823cf..f3f69997f2 100644 --- a/apps/files_encryption/lib/util.php +++ b/apps/files_encryption/lib/util.php @@ -316,7 +316,8 @@ class Util { $found = array( 'plain' => array(), 'encrypted' => array(), - 'legacy' => array() + 'legacy' => array(), + 'broken' => array(), ); } @@ -327,10 +328,7 @@ class Util { if(is_resource($handle)) { while (false !== ($file = readdir($handle))) { - if ( - $file !== "." - && $file !== ".." - ) { + if ($file !== "." && $file !== "..") { $filePath = $directory . '/' . $this->view->getRelativePath('/' . $file); $relPath = \OCA\Encryption\Helper::stripUserFilesPath($filePath); @@ -357,15 +355,23 @@ class Util { // NOTE: This is inefficient; // scanning every file like this // will eat server resources :( - if ( - Keymanager::getFileKey($this->view, $this, $relPath) - && $isEncryptedPath - ) { + if ($isEncryptedPath) { - $found['encrypted'][] = array( - 'name' => $file, - 'path' => $filePath - ); + $fileKey = Keymanager::getFileKey($this->view, $this, $relPath); + $shareKey = Keymanager::getShareKey($this->view, $this->userId, $this, $relPath); + // if file is encrypted but now file key is available, throw exception + if ($fileKey === false || $shareKey === false) { + \OCP\Util::writeLog('encryption library', 'No keys available to decrypt the file: ' . $filePath, \OCP\Util::ERROR); + $found['broken'][] = array( + 'name' => $file, + 'path' => $filePath, + ); + } else { + $found['encrypted'][] = array( + 'name' => $file, + 'path' => $filePath, + ); + } // If the file uses old // encryption system @@ -771,6 +777,12 @@ class Util { $successful = false; } + // if there are broken encrypted files than the complete decryption + // was not successful + if (!empty($found['broken'])) { + $successful = false; + } + if ($successful) { $this->view->deleteAll($this->keyfilesPath); $this->view->deleteAll($this->shareKeysPath); diff --git a/apps/files_encryption/tests/util.php b/apps/files_encryption/tests/util.php index 228f7df1b9..321bdc76f8 100755 --- a/apps/files_encryption/tests/util.php +++ b/apps/files_encryption/tests/util.php @@ -64,6 +64,8 @@ class Test_Encryption_Util extends \PHPUnit_Framework_TestCase { function setUp() { + // login user + \Test_Encryption_Util::loginHelper(\Test_Encryption_Util::TEST_ENCRYPTION_UTIL_USER1); \OC_User::setUserId(\Test_Encryption_Util::TEST_ENCRYPTION_UTIL_USER1); $this->userId = \Test_Encryption_Util::TEST_ENCRYPTION_UTIL_USER1; $this->pass = \Test_Encryption_Util::TEST_ENCRYPTION_UTIL_USER1; @@ -358,9 +360,12 @@ class Test_Encryption_Util extends \PHPUnit_Framework_TestCase { $fileInfoEncrypted = $this->view->getFileInfo($this->userId . '/files/' . $filename); $this->assertTrue($fileInfoEncrypted instanceof \OC\Files\FileInfo); + $this->assertEquals($fileInfoEncrypted['encrypted'], 1); - // encrypt all unencrypted files - $util->decryptAll('/' . $this->userId . '/' . 'files'); + // decrypt all encrypted files + $result = $util->decryptAll('/' . $this->userId . '/' . 'files'); + + $this->assertTrue($result); $fileInfoUnencrypted = $this->view->getFileInfo($this->userId . '/files/' . $filename); @@ -369,11 +374,83 @@ class Test_Encryption_Util extends \PHPUnit_Framework_TestCase { // check if mtime and etags unchanged $this->assertEquals($fileInfoEncrypted['mtime'], $fileInfoUnencrypted['mtime']); $this->assertEquals($fileInfoEncrypted['etag'], $fileInfoUnencrypted['etag']); + // file should no longer be encrypted + $this->assertEquals(0, $fileInfoUnencrypted['encrypted']); $this->view->unlink($this->userId . '/files/' . $filename); } + function testDescryptAllWithBrokenFiles() { + + $file1 = "/decryptAll1" . uniqid() . ".txt"; + $file2 = "/decryptAll2" . uniqid() . ".txt"; + + $util = new Encryption\Util($this->view, $this->userId); + + $this->view->file_put_contents($this->userId . '/files/' . $file1, $this->dataShort); + $this->view->file_put_contents($this->userId . '/files/' . $file2, $this->dataShort); + + $fileInfoEncrypted1 = $this->view->getFileInfo($this->userId . '/files/' . $file1); + $fileInfoEncrypted2 = $this->view->getFileInfo($this->userId . '/files/' . $file2); + + $this->assertTrue($fileInfoEncrypted1 instanceof \OC\Files\FileInfo); + $this->assertTrue($fileInfoEncrypted2 instanceof \OC\Files\FileInfo); + $this->assertEquals($fileInfoEncrypted1['encrypted'], 1); + $this->assertEquals($fileInfoEncrypted2['encrypted'], 1); + + // rename keyfile for file1 so that the decryption for file1 fails + // Expected behaviour: decryptAll() returns false, file2 gets decrypted anyway + $this->view->rename($this->userId . '/files_encryption/keyfiles/' . $file1 . '.key', + $this->userId . '/files_encryption/keyfiles/' . $file1 . '.key.moved'); + + // decrypt all encrypted files + $result = $util->decryptAll('/' . $this->userId . '/' . 'files'); + + $this->assertFalse($result); + + $fileInfoUnencrypted1 = $this->view->getFileInfo($this->userId . '/files/' . $file1); + $fileInfoUnencrypted2 = $this->view->getFileInfo($this->userId . '/files/' . $file2); + + $this->assertTrue($fileInfoUnencrypted1 instanceof \OC\Files\FileInfo); + $this->assertTrue($fileInfoUnencrypted2 instanceof \OC\Files\FileInfo); + + // file1 should be still encrypted; file2 should be decrypted + $this->assertEquals(1, $fileInfoUnencrypted1['encrypted']); + $this->assertEquals(0, $fileInfoUnencrypted2['encrypted']); + + // keyfiles and share keys should still exist + $this->assertTrue($this->view->is_dir($this->userId . '/files_encryption/keyfiles/')); + $this->assertTrue($this->view->is_dir($this->userId . '/files_encryption/share-keys/')); + + // rename the keyfile for file1 back + $this->view->rename($this->userId . '/files_encryption/keyfiles/' . $file1 . '.key.moved', + $this->userId . '/files_encryption/keyfiles/' . $file1 . '.key'); + + // try again to decrypt all encrypted files + $result = $util->decryptAll('/' . $this->userId . '/' . 'files'); + + $this->assertTrue($result); + + $fileInfoUnencrypted1 = $this->view->getFileInfo($this->userId . '/files/' . $file1); + $fileInfoUnencrypted2 = $this->view->getFileInfo($this->userId . '/files/' . $file2); + + $this->assertTrue($fileInfoUnencrypted1 instanceof \OC\Files\FileInfo); + $this->assertTrue($fileInfoUnencrypted2 instanceof \OC\Files\FileInfo); + + // now both files should be decrypted + $this->assertEquals(0, $fileInfoUnencrypted1['encrypted']); + $this->assertEquals(0, $fileInfoUnencrypted2['encrypted']); + + // keyfiles and share keys should be deleted + $this->assertFalse($this->view->is_dir($this->userId . '/files_encryption/keyfiles/')); + $this->assertFalse($this->view->is_dir($this->userId . '/files_encryption/share-keys/')); + + $this->view->unlink($this->userId . '/files/' . $file1); + $this->view->unlink($this->userId . '/files/' . $file2); + + } + /** * @large */ diff --git a/settings/ajax/decryptall.php b/settings/ajax/decryptall.php index ebc17aacfa..d7c104ab15 100644 --- a/settings/ajax/decryptall.php +++ b/settings/ajax/decryptall.php @@ -16,7 +16,14 @@ $util = new \OCA\Encryption\Util($view, \OCP\User::getUser()); $result = $util->initEncryption($params); if ($result !== false) { - $successful = $util->decryptAll(); + + try { + $successful = $util->decryptAll(); + } catch (\Exception $ex) { + \OCP\Util::writeLog('encryption library', "Decryption finished unexpected: " . $ex->getMessage(), \OCP\Util::ERROR); + $successful = false; + } + if ($successful === true) { \OCP\JSON::success(array('data' => array('message' => 'Files decrypted successfully'))); } else {