From 95cd524771e9982464b070e2c55d04878319fd5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 4 Jun 2024 17:20:20 +0200 Subject: [PATCH 1/3] fix: Autodetect legacy filekey instead of trusting the header for legacy header MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- apps/encryption/lib/Crypto/Encryption.php | 20 ++++---------------- apps/encryption/lib/KeyManager.php | 11 +++++------ 2 files changed, 9 insertions(+), 22 deletions(-) diff --git a/apps/encryption/lib/Crypto/Encryption.php b/apps/encryption/lib/Crypto/Encryption.php index 1dd1d91c218..dda93e13306 100644 --- a/apps/encryption/lib/Crypto/Encryption.php +++ b/apps/encryption/lib/Crypto/Encryption.php @@ -54,8 +54,6 @@ class Encryption implements IEncryptionModule { /** @var int Current version of the file */ private int $version = 0; - private bool $useLegacyFileKey = true; - /** @var array remember encryption signature version */ private static $rememberVersion = []; @@ -112,7 +110,6 @@ class Encryption implements IEncryptionModule { $this->writeCache = ''; $this->useLegacyBase64Encoding = true; - $this->useLegacyFileKey = ($header['useLegacyFileKey'] ?? 'true') !== 'false'; if (isset($header['encoding'])) { $this->useLegacyBase64Encoding = $header['encoding'] !== Crypt::BINARY_ENCODING_FORMAT; @@ -126,19 +123,10 @@ class Encryption implements IEncryptionModule { } } - if ($this->session->decryptAllModeActivated()) { - $shareKey = $this->keyManager->getShareKey($this->path, $this->session->getDecryptAllUid()); - if ($this->useLegacyFileKey) { - $encryptedFileKey = $this->keyManager->getEncryptedFileKey($this->path); - $this->fileKey = $this->crypt->multiKeyDecryptLegacy($encryptedFileKey, - $shareKey, - $this->session->getDecryptAllKey()); - } else { - $this->fileKey = $this->crypt->multiKeyDecrypt($shareKey, $this->session->getDecryptAllKey()); - } - } else { - $this->fileKey = $this->keyManager->getFileKey($this->path, $this->user, $this->useLegacyFileKey); - } + /* If useLegacyFileKey is not specified in header, auto-detect, to be safe */ + $useLegacyFileKey = (($header['useLegacyFileKey'] ?? '') == 'false' ? false : null); + + $this->fileKey = $this->keyManager->getFileKey($this->path, $this->user, $useLegacyFileKey, $this->session->decryptAllModeActivated()); // always use the version from the original file, also part files // need to have a correct version number if they get moved over to the diff --git a/apps/encryption/lib/KeyManager.php b/apps/encryption/lib/KeyManager.php index 6771db7947b..b6a0d2b6ea9 100644 --- a/apps/encryption/lib/KeyManager.php +++ b/apps/encryption/lib/KeyManager.php @@ -343,12 +343,9 @@ class KeyManager { } /** - * @param string $path - * @param $uid * @param ?bool $useLegacyFileKey null means try both - * @return string */ - public function getFileKey(string $path, ?string $uid, ?bool $useLegacyFileKey): string { + public function getFileKey(string $path, ?string $uid, ?bool $useLegacyFileKey, bool $useDecryptAll): string { if ($uid === '') { $uid = null; } @@ -361,8 +358,10 @@ class KeyManager { return ''; } } - - if ($this->util->isMasterKeyEnabled()) { + if ($useDecryptAll) { + $shareKey = $this->getShareKey($path, $this->session->getDecryptAllUid()); + $privateKey = $this->session->getDecryptAllKey(); + } elseif ($this->util->isMasterKeyEnabled()) { $uid = $this->getMasterKeyId(); $shareKey = $this->getShareKey($path, $uid); if ($publicAccess) { From 885604ce2de36ea85854db645694eb797ab7785c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 6 Jun 2024 10:23:03 +0200 Subject: [PATCH 2/3] fix: add default value for new flag `$useDecryptAll` on getFileKey MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- apps/encryption/lib/KeyManager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/encryption/lib/KeyManager.php b/apps/encryption/lib/KeyManager.php index b6a0d2b6ea9..9fd6c7655af 100644 --- a/apps/encryption/lib/KeyManager.php +++ b/apps/encryption/lib/KeyManager.php @@ -345,7 +345,7 @@ class KeyManager { /** * @param ?bool $useLegacyFileKey null means try both */ - public function getFileKey(string $path, ?string $uid, ?bool $useLegacyFileKey, bool $useDecryptAll): string { + public function getFileKey(string $path, ?string $uid, ?bool $useLegacyFileKey, bool $useDecryptAll = false): string { if ($uid === '') { $uid = null; } From f244261e85813d3afa251e12cb640f883b9740d6 Mon Sep 17 00:00:00 2001 From: Louis Chemineau Date: Thu, 6 Jun 2024 18:02:32 +0200 Subject: [PATCH 3/3] test: Fix encryption test Signed-off-by: Louis Chemineau --- .../tests/Crypto/EncryptionTest.php | 35 ++++++++----------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/apps/encryption/tests/Crypto/EncryptionTest.php b/apps/encryption/tests/Crypto/EncryptionTest.php index 122990da611..4767d22339d 100644 --- a/apps/encryption/tests/Crypto/EncryptionTest.php +++ b/apps/encryption/tests/Crypto/EncryptionTest.php @@ -103,6 +103,10 @@ class EncryptionTest extends TestCase { * test if public key from one of the recipients is missing */ public function testEndUser1() { + $this->sessionMock->expects($this->once()) + ->method('decryptAllModeActivated') + ->willReturn(false); + $this->instance->begin('/foo/bar', 'user1', 'r', [], ['users' => ['user1', 'user2', 'user3']]); $this->endTest(); } @@ -112,6 +116,10 @@ class EncryptionTest extends TestCase { * */ public function testEndUser2() { + $this->sessionMock->expects($this->once()) + ->method('decryptAllModeActivated') + ->willReturn(false); + $this->expectException(\OCA\Encryption\Exceptions\PublicKeyMissingException::class); $this->instance->begin('/foo/bar', 'user2', 'r', [], ['users' => ['user1', 'user2', 'user3']]); @@ -233,35 +241,16 @@ class EncryptionTest extends TestCase { */ public function testBeginDecryptAll() { $path = '/user/files/foo.txt'; - $recoveryKeyId = 'recoveryKeyId'; - $recoveryShareKey = 'recoveryShareKey'; - $decryptAllKey = 'decryptAllKey'; $fileKey = 'fileKey'; $this->sessionMock->expects($this->once()) ->method('decryptAllModeActivated') ->willReturn(true); - $this->sessionMock->expects($this->once()) - ->method('getDecryptAllUid') - ->willReturn($recoveryKeyId); - $this->sessionMock->expects($this->once()) - ->method('getDecryptAllKey') - ->willReturn($decryptAllKey); - - $this->keyManagerMock->expects($this->once()) - ->method('getEncryptedFileKey') - ->willReturn('encryptedFileKey'); $this->keyManagerMock->expects($this->once()) - ->method('getShareKey') - ->with($path, $recoveryKeyId) - ->willReturn($recoveryShareKey); - $this->cryptMock->expects($this->once()) - ->method('multiKeyDecryptLegacy') - ->with('encryptedFileKey', $recoveryShareKey, $decryptAllKey) + ->method('getFileKey') + ->with($path, 'user', null, true) ->willReturn($fileKey); - $this->keyManagerMock->expects($this->never())->method('getFileKey'); - $this->instance->begin($path, 'user', 'r', [], []); $this->assertSame($fileKey, @@ -275,6 +264,10 @@ class EncryptionTest extends TestCase { * and continue */ public function testBeginInitMasterKey() { + $this->sessionMock->expects($this->once()) + ->method('decryptAllModeActivated') + ->willReturn(false); + $this->sessionMock->expects($this->once())->method('isReady')->willReturn(false); $this->utilMock->expects($this->once())->method('isMasterKeyEnabled') ->willReturn(true);