From 36cfdd320bd766798930dc09acea74b27f58d95c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Wed, 22 Jul 2020 10:05:51 +0200 Subject: [PATCH 1/3] Harden key generation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There might be cases where multiple requests trigger the key generation at the same time and the instance ends up with a non-fitting public/private key pair. Therefore the whole key generation should be locked. Other than that this makes sure that user key generation return values are properly validated. Signed-off-by: Julius Härtl --- apps/encryption/lib/AppInfo/Application.php | 3 +- apps/encryption/lib/KeyManager.php | 104 ++++++++++++------ apps/encryption/lib/Users/Setup.php | 4 +- .../Controller/ChangePasswordController.php | 4 +- 4 files changed, 80 insertions(+), 35 deletions(-) diff --git a/apps/encryption/lib/AppInfo/Application.php b/apps/encryption/lib/AppInfo/Application.php index 688564887d3c0..1e3b2e43d787e 100644 --- a/apps/encryption/lib/AppInfo/Application.php +++ b/apps/encryption/lib/AppInfo/Application.php @@ -152,7 +152,8 @@ function (IAppContainer $c) { $server->getUserSession(), new Session($server->getSession()), $server->getLogger(), - $c->query('Util') + $c->query('Util'), + $server->getLockingProvider() ); }); diff --git a/apps/encryption/lib/KeyManager.php b/apps/encryption/lib/KeyManager.php index 09bcf9371d2c8..0cc29a6a6abd9 100644 --- a/apps/encryption/lib/KeyManager.php +++ b/apps/encryption/lib/KeyManager.php @@ -41,6 +41,7 @@ use OCP\IConfig; use OCP\ILogger; use OCP\IUserSession; +use OCP\Lock\ILockingProvider; class KeyManager { @@ -103,6 +104,11 @@ class KeyManager { */ private $util; + /** + * @var ILockingProvider + */ + private $lockingProvider; + /** * @param IStorage $keyStorage * @param Crypt $crypt @@ -119,7 +125,8 @@ public function __construct( IUserSession $userSession, Session $session, ILogger $log, - Util $util + Util $util, + ILockingProvider $lockingProvider ) { $this->util = $util; $this->session = $session; @@ -127,6 +134,7 @@ public function __construct( $this->crypt = $crypt; $this->config = $config; $this->log = $log; + $this->lockingProvider = $lockingProvider; $this->recoveryKeyId = $this->config->getAppValue('encryption', 'recoveryKeyId'); @@ -161,17 +169,24 @@ public function __construct( public function validateShareKey() { $shareKey = $this->getPublicShareKey(); if (empty($shareKey)) { - $keyPair = $this->crypt->createKeyPair(); - - // Save public key - $this->keyStorage->setSystemUserKey( - $this->publicShareKeyId . '.publicKey', $keyPair['publicKey'], - Encryption::ID); - - // Encrypt private key empty passphrase - $encryptedKey = $this->crypt->encryptPrivateKey($keyPair['privateKey'], ''); - $header = $this->crypt->generateHeader(); - $this->setSystemPrivateKey($this->publicShareKeyId, $header . $encryptedKey); + $this->lockingProvider->acquireLock('encryption-generateSharedKey', ILockingProvider::LOCK_EXCLUSIVE, 'Encryption: shared key generation'); + try { + $keyPair = $this->crypt->createKeyPair(); + + // Save public key + $this->keyStorage->setSystemUserKey( + $this->publicShareKeyId . '.' . $this->publicKeyId, $keyPair['publicKey'], + Encryption::ID); + + // Encrypt private key empty passphrase + $encryptedKey = $this->crypt->encryptPrivateKey($keyPair['privateKey'], ''); + $header = $this->crypt->generateHeader(); + $this->setSystemPrivateKey($this->publicShareKeyId, $header . $encryptedKey); + } catch (\Throwable $e) { + $this->lockingProvider->releaseLock('encryption-generateSharedKey', ILockingProvider::LOCK_EXCLUSIVE); + throw $e; + } + $this->lockingProvider->releaseLock('encryption-generateSharedKey', ILockingProvider::LOCK_EXCLUSIVE); } } @@ -184,18 +199,36 @@ public function validateMasterKey() { } $publicMasterKey = $this->getPublicMasterKey(); - if (empty($publicMasterKey)) { - $keyPair = $this->crypt->createKeyPair(); - - // Save public key - $this->keyStorage->setSystemUserKey( - $this->masterKeyId . '.publicKey', $keyPair['publicKey'], - Encryption::ID); - - // Encrypt private key with system password - $encryptedKey = $this->crypt->encryptPrivateKey($keyPair['privateKey'], $this->getMasterKeyPassword(), $this->masterKeyId); - $header = $this->crypt->generateHeader(); - $this->setSystemPrivateKey($this->masterKeyId, $header . $encryptedKey); + $privateMasterKey = $this->getPrivateMasterKey(); + + if (empty($publicMasterKey) && empty($privateMasterKey)) { + // There could be a race condition here if two requests would trigger + // the generation the second one would enter the key generation as long + // as the first one didn't write the key to the keystorage yet + $this->lockingProvider->acquireLock('encryption-generateMasterKey', ILockingProvider::LOCK_EXCLUSIVE, 'Encryption: master key generation'); + try { + $keyPair = $this->crypt->createKeyPair(); + + // Save public key + $this->keyStorage->setSystemUserKey( + $this->masterKeyId . '.' . $this->publicKeyId, $keyPair['publicKey'], + Encryption::ID); + + // Encrypt private key with system password + $encryptedKey = $this->crypt->encryptPrivateKey($keyPair['privateKey'], $this->getMasterKeyPassword(), $this->masterKeyId); + $header = $this->crypt->generateHeader(); + $this->setSystemPrivateKey($this->masterKeyId, $header . $encryptedKey); + } catch (\Throwable $e) { + $this->lockingProvider->releaseLock('encryption-generateMasterKey', ILockingProvider::LOCK_EXCLUSIVE); + throw $e; + } + $this->lockingProvider->releaseLock('encryption-generateMasterKey', ILockingProvider::LOCK_EXCLUSIVE); + } elseif (empty($publicMasterKey)) { + $this->log->error('A private master key is available but the public key could not be found. This should never happen.'); + return; + } elseif (empty($privateMasterKey)) { + $this->log->error('A private master key is available but the public key could not be found. This should never happen.'); + return; } if (!$this->session->isPrivateKeySet()) { @@ -222,7 +255,7 @@ public function recoveryKeyExists() { * @return string */ public function getRecoveryKey() { - return $this->keyStorage->getSystemUserKey($this->recoveryKeyId . '.publicKey', Encryption::ID); + return $this->keyStorage->getSystemUserKey($this->recoveryKeyId . '.' . $this->publicKeyId, Encryption::ID); } /** @@ -239,7 +272,7 @@ public function getRecoveryKeyId() { * @return bool */ public function checkRecoveryPassword($password) { - $recoveryKey = $this->keyStorage->getSystemUserKey($this->recoveryKeyId . '.privateKey', Encryption::ID); + $recoveryKey = $this->keyStorage->getSystemUserKey($this->recoveryKeyId . '.' . $this->privateKeyId, Encryption::ID); $decryptedRecoveryKey = $this->crypt->decryptPrivateKey($recoveryKey, $password); if ($decryptedRecoveryKey) { @@ -251,7 +284,7 @@ public function checkRecoveryPassword($password) { /** * @param string $uid * @param string $password - * @param string $keyPair + * @param array $keyPair * @return bool */ public function storeKeyPair($uid, $password, $keyPair) { @@ -277,7 +310,7 @@ public function storeKeyPair($uid, $password, $keyPair) { public function setRecoveryKey($password, $keyPair) { // Save Public Key $this->keyStorage->setSystemUserKey($this->getRecoveryKeyId(). - '.publicKey', + '.' . $this->publicKeyId, $keyPair['publicKey'], Encryption::ID); @@ -435,7 +468,7 @@ public function getFileKey($path, $uid) { // use public share key for public links $uid = $this->getPublicShareKeyId(); $shareKey = $this->getShareKey($path, $uid); - $privateKey = $this->keyStorage->getSystemUserKey($this->publicShareKeyId . '.privateKey', Encryption::ID); + $privateKey = $this->keyStorage->getSystemUserKey($this->publicShareKeyId . '.' . $this->privateKeyId, Encryption::ID); $privateKey = $this->crypt->decryptPrivateKey($privateKey); } else { $shareKey = $this->getShareKey($path, $uid); @@ -578,7 +611,7 @@ public function getPublicShareKeyId() { * @return string */ public function getPublicShareKey() { - return $this->keyStorage->getSystemUserKey($this->publicShareKeyId . '.publicKey', Encryption::ID); + return $this->keyStorage->getSystemUserKey($this->publicShareKeyId . '.' . $this->publicKeyId, Encryption::ID); } /** @@ -718,6 +751,15 @@ public function getMasterKeyId() { * @return string */ public function getPublicMasterKey() { - return $this->keyStorage->getSystemUserKey($this->masterKeyId . '.publicKey', Encryption::ID); + return $this->keyStorage->getSystemUserKey($this->masterKeyId . '.' . $this->publicKeyId, Encryption::ID); + } + + /** + * get public master key + * + * @return string + */ + public function getPrivateMasterKey() { + return $this->keyStorage->getSystemUserKey($this->masterKeyId . '.' . $this->privateKeyId, Encryption::ID); } } diff --git a/apps/encryption/lib/Users/Setup.php b/apps/encryption/lib/Users/Setup.php index 5406c4e51cc97..e80435ac6988b 100644 --- a/apps/encryption/lib/Users/Setup.php +++ b/apps/encryption/lib/Users/Setup.php @@ -73,8 +73,8 @@ public function __construct(ILogger $logger, */ public function setupUser($uid, $password) { if (!$this->keyManager->userHasKeys($uid)) { - return $this->keyManager->storeKeyPair($uid, $password, - $this->crypt->createKeyPair()); + $keyPair = $this->crypt->createKeyPair(); + return is_array($keyPair) ? $this->keyManager->storeKeyPair($uid, $password, $keyPair) : false; } return true; } diff --git a/apps/settings/lib/Controller/ChangePasswordController.php b/apps/settings/lib/Controller/ChangePasswordController.php index 3006e89318fce..668b0e49e66f1 100644 --- a/apps/settings/lib/Controller/ChangePasswordController.php +++ b/apps/settings/lib/Controller/ChangePasswordController.php @@ -188,7 +188,9 @@ public function changeUserPassword(string $username = null, string $password = n \OC::$server->getUserSession(), new \OCA\Encryption\Session(\OC::$server->getSession()), \OC::$server->getLogger(), - $util); + $util, + \OC::$server->getLockingProvider() + ); $recovery = new \OCA\Encryption\Recovery( \OC::$server->getUserSession(), $crypt, From 00e62171d22a65c552aaff85a61f38cae6166d15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Mon, 27 Jul 2020 13:28:44 +0200 Subject: [PATCH 2/3] Test for locking state in key generation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- apps/encryption/tests/KeyManagerTest.php | 52 +++++++++++++++++++++-- apps/encryption/tests/Users/SetupTest.php | 4 +- 2 files changed, 51 insertions(+), 5 deletions(-) diff --git a/apps/encryption/tests/KeyManagerTest.php b/apps/encryption/tests/KeyManagerTest.php index 78c506a18d0d2..37d6c203da1a6 100644 --- a/apps/encryption/tests/KeyManagerTest.php +++ b/apps/encryption/tests/KeyManagerTest.php @@ -43,6 +43,9 @@ use OCP\IConfig; use OCP\ILogger; use OCP\IUserSession; +use OCP\Lock\ILockingProvider; +use OCP\Lock\LockedException; +use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; class KeyManagerTest extends TestCase { @@ -79,6 +82,9 @@ class KeyManagerTest extends TestCase { /** @var \OCP\IConfig|\PHPUnit\Framework\MockObject\MockObject */ private $configMock; + /** @var ILockingProvider|MockObject */ + private $lockingProviderMock; + protected function setUp(): void { parent::setUp(); $this->userId = 'user1'; @@ -99,6 +105,7 @@ protected function setUp(): void { $this->utilMock = $this->getMockBuilder(Util::class) ->disableOriginalConstructor() ->getMock(); + $this->lockingProviderMock = $this->createMock(ILockingProvider::class); $this->instance = new KeyManager( $this->keyStorageMock, @@ -107,7 +114,9 @@ protected function setUp(): void { $this->userMock, $this->sessionMock, $this->logMock, - $this->utilMock); + $this->utilMock, + $this->lockingProviderMock + ); } public function testDeleteShareKey() { @@ -269,7 +278,8 @@ public function testInit($useMasterKey) { $this->userMock, $this->sessionMock, $this->logMock, - $this->utilMock + $this->utilMock, + $this->lockingProviderMock ] )->setMethods(['getMasterKeyId', 'getMasterKeyPassword', 'getSystemPrivateKey', 'getPrivateKey']) ->getMock(); @@ -559,7 +569,8 @@ public function testValidateMasterKey($masterKey) { $this->userMock, $this->sessionMock, $this->logMock, - $this->utilMock + $this->utilMock, + $this->lockingProviderMock ] )->setMethods(['getPublicMasterKey', 'setSystemPrivateKey', 'getMasterKeyPassword']) ->getMock(); @@ -578,6 +589,8 @@ public function testValidateMasterKey($masterKey) { $this->cryptMock->expects($this->once())->method('encryptPrivateKey') ->with('private', 'masterKeyPassword', 'systemKeyId') ->willReturn('EncryptedKey'); + $this->lockingProviderMock->expects($this->once()) + ->method('acquireLock'); $instance->expects($this->once())->method('setSystemPrivateKey') ->with('systemKeyId', 'headerEncryptedKey'); } else { @@ -590,6 +603,39 @@ public function testValidateMasterKey($masterKey) { $instance->validateMasterKey(); } + public function testValidateMasterKeyLocked() { + /** @var \OCA\Encryption\KeyManager | \PHPUnit_Framework_MockObject_MockObject $instance */ + $instance = $this->getMockBuilder(KeyManager::class) + ->setConstructorArgs( + [ + $this->keyStorageMock, + $this->cryptMock, + $this->configMock, + $this->userMock, + $this->sessionMock, + $this->logMock, + $this->utilMock, + $this->lockingProviderMock + ] + )->setMethods(['getPublicMasterKey', 'getPrivateMasterKey', 'setSystemPrivateKey', 'getMasterKeyPassword']) + ->getMock(); + + $instance->expects($this->once())->method('getPublicMasterKey') + ->willReturn(''); + $instance->expects($this->once())->method('getPrivateMasterKey') + ->willReturn(''); + + $instance->expects($this->any())->method('getMasterKeyPassword')->willReturn('masterKeyPassword'); + $this->cryptMock->expects($this->any())->method('generateHeader')->willReturn('header'); + + $this->lockingProviderMock->expects($this->once()) + ->method('acquireLock') + ->willThrowException(new LockedException('encryption-generateMasterKey')); + + $this->expectException(LockedException::class); + $instance->validateMasterKey(); + } + public function dataTestValidateMasterKey() { return [ ['masterKey'], diff --git a/apps/encryption/tests/Users/SetupTest.php b/apps/encryption/tests/Users/SetupTest.php index 779bb5d82ea45..76c4647f7746f 100644 --- a/apps/encryption/tests/Users/SetupTest.php +++ b/apps/encryption/tests/Users/SetupTest.php @@ -90,9 +90,9 @@ public function testSetupUser($hasKeys, $expected) { if ($hasKeys) { $this->keyManagerMock->expects($this->never())->method('storeKeyPair'); } else { - $this->cryptMock->expects($this->once())->method('createKeyPair')->willReturn('keyPair'); + $this->cryptMock->expects($this->once())->method('createKeyPair')->willReturn(['publicKey' => 'publicKey', 'privateKey' => 'privateKey']); $this->keyManagerMock->expects($this->once())->method('storeKeyPair') - ->with('uid', 'password', 'keyPair')->willReturn(true); + ->with('uid', 'password', ['publicKey' => 'publicKey', 'privateKey' => 'privateKey'])->willReturn(true); } $this->assertSame($expected, From 5a064ec28b05ab1976543a9a68a699fba5eb4af2 Mon Sep 17 00:00:00 2001 From: Morris Jobke Date: Wed, 19 Aug 2020 20:42:27 +0200 Subject: [PATCH 3/3] Fix typo in error message Signed-off-by: Morris Jobke --- 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 0cc29a6a6abd9..b5c98bddf1ac1 100644 --- a/apps/encryption/lib/KeyManager.php +++ b/apps/encryption/lib/KeyManager.php @@ -227,7 +227,7 @@ public function validateMasterKey() { $this->log->error('A private master key is available but the public key could not be found. This should never happen.'); return; } elseif (empty($privateMasterKey)) { - $this->log->error('A private master key is available but the public key could not be found. This should never happen.'); + $this->log->error('A public master key is available but the private key could not be found. This should never happen.'); return; }