From 1285ebc3cfb2b1a68778e1d7ef7bd1a2b9989021 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 24 Jun 2025 17:01:21 +0200 Subject: [PATCH 1/3] fix(encryption): Catch exceptions in encrypt-all command and continue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- apps/encryption/lib/Crypto/EncryptAll.php | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/apps/encryption/lib/Crypto/EncryptAll.php b/apps/encryption/lib/Crypto/EncryptAll.php index 6dfa36e6e3dd7..6454c7d6e132d 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, + ] + ); } } } From 58e8626f5f331b5e28ea8658763493cac4dcf70c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 26 Jun 2025 14:02:13 +0200 Subject: [PATCH 2/3] chore: Adapt tests to new parameter in EncryptAll constructor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- .../tests/Crypto/EncryptAllTest.php | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/apps/encryption/tests/Crypto/EncryptAllTest.php b/apps/encryption/tests/Crypto/EncryptAllTest.php index 6dbbd55df78d7..9b39c62b65047 100644 --- a/apps/encryption/tests/Crypto/EncryptAllTest.php +++ b/apps/encryption/tests/Crypto/EncryptAllTest.php @@ -23,6 +23,7 @@ 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; @@ -46,6 +47,7 @@ class EncryptAllTest extends TestCase { protected \Symfony\Component\Console\Output\OutputInterface&MockObject $outputInterface; protected UserInterface&MockObject $userInterface; protected ISecureRandom&MockObject $secureRandom; + protected LoggerInterface&MockObject $logger; protected EncryptAll $encryptAll; @@ -76,6 +78,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 @@ -106,12 +109,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( [ @@ -125,7 +129,8 @@ public function testEncryptAll(): void { $this->l, $this->l10nFactory, $this->questionHelper, - $this->secureRandom + $this->secureRandom, + $this->logger, ] ) ->onlyMethods(['createKeyPairs', 'encryptAllUsersFiles', 'outputPasswords']) @@ -140,7 +145,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( [ @@ -154,7 +159,8 @@ public function testEncryptAllWithMasterKey(): void { $this->l, $this->l10nFactory, $this->questionHelper, - $this->secureRandom + $this->secureRandom, + $this->logger, ] ) ->onlyMethods(['createKeyPairs', 'encryptAllUsersFiles', 'outputPasswords']) @@ -170,7 +176,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( [ @@ -184,7 +190,8 @@ public function testCreateKeyPairs(): void { $this->l, $this->l10nFactory, $this->questionHelper, - $this->secureRandom + $this->secureRandom, + $this->logger, ] ) ->onlyMethods(['setupUserFS', 'generateOneTimePassword']) @@ -234,7 +241,8 @@ public function testEncryptAllUsersFiles(): void { $this->l, $this->l10nFactory, $this->questionHelper, - $this->secureRandom + $this->secureRandom, + $this->logger, ] ) ->onlyMethods(['encryptUsersFiles']) @@ -275,7 +283,8 @@ public function testEncryptUsersFiles(): void { $this->l, $this->l10nFactory, $this->questionHelper, - $this->secureRandom + $this->secureRandom, + $this->logger, ] ) ->onlyMethods(['encryptFile', 'setupUserFS']) From 4427050f840afee97a0de8d5e58d8ca5f5eb6196 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 3 Jul 2025 11:02:06 +0200 Subject: [PATCH 3/3] fix(encryption): Correctly handle file opening and copying failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- apps/encryption/lib/Crypto/EncryptAll.php | 9 ++++++++- .../Files/Storage/Wrapper/Encryption.php | 20 ++++++++++++++++--- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/apps/encryption/lib/Crypto/EncryptAll.php b/apps/encryption/lib/Crypto/EncryptAll.php index 6454c7d6e132d..d9db616e6f15b 100644 --- a/apps/encryption/lib/Crypto/EncryptAll.php +++ b/apps/encryption/lib/Crypto/EncryptAll.php @@ -249,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/lib/private/Files/Storage/Wrapper/Encryption.php b/lib/private/Files/Storage/Wrapper/Encryption.php index 51a5f99908df8..58bd4dfddcf1d 100644 --- a/lib/private/Files/Storage/Wrapper/Encryption.php +++ b/lib/private/Files/Storage/Wrapper/Encryption.php @@ -23,6 +23,7 @@ use OCP\Encryption\Keys\IStorage; use OCP\Files; use OCP\Files\Cache\ICacheEntry; +use OCP\Files\GenericFileException; use OCP\Files\Mount\IMountPoint; use OCP\Files\Storage; use Psr\Log\LoggerInterface; @@ -685,12 +686,16 @@ private function copyBetweenStorage( try { $source = $sourceStorage->fopen($sourceInternalPath, 'r'); $target = $this->fopen($targetInternalPath, 'w'); - [, $result] = Files::streamCopy($source, $target, true); + if ($source === false || $target === false) { + $result = false; + } else { + [, $result] = Files::streamCopy($source, $target, true); + } } finally { - if (is_resource($source)) { + if (isset($source) && $source !== false) { fclose($source); } - if (is_resource($target)) { + if (isset($target) && $target !== false) { fclose($target); } } @@ -740,6 +745,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); @@ -764,6 +772,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); } @@ -894,6 +905,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] = Files::streamCopy($stream, $target, true); fclose($stream); fclose($target);