From 6655a2a2285e2c2bb80f468fe0b2bb5c09a1f3e1 Mon Sep 17 00:00:00 2001 From: Robin Delattre Date: Sun, 11 Oct 2026 21:46:17 +0200 Subject: [PATCH] perf(crypto): reuse the key for consecutive operations Assisted-by: Codex:GPT-6 --- lib/private/Security/Crypto.php | 26 +++++-- tests/lib/Security/CryptoTest.php | 123 ++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 8 deletions(-) diff --git a/lib/private/Security/Crypto.php b/lib/private/Security/Crypto.php index 8db44ecee00b6..821d20808fd3b 100644 --- a/lib/private/Security/Crypto.php +++ b/lib/private/Security/Crypto.php @@ -28,6 +28,7 @@ */ class Crypto implements ICrypto { private AES $cipher; + private ?string $cipherPasswordHash = null; private int $ivLength = 16; public function __construct( @@ -77,11 +78,7 @@ public function encrypt( $password = $this->config->getSystemValueString('secret'); } $keyMaterial = hash_hkdf('sha512', $password); - // Pin the PBKDF2 parameters to phpseclib v2 defaults ('sha1' hash, 'phpseclib' salt). - // phpseclib v3 changed the default salt to 'phpseclib/salt', which would derive a - // different AES key and break decryption of previously stored ciphertexts. - // TODO: We should put our own salt into the HKDF derivation to avoid this dependency on phpseclib's defaults! - $this->cipher->setPassword(substr($keyMaterial, 0, 32), 'pbkdf2', 'sha1', 'phpseclib'); + $this->setCipherPassword(substr($keyMaterial, 0, 32)); $iv = \random_bytes($this->ivLength); $this->cipher->setIV($iv); @@ -176,9 +173,7 @@ private function decryptWithoutSecret(string $authenticatedCiphertext, string $p $hmacKey = substr($keyMaterial, 32); } } - // Match the v2-compatible PBKDF2 parameters used in encrypt(), see the note there. - // TODO: We should put our own salt into the HKDF derivation to avoid this dependency on phpseclib's defaults! - $this->cipher->setPassword($encryptionKey, 'pbkdf2', 'sha1', 'phpseclib'); + $this->setCipherPassword($encryptionKey); $this->cipher->setIV($iv); if ($isOwnCloudV2Migration) { @@ -200,6 +195,21 @@ private function decryptWithoutSecret(string $authenticatedCiphertext, string $p return $result; } + private function setCipherPassword(#[SensitiveParameter] string $password): void { + // Keep only one fingerprint; the derived key remains in the cipher. + $passwordHash = hash('sha256', $password, true); + if ($this->cipherPasswordHash !== null && hash_equals($this->cipherPasswordHash, $passwordHash)) { + return; + } + + // Pin the PBKDF2 parameters to phpseclib v2 defaults ('sha1' hash, 'phpseclib' salt). + // phpseclib v3 changed the default salt to 'phpseclib/salt', which would derive a + // different AES key and break decryption of previously stored ciphertexts. + // TODO: We should put our own salt into the HKDF derivation to avoid this dependency on phpseclib's defaults! + $this->cipher->setPassword($password, 'pbkdf2', 'sha1', 'phpseclib'); + $this->cipherPasswordHash = $passwordHash; + } + private function hex2bin(string $hex): string { if (!ctype_xdigit($hex)) { throw new \RuntimeException('String contains non hex chars: ' . $hex); diff --git a/tests/lib/Security/CryptoTest.php b/tests/lib/Security/CryptoTest.php index 4c1035996fd70..536bd60516d0a 100644 --- a/tests/lib/Security/CryptoTest.php +++ b/tests/lib/Security/CryptoTest.php @@ -13,6 +13,7 @@ use OC\Security\Crypto; use OCP\IConfig; use OCP\Server; +use phpseclib3\Crypt\AES; class CryptoTest extends \Test\TestCase { public static function defaultEncryptionProvider(): array { @@ -129,4 +130,126 @@ public function testVersion3CiphertextDecryptsToCorrectPlaintext(): void { ) ); } + + public static function consecutivePasswordProvider(): array { + return [[''], ['explicit-password']]; + } + + #[\PHPUnit\Framework\Attributes\DataProvider('consecutivePasswordProvider')] + public function testDecryptConsecutiveFieldsDerivesKeyOnce(string $password): void { + $writer = $this->createCryptoWithDerivationCount(1); + $ciphertexts = []; + for ($field = 0; $field < 6; $field++) { + $ciphertexts[] = $writer->encrypt('field ' . $field, $password); + } + + $reader = $this->createCryptoWithDerivationCount(1); + foreach ($ciphertexts as $field => $ciphertext) { + $this->assertSame('field ' . $field, $reader->decrypt($ciphertext, $password)); + } + } + + public function testAlternatingPasswordsDeriveAgain(): void { + $first = $this->crypto->encrypt('first', 'password-A'); + $second = $this->crypto->encrypt('second', 'password-B'); + $reader = $this->createCryptoWithDerivationCount(3); + + $this->assertSame('first', $reader->decrypt($first, 'password-A')); + $this->assertSame('second', $reader->decrypt($second, 'password-B')); + $this->assertSame('first', $reader->decrypt($first, 'password-A')); + } + + public function testAlternatingEncryptDecryptKeepsCurrentKeyAndRefreshesIv(): void { + $crypto = $this->createCryptoWithDerivationCount(1); + $ivs = []; + for ($field = 0; $field < 6; $field++) { + $ciphertext = $crypto->encrypt('same plaintext', 'same-password'); + $ivs[] = explode('|', $ciphertext)[1]; + $this->assertSame('same plaintext', $crypto->decrypt($ciphertext, 'same-password')); + } + $this->assertCount(6, array_unique($ivs)); + } + + public function testLegacyAndVersion3DeriveDifferentKeys(): void { + $legacy = 'e16599188e3d212f5c7f17fdc2abca46|M1WfLAxbcAmITeD6|509457885d6ca5e6c3bfd3741852687a7f2bffce197f8d5ae97b65818b15a1b7f616b68326ff312371540f4ca8ac55f8e2de4aa13aab3474bd3431e51214e3ee'; + $current = $this->crypto->encrypt('current', 'mypass'); + $reader = $this->createCryptoWithDerivationCount(3); + + $this->assertSame('current', $reader->decrypt($current, 'mypass')); + $this->assertSame('legacy test', $reader->decrypt($legacy, 'mypass')); + $this->assertSame('current', $reader->decrypt($current, 'mypass')); + } + + public function testReusedKeyStillRejectsTamperedCiphertextIvAndHmac(): void { + $ciphertext = $this->crypto->encrypt('plaintext', 'password'); + $reader = $this->createCryptoWithDerivationCount(1); + $this->assertSame('plaintext', $reader->decrypt($ciphertext, 'password')); + + foreach ([0, 1, 2] as $part) { + $parts = explode('|', $ciphertext); + $parts[$part][0] = $parts[$part][0] === '0' ? '1' : '0'; + try { + $reader->decrypt(implode('|', $parts), 'password'); + $this->fail('Tampered ciphertext was accepted'); + } catch (\RuntimeException $e) { + $this->assertSame('HMAC does not match.', $e->getMessage()); + } + $this->assertSame('plaintext', $reader->decrypt($ciphertext, 'password')); + } + } + + public function testWrongPasswordDoesNotPoisonKeyReuse(): void { + $ciphertext = $this->crypto->encrypt('plaintext', 'password'); + $reader = $this->createCryptoWithDerivationCount(3); + $this->assertSame('plaintext', $reader->decrypt($ciphertext, 'password')); + try { + $reader->decrypt($ciphertext, 'wrong-password'); + $this->fail('Wrong password was accepted'); + } catch (\RuntimeException $e) { + $this->assertSame('HMAC does not match.', $e->getMessage()); + } + $this->assertSame('plaintext', $reader->decrypt($ciphertext, 'password')); + } + + public function testChangedDefaultSecretDerivesAgain(): void { + $first = $this->crypto->encrypt('first', 'secret-A'); + $second = $this->crypto->encrypt('second', 'secret-B'); + $config = $this->createMock(IConfig::class); + $config->method('getSystemValue')->with('secret')->willReturnOnConsecutiveCalls('secret-A', 'secret-B'); + $reader = $this->createCryptoWithDerivationCount(2, $config); + + $this->assertSame('first', $reader->decrypt($first)); + $this->assertSame('second', $reader->decrypt($second)); + } + + public function testLegacyEmptySecretFallbackCanReplaceTheCurrentKey(): void { + $reader = $this->createCryptoWithDerivationCount(2); + $cipher = new AES('cbc'); + $cipher->setPassword('', 'pbkdf2', 'sha1', 'phpseclib'); + $iv = str_repeat("\0", 16); + $cipher->setIV($iv); + $ciphertext = bin2hex($cipher->encrypt('empty-key fixture')); + $iv = bin2hex($iv); + $hmac = bin2hex($reader->calculateHMAC($ciphertext . $iv, 'configured-secret')); + + $this->assertSame('empty-key fixture', $reader->decrypt($ciphertext . '|' . $iv . '|' . $hmac . '|2')); + } + + private function createCryptoWithDerivationCount(int $count, ?IConfig $config = null): Crypto { + if ($config === null) { + $config = $this->createMock(IConfig::class); + $config->method('getSystemValue')->with('secret')->willReturn('configured-secret'); + $config->method('getSystemValueString')->with('secret')->willReturn('configured-secret'); + } + $crypto = new Crypto($config); + $cipher = $this->getMockBuilder(AES::class) + ->setConstructorArgs(['cbc']) + ->onlyMethods(['setPassword']) + ->getMock(); + $cipher->expects($this->exactly($count)) + ->method('setPassword') + ->willReturnCallback(fn (...$parameters) => (new \ReflectionMethod(AES::class, 'setPassword'))->invokeArgs($cipher, $parameters)); + $this->invokePrivate($crypto, 'cipher', [$cipher]); + return $crypto; + } }