Skip to content

[stable32] fix(files): Prevent corruption of files when moving from encrypted to unencrypted folders within the same object storage #62479

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
backportbot wants to merge 1 commit into
base: stable32
Choose a base branch
from
+196 −2
Draft

[stable32] fix(files): Prevent corruption of files when moving from encrypted to unencrypted folders within the same object storage #62479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
152 changes: 152 additions & 0 deletions apps/encryption/tests/EncryptedStorageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -24,6 +27,10 @@ class TemporaryNoEncrypted extends Temporary implements IDisableEncryptionStorag

}

class ObjectStoreNoEncrypted extends ObjectStoreStorage implements IDisableEncryptionStorage {

}

/**
* @group DB
*/
Expand Down Expand Up @@ -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;
}
}
38 changes: 38 additions & 0 deletions lib/private/Files/ObjectStore/ObjectStoreStorage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()) {
Expand All @@ -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()
Expand Down Expand Up @@ -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
*/
Expand Down
8 changes: 6 additions & 2 deletions lib/private/Files/Storage/Common.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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);
}
Expand Down
Loading