Harden key generation

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 <jus@bitgrid.net>

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

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 {
 
@@ -103,6 +104,11 @@ class KeyManager {
 	 */
 	private $util;
 
+	/**
+	 * @var ILockingProvider
+	 */
+	private $lockingProvider;
+
 	/**
 	 * @param IStorage $keyStorage
 	 * @param Crypt $crypt
@@ -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)) {
+			$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 . '.publicKey', $keyPair['publicKey'],
+					$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)) {
+		$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 . '.publicKey', $keyPair['publicKey'],
+					$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 @@ 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);
 	}
 }

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,
+			$keyPair = $this->crypt->createKeyPair();
-				$this->crypt->createKeyPair());
+			return is_array($keyPair) ? $this->keyManager->storeKeyPair($uid, $password, $keyPair) : false;
 		}
 		return true;
 	}

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,