Skip to content

Harden SSE key generation #22018

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
MorrisJobke merged 3 commits into from
Aug 19, 2020
Merged

Harden SSE key generation #22018

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
36cfdd3
Harden key generation
juliusknorr Jul 22, 2020
00e6217
Test for locking state in key generation
juliusknorr Jul 27, 2020
5a064ec
Fix typo in error message
MorrisJobke Aug 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
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 <[email protected]>
  • Loading branch information
@juliusknorr
juliusknorr committed Aug 14, 2020
commit 36cfdd320bd766798930dc09acea74b27f58d95c
3 changes: 2 additions & 1 deletion apps/encryption/lib/AppInfo/Application.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,8 @@ function (IAppContainer $c) {
$server->getUserSession(),
new Session($server->getSession()),
$server->getLogger(),
$c->query('Util')
$c->query('Util'),
$server->getLockingProvider()
);
});

Expand Down
104 changes: 73 additions & 31 deletions apps/encryption/lib/KeyManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
use OCP\IConfig;
use OCP\ILogger;
use OCP\IUserSession;
use OCP\Lock\ILockingProvider;

class KeyManager {

Expand Down Expand Up @@ -103,6 +104,11 @@ class KeyManager {
*/
private $util;

/**
* @var ILockingProvider
*/
private $lockingProvider;

/**
* @param IStorage $keyStorage
* @param Crypt $crypt
Expand All @@ -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');
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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()) {
Expand All @@ -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);
}

/**
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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);

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}

/**
Expand Down Expand Up @@ -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);
}
}
4 changes: 2 additions & 2 deletions apps/encryption/lib/Users/Setup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
4 changes: 3 additions & 1 deletion apps/settings/lib/Controller/ChangePasswordController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down