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>

diff --git a/apps/encryption/lib/AppInfo/Application.php b/apps/encryption/lib/AppInfo/Application.php
index 688564887d3c03057d1423eb31fe1dd1f1802bec..1e3b2e43d787e986aed082cf587e910fecf13681 100644 (file)
--- 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 09bcf9371d2c8b0d8ae5a0f42777164dfe5b655a..0cc29a6a6abd9839c01032d51e55afcfba5da58f 100644 (file)
--- 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 {
 
@@ -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)) {
-                       $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 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);
        }
 }

diff --git a/apps/encryption/lib/Users/Setup.php b/apps/encryption/lib/Users/Setup.php
index 5406c4e51cc9720bde1ea6bd2b2d2445e2ba0fc7..e80435ac6988b2b2b60ecdb15a6ecd1feb3c67dd 100644 (file)
--- 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/settings/lib/Controller/ChangePasswordController.php b/apps/settings/lib/Controller/ChangePasswordController.php
index 3006e89318fce5ffe9e16b03002cb816ffc4a0f6..668b0e49e66f16ba4f49a6597764cb0e9eeeaa80 100644 (file)
--- 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,