diff options
-rw-r--r-- | apps/encryption/lib/Crypto/EncryptAll.php | 30 | ||||
-rw-r--r-- | apps/encryption/tests/Crypto/EncryptAllTest.php | 35 | ||||
-rw-r--r-- | lib/private/Files/Storage/Wrapper/Encryption.php | 48 |
3 files changed, 80 insertions, 33 deletions
diff --git a/apps/encryption/lib/Crypto/EncryptAll.php b/apps/encryption/lib/Crypto/EncryptAll.php index 310f37aba83..2cf7b51cdbf 100644 --- a/apps/encryption/lib/Crypto/EncryptAll.php +++ b/apps/encryption/lib/Crypto/EncryptAll.php @@ -20,6 +20,7 @@ use OCP\L10N\IFactory; use OCP\Mail\Headers\AutoSubmitted; use OCP\Mail\IMailer; use OCP\Security\ISecureRandom; +use Psr\Log\LoggerInterface; use Symfony\Component\Console\Helper\ProgressBar; use Symfony\Component\Console\Helper\QuestionHelper; use Symfony\Component\Console\Helper\Table; @@ -82,7 +83,8 @@ class EncryptAll { IL10N $l, IFactory $l10nFactory, QuestionHelper $questionHelper, - ISecureRandom $secureRandom + ISecureRandom $secureRandom, + protected LoggerInterface $logger, ) { $this->userSetup = $userSetup; $this->userManager = $userManager; @@ -251,9 +253,22 @@ class EncryptAll { } else { $progress->setMessage("encrypt files for user $userCount: $path"); $progress->advance(); - if ($this->encryptFile($path) === false) { - $progress->setMessage("encrypt files for user $userCount: $path (already encrypted)"); + try { + if ($this->encryptFile($path) === false) { + $progress->setMessage("encrypt files for user $userCount: $path (already encrypted)"); + $progress->advance(); + } + } catch (\Exception $e) { + $progress->setMessage("Failed to encrypt path $path: " . $e->getMessage()); $progress->advance(); + $this->logger->error( + 'Failed to encrypt path {path}', + [ + 'user' => $uid, + 'path' => $path, + 'exception' => $e, + ] + ); } } } @@ -278,7 +293,14 @@ class EncryptAll { $target = $path . '.encrypted.' . time(); try { - $this->rootView->copy($source, $target); + $copySuccess = $this->rootView->copy($source, $target); + if ($copySuccess === false) { + /* Copy failed, abort */ + if ($this->rootView->file_exists($target)) { + $this->rootView->unlink($target); + } + throw new \Exception('Copy failed for ' . $source); + } $this->rootView->rename($target, $source); } catch (DecryptionFailedException $e) { if ($this->rootView->file_exists($target)) { diff --git a/apps/encryption/tests/Crypto/EncryptAllTest.php b/apps/encryption/tests/Crypto/EncryptAllTest.php index a63b826bf3c..5ec0c1baf86 100644 --- a/apps/encryption/tests/Crypto/EncryptAllTest.php +++ b/apps/encryption/tests/Crypto/EncryptAllTest.php @@ -20,6 +20,8 @@ use OCP\L10N\IFactory; use OCP\Mail\IMailer; use OCP\Security\ISecureRandom; use OCP\UserInterface; +use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; use Symfony\Component\Console\Formatter\OutputFormatterInterface; use Symfony\Component\Console\Helper\ProgressBar; use Symfony\Component\Console\Helper\QuestionHelper; @@ -71,6 +73,8 @@ class EncryptAllTest extends TestCase { /** @var \PHPUnit\Framework\MockObject\MockObject | \OCP\Security\ISecureRandom */ protected $secureRandom; + protected LoggerInterface&MockObject $logger; + /** @var EncryptAll */ protected $encryptAll; @@ -101,6 +105,7 @@ class EncryptAllTest extends TestCase { ->disableOriginalConstructor()->getMock(); $this->userInterface = $this->getMockBuilder(UserInterface::class) ->disableOriginalConstructor()->getMock(); + $this->logger = $this->createMock(LoggerInterface::class); /** * We need format method to return a string @@ -131,12 +136,13 @@ class EncryptAllTest extends TestCase { $this->l, $this->l10nFactory, $this->questionHelper, - $this->secureRandom + $this->secureRandom, + $this->logger, ); } - public function testEncryptAll() { - /** @var EncryptAll | \PHPUnit\Framework\MockObject\MockObject $encryptAll */ + public function testEncryptAll(): void { + /** @var EncryptAll&MockObject $encryptAll */ $encryptAll = $this->getMockBuilder(EncryptAll::class) ->setConstructorArgs( [ @@ -150,7 +156,8 @@ class EncryptAllTest extends TestCase { $this->l, $this->l10nFactory, $this->questionHelper, - $this->secureRandom + $this->secureRandom, + $this->logger, ] ) ->setMethods(['createKeyPairs', 'encryptAllUsersFiles', 'outputPasswords']) @@ -164,8 +171,8 @@ class EncryptAllTest extends TestCase { $encryptAll->encryptAll($this->inputInterface, $this->outputInterface); } - public function testEncryptAllWithMasterKey() { - /** @var EncryptAll | \PHPUnit\Framework\MockObject\MockObject $encryptAll */ + public function testEncryptAllWithMasterKey(): void { + /** @var EncryptAll&MockObject $encryptAll */ $encryptAll = $this->getMockBuilder(EncryptAll::class) ->setConstructorArgs( [ @@ -179,7 +186,8 @@ class EncryptAllTest extends TestCase { $this->l, $this->l10nFactory, $this->questionHelper, - $this->secureRandom + $this->secureRandom, + $this->logger, ] ) ->setMethods(['createKeyPairs', 'encryptAllUsersFiles', 'outputPasswords']) @@ -194,8 +202,8 @@ class EncryptAllTest extends TestCase { $encryptAll->encryptAll($this->inputInterface, $this->outputInterface); } - public function testCreateKeyPairs() { - /** @var EncryptAll | \PHPUnit\Framework\MockObject\MockObject $encryptAll */ + public function testCreateKeyPairs(): void { + /** @var EncryptAll&MockObject $encryptAll */ $encryptAll = $this->getMockBuilder(EncryptAll::class) ->setConstructorArgs( [ @@ -209,7 +217,8 @@ class EncryptAllTest extends TestCase { $this->l, $this->l10nFactory, $this->questionHelper, - $this->secureRandom + $this->secureRandom, + $this->logger, ] ) ->setMethods(['setupUserFS', 'generateOneTimePassword']) @@ -259,7 +268,8 @@ class EncryptAllTest extends TestCase { $this->l, $this->l10nFactory, $this->questionHelper, - $this->secureRandom + $this->secureRandom, + $this->logger, ] ) ->setMethods(['encryptUsersFiles']) @@ -295,7 +305,8 @@ class EncryptAllTest extends TestCase { $this->l, $this->l10nFactory, $this->questionHelper, - $this->secureRandom + $this->secureRandom, + $this->logger, ] ) ->setMethods(['encryptFile', 'setupUserFS']) diff --git a/lib/private/Files/Storage/Wrapper/Encryption.php b/lib/private/Files/Storage/Wrapper/Encryption.php index 265a554b240..7cbfa8a126a 100644 --- a/lib/private/Files/Storage/Wrapper/Encryption.php +++ b/lib/private/Files/Storage/Wrapper/Encryption.php @@ -24,6 +24,7 @@ use OCP\Encryption\IFile; use OCP\Encryption\IManager; use OCP\Encryption\Keys\IStorage; use OCP\Files\Cache\ICacheEntry; +use OCP\Files\GenericFileException; use OCP\Files\Mount\IMountPoint; use OCP\Files\Storage; use Psr\Log\LoggerInterface; @@ -265,11 +266,11 @@ class Encryption extends Wrapper { public function rename($source, $target) { $result = $this->storage->rename($source, $target); - if ($result && + if ($result // versions always use the keys from the original file, so we can skip // this step for versions - $this->isVersion($target) === false && - $this->encryptionManager->isEnabled()) { + && $this->isVersion($target) === false + && $this->encryptionManager->isEnabled()) { $sourcePath = $this->getFullPath($source); if (!$this->util->isExcluded($sourcePath)) { $targetPath = $this->getFullPath($target); @@ -296,9 +297,9 @@ class Encryption extends Wrapper { public function rmdir($path) { $result = $this->storage->rmdir($path); $fullPath = $this->getFullPath($path); - if ($result && - $this->util->isExcluded($fullPath) === false && - $this->encryptionManager->isEnabled() + if ($result + && $this->util->isExcluded($fullPath) === false + && $this->encryptionManager->isEnabled() ) { $this->keyStorage->deleteAllFileKeys($fullPath); } @@ -317,9 +318,9 @@ class Encryption extends Wrapper { $metaData = $this->getMetaData($path); if ( - !$this->is_dir($path) && - isset($metaData['encrypted']) && - $metaData['encrypted'] === true + !$this->is_dir($path) + && isset($metaData['encrypted']) + && $metaData['encrypted'] === true ) { $fullPath = $this->getFullPath($path); $module = $this->getEncryptionModule($path); @@ -493,9 +494,9 @@ class Encryption extends Wrapper { $size = $this->storage->filesize($path); $result = $unencryptedSize; - if ($unencryptedSize < 0 || - ($size > 0 && $unencryptedSize === $size) || - $unencryptedSize > $size + if ($unencryptedSize < 0 + || ($size > 0 && $unencryptedSize === $size) + || $unencryptedSize > $size ) { // check if we already calculate the unencrypted size for the // given path to avoid recursions @@ -764,8 +765,8 @@ class Encryption extends Wrapper { ) { // for versions we have nothing to do, because versions should always use the // key from the original file. Just create a 1:1 copy and done - if ($this->isVersion($targetInternalPath) || - $this->isVersion($sourceInternalPath)) { + if ($this->isVersion($targetInternalPath) + || $this->isVersion($sourceInternalPath)) { // remember that we try to create a version so that we can detect it during // fopen($sourceInternalPath) and by-pass the encryption in order to // create a 1:1 copy of the file @@ -815,12 +816,16 @@ class Encryption extends Wrapper { try { $source = $sourceStorage->fopen($sourceInternalPath, 'r'); $target = $this->fopen($targetInternalPath, 'w'); - [, $result] = \OC_Helper::streamCopy($source, $target); + if ($source === false || $target === false) { + $result = false; + } else { + [, $result] = \OC_Helper::streamCopy($source, $target); + } } finally { - if (is_resource($source)) { + if (isset($source) && $source !== false) { fclose($source); } - if (is_resource($target)) { + if (isset($target) && $target !== false) { fclose($target); } } @@ -870,6 +875,9 @@ class Encryption extends Wrapper { public function hash($type, $path, $raw = false) { $fh = $this->fopen($path, 'rb'); + if ($fh === false) { + return false; + } $ctx = hash_init($type); hash_update_stream($ctx, $fh); fclose($fh); @@ -897,6 +905,9 @@ class Encryption extends Wrapper { $firstBlock = ''; if ($this->storage->is_file($path)) { $handle = $this->storage->fopen($path, 'r'); + if ($handle === false) { + return ''; + } $firstBlock = fread($handle, $this->util->getHeaderSize()); fclose($handle); } @@ -1046,6 +1057,9 @@ class Encryption extends Wrapper { public function writeStream(string $path, $stream, ?int $size = null): int { // always fall back to fopen $target = $this->fopen($path, 'w'); + if ($target === false) { + throw new GenericFileException("Failed to open $path for writing"); + } [$count, $result] = \OC_Helper::streamCopy($stream, $target); fclose($stream); fclose($target); |