From f4dd89c563def52645a13bde8177a40267b4777a Mon Sep 17 00:00:00 2001 From: David Dreschner Date: Tue, 21 Jul 2026 14:22:09 +0200 Subject: [PATCH] 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 4b7a4979b36b9..40fabba91f9ee 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 { + +} + /** * @group DB */ @@ -71,4 +78,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 c98b5b1caa13c..8de81bcf8acf4 100644 --- a/lib/private/Files/ObjectStore/ObjectStoreStorage.php +++ b/lib/private/Files/ObjectStore/ObjectStoreStorage.php @@ -15,6 +15,7 @@ use OC\Files\Cache\Cache; use OC\Files\Cache\CacheEntry; use OC\Files\Storage\PolyFill\CopyDirectory; +use OC\Files\Storage\Wrapper\Encryption; use OCP\Files\Cache\ICache; use OCP\Files\Cache\ICacheEntry; use OCP\Files\Cache\IScanner; @@ -569,6 +570,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()) { @@ -591,6 +600,19 @@ public function copyFromStorage( 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() @@ -630,6 +652,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 106dc722c7f39..1488e3c3a552a 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -576,7 +576,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); } @@ -587,7 +591,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); }