open-dsi
/
nextcloud-server
mirror of https://github.com/nextcloud/server
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Merge pull request #62364 from nextcloud/fix/prevent-s3-corruption

Browse Source 
fix(files): Prevent corruption of files when moving from encrypted to unencrypted folders within the same object storage
pull/61580/merge
David Dreschner 2 weeks ago committed by GitHub
parent e0cccd3a78 672542f587
commit 1ba2971c46
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 196 additions and 2 deletions
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Show Stats Download Patch File Download Diff File
  1. 152
      apps/encryption/tests/EncryptedStorageTest.php
  2. 38
      lib/private/Files/ObjectStore/ObjectStoreStorage.php
  3. 8
      lib/private/Files/Storage/Common.php

152
apps/encryption/tests/EncryptedStorageTest.php
Unescape View File

@ -8,11 +8,14 @@ declare(strict_types=1);
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 {
}
#[\PHPUnit\Framework\Attributes\Group(name: 'DB')]
class EncryptedStorageTest extends TestCase {
use MountProviderTrait;
@ -69,4 +76,149 @@ class EncryptedStorageTest extends TestCase {
$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
lib/private/Files/ObjectStore/ObjectStoreStorage.php
Unescape View File

@ -17,6 +17,7 @@ use OC\Files\Cache\Cache;
use OC\Files\Cache\CacheEntry;
use OC\Files\Storage\Common;
use OC\Files\Storage\PolyFill\CopyDirectory;
use OC\Files\Storage\Wrapper\Encryption;
use OCP\Constants;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\Files\Cache\ICache;
@ -601,6 +602,14 @@ class ObjectStoreStorage extends Common implements IChunkedFileWrite {
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()) {
@ -624,6 +633,19 @@ class ObjectStoreStorage extends Common implements IChunkedFileWrite {
#[\Override]
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()
@ -663,6 +685,22 @@ class ObjectStoreStorage extends Common implements IChunkedFileWrite {
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
*/

8
lib/private/Files/Storage/Common.php
Unescape View File

@ -624,7 +624,11 @@ abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage,
$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);
}
@ -635,7 +639,7 @@ abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage,
$result = $sourceStorage->unlink($sourceInternalPath);
}
} finally {
if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) {
if ($preserveCacheOnDelete) {
/** @var ObjectStoreStorage $sourceStorage */
$sourceStorage->setPreserveCacheOnDelete(false);
}

Write Preview
Loading…
Cancel
Save