Skip to content

Commit 9ca45d5

Browse files
DerDreschnerbackportbot[bot]
DerDreschner
authored and
backportbot[bot]
committed
fix(files): Prevent corruption of files when moving from encrypted to unencrypted folders within the same object storage
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 <david.dreschner@nextcloud.com> [skip ci]
1 parent 907168a commit 9ca45d5

3 files changed

Lines changed: 191 additions & 2 deletions

File tree

‎apps/encryption/tests/EncryptedStorageTest.php‎

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,14 @@
88

99
namespace OCA\encryption\tests;
1010

11+
use OC\Files\ObjectStore\ObjectStoreStorage;
12+
use OC\Files\ObjectStore\StorageObjectStore;
1113
use OC\Files\Storage\Temporary;
1214
use OC\Files\Storage\Wrapper\Encryption;
1315
use OC\Files\View;
1416
use OCA\Encryption\KeyManager;
1517
use OCP\Files\Mount\IMountManager;
18+
use OCP\Files\ObjectStore\IObjectStore;
1619
use OCP\Files\Storage\IDisableEncryptionStorage;
1720
use OCP\Server;
1821
use Test\TestCase;
@@ -71,4 +74,149 @@ public function testMoveFromEncrypted(): void {
7174
$this->assertEquals('bar', $unencryptedStorage->file_get_contents('foo.txt'));
7275
$this->assertFalse($unencryptedCache->get('foo.txt')->isEncrypted());
7376
}
77+
78+
/**
79+
* The metadata only move between storages sharing an object store must not be taken
80+
* for an encrypted source: the ciphertext would stay in the object store while the
81+
* cache entry loses its `encrypted` mark.
82+
*/
83+
public function testMoveFromEncryptedObjectStore(): void {
84+
[
85+
'view' => $view,
86+
'objectStore' => $objectStore,
87+
'unencryptedStorage' => $unencryptedStorage,
88+
] = $this->setUpSharedObjectStoreMounts();
89+
90+
$view->file_put_contents('enc/foo.txt', 'bar');
91+
$this->assertEquals('bar', $view->file_get_contents('enc/foo.txt'));
92+
93+
$view->rename('enc/foo.txt', 'unenc/foo.txt');
94+
95+
$this->assertEquals('bar', $view->file_get_contents('unenc/foo.txt'));
96+
$this->assertFalse($unencryptedStorage->getCache()->get('foo.txt')->isEncrypted());
97+
$this->assertStringStartsNotWith(
98+
'HBEGIN:',
99+
$this->readRawObject($objectStore, $unencryptedStorage, 'foo.txt'),
100+
'the object was moved verbatim and is still encrypted at rest'
101+
);
102+
// a move must not leave the source behind, neither on disk nor in the cache
103+
$this->assertFalse($view->file_exists('enc/foo.txt'), 'the source file still exists after the move');
104+
}
105+
106+
/**
107+
* Same as above for the copy shortcut, which hands the ciphertext to the object
108+
* store's server side copy.
109+
*/
110+
public function testCopyFromEncryptedObjectStore(): void {
111+
[
112+
'view' => $view,
113+
'objectStore' => $objectStore,
114+
'unencryptedStorage' => $unencryptedStorage,
115+
] = $this->setUpSharedObjectStoreMounts();
116+
117+
$view->file_put_contents('enc/foo.txt', 'bar');
118+
119+
$view->copy('enc/foo.txt', 'unenc/foo.txt');
120+
121+
$this->assertEquals('bar', $view->file_get_contents('enc/foo.txt'));
122+
$this->assertEquals('bar', $view->file_get_contents('unenc/foo.txt'));
123+
$this->assertFalse($unencryptedStorage->getCache()->get('foo.txt')->isEncrypted());
124+
$this->assertStringStartsNotWith(
125+
'HBEGIN:',
126+
$this->readRawObject($objectStore, $unencryptedStorage, 'foo.txt'),
127+
'the object was copied verbatim and is still encrypted at rest'
128+
);
129+
}
130+
131+
/**
132+
* A file without the `encrypted` mark holds plain content even on a wrapped storage
133+
* (only some paths encrypt, e.g. not uploads/) and must keep the metadata only move.
134+
*/
135+
public function testMoveUnencryptedFileFromEncryptionWrappedObjectStore(): void {
136+
[
137+
'view' => $view,
138+
'unencryptedStorage' => $unencryptedStorage,
139+
'encryptedBackingStorage' => $encryptedBackingStorage,
140+
] = $this->setUpSharedObjectStoreMounts();
141+
142+
// bypasses the encryption wrapper: plain content, no `encrypted` mark
143+
$encryptedBackingStorage->file_put_contents('plain.txt', 'plain content');
144+
$sourceEntry = $encryptedBackingStorage->getCache()->get('plain.txt');
145+
$this->assertFalse($sourceEntry->isEncrypted());
146+
147+
$view->rename('enc/plain.txt', 'unenc/plain.txt');
148+
149+
$this->assertEquals('plain content', $view->file_get_contents('unenc/plain.txt'));
150+
$this->assertSame(
151+
$sourceEntry->getId(),
152+
$unencryptedStorage->getCache()->get('plain.txt')->getId(),
153+
'a plain file must keep the metadata only move that preserves the file id'
154+
);
155+
$this->assertFalse($view->file_exists('enc/plain.txt'), 'the source file still exists after the move');
156+
}
157+
158+
/**
159+
* A folder carries no `encrypted` mark of its own while any of its children may be
160+
* encrypted, so a folder move must always take the encryption aware path.
161+
*/
162+
public function testMoveFolderFromEncryptedObjectStore(): void {
163+
[
164+
'view' => $view,
165+
'objectStore' => $objectStore,
166+
'unencryptedStorage' => $unencryptedStorage,
167+
] = $this->setUpSharedObjectStoreMounts();
168+
169+
$view->mkdir('enc/dir');
170+
$view->file_put_contents('enc/dir/foo.txt', 'bar');
171+
172+
$view->rename('enc/dir', 'unenc/dir');
173+
174+
$this->assertEquals('bar', $view->file_get_contents('unenc/dir/foo.txt'));
175+
$this->assertFalse($unencryptedStorage->getCache()->get('dir/foo.txt')->isEncrypted());
176+
$this->assertStringStartsNotWith(
177+
'HBEGIN:',
178+
$this->readRawObject($objectStore, $unencryptedStorage, 'dir/foo.txt'),
179+
'the folder took the metadata only move and left the child encrypted at rest'
180+
);
181+
$this->assertFalse($view->file_exists('enc/dir'), 'the source folder still exists after the move');
182+
}
183+
184+
/**
185+
* Two object store storages backed by the same object store, one mounted with and one
186+
* without the encryption wrapper.
187+
*
188+
* @return array{view: View, objectStore: IObjectStore, unencryptedStorage: ObjectStoreStorage, encryptedBackingStorage: ObjectStoreStorage}
189+
*/
190+
private function setUpSharedObjectStoreMounts(): array {
191+
Server::get(KeyManager::class)->validateMasterKey();
192+
Server::get(KeyManager::class)->validateShareKey();
193+
$this->createUser('test1', 'test2');
194+
$this->setupForUser('test1', 'test2');
195+
196+
// a shared object store instance makes the storage ids match, enabling the shortcuts
197+
$objectStore = new StorageObjectStore(new Temporary());
198+
$encrypted = new ObjectStoreStorage(['objectstore' => $objectStore, 'storageid' => 'test-enc']);
199+
$unencrypted = new ObjectStoreNoEncrypted(['objectstore' => $objectStore, 'storageid' => 'test-unenc']);
200+
201+
$this->registerMount('test1', $encrypted, '/test1/files/enc');
202+
$this->registerMount('test1', $unencrypted, '/test1/files/unenc');
203+
204+
$this->loginWithEncryption('test1');
205+
206+
return [
207+
'view' => new View('/test1/files'),
208+
'objectStore' => $objectStore,
209+
'unencryptedStorage' => $unencrypted,
210+
'encryptedBackingStorage' => $encrypted,
211+
];
212+
}
213+
214+
private function readRawObject(IObjectStore $objectStore, ObjectStoreStorage $storage, string $path): string {
215+
$fileId = $storage->getCache()->get($path)->getId();
216+
$handle = $objectStore->readObject($storage->getURN($fileId));
217+
$content = stream_get_contents($handle);
218+
fclose($handle);
219+
220+
return $content;
221+
}
74222
}

‎lib/private/Files/ObjectStore/ObjectStoreStorage.php‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -569,6 +569,14 @@ public function copyFromStorage(
569569
string $targetInternalPath,
570570
bool $preserveMtime = false,
571571
): bool {
572+
// the shortcuts below copy the object verbatim, an encrypted source has to be
573+
// read through its encryption wrapper instead
574+
if ($sourceStorage->instanceOfStorage(Encryption::class)
575+
&& $this->sourceMayContainEncryptedContent($sourceStorage->getCache()->get($sourceInternalPath))
576+
) {
577+
return parent::copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
578+
}
579+
572580
if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) {
573581
/** @var ObjectStoreStorage $sourceStorage */
574582
if ($sourceStorage->getObjectStore()->getStorageId() === $this->getObjectStore()->getStorageId()) {
@@ -591,6 +599,19 @@ public function copyFromStorage(
591599

592600
public function moveFromStorage(IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath, ?ICacheEntry $sourceCacheEntry = null): bool {
593601
$sourceCache = $sourceStorage->getCache();
602+
603+
// An encrypted source has to be read through its encryption wrapper: the metadata
604+
// only move below would leave the ciphertext untouched, and copyObjects() reuses
605+
// the source file id, which resolves to the same object on a shared object store.
606+
if ($sourceStorage->instanceOfStorage(Encryption::class)) {
607+
if (!$sourceCacheEntry) {
608+
$sourceCacheEntry = $sourceCache->get($sourceInternalPath);
609+
}
610+
if ($this->sourceMayContainEncryptedContent($sourceCacheEntry)) {
611+
return parent::moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
612+
}
613+
}
614+
594615
if (
595616
$sourceStorage->instanceOfStorage(ObjectStoreStorage::class)
596617
&& $sourceStorage->getObjectStore()->getStorageId() === $this->getObjectStore()->getStorageId()
@@ -630,6 +651,22 @@ public function moveFromStorage(IStorage $sourceStorage, string $sourceInternalP
630651
return true;
631652
}
632653

654+
/**
655+
* The encryption wrapper covers a whole storage while only some of its paths are
656+
* encrypted (files/ but not e.g. uploads/), so the wrapper alone is too coarse a
657+
* signal for skipping the raw object shortcuts. Folders and unreadable cache
658+
* entries count as encrypted, a folder's own flag says nothing about its children.
659+
*/
660+
private function sourceMayContainEncryptedContent(ICacheEntry|false|null $sourceCacheEntry): bool {
661+
if (!$sourceCacheEntry instanceof ICacheEntry) {
662+
return true;
663+
}
664+
if ($sourceCacheEntry->getMimeType() === ICacheEntry::DIRECTORY_MIMETYPE) {
665+
return true;
666+
}
667+
return $sourceCacheEntry->isEncrypted();
668+
}
669+
633670
/**
634671
* Copy the object(s) of a file or folder into this storage, without touching the cache
635672
*/

‎lib/private/Files/Storage/Common.php‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -580,7 +580,11 @@ public function moveFromStorage(IStorage $sourceStorage, string $sourceInternalP
580580

581581
$result = $this->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath, true);
582582
if ($result) {
583-
if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) {
583+
// keeping the source cache entry preserves the file id when leaving an object
584+
// store, but between two object stores it would leave a dangling entry behind
585+
$preserveCacheOnDelete = $sourceStorage->instanceOfStorage(ObjectStoreStorage::class)
586+
&& !$this->instanceOfStorage(ObjectStoreStorage::class);
587+
if ($preserveCacheOnDelete) {
584588
/** @var ObjectStoreStorage $sourceStorage */
585589
$sourceStorage->setPreserveCacheOnDelete(true);
586590
}
@@ -591,7 +595,7 @@ public function moveFromStorage(IStorage $sourceStorage, string $sourceInternalP
591595
$result = $sourceStorage->unlink($sourceInternalPath);
592596
}
593597
} finally {
594-
if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) {
598+
if ($preserveCacheOnDelete) {
595599
/** @var ObjectStoreStorage $sourceStorage */
596600
$sourceStorage->setPreserveCacheOnDelete(false);
597601
}

0 commit comments

Comments
 (0)