Merge pull request #22018 from nextcloud/bugfix/noid/harden-key-generation

Harden SSE key generation

6 files changed, 131 insertions, 40 deletions

diff --git a/apps/encryption/lib/AppInfo/Application.php b/apps/encryption/lib/AppInfo/Application.php
index 688564887d3..1e3b2e43d78 100644
--- a/apps/encryption/lib/AppInfo/Application.php
+++ b/apps/encryption/lib/AppInfo/Application.php
@@ -152,7 +152,8 @@ class Application extends \OCP\AppFramework\App {
 					$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 09bcf9371d2..b5c98bddf1a 100644
--- a/apps/encryption/lib/KeyManager.php
+++ b/apps/encryption/lib/KeyManager.php
@@ -41,6 +41,7 @@ use OCP\Encryption\Keys\IStorage;
 use OCP\IConfig;
 use OCP\ILogger;
 use OCP\IUserSession;
+use OCP\Lock\ILockingProvider;
 
 class KeyManager {
 
@@ -104,6 +105,11 @@ class KeyManager {
 	private $util;
 
 	/**
+	 * @var ILockingProvider
+	 */
+	private $lockingProvider;
+
+	/**
 	 * @param IStorage $keyStorage
 	 * @param Crypt $crypt
 	 * @param IConfig $config
@@ -119,7 +125,8 @@ class KeyManager {
 		IUserSession $userSession,
 		Session $session,
 		ILogger $log,
-		Util $util
+		Util $util,
+		ILockingProvider $lockingProvider
 	) {
 		$this->util = $util;
 		$this->session = $session;
@@ -127,6 +134,7 @@ class KeyManager {
 		$this->crypt = $crypt;
 		$this->config = $config;
 		$this->log = $log;
+		$this->lockingProvider = $lockingProvider;
 
 		$this->recoveryKeyId = $this->config->getAppValue('encryption',
 			'recoveryKeyId');
@@ -161,17 +169,24 @@ class KeyManager {
 	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 @@ class KeyManager {
 		}
 
 		$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 @@ class KeyManager {
 	 * @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 @@ class KeyManager {
 	 * @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 @@ class KeyManager {
 	/**
 	 * @param string $uid
 	 * @param string $password
-	 * @param string $keyPair
+	 * @param array $keyPair
 	 * @return bool
 	 */
 	public function storeKeyPair($uid, $password, $keyPair) {
@@ -277,7 +310,7 @@ class KeyManager {
 	public function setRecoveryKey($password, $keyPair) {
 		// Save Public Key
 		$this->keyStorage->setSystemUserKey($this->getRecoveryKeyId().
-			'.publicKey',
+			'.' . $this->publicKeyId,
 			$keyPair['publicKey'],
 			Encryption::ID);
 
@@ -435,7 +468,7 @@ class KeyManager {
 			// 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 @@ class KeyManager {
 	 * @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 @@ class KeyManager {
 	 * @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 5406c4e51cc..e80435ac698 100644
--- a/apps/encryption/lib/Users/Setup.php
+++ b/apps/encryption/lib/Users/Setup.php
@@ -73,8 +73,8 @@ class Setup {
 	 */
 	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/encryption/tests/KeyManagerTest.php b/apps/encryption/tests/KeyManagerTest.php
index 78c506a18d0..37d6c203da1 100644
--- a/apps/encryption/tests/KeyManagerTest.php
+++ b/apps/encryption/tests/KeyManagerTest.php
@@ -43,6 +43,9 @@ use OCP\Files\Storage;
 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 @@ class KeyManagerTest extends TestCase {
 		$this->utilMock = $this->getMockBuilder(Util::class)
 			->disableOriginalConstructor()
 			->getMock();
+		$this->lockingProviderMock = $this->createMock(ILockingProvider::class);
 
 		$this->instance = new KeyManager(
 			$this->keyStorageMock,
@@ -107,7 +114,9 @@ class KeyManagerTest extends TestCase {
 			$this->userMock,
 			$this->sessionMock,
 			$this->logMock,
-			$this->utilMock);
+			$this->utilMock,
+			$this->lockingProviderMock
+		);
 	}
 
 	public function testDeleteShareKey() {
@@ -269,7 +278,8 @@ class KeyManagerTest extends TestCase {
 					$this->userMock,
 					$this->sessionMock,
 					$this->logMock,
-					$this->utilMock
+					$this->utilMock,
+					$this->lockingProviderMock
 				]
 			)->setMethods(['getMasterKeyId', 'getMasterKeyPassword', 'getSystemPrivateKey', 'getPrivateKey'])
 			->getMock();
@@ -559,7 +569,8 @@ class KeyManagerTest extends TestCase {
 					$this->userMock,
 					$this->sessionMock,
 					$this->logMock,
-					$this->utilMock
+					$this->utilMock,
+					$this->lockingProviderMock
 				]
 			)->setMethods(['getPublicMasterKey', 'setSystemPrivateKey', 'getMasterKeyPassword'])
 			->getMock();
@@ -578,6 +589,8 @@ class KeyManagerTest extends TestCase {
 			$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 @@ class KeyManagerTest extends TestCase {
 		$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 779bb5d82ea..76c4647f774 100644
--- a/apps/encryption/tests/Users/SetupTest.php
+++ b/apps/encryption/tests/Users/SetupTest.php
@@ -90,9 +90,9 @@ class SetupTest extends TestCase {
 		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,
diff --git a/apps/settings/lib/Controller/ChangePasswordController.php b/apps/settings/lib/Controller/ChangePasswordController.php
index 3006e89318f..668b0e49e66 100644
--- a/apps/settings/lib/Controller/ChangePasswordController.php
+++ b/apps/settings/lib/Controller/ChangePasswordController.php
@@ -188,7 +188,9 @@ class ChangePasswordController extends Controller {
 				\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,