Merge pull request #23755 from nextcloud/backport/22018/stable19

[stable19] Harden SSE key generation
This commit is contained in:
Roeland Jago Douma 2020-10-29 10:32:07 +01:00 committed by GitHub
commit b1a517d57f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 127 additions and 36 deletions

View File

@ -152,7 +152,8 @@ class Application extends \OCP\AppFramework\App {
$server->getUserSession(), $server->getUserSession(),
new Session($server->getSession()), new Session($server->getSession()),
$server->getLogger(), $server->getLogger(),
$c->query('Util') $c->query('Util'),
$server->getLockingProvider()
); );
}); });

View File

@ -41,6 +41,7 @@ use OCP\Encryption\Keys\IStorage;
use OCP\IConfig; use OCP\IConfig;
use OCP\ILogger; use OCP\ILogger;
use OCP\IUserSession; use OCP\IUserSession;
use OCP\Lock\ILockingProvider;
class KeyManager { class KeyManager {
@ -103,6 +104,11 @@ class KeyManager {
*/ */
private $util; private $util;
/**
* @var ILockingProvider
*/
private $lockingProvider;
/** /**
* @param IStorage $keyStorage * @param IStorage $keyStorage
* @param Crypt $crypt * @param Crypt $crypt
@ -119,7 +125,8 @@ class KeyManager {
IUserSession $userSession, IUserSession $userSession,
Session $session, Session $session,
ILogger $log, ILogger $log,
Util $util Util $util,
ILockingProvider $lockingProvider
) { ) {
$this->util = $util; $this->util = $util;
$this->session = $session; $this->session = $session;
@ -127,6 +134,7 @@ class KeyManager {
$this->crypt = $crypt; $this->crypt = $crypt;
$this->config = $config; $this->config = $config;
$this->log = $log; $this->log = $log;
$this->lockingProvider = $lockingProvider;
$this->recoveryKeyId = $this->config->getAppValue('encryption', $this->recoveryKeyId = $this->config->getAppValue('encryption',
'recoveryKeyId'); 'recoveryKeyId');
@ -161,17 +169,24 @@ class KeyManager {
public function validateShareKey() { public function validateShareKey() {
$shareKey = $this->getPublicShareKey(); $shareKey = $this->getPublicShareKey();
if (empty($shareKey)) { if (empty($shareKey)) {
$keyPair = $this->crypt->createKeyPair(); $this->lockingProvider->acquireLock('encryption-generateSharedKey', ILockingProvider::LOCK_EXCLUSIVE, 'Encryption: shared key generation');
try {
$keyPair = $this->crypt->createKeyPair();
// Save public key // Save public key
$this->keyStorage->setSystemUserKey( $this->keyStorage->setSystemUserKey(
$this->publicShareKeyId . '.publicKey', $keyPair['publicKey'], $this->publicShareKeyId . '.' . $this->publicKeyId, $keyPair['publicKey'],
Encryption::ID); Encryption::ID);
// Encrypt private key empty passphrase // Encrypt private key empty passphrase
$encryptedKey = $this->crypt->encryptPrivateKey($keyPair['privateKey'], ''); $encryptedKey = $this->crypt->encryptPrivateKey($keyPair['privateKey'], '');
$header = $this->crypt->generateHeader(); $header = $this->crypt->generateHeader();
$this->setSystemPrivateKey($this->publicShareKeyId, $header . $encryptedKey); $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(); $publicMasterKey = $this->getPublicMasterKey();
if (empty($publicMasterKey)) { $privateMasterKey = $this->getPrivateMasterKey();
$keyPair = $this->crypt->createKeyPair();
// Save public key if (empty($publicMasterKey) && empty($privateMasterKey)) {
$this->keyStorage->setSystemUserKey( // There could be a race condition here if two requests would trigger
$this->masterKeyId . '.publicKey', $keyPair['publicKey'], // the generation the second one would enter the key generation as long
Encryption::ID); // 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();
// Encrypt private key with system password // Save public key
$encryptedKey = $this->crypt->encryptPrivateKey($keyPair['privateKey'], $this->getMasterKeyPassword(), $this->masterKeyId); $this->keyStorage->setSystemUserKey(
$header = $this->crypt->generateHeader(); $this->masterKeyId . '.' . $this->publicKeyId, $keyPair['publicKey'],
$this->setSystemPrivateKey($this->masterKeyId, $header . $encryptedKey); 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()) { if (!$this->session->isPrivateKeySet()) {
@ -222,7 +255,7 @@ class KeyManager {
* @return string * @return string
*/ */
public function getRecoveryKey() { 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 * @return bool
*/ */
public function checkRecoveryPassword($password) { 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); $decryptedRecoveryKey = $this->crypt->decryptPrivateKey($recoveryKey, $password);
if ($decryptedRecoveryKey) { if ($decryptedRecoveryKey) {
@ -251,7 +284,7 @@ class KeyManager {
/** /**
* @param string $uid * @param string $uid
* @param string $password * @param string $password
* @param string $keyPair * @param array $keyPair
* @return bool * @return bool
*/ */
public function storeKeyPair($uid, $password, $keyPair) { public function storeKeyPair($uid, $password, $keyPair) {
@ -277,7 +310,7 @@ class KeyManager {
public function setRecoveryKey($password, $keyPair) { public function setRecoveryKey($password, $keyPair) {
// Save Public Key // Save Public Key
$this->keyStorage->setSystemUserKey($this->getRecoveryKeyId(). $this->keyStorage->setSystemUserKey($this->getRecoveryKeyId().
'.publicKey', '.' . $this->publicKeyId,
$keyPair['publicKey'], $keyPair['publicKey'],
Encryption::ID); Encryption::ID);
@ -435,7 +468,7 @@ class KeyManager {
// use public share key for public links // use public share key for public links
$uid = $this->getPublicShareKeyId(); $uid = $this->getPublicShareKeyId();
$shareKey = $this->getShareKey($path, $uid); $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); $privateKey = $this->crypt->decryptPrivateKey($privateKey);
} else { } else {
$shareKey = $this->getShareKey($path, $uid); $shareKey = $this->getShareKey($path, $uid);
@ -578,7 +611,7 @@ class KeyManager {
* @return string * @return string
*/ */
public function getPublicShareKey() { 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 * @return string
*/ */
public function getPublicMasterKey() { 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);
} }
} }

View File

@ -73,8 +73,8 @@ class Setup {
*/ */
public function setupUser($uid, $password) { public function setupUser($uid, $password) {
if (!$this->keyManager->userHasKeys($uid)) { 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; return true;
} }

View File

@ -43,6 +43,9 @@ use OCP\Files\Storage;
use OCP\IConfig; use OCP\IConfig;
use OCP\ILogger; use OCP\ILogger;
use OCP\IUserSession; use OCP\IUserSession;
use OCP\Lock\ILockingProvider;
use OCP\Lock\LockedException;
use PHPUnit\Framework\MockObject\MockObject;
use Test\TestCase; use Test\TestCase;
class KeyManagerTest extends TestCase { class KeyManagerTest extends TestCase {
@ -79,6 +82,9 @@ class KeyManagerTest extends TestCase {
/** @var \OCP\IConfig|\PHPUnit_Framework_MockObject_MockObject */ /** @var \OCP\IConfig|\PHPUnit_Framework_MockObject_MockObject */
private $configMock; private $configMock;
/** @var ILockingProvider|MockObject */
private $lockingProviderMock;
protected function setUp(): void { protected function setUp(): void {
parent::setUp(); parent::setUp();
$this->userId = 'user1'; $this->userId = 'user1';
@ -99,6 +105,7 @@ class KeyManagerTest extends TestCase {
$this->utilMock = $this->getMockBuilder(Util::class) $this->utilMock = $this->getMockBuilder(Util::class)
->disableOriginalConstructor() ->disableOriginalConstructor()
->getMock(); ->getMock();
$this->lockingProviderMock = $this->createMock(ILockingProvider::class);
$this->instance = new KeyManager( $this->instance = new KeyManager(
$this->keyStorageMock, $this->keyStorageMock,
@ -107,7 +114,9 @@ class KeyManagerTest extends TestCase {
$this->userMock, $this->userMock,
$this->sessionMock, $this->sessionMock,
$this->logMock, $this->logMock,
$this->utilMock); $this->utilMock,
$this->lockingProviderMock
);
} }
public function testDeleteShareKey() { public function testDeleteShareKey() {
@ -269,7 +278,8 @@ class KeyManagerTest extends TestCase {
$this->userMock, $this->userMock,
$this->sessionMock, $this->sessionMock,
$this->logMock, $this->logMock,
$this->utilMock $this->utilMock,
$this->lockingProviderMock
] ]
)->setMethods(['getMasterKeyId', 'getMasterKeyPassword', 'getSystemPrivateKey', 'getPrivateKey']) )->setMethods(['getMasterKeyId', 'getMasterKeyPassword', 'getSystemPrivateKey', 'getPrivateKey'])
->getMock(); ->getMock();
@ -559,7 +569,8 @@ class KeyManagerTest extends TestCase {
$this->userMock, $this->userMock,
$this->sessionMock, $this->sessionMock,
$this->logMock, $this->logMock,
$this->utilMock $this->utilMock,
$this->lockingProviderMock
] ]
)->setMethods(['getPublicMasterKey', 'setSystemPrivateKey', 'getMasterKeyPassword']) )->setMethods(['getPublicMasterKey', 'setSystemPrivateKey', 'getMasterKeyPassword'])
->getMock(); ->getMock();
@ -578,6 +589,8 @@ class KeyManagerTest extends TestCase {
$this->cryptMock->expects($this->once())->method('encryptPrivateKey') $this->cryptMock->expects($this->once())->method('encryptPrivateKey')
->with('private', 'masterKeyPassword', 'systemKeyId') ->with('private', 'masterKeyPassword', 'systemKeyId')
->willReturn('EncryptedKey'); ->willReturn('EncryptedKey');
$this->lockingProviderMock->expects($this->once())
->method('acquireLock');
$instance->expects($this->once())->method('setSystemPrivateKey') $instance->expects($this->once())->method('setSystemPrivateKey')
->with('systemKeyId', 'headerEncryptedKey'); ->with('systemKeyId', 'headerEncryptedKey');
} else { } else {
@ -590,6 +603,39 @@ class KeyManagerTest extends TestCase {
$instance->validateMasterKey(); $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() { public function dataTestValidateMasterKey() {
return [ return [
['masterKey'], ['masterKey'],

View File

@ -90,9 +90,9 @@ class SetupTest extends TestCase {
if ($hasKeys) { if ($hasKeys) {
$this->keyManagerMock->expects($this->never())->method('storeKeyPair'); $this->keyManagerMock->expects($this->never())->method('storeKeyPair');
} else { } 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') $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, $this->assertSame($expected,

View File

@ -188,7 +188,9 @@ class ChangePasswordController extends Controller {
\OC::$server->getUserSession(), \OC::$server->getUserSession(),
new \OCA\Encryption\Session(\OC::$server->getSession()), new \OCA\Encryption\Session(\OC::$server->getSession()),
\OC::$server->getLogger(), \OC::$server->getLogger(),
$util); $util,
\OC::$server->getLockingProvider()
);
$recovery = new \OCA\Encryption\Recovery( $recovery = new \OCA\Encryption\Recovery(
\OC::$server->getUserSession(), \OC::$server->getUserSession(),
$crypt, $crypt,