diff --git a/apps/encryption/lib/Crypto/EncryptAll.php b/apps/encryption/lib/Crypto/EncryptAll.php index 6dfa36e6e3dd7..d9db616e6f15b 100644 --- a/apps/encryption/lib/Crypto/EncryptAll.php +++ b/apps/encryption/lib/Crypto/EncryptAll.php @@ -20,6 +20,7 @@ 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; @@ -50,6 +51,7 @@ public function __construct( protected IFactory $l10nFactory, protected QuestionHelper $questionHelper, protected ISecureRandom $secureRandom, + protected LoggerInterface $logger, ) { // store one time passwords for the users $this->userPasswords = []; @@ -207,9 +209,22 @@ protected function encryptUsersFiles($uid, ProgressBar $progress, $userCount) { } 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, + ] + ); } } } @@ -234,7 +249,14 @@ protected function encryptFile($path) { $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 d702c123b9b4d..779248a0a748f 100644 --- a/apps/encryption/tests/Crypto/EncryptAllTest.php +++ b/apps/encryption/tests/Crypto/EncryptAllTest.php @@ -20,6 +20,8 @@ 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; @@ -28,48 +30,21 @@ use Test\TestCase; class EncryptAllTest extends TestCase { - - /** @var \PHPUnit\Framework\MockObject\MockObject|KeyManager */ - protected $keyManager; - - /** @var \PHPUnit\Framework\MockObject\MockObject|Util */ - protected $util; - - /** @var \PHPUnit\Framework\MockObject\MockObject|IUserManager */ - protected $userManager; - - /** @var \PHPUnit\Framework\MockObject\MockObject|Setup */ - protected $setupUser; - - /** @var \PHPUnit\Framework\MockObject\MockObject|View */ - protected $view; - - /** @var \PHPUnit\Framework\MockObject\MockObject|IConfig */ - protected $config; - - /** @var \PHPUnit\Framework\MockObject\MockObject|IMailer */ - protected $mailer; - - /** @var \PHPUnit\Framework\MockObject\MockObject|IL10N */ - protected $l; - - /** @var \PHPUnit\Framework\MockObject\MockObject | IFactory */ - protected $l10nFactory; - - /** @var \PHPUnit\Framework\MockObject\MockObject | \Symfony\Component\Console\Helper\QuestionHelper */ - protected $questionHelper; - - /** @var \PHPUnit\Framework\MockObject\MockObject | \Symfony\Component\Console\Input\InputInterface */ - protected $inputInterface; - - /** @var \PHPUnit\Framework\MockObject\MockObject | \Symfony\Component\Console\Output\OutputInterface */ - protected $outputInterface; - - /** @var \PHPUnit\Framework\MockObject\MockObject|UserInterface */ - protected $userInterface; - - /** @var \PHPUnit\Framework\MockObject\MockObject|ISecureRandom */ - protected $secureRandom; + protected KeyManager&MockObject $keyManager; + protected Util&MockObject $util; + protected IUserManager&MockObject $userManager; + protected Setup&MockObject $setupUser; + protected View&MockObject $view; + protected IConfig&MockObject $config; + protected IMailer&MockObject $mailer; + protected IL10N&MockObject $l; + protected IFactory&MockObject $l10nFactory; + protected \Symfony\Component\Console\Helper\QuestionHelper&MockObject $questionHelper; + protected \Symfony\Component\Console\Input\InputInterface&MockObject $inputInterface; + protected \Symfony\Component\Console\Output\OutputInterface&MockObject $outputInterface; + protected UserInterface&MockObject $userInterface; + protected ISecureRandom&MockObject $secureRandom; + protected LoggerInterface&MockObject $logger; /** @var EncryptAll */ protected $encryptAll; @@ -101,6 +76,7 @@ protected function setUp(): void { ->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 +107,13 @@ protected function setUp(): void { $this->l, $this->l10nFactory, $this->questionHelper, - $this->secureRandom + $this->secureRandom, + $this->logger, ); } public function testEncryptAll(): void { - /** @var EncryptAll | \PHPUnit\Framework\MockObject\MockObject $encryptAll */ + /** @var EncryptAll&MockObject $encryptAll */ $encryptAll = $this->getMockBuilder(EncryptAll::class) ->setConstructorArgs( [ @@ -150,7 +127,8 @@ public function testEncryptAll(): void { $this->l, $this->l10nFactory, $this->questionHelper, - $this->secureRandom + $this->secureRandom, + $this->logger, ] ) ->setMethods(['createKeyPairs', 'encryptAllUsersFiles', 'outputPasswords']) @@ -165,7 +143,7 @@ public function testEncryptAll(): void { } public function testEncryptAllWithMasterKey(): void { - /** @var EncryptAll | \PHPUnit\Framework\MockObject\MockObject $encryptAll */ + /** @var EncryptAll&MockObject $encryptAll */ $encryptAll = $this->getMockBuilder(EncryptAll::class) ->setConstructorArgs( [ @@ -179,7 +157,8 @@ public function testEncryptAllWithMasterKey(): void { $this->l, $this->l10nFactory, $this->questionHelper, - $this->secureRandom + $this->secureRandom, + $this->logger, ] ) ->setMethods(['createKeyPairs', 'encryptAllUsersFiles', 'outputPasswords']) @@ -195,7 +174,7 @@ public function testEncryptAllWithMasterKey(): void { } public function testCreateKeyPairs(): void { - /** @var EncryptAll | \PHPUnit\Framework\MockObject\MockObject $encryptAll */ + /** @var EncryptAll&MockObject $encryptAll */ $encryptAll = $this->getMockBuilder(EncryptAll::class) ->setConstructorArgs( [ @@ -209,7 +188,8 @@ public function testCreateKeyPairs(): void { $this->l, $this->l10nFactory, $this->questionHelper, - $this->secureRandom + $this->secureRandom, + $this->logger, ] ) ->setMethods(['setupUserFS', 'generateOneTimePassword']) @@ -259,7 +239,8 @@ public function testEncryptAllUsersFiles(): void { $this->l, $this->l10nFactory, $this->questionHelper, - $this->secureRandom + $this->secureRandom, + $this->logger, ] ) ->setMethods(['encryptUsersFiles']) @@ -295,7 +276,8 @@ public function testEncryptUsersFiles(): void { $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 bdaba57687a52..4bc5ec8c81357 100644 --- a/lib/private/Files/Storage/Wrapper/Encryption.php +++ b/lib/private/Files/Storage/Wrapper/Encryption.php @@ -23,6 +23,7 @@ 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; @@ -185,11 +186,11 @@ public function unlink(string $path): bool { public function rename(string $source, string $target): bool { $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); @@ -210,9 +211,9 @@ public function rename(string $source, string $target): bool { public function rmdir(string $path): bool { $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); } @@ -225,9 +226,9 @@ public function isReadable(string $path): bool { $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); @@ -384,9 +385,9 @@ protected function verifyUnencryptedSize(string $path, int $unencryptedSize): in $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 @@ -622,8 +623,8 @@ private function copyBetweenStorage( ): bool { // 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 @@ -673,12 +674,16 @@ private function copyBetweenStorage( 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); } } @@ -728,6 +733,9 @@ public function stat(string $path): array|false { public function hash(string $type, string $path, bool $raw = false): string|false { $fh = $this->fopen($path, 'rb'); + if ($fh === false) { + return false; + } $ctx = hash_init($type); hash_update_stream($ctx, $fh); fclose($fh); @@ -752,6 +760,9 @@ protected function readFirstBlock(string $path): string { $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); } @@ -882,6 +893,9 @@ protected function shouldEncrypt(string $path): bool { 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);