From e5ae4aaaab706c141ef9a637ca3869dc882a8af0 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Tue, 15 Sep 2026 19:00:49 +0200 Subject: [PATCH] fix(encryption): keep the encrypted version the copy was written with A copy re-encrypts the target, so the signature of its blocks is keyed on the version of the target - the version of the file it overwrites plus one - and not on the version of the source. Two writers overwrite that value after the stream recorded it: updateEncryptedVersion() resets it to 1 for every copy, and Cache::copyFromCache() then puts the source's version on the target. Reading the copy back fails with "Bad Signature" whenever those differ, which is the case for every copy of a file that was written more than once and for every copy onto an existing file. Take the version the stream recorded for the target instead of resetting it, and let it win over the source's version when the cache entry of the copy is written. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Ferdinand Thiessen --- apps/encryption/tests/EncryptedCopyTest.php | 139 ++++++++++++++++++ .../encryption_features/encryption.feature | 22 +++ lib/private/Files/Cache/Cache.php | 8 + .../Files/Storage/Wrapper/Encryption.php | 25 +++- .../Files/Storage/Wrapper/EncryptionTest.php | 69 +++++++++ 5 files changed, 257 insertions(+), 6 deletions(-) create mode 100644 apps/encryption/tests/EncryptedCopyTest.php diff --git a/apps/encryption/tests/EncryptedCopyTest.php b/apps/encryption/tests/EncryptedCopyTest.php new file mode 100644 index 0000000000000..6aa44509af277 --- /dev/null +++ b/apps/encryption/tests/EncryptedCopyTest.php @@ -0,0 +1,139 @@ +validateMasterKey(); + Server::get(KeyManager::class)->validateShareKey(); + $this->createUser('test1', 'test2'); + $this->setupForUser('test1', 'test2'); + $this->registerMount('test1', new Temporary(), '/test1/files/other'); + $this->loginWithEncryption('test1'); + + return new View('/test1/files'); + } + + /** + * The version the target ends up at depends on the storage: a copy unlinks the + * target first, which keeps the cache entry - and with it the version to bump - + * on a local storage but drops it on an object store, where the copy therefore + * starts over at version 1. Reading the target back is what shows that the + * recorded version is the one its blocks were signed with. + */ + public function testCopyOverExistingFile(): void { + $view = $this->setUpView(); + $source = str_repeat('a', 20000); + + $view->file_put_contents('source.bin', $source); + $view->file_put_contents('target.bin', str_repeat('b', 20000)); + + $this->assertTrue($view->copy('source.bin', 'target.bin')); + + $this->assertEquals($source, $view->file_get_contents('target.bin')); + } + + /** + * Every write bumps the version of the source, while the copy of it starts over + * at the version of the target. + */ + public function testCopyFileWrittenSeveralTimes(): void { + $view = $this->setUpView(); + $source = str_repeat('c', 20000); + + $view->file_put_contents('source.bin', str_repeat('a', 20000)); + $view->file_put_contents('source.bin', str_repeat('b', 20000)); + $view->file_put_contents('source.bin', $source); + + $this->assertTrue($view->copy('source.bin', 'target.bin')); + + $this->assertEquals($source, $view->file_get_contents('target.bin')); + } + + public function testCopyOverExistingFileWrittenSeveralTimes(): void { + $view = $this->setUpView(); + $source = str_repeat('a', 20000); + + $view->file_put_contents('source.bin', $source); + $view->file_put_contents('target.bin', str_repeat('b', 20000)); + $view->file_put_contents('target.bin', str_repeat('c', 20000)); + + $this->assertTrue($view->copy('source.bin', 'target.bin')); + + $this->assertEquals($source, $view->file_get_contents('target.bin')); + } + + public function testCopyFolderOverExistingFolder(): void { + $view = $this->setUpView(); + $source = str_repeat('a', 20000); + + $view->mkdir('source'); + $view->file_put_contents('source/file.bin', $source); + $view->mkdir('target'); + $view->file_put_contents('target/file.bin', str_repeat('b', 20000)); + + $this->assertTrue($view->copy('source', 'target')); + + $this->assertEquals($source, $view->file_get_contents('target/file.bin')); + } + + public function testMoveOverExistingFileOnAnotherStorage(): void { + $view = $this->setUpView(); + $source = str_repeat('a', 20000); + + $view->file_put_contents('source.bin', $source); + $view->file_put_contents('other/target.bin', str_repeat('b', 20000)); + + $this->assertTrue($view->rename('source.bin', 'other/target.bin')); + + $this->assertEquals($source, $view->file_get_contents('other/target.bin')); + } + + /** + * The target is not unlinked when it lives on another storage, so it keeps its + * cache entry and the copy is signed with the version that follows the one of + * the file it overwrites. + */ + public function testCopyOverExistingFileOnAnotherStorage(): void { + $view = $this->setUpView(); + $source = str_repeat('a', 20000); + + $view->file_put_contents('source.bin', $source); + $view->file_put_contents('other/target.bin', str_repeat('b', 20000)); + + $this->assertTrue($view->copy('source.bin', 'other/target.bin')); + + $this->assertEquals($source, $view->file_get_contents('other/target.bin')); + $this->assertEquals( + 2, + $view->getFileInfo('other/target.bin')->getEncryptedVersion(), + 'the version of the overwritten file was not bumped' + ); + } +} diff --git a/build/integration/encryption_features/encryption.feature b/build/integration/encryption_features/encryption.feature index 15704609d79df..d64d429e72ce9 100644 --- a/build/integration/encryption_features/encryption.feature +++ b/build/integration/encryption_features/encryption.feature @@ -29,6 +29,28 @@ Feature: encryption When Downloading file "/copy.bin" Then the HTTP status code should be "200" + Scenario: Copy a file over an existing file + Given user "user0" exists + And As an "user0" + And User "user0" uploads file with content "the source content" to "/source.txt" + And User "user0" uploads file with content "the target content" to "/target.txt" + When User "user0" copies file "/source.txt" to "/target.txt" + Then the HTTP status code should be "204" + When Downloading file "/target.txt" + Then the HTTP status code should be "200" + And Downloaded content should be "the source content" + + Scenario: Copy a file that was written several times + Given user "user0" exists + And As an "user0" + And User "user0" uploads file with content "the first content" to "/source.txt" + And User "user0" uploads file with content "the second content" to "/source.txt" + When User "user0" copies file "/source.txt" to "/copy.txt" + Then the HTTP status code should be "201" + When Downloading file "/copy.txt" + Then the HTTP status code should be "200" + And Downloaded content should be "the second content" + # With "part_file_in_storage" disabled the part file is written to the user # home while the target lives on another storage, so the upload has to read the # part file back to move it over. A part file never has a file cache entry, so diff --git a/lib/private/Files/Cache/Cache.php b/lib/private/Files/Cache/Cache.php index eec26ecc96608..3e22e274ae325 100644 --- a/lib/private/Files/Cache/Cache.php +++ b/lib/private/Files/Cache/Cache.php @@ -1281,6 +1281,14 @@ public function copyFromCache(ICache $sourceCache, ICacheEntry $sourceEntry, str // normalizeData() prefers 'encryptedVersion' over 'encrypted' when both are // set, so it has to be cleared too or the mark above gets ignored unset($data['encryptedVersion']); + } elseif (isset($data['encryptedVersion'])) { + // The storage re-encrypts the content it writes to the target, so the target + // is at its own version - the one recorded for it while it was written - and + // not at the version of the source. + $targetEntry = $this->get($targetPath); + if ($targetEntry !== false && !empty($targetEntry['encryptedVersion'])) { + $data['encryptedVersion'] = $targetEntry['encryptedVersion']; + } } $fileId = $this->put($targetPath, $data); diff --git a/lib/private/Files/Storage/Wrapper/Encryption.php b/lib/private/Files/Storage/Wrapper/Encryption.php index d358e660f1db5..624a8112b76bd 100644 --- a/lib/private/Files/Storage/Wrapper/Encryption.php +++ b/lib/private/Files/Storage/Wrapper/Encryption.php @@ -635,13 +635,26 @@ private function updateEncryptedVersion( // Rename of the cache already happened, so we do the cleanup on the target if ($sourceCacheEntry === false && $targetCacheEntry !== false) { - $encryptedVersion = $targetCacheEntry['encryptedVersion']; $isRename = false; - } elseif ($sourceCacheEntry === false) { - // a file that is not in the file cache, e.g. a part file, is at version 1 - $encryptedVersion = 1; + } + + if ($keepEncryptionVersion) { + // a 1:1 copy reuses the keys and the ciphertext of the source, so the + // target stays at the version of the source + if ($sourceCacheEntry !== false) { + $encryptedVersion = (int)($sourceCacheEntry['encryptedVersion'] ?? 0); + } elseif ($targetCacheEntry !== false) { + $encryptedVersion = (int)($targetCacheEntry['encryptedVersion'] ?? 0); + } else { + // a file that is not in the file cache, e.g. a part file, is at version 1 + $encryptedVersion = 1; + } } else { - $encryptedVersion = $sourceCacheEntry['encryptedVersion']; + // The target was written through the encryption stream, which signs the + // blocks with the version that follows the version of the file they + // replaced and records it on the target's cache entry. A target that has + // no cache entry was written at version 1. + $encryptedVersion = $targetCacheEntry === false ? 1 : (int)($targetCacheEntry['encryptedVersion'] ?? 0); } // In case of a move operation from an unencrypted to an encrypted @@ -649,7 +662,7 @@ private function updateEncryptedVersion( // correct value would be "1". Thus we manually set the value to "1" // for those cases. // See also https://github.com/owncloud/core/issues/23078 - if ($encryptedVersion === 0 || !$keepEncryptionVersion) { + if ($encryptedVersion === 0) { $encryptedVersion = 1; } diff --git a/tests/lib/Files/Storage/Wrapper/EncryptionTest.php b/tests/lib/Files/Storage/Wrapper/EncryptionTest.php index 0e22db203d1ce..2ed837236a272 100644 --- a/tests/lib/Files/Storage/Wrapper/EncryptionTest.php +++ b/tests/lib/Files/Storage/Wrapper/EncryptionTest.php @@ -935,6 +935,75 @@ public static function dataCopyBetweenStorage(): array { ]; } + public static function dataUpdateEncryptedVersion(): array { + return [ + // the target is written through the encryption stream, which signs its blocks + // with the version that follows the version of the file they replace + 'copy onto an existing file' => [['encryptedVersion' => 4], ['encryptedVersion' => 3], false, 3], + 'copy onto a new file' => [['encryptedVersion' => 4], false, false, 1], + 'copy onto a file that is not encrypted yet' => [['encryptedVersion' => 4], ['encryptedVersion' => 0], false, 1], + // a 1:1 copy reuses the keys and the ciphertext of the source + '1:1 copy' => [['encryptedVersion' => 5], false, true, 5], + '1:1 copy of a file that is not encrypted yet' => [['encryptedVersion' => 0], false, true, 1], + ]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('dataUpdateEncryptedVersion')] + public function testUpdateEncryptedVersion( + array|false $sourceCacheEntry, + array|false $targetCacheEntry, + bool $keepEncryptionVersion, + int $expectedVersion, + ): void { + $sourceCache = $this->createMock(ICache::class); + $sourceCache->method('get') + ->with('source.txt') + ->willReturn($sourceCacheEntry); + $sourceStorage = $this->createMock(\OC\Files\Storage\Storage::class); + $sourceStorage->method('getCache') + ->willReturn($sourceCache); + + $targetCache = $this->createMock(ICache::class); + $targetCache->method('get') + ->with('target.txt') + ->willReturn($targetCacheEntry); + $targetCache->expects($this->once()) + ->method('put') + ->with('target.txt', ['encrypted' => true, 'encryptedVersion' => $expectedVersion]); + + $instance = $this->getMockBuilder(Encryption::class) + ->setConstructorArgs( + [ + [ + 'storage' => $this->sourceStorage, + 'root' => 'foo', + 'mountPoint' => '/', + 'mount' => $this->mount + ], + $this->encryptionManager, + $this->util, + $this->logger, + $this->file, + null, + $this->keyStore, + $this->mountManager, + $this->arrayCache + ] + ) + ->onlyMethods(['getCache', 'getEncryptionModule']) + ->getMock(); + $instance->method('getCache')->willReturn($targetCache); + $instance->method('getEncryptionModule')->willReturn($this->encryptionModule); + + $this->encryptionManager->expects($this->any()) + ->method('isEnabled') + ->willReturn(true); + global $mockedMountPointEncryptionEnabled; + $mockedMountPointEncryptionEnabled = true; + + $this->invokePrivate($instance, 'updateEncryptedVersion', [$sourceStorage, 'source.txt', 'target.txt', false, $keepEncryptionVersion]); + } + public function testCopyBetweenStorageMinimumEncryptedVersion(): void { $storage2 = $this->createMock(\OC\Files\Storage\Storage::class);