Skip to content

Commit b1b4b06

Browse files
AndyScherzingerAndyScherzinger
authored
Merge pull request #63019 from nextcloud/backport/62364/stable33
[stable33] fix(files): Prevent corruption of files when moving from encrypted to unencrypted folders within the same object storage
2 parents 48ea022 + 3c66efd commit b1b4b06

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

‎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\DB\QueryBuilder\IQueryBuilder;
1920
use OCP\Files\Cache\ICache;
2021
use OCP\Files\Cache\ICacheEntry;
@@ -577,6 +578,14 @@ public function copyFromStorage(
577578
string $targetInternalPath,
578579
bool $preserveMtime = false,
579580
): bool {
581+
// the shortcuts below copy the object verbatim, an encrypted source has to be
582+
// read through its encryption wrapper instead
583+
if ($sourceStorage->instanceOfStorage(Encryption::class)
584+
&& $this->sourceMayContainEncryptedContent($sourceStorage->getCache()->get($sourceInternalPath))
585+
) {
586+
return parent::copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
587+
}
588+
580589
if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) {
581590
/** @var ObjectStoreStorage $sourceStorage */
582591
if ($sourceStorage->getObjectStore()->getStorageId() === $this->getObjectStore()->getStorageId()) {
@@ -599,6 +608,19 @@ public function copyFromStorage(
599608

600609
public function moveFromStorage(IStorage $sourceStorage, string $sourceInternalPath, string $targetInternalPath, ?ICacheEntry $sourceCacheEntry = null): bool {
601610
$sourceCache = $sourceStorage->getCache();
611+
612+
// An encrypted source has to be read through its encryption wrapper: the metadata
613+
// only move below would leave the ciphertext untouched, and copyObjects() reuses
614+
// the source file id, which resolves to the same object on a shared object store.
615+
if ($sourceStorage->instanceOfStorage(Encryption::class)) {
616+
if (!$sourceCacheEntry) {
617+
$sourceCacheEntry = $sourceCache->get($sourceInternalPath);
618+
}
619+
if ($this->sourceMayContainEncryptedContent($sourceCacheEntry)) {
620+
return parent::moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
621+
}
622+
}
623+
602624
if (
603625
$sourceStorage->instanceOfStorage(ObjectStoreStorage::class)
604626
&& $sourceStorage->getObjectStore()->getStorageId() === $this->getObjectStore()->getStorageId()
@@ -638,6 +660,22 @@ public function moveFromStorage(IStorage $sourceStorage, string $sourceInternalP
638660
return true;
639661
}
640662

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

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

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

583583
$result = $this->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath, true);
584584
if ($result) {
585-
if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) {
585+
// keeping the source cache entry preserves the file id when leaving an object
586+
// store, but between two object stores it would leave a dangling entry behind
587+
$preserveCacheOnDelete = $sourceStorage->instanceOfStorage(ObjectStoreStorage::class)
588+
&& !$this->instanceOfStorage(ObjectStoreStorage::class);
589+
if ($preserveCacheOnDelete) {
586590
/** @var ObjectStoreStorage $sourceStorage */
587591
$sourceStorage->setPreserveCacheOnDelete(true);
588592
}
@@ -593,7 +597,7 @@ public function moveFromStorage(IStorage $sourceStorage, string $sourceInternalP
593597
$result = $sourceStorage->unlink($sourceInternalPath);
594598
}
595599
} finally {
596-
if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) {
600+
if ($preserveCacheOnDelete) {
597601
/** @var ObjectStoreStorage $sourceStorage */
598602
$sourceStorage->setPreserveCacheOnDelete(false);
599603
}

0 commit comments

Comments
 (0)