Harden SSE key generation

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()
  );
  });
 

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,14 +125,16 @@ public function __construct(
  IUserSession $userSession,
  Session $session,
  ILogger $log,
- Util $util
+ Util $util,
+ ILockingProvider $lockingProvider
  ) {
  $this->util = $util;
  $this->session = $session;
  $this->keyStorage = $keyStorage;
  $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 public master key is available but the private 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);
  }
 }

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;
  }

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'],

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,

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,