From 242cc8cd828b931b7cdc30e5b7e122ddbbc0daef Mon Sep 17 00:00:00 2001 From: David Dreschner Date: Tue, 21 Jul 2026 14:22:09 +0200 Subject: [PATCH 1/2] fix(files): Prevent corruption of files when moving from encrypted to unencrypted folders within the same object storage Assisted-by: ClaudeCode:claude-opus-4-8 Signed-off-by: David Dreschner --- .../encryption/tests/EncryptedStorageTest.php | 152 ++++++++++++++++++ .../Files/ObjectStore/ObjectStoreStorage.php | 38 +++++ lib/private/Files/Storage/Common.php | 8 +- 3 files changed, 196 insertions(+), 2 deletions(-) diff --git a/apps/encryption/tests/EncryptedStorageTest.php b/apps/encryption/tests/EncryptedStorageTest.php index 8fa5672a747f8..6112d94f86b64 100644 --- a/apps/encryption/tests/EncryptedStorageTest.php +++ b/apps/encryption/tests/EncryptedStorageTest.php @@ -8,11 +8,14 @@ namespace OCA\encryption\tests; +use OC\Files\ObjectStore\ObjectStoreStorage; +use OC\Files\ObjectStore\StorageObjectStore; use OC\Files\Storage\Temporary; use OC\Files\Storage\Wrapper\Encryption; use OC\Files\View; use OCA\Encryption\KeyManager; use OCP\Files\Mount\IMountManager; +use OCP\Files\ObjectStore\IObjectStore; use OCP\Files\Storage\IDisableEncryptionStorage; use OCP\Server; use Test\TestCase; @@ -24,6 +27,10 @@ class TemporaryNoEncrypted extends Temporary implements IDisableEncryptionStorag } +class ObjectStoreNoEncrypted extends ObjectStoreStorage implements IDisableEncryptionStorage { + +} + #[\PHPUnit\Framework\Attributes\Group(name: 'DB')] class EncryptedStorageTest extends TestCase { use MountProviderTrait; @@ -69,4 +76,149 @@ public function testMoveFromEncrypted(): void { $this->assertEquals('bar', $unencryptedStorage->file_get_contents('foo.txt')); $this->assertFalse($unencryptedCache->get('foo.txt')->isEncrypted()); } + + /** + * The metadata only move between storages sharing an object store must not be taken + * for an encrypted source: the ciphertext would stay in the object store while the + * cache entry loses its `encrypted` mark. + */ + public function testMoveFromEncryptedObjectStore(): void { + [ + 'view' => $view, + 'objectStore' => $objectStore, + 'unencryptedStorage' => $unencryptedStorage, + ] = $this->setUpSharedObjectStoreMounts(); + + $view->file_put_contents('enc/foo.txt', 'bar'); + $this->assertEquals('bar', $view->file_get_contents('enc/foo.txt')); + + $view->rename('enc/foo.txt', 'unenc/foo.txt'); + + $this->assertEquals('bar', $view->file_get_contents('unenc/foo.txt')); + $this->assertFalse($unencryptedStorage->getCache()->get('foo.txt')->isEncrypted()); + $this->assertStringStartsNotWith( + 'HBEGIN:', + $this->readRawObject($objectStore, $unencryptedStorage, 'foo.txt'), + 'the object was moved verbatim and is still encrypted at rest' + ); + // a move must not leave the source behind, neither on disk nor in the cache + $this->assertFalse($view->file_exists('enc/foo.txt'), 'the source file still exists after the move'); + } + + /** + * Same as above for the copy shortcut, which hands the ciphertext to the object + * store's server side copy. + */ + public function testCopyFromEncryptedObjectStore(): void { + [ + 'view' => $view, + 'objectStore' => $objectStore, + 'unencryptedStorage' => $unencryptedStorage, + ] = $this->setUpSharedObjectStoreMounts(); + + $view->file_put_contents('enc/foo.txt', 'bar'); + + $view->copy('enc/foo.txt', 'unenc/foo.txt'); + + $this->assertEquals('bar', $view->file_get_contents('enc/foo.txt')); + $this->assertEquals('bar', $view->file_get_contents('unenc/foo.txt')); + $this->assertFalse($unencryptedStorage->getCache()->get('foo.txt')->isEncrypted()); + $this->assertStringStartsNotWith( + 'HBEGIN:', + $this->readRawObject($objectStore, $unencryptedStorage, 'foo.txt'), + 'the object was copied verbatim and is still encrypted at rest' + ); + } + + /** + * A file without the `encrypted` mark holds plain content even on a wrapped storage + * (only some paths encrypt, e.g. not uploads/) and must keep the metadata only move. + */ + public function testMoveUnencryptedFileFromEncryptionWrappedObjectStore(): void { + [ + 'view' => $view, + 'unencryptedStorage' => $unencryptedStorage, + 'encryptedBackingStorage' => $encryptedBackingStorage, + ] = $this->setUpSharedObjectStoreMounts(); + + // bypasses the encryption wrapper: plain content, no `encrypted` mark + $encryptedBackingStorage->file_put_contents('plain.txt', 'plain content'); + $sourceEntry = $encryptedBackingStorage->getCache()->get('plain.txt'); + $this->assertFalse($sourceEntry->isEncrypted()); + + $view->rename('enc/plain.txt', 'unenc/plain.txt'); + + $this->assertEquals('plain content', $view->file_get_contents('unenc/plain.txt')); + $this->assertSame( + $sourceEntry->getId(), + $unencryptedStorage->getCache()->get('plain.txt')->getId(), + 'a plain file must keep the metadata only move that preserves the file id' + ); + $this->assertFalse($view->file_exists('enc/plain.txt'), 'the source file still exists after the move'); + } + + /** + * A folder carries no `encrypted` mark of its own while any of its children may be + * encrypted, so a folder move must always take the encryption aware path. + */ + public function testMoveFolderFromEncryptedObjectStore(): void { + [ + 'view' => $view, + 'objectStore' => $objectStore, + 'unencryptedStorage' => $unencryptedStorage, + ] = $this->setUpSharedObjectStoreMounts(); + + $view->mkdir('enc/dir'); + $view->file_put_contents('enc/dir/foo.txt', 'bar'); + + $view->rename('enc/dir', 'unenc/dir'); + + $this->assertEquals('bar', $view->file_get_contents('unenc/dir/foo.txt')); + $this->assertFalse($unencryptedStorage->getCache()->get('dir/foo.txt')->isEncrypted()); + $this->assertStringStartsNotWith( + 'HBEGIN:', + $this->readRawObject($objectStore, $unencryptedStorage, 'dir/foo.txt'), + 'the folder took the metadata only move and left the child encrypted at rest' + ); + $this->assertFalse($view->file_exists('enc/dir'), 'the source folder still exists after the move'); + } + + /** + * Two object store storages backed by the same object store, one mounted with and one + * without the encryption wrapper. + * + * @return array{view: View, objectStore: IObjectStore, unencryptedStorage: ObjectStoreStorage, encryptedBackingStorage: ObjectStoreStorage} + */ + private function setUpSharedObjectStoreMounts(): array { + Server::get(KeyManager::class)->validateMasterKey(); + Server::get(KeyManager::class)->validateShareKey(); + $this->createUser('test1', 'test2'); + $this->setupForUser('test1', 'test2'); + + // a shared object store instance makes the storage ids match, enabling the shortcuts + $objectStore = new StorageObjectStore(new Temporary()); + $encrypted = new ObjectStoreStorage(['objectstore' => $objectStore, 'storageid' => 'test-enc']); + $unencrypted = new ObjectStoreNoEncrypted(['objectstore' => $objectStore, 'storageid' => 'test-unenc']); + + $this->registerMount('test1', $encrypted, '/test1/files/enc'); + $this->registerMount('test1', $unencrypted, '/test1/files/unenc'); + + $this->loginWithEncryption('test1'); + + return [ + 'view' => new View('/test1/files'), + 'objectStore' => $objectStore, + 'unencryptedStorage' => $unencrypted, + 'encryptedBackingStorage' => $encrypted, + ]; + } + + private function readRawObject(IObjectStore $objectStore, ObjectStoreStorage $storage, string $path): string { + $fileId = $storage->getCache()->get($path)->getId(); + $handle = $objectStore->readObject($storage->getURN($fileId)); + $content = stream_get_contents($handle); + fclose($handle); + + return $content; + } } diff --git a/lib/private/Files/ObjectStore/ObjectStoreStorage.php b/lib/private/Files/ObjectStore/ObjectStoreStorage.php index d776c2a59c559..58359787f5769 100644 --- a/lib/private/Files/ObjectStore/ObjectStoreStorage.php +++ b/lib/private/Files/ObjectStore/ObjectStoreStorage.php @@ -17,6 +17,7 @@ use OC\Files\Cache\CacheEntry; use OC\Files\Storage\Common; use OC\Files\Storage\PolyFill\CopyDirectory; +use OC\Files\Storage\Wrapper\Encryption; use OCP\Constants; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\Files\Cache\ICache; @@ -601,6 +602,14 @@ public function copyFromStorage( string $targetInternalPath, bool $preserveMtime = false, ): bool { + // the shortcuts below copy the object verbatim, an encrypted source has to be + // read through its encryption wrapper instead + if ($sourceStorage->instanceOfStorage(Encryption::class) + && $this->sourceMayContainEncryptedContent($sourceStorage->getCache()->get($sourceInternalPath)) + ) { + return parent::copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath); + } + if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) { /** @var ObjectStoreStorage $sourceStorage */ if ($sourceStorage->getObjectStore()->getStorageId() === $this->getObjectStore()->getStorageId()) { @@ -624,6 +633,19 @@ public function copyFromStorage( #[\Override] public function moveFromStorage(IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath, ?ICacheEntry $sourceCacheEntry = null): bool { $sourceCache = $sourceStorage->getCache(); + + // An encrypted source has to be read through its encryption wrapper: the metadata + // only move below would leave the ciphertext untouched, and copyObjects() reuses + // the source file id, which resolves to the same object on a shared object store. + if ($sourceStorage->instanceOfStorage(Encryption::class)) { + if (!$sourceCacheEntry) { + $sourceCacheEntry = $sourceCache->get($sourceInternalPath); + } + if ($this->sourceMayContainEncryptedContent($sourceCacheEntry)) { + return parent::moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath); + } + } + if ( $sourceStorage->instanceOfStorage(ObjectStoreStorage::class) && $sourceStorage->getObjectStore()->getStorageId() === $this->getObjectStore()->getStorageId() @@ -663,6 +685,22 @@ public function moveFromStorage(IStorage $sourceStorage, string $sourceInternalP return true; } + /** + * The encryption wrapper covers a whole storage while only some of its paths are + * encrypted (files/ but not e.g. uploads/), so the wrapper alone is too coarse a + * signal for skipping the raw object shortcuts. Folders and unreadable cache + * entries count as encrypted, a folder's own flag says nothing about its children. + */ + private function sourceMayContainEncryptedContent(ICacheEntry|false|null $sourceCacheEntry): bool { + if (!$sourceCacheEntry instanceof ICacheEntry) { + return true; + } + if ($sourceCacheEntry->getMimeType() === ICacheEntry::DIRECTORY_MIMETYPE) { + return true; + } + return $sourceCacheEntry->isEncrypted(); + } + /** * Copy the object(s) of a file or folder into this storage, without touching the cache */ diff --git a/lib/private/Files/Storage/Common.php b/lib/private/Files/Storage/Common.php index 35e4a0425913f..2aa2bc6a3b57d 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -624,7 +624,11 @@ public function moveFromStorage(IStorage $sourceStorage, string $sourceInternalP $result = $this->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath, true); if ($result) { - if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) { + // keeping the source cache entry preserves the file id when leaving an object + // store, but between two object stores it would leave a dangling entry behind + $preserveCacheOnDelete = $sourceStorage->instanceOfStorage(ObjectStoreStorage::class) + && !$this->instanceOfStorage(ObjectStoreStorage::class); + if ($preserveCacheOnDelete) { /** @var ObjectStoreStorage $sourceStorage */ $sourceStorage->setPreserveCacheOnDelete(true); } @@ -635,7 +639,7 @@ public function moveFromStorage(IStorage $sourceStorage, string $sourceInternalP $result = $sourceStorage->unlink($sourceInternalPath); } } finally { - if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) { + if ($preserveCacheOnDelete) { /** @var ObjectStoreStorage $sourceStorage */ $sourceStorage->setPreserveCacheOnDelete(false); } From 15842cde6db0fedd3008b9c8a9d4dba69e206ac3 Mon Sep 17 00:00:00 2001 From: David Dreschner Date: Fri, 24 Jul 2026 14:04:04 +0200 Subject: [PATCH 2/2] fix(FixKeyLocation): Don't abort execution on exceptions Assisted-by: ClaudeCode:claude-fable-5 Signed-off-by: David Dreschner --- .../encryption/lib/Command/FixKeyLocation.php | 325 +++++++++++++----- .../tests/Command/FixKeyLocationTest.php | 310 +++++++++++++++++ 2 files changed, 549 insertions(+), 86 deletions(-) create mode 100644 apps/encryption/tests/Command/FixKeyLocationTest.php diff --git a/apps/encryption/lib/Command/FixKeyLocation.php b/apps/encryption/lib/Command/FixKeyLocation.php index a869f967277c9..7c06c7d85b190 100644 --- a/apps/encryption/lib/Command/FixKeyLocation.php +++ b/apps/encryption/lib/Command/FixKeyLocation.php @@ -20,8 +20,10 @@ use OCP\Files\IRootFolder; use OCP\Files\ISetupManager; use OCP\Files\Node; +use OCP\Files\NotFoundException; use OCP\IUser; use OCP\IUserManager; +use Psr\Log\LoggerInterface; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; @@ -39,6 +41,7 @@ public function __construct( private readonly Util $encryptionUtil, private readonly IRootFolder $rootFolder, private readonly ISetupManager $setupManager, + private readonly LoggerInterface $logger, IManager $encryptionManager, ) { $this->keyRootDirectory = rtrim($this->encryptionUtil->getKeyStorageRoot(), '/'); @@ -59,6 +62,7 @@ protected function configure(): void { ->setName('encryption:fix-key-location') ->setDescription('Fix the location of encryption keys for external storage') ->addOption('dry-run', null, InputOption::VALUE_NONE, "Only list files that require key migration, don't try to perform any migration") + ->addOption('personal', null, InputOption::VALUE_NONE, 'Also check the encrypted files in the personal space of the user and restore keys found in the key trees of other users') ->addArgument('user', InputArgument::REQUIRED, 'User id to fix the key locations for'); } @@ -75,8 +79,18 @@ protected function execute(InputInterface $input, OutputInterface $output): int $this->setupManager->setupForUser($user); $mounts = $this->getSystemMountsForUser($user); + $failedPaths = []; foreach ($mounts as $mount) { - $mountRootFolder = $this->rootFolder->get($mount->getMountPoint()); + try { + $mountRootFolder = $this->rootFolder->get($mount->getMountPoint()); + } catch (NotFoundException $e) { + $this->logger->warning('Mount point of user ' . $user->getUID() . ' not found: ' . $mount->getMountPoint(), [ + 'app' => 'encryption', + 'exception' => $e, + ]); + $output->writeln('System wide mount point not found, skipping: ' . $mount->getMountPoint() . ''); + continue; + } if (!$mountRootFolder instanceof Folder) { $output->writeln('System wide mount point is not a directory, skipping: ' . $mount->getMountPoint() . ''); continue; @@ -85,75 +99,160 @@ protected function execute(InputInterface $input, OutputInterface $output): int $files = $this->getAllEncryptedFiles($mountRootFolder); foreach ($files as $file) { /** @var File $file */ - $hasSystemKey = $this->hasSystemKey($file); - $hasUserKey = $this->hasUserKey($user, $file); - if (!$hasSystemKey) { - if ($hasUserKey) { - // key was stored incorrectly as user key, migrate - - if ($dryRun) { - $output->writeln('' . $file->getPath() . ' needs migration'); + try { + $this->fixKeysForFile($user, $file, $dryRun, $output); + } catch (\Throwable $e) { + $failedPaths[] = $file->getPath(); + $this->logger->error('Failed to fix the key location of ' . $file->getPath(), [ + 'app' => 'encryption', + 'exception' => $e, + ]); + $output->writeln('Failed to process ' . $file->getPath() . ': ' . $e->getMessage() . ''); + } + } + } + + if ($input->getOption('personal')) { + $userFolder = $this->rootFolder->getUserFolder($user->getUID()); + $personalMountPoint = $userFolder->getMountPoint()->getMountPoint(); + foreach ($this->getAllEncryptedFiles($userFolder) as $file) { + /** @var File $file */ + // group folders, external storages and received shares are their own + // mounts and follow the system wide handling + if ($file->getMountPoint()->getMountPoint() !== $personalMountPoint) { + continue; + } + try { + $this->fixKeysForPersonalFile($user, $file, $dryRun, $output); + } catch (\Throwable $e) { + $failedPaths[] = $file->getPath(); + $this->logger->error('Failed to fix the key location of ' . $file->getPath(), [ + 'app' => 'encryption', + 'exception' => $e, + ]); + $output->writeln('Failed to process ' . $file->getPath() . ': ' . $e->getMessage() . ''); + } + } + } + + if ($failedPaths !== []) { + $output->writeln(''); + $output->writeln('' . count($failedPaths) . ' file(s) could not be processed, see the log for details:'); + foreach ($failedPaths as $failedPath) { + $output->writeln(' - ' . $failedPath . ''); + } + return self::FAILURE; + } + + return self::SUCCESS; + } + + /** + * A personal file is healthy when its key sits in the tree of the user at the path + * of the file. A missing key can only be restored there, the personal storage + * carries the encryption wrapper, so the file decrypts transparently once the key + * is back in place. + */ + private function fixKeysForPersonalFile(IUser $user, File $file, bool $dryRun, OutputInterface $output): void { + if ($this->hasUserKey($user, $file)) { + return; + } + if (!$this->isDataEncrypted($file)) { + if ($dryRun) { + $output->writeln('' . $file->getPath() . ' needs to be marked as not encrypted'); + } else { + $this->markAsUnEncrypted($file); + $output->writeln('' . $file->getPath() . ' marked as not encrypted'); + } + return; + } + + $targetKeyPath = $this->getUserKeyPath($user, $file); + $foundKey = $this->findKeyInUserTrees($user, $file, $targetKeyPath); + if ($dryRun) { + $output->write('' . $file->getPath() . ' needs migration'); + if ($foundKey) { + $output->writeln(', valid key found at ' . $foundKey . ''); + } else { + $output->writeln(' ❌ No key found'); + } + return; + } + $output->write('Migrating key for ' . $file->getPath() . ''); + if ($foundKey) { + $this->rootView->copy($foundKey, $targetKeyPath); + $output->writeln(' Migrated key from ' . $foundKey . ''); + } else { + $output->writeln(' ❌ No key found'); + } + } + + private function fixKeysForFile(IUser $user, File $file, bool $dryRun, OutputInterface $output): void { + $hasSystemKey = $this->hasSystemKey($file); + $hasUserKey = $this->hasUserKey($user, $file); + if (!$hasSystemKey) { + if ($hasUserKey) { + // key was stored incorrectly as user key, migrate + + if ($dryRun) { + $output->writeln('' . $file->getPath() . ' needs migration'); + } else { + $output->write('Migrating key for ' . $file->getPath() . ' '); + if ($this->copyUserKeyToSystemAndValidate($user, $file)) { + $output->writeln('✓'); + } else { + $output->writeln('❌'); + $output->writeln(' Failed to validate key for ' . $file->getPath() . ', key will not be migrated'); + } + } + } else { + // no matching key, probably from a broken cross-storage move + + $shouldBeEncrypted = $file->getStorage()->instanceOfStorage(Encryption::class); + $isActuallyEncrypted = $this->isDataEncrypted($file); + if ($isActuallyEncrypted) { + if ($dryRun) { + if ($shouldBeEncrypted) { + $output->write('' . $file->getPath() . ' needs migration'); } else { - $output->write('Migrating key for ' . $file->getPath() . ' '); - if ($this->copyUserKeyToSystemAndValidate($user, $file)) { - $output->writeln('✓'); - } else { - $output->writeln('❌'); - $output->writeln(' Failed to validate key for ' . $file->getPath() . ', key will not be migrated'); - } + $output->write('' . $file->getPath() . ' needs decryption'); + } + $foundKey = $this->findUserKeyForSystemFile($user, $file); + if ($foundKey) { + $output->writeln(', valid key found at ' . $foundKey . ''); + } else { + $output->writeln(' ❌ No key found'); } } else { - // no matching key, probably from a broken cross-storage move - - $shouldBeEncrypted = $file->getStorage()->instanceOfStorage(Encryption::class); - $isActuallyEncrypted = $this->isDataEncrypted($file); - if ($isActuallyEncrypted) { - if ($dryRun) { - if ($shouldBeEncrypted) { - $output->write('' . $file->getPath() . ' needs migration'); - } else { - $output->write('' . $file->getPath() . ' needs decryption'); - } - $foundKey = $this->findUserKeyForSystemFile($user, $file); - if ($foundKey) { - $output->writeln(', valid key found at ' . $foundKey . ''); - } else { - $output->writeln(' ❌ No key found'); - } - } else { - if ($shouldBeEncrypted) { - $output->write('Migrating key for ' . $file->getPath() . ''); - } else { - $output->write('Decrypting ' . $file->getPath() . ''); - } - $foundKey = $this->findUserKeyForSystemFile($user, $file); - if ($foundKey) { - if ($shouldBeEncrypted) { - $systemKeyPath = $this->getSystemKeyPath($file); - $this->rootView->copy($foundKey, $systemKeyPath); - $output->writeln(' Migrated key from ' . $foundKey . ''); - } else { - $this->decryptWithSystemKey($file, $foundKey); - $output->writeln(' Decrypted with key from ' . $foundKey . ''); - } - } else { - $output->writeln(' ❌ No key found'); - } - } + if ($shouldBeEncrypted) { + $output->write('Migrating key for ' . $file->getPath() . ''); } else { - if ($dryRun) { - $output->writeln('' . $file->getPath() . ' needs to be marked as not encrypted'); + $output->write('Decrypting ' . $file->getPath() . ''); + } + $foundKey = $this->findUserKeyForSystemFile($user, $file); + if ($foundKey) { + if ($shouldBeEncrypted) { + $systemKeyPath = $this->getSystemKeyPath($file); + $this->rootView->copy($foundKey, $systemKeyPath); + $output->writeln(' Migrated key from ' . $foundKey . ''); } else { - $this->markAsUnEncrypted($file); - $output->writeln('' . $file->getPath() . ' marked as not encrypted'); + $this->decryptWithSystemKey($file, $foundKey); + $output->writeln(' Decrypted with key from ' . $foundKey . ''); } + } else { + $output->writeln(' ❌ No key found'); } } + } else { + if ($dryRun) { + $output->writeln('' . $file->getPath() . ' needs to be marked as not encrypted'); + } else { + $this->markAsUnEncrypted($file); + $output->writeln('' . $file->getPath() . ' marked as not encrypted'); + } } } } - - return self::SUCCESS; } private function getUserRelativePath(string $path): string { @@ -168,7 +267,7 @@ private function getUserRelativePath(string $path): string { /** * @return ICachedMountInfo[] */ - private function getSystemMountsForUser(IUser $user): array { + protected function getSystemMountsForUser(IUser $user): array { return array_filter($this->userMountCache->getMountsForUser($user), function (ICachedMountInfo $mount) use ( $user ) { @@ -241,15 +340,21 @@ private function copyUserKeyToSystemAndValidate(IUser $user, File $node): bool { private function tryReadFile(File $node): bool { try { - $fh = $node->fopen('r'); - // read a single chunk - $data = fread($fh, 8192); - if ($data === false) { + // a raw read on an unwrapped mount would succeed with any key + $storage = $node->getStorage(); + if (!$storage->instanceOfStorage(Encryption::class)) { + $storage = $this->encryptionManager->forceWrapStorage($node->getMountPoint(), $storage); + } + $fh = $storage->fopen($node->getInternalPath(), 'r'); + if ($fh === false) { return false; - } else { - return true; } - } catch (\Exception $e) { + $data = fread($fh, 8192); + fclose($fh); + // a broken unencrypted_size of 0 makes the stream return nothing at all + // instead of failing, an empty read proves nothing about the key + return $data !== false && $data !== ''; + } catch (\Exception) { return false; } } @@ -298,22 +403,51 @@ private function isDataEncrypted(File $node): bool { * Attempt to find a key (stored for user) for a file (that needs a system key) even when it's not stored in the expected location */ private function findUserKeyForSystemFile(IUser $user, File $node): ?string { - $userKeyPath = $this->getUserBaseKeyPath($user); - $possibleKeys = $this->findKeysByFileName($userKeyPath, $node->getName()); - foreach ($possibleKeys as $possibleKey) { - if ($this->testSystemKey($user, $possibleKey, $node)) { - return $possibleKey; + return $this->findKeyInUserTrees($user, $node, $this->getSystemKeyPath($node)); + } + + /** + * Search the key trees of all users for a key that decrypts the file, the tree of + * the given user first. Candidates are matched by file name and validated by a + * decryption attempt with the key staged at the given path. + */ + private function findKeyInUserTrees(IUser $user, File $node, string $stageKeyPath): ?string { + foreach ($this->getUserBaseKeyPaths($user) as $basePath) { + foreach ($this->findKeysByFileName($basePath, $node->getName()) as $possibleKey) { + if ($this->testKeyAtPath($node, $possibleKey, $stageKeyPath)) { + return $possibleKey; + } } } return null; } + /** + * Base key paths of all users, the given user first. Users without a key tree are + * skipped cheaply by the key search. + * + * @return \Generator + */ + private function getUserBaseKeyPaths(IUser $firstUser): \Generator { + yield $this->getUserBaseKeyPath($firstUser); + + foreach ($this->userManager->search('') as $user) { + if ($user->getUID() !== $firstUser->getUID()) { + yield $this->keyRootDirectory . '/' . $user->getUID() . '/files_encryption/keys'; + } + } + } + /** * Attempt to find a key for a file even when it's not stored in the expected location * * @return \Generator */ private function findKeysByFileName(string $basePath, string $name) { + if (!$this->rootView->is_dir($basePath)) { + // no keys stored for the user at all + return; + } if ($this->rootView->is_dir($basePath . '/' . $name . '/OC_DEFAULT_MODULE')) { yield $basePath . '/' . $name; } else { @@ -327,6 +461,7 @@ private function findKeysByFileName(string $basePath, string $name) { $childPath = $basePath . '/' . $child; // recurse if the child is not a key folder + /** @psalm-suppress InternalMethod */ if ($this->rootView->is_dir($childPath) && !is_dir($childPath . '/OC_DEFAULT_MODULE')) { yield from $this->findKeysByFileName($childPath, $name); } @@ -336,19 +471,17 @@ private function findKeysByFileName(string $basePath, string $name) { } /** - * Test if the provided key is valid as a system key for the file + * Test whether the key decrypts the file when staged at the given key path */ - private function testSystemKey(IUser $user, string $key, File $node): bool { - $systemKeyPath = $this->getSystemKeyPath($node); - - if ($this->rootView->file_exists($systemKeyPath)) { + private function testKeyAtPath(File $node, string $key, string $stageKeyPath): bool { + if ($this->rootView->file_exists($stageKeyPath)) { // already has a key, reject new key return false; } - $this->rootView->copy($key, $systemKeyPath); + $this->rootView->copy($key, $stageKeyPath); $isValid = $this->tryReadFile($node); - $this->rootView->rmdir($systemKeyPath); + $this->rootView->rmdir($stageKeyPath); return $isValid; } @@ -363,6 +496,7 @@ private function decryptWithSystemKey(File $node, string $key): void { $systemKeyPath = $this->getSystemKeyPath($node); $this->rootView->copy($key, $systemKeyPath); + $decryptedNode = null; try { if (!$storage->instanceOfStorage(Encryption::class)) { $storage = $this->encryptionManager->forceWrapStorage($node->getMountPoint(), $storage); @@ -380,25 +514,44 @@ private function decryptWithSystemKey(File $node, string $key): void { fclose($source); $decryptedNode->getStorage()->getScanner()->scan($decryptedNode->getInternalPath()); - } catch (\Exception $e) { + + if ($this->isDataEncrypted($decryptedNode)) { + throw new \Exception($node->getPath() . ' still encrypted after attempting to decrypt with ' . $key); + } + // a broken unencrypted_size of 0 makes the decryption stream produce + // nothing at all, an empty result for a non empty source is data loss + if ($decryptedNode->getSize() === 0 && $node->getSize() > 0) { + throw new \Exception($node->getPath() . ' decrypted to an empty file, refusing the result'); + } + } catch (\Throwable $e) { + // the target has to go first so the .bak can move back onto its name + if ($decryptedNode !== null) { + try { + $decryptedNode->delete(); + } catch (\Throwable $cleanupError) { + $this->logger->warning('Failed to remove the partial decryption target ' . $decryptedNode->getPath(), [ + 'app' => 'encryption', + 'exception' => $cleanupError, + ]); + } + } $this->rootView->rmdir($systemKeyPath); - // remove the .bak + // move the backup back onto the original name $node->move(substr($node->getPath(), 0, -4)); throw $e; } - if ($this->isDataEncrypted($decryptedNode)) { - throw new \Exception($node->getPath() . ' still encrypted after attempting to decrypt with ' . $key); - } - $this->markAsUnEncrypted($decryptedNode); $this->rootView->rmdir($systemKeyPath); } private function markAsUnEncrypted(Node $node): void { - $node->getStorage()->getCache()->update($node->getId(), ['encrypted' => 0]); + $node->getStorage()->getCache()->update($node->getId(), [ + 'encrypted' => 0, + 'unencrypted_size' => 0, + ]); } } diff --git a/apps/encryption/tests/Command/FixKeyLocationTest.php b/apps/encryption/tests/Command/FixKeyLocationTest.php new file mode 100644 index 0000000000000..25970b100a750 --- /dev/null +++ b/apps/encryption/tests/Command/FixKeyLocationTest.php @@ -0,0 +1,310 @@ +systemMounts; + } +} + +#[\PHPUnit\Framework\Attributes\Group(name: 'DB')] +class FixKeyLocationTest extends TestCase { + use MountProviderTrait; + use EncryptionTrait; + use UserTrait; + + /** + * One mount with the encryption wrapper and one without, so ciphertext can be + * placed on a mount that cannot decrypt it, like a broken cross storage move + * leaves it behind. + * + * @return array{view: View, encryptedBackingStorage: Temporary, strayStorage: TemporaryUnwrapped} + */ + private function setUpMounts(): array { + Server::get(KeyManager::class)->validateMasterKey(); + Server::get(KeyManager::class)->validateShareKey(); + $this->createUser('test1', 'test2'); + $this->setupForUser('test1', 'test2'); + + $encryptedBacking = new Temporary(); + $stray = new TemporaryUnwrapped(); + + $this->registerMount('test1', $encryptedBacking, '/test1/files/enc'); + $this->registerMount('test1', $stray, '/test1/files/stray'); + + $this->loginWithEncryption('test1'); + + return [ + 'view' => new View('/test1/files'), + 'encryptedBackingStorage' => $encryptedBacking, + 'strayStorage' => $stray, + ]; + } + + private function getCommand(): TestableFixKeyLocation { + return new TestableFixKeyLocation( + Server::get(IUserManager::class), + Server::get(IUserMountCache::class), + Server::get(EncryptionUtil::class), + Server::get(IRootFolder::class), + Server::get(ISetupManager::class), + Server::get(LoggerInterface::class), + Server::get(IManager::class), + ); + } + + private function markAsEncrypted(TemporaryUnwrapped $storage, string $path, int $unencryptedSize): void { + $cache = $storage->getCache(); + $cache->update($cache->get($path)->getId(), [ + 'encrypted' => 1, + 'unencrypted_size' => $unencryptedSize, + ]); + } + + /** + * A second user whose key tree can hold misplaced keys. The key tree is created + * directly, no login needed, the encryption session of the first user stays + * untouched. + */ + private function setUpSecondUser(): void { + $this->createUser('test2', 'test2'); + } + + private function moveKeyToSecondUserTree(string $keyPath, string $name): void { + $rootView = new View(); + foreach (['/test2', '/test2/files_encryption', '/test2/files_encryption/keys'] as $dir) { + if (!$rootView->file_exists($dir)) { + $rootView->mkdir($dir); + } + } + $rootView->rename(rtrim($keyPath, '/'), '/test2/files_encryption/keys/' . $name); + } + + /** + * Key validation must read through the encryption wrapper: a raw read succeeds + * with any key, so on an unwrapped mount the first candidate key would always + * "validate" and a wrong key only fails later, during the actual decryption. + */ + public function testKeyValidationReadsThroughEncryption(): void { + [ + 'view' => $view, + 'encryptedBackingStorage' => $encryptedBackingStorage, + ] = $this->setUpMounts(); + + $view->file_put_contents('enc/original.txt', 'secret content'); + // ciphertext under a path that has no key + $view->file_put_contents('stray/stray.txt', $encryptedBackingStorage->file_get_contents('original.txt')); + $strayStorage = $view->getFileInfo('stray/stray.txt')->getStorage(); + $this->markAsEncrypted($strayStorage, 'stray.txt', strlen('secret content')); + + $userFolder = Server::get(IRootFolder::class)->getUserFolder('test1'); + $command = $this->getCommand(); + + $this->assertFalse( + self::invokePrivate($command, 'tryReadFile', [$userFolder->get('stray/stray.txt')]), + 'ciphertext without a key must not validate' + ); + $this->assertTrue( + self::invokePrivate($command, 'tryReadFile', [$userFolder->get('enc/original.txt')]), + 'a readable encrypted file must validate' + ); + } + + /** + * A failed decryption must roll everything back: no .bak left behind, no partial + * target, no temporary system key. + */ + public function testFailedDecryptionRollsBack(): void { + [ + 'view' => $view, + 'encryptedBackingStorage' => $encryptedBackingStorage, + ] = $this->setUpMounts(); + + $view->file_put_contents('enc/original.txt', 'secret content'); + $view->file_put_contents('enc/other.txt', 'other content'); + $cipher = $encryptedBackingStorage->file_get_contents('original.txt'); + $view->file_put_contents('stray/broken.txt', $cipher); + $strayStorage = $view->getFileInfo('stray/broken.txt')->getStorage(); + $this->markAsEncrypted($strayStorage, 'broken.txt', strlen('secret content')); + + $userFolder = Server::get(IRootFolder::class)->getUserFolder('test1'); + $strayNode = $userFolder->get('stray/broken.txt'); + $command = $this->getCommand(); + $user = Server::get(IUserManager::class)->get('test1'); + + // the key of a different file cannot decrypt this ciphertext + $wrongKey = self::invokePrivate($command, 'getUserKeyPath', [$user, $userFolder->get('enc/other.txt')]); + $rootView = new View(); + $this->assertTrue($rootView->file_exists($wrongKey), 'test setup: key of the other file has to exist'); + // the decryption reads under the .bak name, stage the wrong key where the + // wrapper will look for it so the failure is a real signature mismatch + $brokenKeyPath = self::invokePrivate($command, 'getUserKeyPath', [$user, $strayNode]); + $rootView->copy($wrongKey, str_replace('broken.txt', 'broken.txt.bak', $brokenKeyPath)); + + $threw = false; + try { + self::invokePrivate($command, 'decryptWithSystemKey', [$strayNode, $wrongKey]); + } catch (\Exception) { + $threw = true; + } + $this->assertTrue($threw, 'decrypting with the wrong key must fail'); + + $this->assertTrue($view->file_exists('stray/broken.txt'), 'the original file has to be restored'); + $this->assertSame($cipher, $view->file_get_contents('stray/broken.txt'), 'the original content has to be intact'); + $this->assertFalse($view->file_exists('stray/broken.txt.bak'), 'no .bak must be left behind'); + + $systemKeyPath = self::invokePrivate($command, 'getSystemKeyPath', [$strayNode]); + $systemKeyPathBak = str_replace('broken.txt', 'broken.txt.bak', $systemKeyPath); + $this->assertFalse($rootView->file_exists($systemKeyPath), 'no temporary system key must be left behind'); + $this->assertFalse($rootView->file_exists($systemKeyPathBak), 'no temporary system key must be left behind'); + } + + /** + * A key that only exists in the tree of another user must be found and validated. + * The harness mounts are not system wide, the encryption wrapper resolves the keys + * of the stray file through the user tree, so the search is exercised with that + * staging path, the system wide flow only stages at a different location. + */ + public function testKeyFoundInAnotherUsersTree(): void { + [ + 'view' => $view, + 'encryptedBackingStorage' => $encryptedBackingStorage, + ] = $this->setUpMounts(); + $this->setUpSecondUser(); + + $view->file_put_contents('enc/original.txt', 'secret content'); + $view->file_put_contents('stray/orphan.txt', $encryptedBackingStorage->file_get_contents('original.txt')); + $strayStorage = $view->getFileInfo('stray/orphan.txt')->getStorage(); + $this->markAsEncrypted($strayStorage, 'orphan.txt', strlen('secret content')); + + $command = $this->getCommand(); + $user = Server::get(IUserManager::class)->get('test1'); + $userFolder = Server::get(IRootFolder::class)->getUserFolder('test1'); + $originalKey = self::invokePrivate($command, 'getUserKeyPath', [$user, $userFolder->get('enc/original.txt')]); + // the key sits in ANOTHER user's tree, under the name of the stray file + $this->moveKeyToSecondUserTree($originalKey, 'orphan.txt'); + + $strayNode = $userFolder->get('stray/orphan.txt'); + $stagePath = self::invokePrivate($command, 'getUserKeyPath', [$user, $strayNode]); + $foundKey = self::invokePrivate($command, 'findKeyInUserTrees', [$user, $strayNode, $stagePath]); + + $this->assertNotNull($foundKey, 'the key in the other tree has to be found'); + $this->assertStringContainsString('/test2/', $foundKey); + } + + /** + * With --personal an encrypted file in the personal space whose key was lost is + * restored from another user's tree, healthy files stay untouched. + */ + public function testPersonalFileKeyFoundInAnotherUsersTree(): void { + $this->setUpMounts(); + $this->setUpSecondUser(); + + $view = new View('/test1/files'); + $view->file_put_contents('personal.txt', 'personal content'); + $view->file_put_contents('healthy.txt', 'healthy content'); + + $command = $this->getCommand(); + $user = Server::get(IUserManager::class)->get('test1'); + $userFolder = Server::get(IRootFolder::class)->getUserFolder('test1'); + $personalKey = self::invokePrivate($command, 'getUserKeyPath', [$user, $userFolder->get('personal.txt')]); + $this->moveKeyToSecondUserTree($personalKey, 'personal.txt'); + + $command->systemMounts = []; + $tester = new CommandTester($command); + $exitCode = $tester->execute(['user' => 'test1', '--personal' => true]); + $display = $tester->getDisplay(); + + $this->assertSame(Command::SUCCESS, $exitCode, $display); + $this->assertStringContainsString('Migrated key from', $display); + $this->assertEquals('personal content', $view->file_get_contents('personal.txt')); + $rootView = new View(); + $this->assertTrue($rootView->file_exists($personalKey), 'the key has to be back at the path of the file'); + $this->assertStringNotContainsString('healthy.txt', $display, 'healthy files must not be touched'); + } + + /** + * One broken file must not abort the whole run, the remaining files still get + * processed and the failure is reported. + */ + public function testExecuteContinuesAfterFailure(): void { + [ + 'view' => $view, + ] = $this->setUpMounts(); + + $view->file_put_contents('stray/a-ghost.txt', 'gone'); + $strayStorage = $view->getFileInfo('stray/a-ghost.txt')->getStorage(); + $this->markAsEncrypted($strayStorage, 'a-ghost.txt', strlen('gone')); + // cache row without a backing file, reading it fails like an object store 404 + $strayStorage->unlink('a-ghost.txt'); + + $view->file_put_contents('stray/b-plain.txt', 'plain data'); + $this->markAsEncrypted($strayStorage, 'b-plain.txt', strlen('plain data')); + + // ciphertext without any key while the user has no key directory at all, + // the key search has to come up empty instead of erroring out + $view->file_put_contents('stray/c-cipher.txt', 'HBEGIN:oc_encryption_module:OC_DEFAULT_MODULE:HEND'); + $this->markAsEncrypted($strayStorage, 'c-cipher.txt', 8); + + $command = $this->getCommand(); + $mount = $this->createMock(ICachedMountInfo::class); + $mount->method('getMountPoint')->willReturn('/test1/files/stray/'); + $command->systemMounts = [$mount]; + + $tester = new CommandTester($command); + $exitCode = $tester->execute(['user' => 'test1']); + $display = $tester->getDisplay(); + + $this->assertSame(Command::FAILURE, $exitCode, 'failures have to be reflected in the exit code'); + $this->assertStringContainsString('Failed to process', $display); + $this->assertStringContainsString('could not be processed', $display); + $this->assertStringContainsString(' - /test1/files/stray/a-ghost.txt', $display, 'the summary has to name the affected file'); + $this->assertFalse( + $strayStorage->getCache()->get('b-plain.txt')->isEncrypted(), + 'the remaining file was not processed, the run stopped at the broken one' + ); + $this->assertSame( + 0, + (int)$strayStorage->getCache()->get('b-plain.txt')->getData()['unencrypted_size'], + 'clearing the mark has to reset the leftover unencrypted size as well' + ); + $this->assertStringContainsString('No key found', $display, 'a missing key directory has to read as "no candidates"'); + $this->assertStringNotContainsString('c-cipher.txt: ', $display, 'the missing key directory failed the file'); + } +}