Skip to content

Commit 25a12a5

Browse files
DerDreschnerDerDreschner
authored
Merge pull request #63098 from nextcloud/backport/62364/stable32
[stable32] fix(files): Prevent corruption of files when moving from encrypted to unencrypted folders within the same object storage
2 parents e045b56 + 528de9f commit 25a12a5

3 files changed

Lines changed: 196 additions & 2 deletions

File tree

‎apps/encryption/tests/EncryptedStorageTest.php‎

Lines changed: 152 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;
@@ -24,6 +27,10 @@ class TemporaryNoEncrypted extends Temporary implements IDisableEncryptionStorag
2427

2528
}
2629

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

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use OC\Files\Cache\Cache;
1616
use OC\Files\Cache\CacheEntry;
1717
use OC\Files\Storage\PolyFill\CopyDirectory;
18+
use OC\Files\Storage\Wrapper\Encryption;
1819
use OCP\Files\Cache\ICache;
1920
use OCP\Files\Cache\ICacheEntry;
2021
use OCP\Files\Cache\IScanner;
@@ -569,6 +570,14 @@ public function copyFromStorage(
569570
string $targetInternalPath,
570571
bool $preserveMtime = false,
571572
): bool {
573+
// the shortcuts below copy the object verbatim, an encrypted source has to be
574+
// read through its encryption wrapper instead
575+
if ($sourceStorage->instanceOfStorage(Encryption::class)
576+
&& $this->sourceMayContainEncryptedContent($sourceStorage->getCache()->get($sourceInternalPath))
577+
) {
578+
return parent::copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
579+
}
580+
572581
if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) {
573582
/** @var ObjectStoreStorage $sourceStorage */
574583
if ($sourceStorage->getObjectStore()->getStorageId() === $this->getObjectStore()->getStorageId()) {
@@ -591,6 +600,19 @@ public function copyFromStorage(
591600

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

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

‎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)