Skip to content

Commit da58446

Browse files
MorrisJobkeMorrisJobke
authored
Merge pull request #22018 from nextcloud/bugfix/noid/harden-key-generation
Harden SSE key generation
2 parents b604d52 + 5a064ec commit da58446

File tree

6 files changed

+131
-40
lines changed

6 files changed

+131
-40
lines changed

‎apps/encryption/lib/AppInfo/Application.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,8 @@ function (IAppContainer $c) {
152152
$server->getUserSession(),
153153
new Session($server->getSession()),
154154
$server->getLogger(),
155-
$c->query('Util')
155+
$c->query('Util'),
156+
$server->getLockingProvider()
156157
);
157158
});
158159

‎apps/encryption/lib/KeyManager.php‎

Lines changed: 73 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
use OCP\IConfig;
4242
use OCP\ILogger;
4343
use OCP\IUserSession;
44+
use OCP\Lock\ILockingProvider;
4445

4546
class KeyManager {
4647

@@ -103,6 +104,11 @@ class KeyManager {
103104
*/
104105
private $util;
105106

107+
/**
108+
* @var ILockingProvider
109+
*/
110+
private $lockingProvider;
111+
106112
/**
107113
* @param IStorage $keyStorage
108114
* @param Crypt $crypt
@@ -119,14 +125,16 @@ public function __construct(
119125
IUserSession $userSession,
120126
Session $session,
121127
ILogger $log,
122-
Util $util
128+
Util $util,
129+
ILockingProvider $lockingProvider
123130
) {
124131
$this->util = $util;
125132
$this->session = $session;
126133
$this->keyStorage = $keyStorage;
127134
$this->crypt = $crypt;
128135
$this->config = $config;
129136
$this->log = $log;
137+
$this->lockingProvider = $lockingProvider;
130138

131139
$this->recoveryKeyId = $this->config->getAppValue('encryption',
132140
'recoveryKeyId');
@@ -161,17 +169,24 @@ public function __construct(
161169
public function validateShareKey() {
162170
$shareKey = $this->getPublicShareKey();
163171
if (empty($shareKey)) {
164-
$keyPair = $this->crypt->createKeyPair();
165-
166-
// Save public key
167-
$this->keyStorage->setSystemUserKey(
168-
$this->publicShareKeyId . '.publicKey', $keyPair['publicKey'],
169-
Encryption::ID);
170-
171-
// Encrypt private key empty passphrase
172-
$encryptedKey = $this->crypt->encryptPrivateKey($keyPair['privateKey'], '');
173-
$header = $this->crypt->generateHeader();
174-
$this->setSystemPrivateKey($this->publicShareKeyId, $header . $encryptedKey);
172+
$this->lockingProvider->acquireLock('encryption-generateSharedKey', ILockingProvider::LOCK_EXCLUSIVE, 'Encryption: shared key generation');
173+
try {
174+
$keyPair = $this->crypt->createKeyPair();
175+
176+
// Save public key
177+
$this->keyStorage->setSystemUserKey(
178+
$this->publicShareKeyId . '.' . $this->publicKeyId, $keyPair['publicKey'],
179+
Encryption::ID);
180+
181+
// Encrypt private key empty passphrase
182+
$encryptedKey = $this->crypt->encryptPrivateKey($keyPair['privateKey'], '');
183+
$header = $this->crypt->generateHeader();
184+
$this->setSystemPrivateKey($this->publicShareKeyId, $header . $encryptedKey);
185+
} catch (\Throwable $e) {
186+
$this->lockingProvider->releaseLock('encryption-generateSharedKey', ILockingProvider::LOCK_EXCLUSIVE);
187+
throw $e;
188+
}
189+
$this->lockingProvider->releaseLock('encryption-generateSharedKey', ILockingProvider::LOCK_EXCLUSIVE);
175190
}
176191
}
177192

@@ -184,18 +199,36 @@ public function validateMasterKey() {
184199
}
185200

186201
$publicMasterKey = $this->getPublicMasterKey();
187-
if (empty($publicMasterKey)) {
188-
$keyPair = $this->crypt->createKeyPair();
189-
190-
// Save public key
191-
$this->keyStorage->setSystemUserKey(
192-
$this->masterKeyId . '.publicKey', $keyPair['publicKey'],
193-
Encryption::ID);
194-
195-
// Encrypt private key with system password
196-
$encryptedKey = $this->crypt->encryptPrivateKey($keyPair['privateKey'], $this->getMasterKeyPassword(), $this->masterKeyId);
197-
$header = $this->crypt->generateHeader();
198-
$this->setSystemPrivateKey($this->masterKeyId, $header . $encryptedKey);
202+
$privateMasterKey = $this->getPrivateMasterKey();
203+
204+
if (empty($publicMasterKey) && empty($privateMasterKey)) {
205+
// There could be a race condition here if two requests would trigger
206+
// the generation the second one would enter the key generation as long
207+
// as the first one didn't write the key to the keystorage yet
208+
$this->lockingProvider->acquireLock('encryption-generateMasterKey', ILockingProvider::LOCK_EXCLUSIVE, 'Encryption: master key generation');
209+
try {
210+
$keyPair = $this->crypt->createKeyPair();
211+
212+
// Save public key
213+
$this->keyStorage->setSystemUserKey(
214+
$this->masterKeyId . '.' . $this->publicKeyId, $keyPair['publicKey'],
215+
Encryption::ID);
216+
217+
// Encrypt private key with system password
218+
$encryptedKey = $this->crypt->encryptPrivateKey($keyPair['privateKey'], $this->getMasterKeyPassword(), $this->masterKeyId);
219+
$header = $this->crypt->generateHeader();
220+
$this->setSystemPrivateKey($this->masterKeyId, $header . $encryptedKey);
221+
} catch (\Throwable $e) {
222+
$this->lockingProvider->releaseLock('encryption-generateMasterKey', ILockingProvider::LOCK_EXCLUSIVE);
223+
throw $e;
224+
}
225+
$this->lockingProvider->releaseLock('encryption-generateMasterKey', ILockingProvider::LOCK_EXCLUSIVE);
226+
} elseif (empty($publicMasterKey)) {
227+
$this->log->error('A private master key is available but the public key could not be found. This should never happen.');
228+
return;
229+
} elseif (empty($privateMasterKey)) {
230+
$this->log->error('A public master key is available but the private key could not be found. This should never happen.');
231+
return;
199232
}
200233

201234
if (!$this->session->isPrivateKeySet()) {
@@ -222,7 +255,7 @@ public function recoveryKeyExists() {
222255
* @return string
223256
*/
224257
public function getRecoveryKey() {
225-
return $this->keyStorage->getSystemUserKey($this->recoveryKeyId . '.publicKey', Encryption::ID);
258+
return $this->keyStorage->getSystemUserKey($this->recoveryKeyId . '.' . $this->publicKeyId, Encryption::ID);
226259
}
227260

228261
/**
@@ -239,7 +272,7 @@ public function getRecoveryKeyId() {
239272
* @return bool
240273
*/
241274
public function checkRecoveryPassword($password) {
242-
$recoveryKey = $this->keyStorage->getSystemUserKey($this->recoveryKeyId . '.privateKey', Encryption::ID);
275+
$recoveryKey = $this->keyStorage->getSystemUserKey($this->recoveryKeyId . '.' . $this->privateKeyId, Encryption::ID);
243276
$decryptedRecoveryKey = $this->crypt->decryptPrivateKey($recoveryKey, $password);
244277

245278
if ($decryptedRecoveryKey) {
@@ -251,7 +284,7 @@ public function checkRecoveryPassword($password) {
251284
/**
252285
* @param string $uid
253286
* @param string $password
254-
* @param string $keyPair
287+
* @param array $keyPair
255288
* @return bool
256289
*/
257290
public function storeKeyPair($uid, $password, $keyPair) {
@@ -277,7 +310,7 @@ public function storeKeyPair($uid, $password, $keyPair) {
277310
public function setRecoveryKey($password, $keyPair) {
278311
// Save Public Key
279312
$this->keyStorage->setSystemUserKey($this->getRecoveryKeyId().
280-
'.publicKey',
313+
'.' . $this->publicKeyId,
281314
$keyPair['publicKey'],
282315
Encryption::ID);
283316

@@ -435,7 +468,7 @@ public function getFileKey($path, $uid) {
435468
// use public share key for public links
436469
$uid = $this->getPublicShareKeyId();
437470
$shareKey = $this->getShareKey($path, $uid);
438-
$privateKey = $this->keyStorage->getSystemUserKey($this->publicShareKeyId . '.privateKey', Encryption::ID);
471+
$privateKey = $this->keyStorage->getSystemUserKey($this->publicShareKeyId . '.' . $this->privateKeyId, Encryption::ID);
439472
$privateKey = $this->crypt->decryptPrivateKey($privateKey);
440473
} else {
441474
$shareKey = $this->getShareKey($path, $uid);
@@ -578,7 +611,7 @@ public function getPublicShareKeyId() {
578611
* @return string
579612
*/
580613
public function getPublicShareKey() {
581-
return $this->keyStorage->getSystemUserKey($this->publicShareKeyId . '.publicKey', Encryption::ID);
614+
return $this->keyStorage->getSystemUserKey($this->publicShareKeyId . '.' . $this->publicKeyId, Encryption::ID);
582615
}
583616

584617
/**
@@ -718,6 +751,15 @@ public function getMasterKeyId() {
718751
* @return string
719752
*/
720753
public function getPublicMasterKey() {
721-
return $this->keyStorage->getSystemUserKey($this->masterKeyId . '.publicKey', Encryption::ID);
754+
return $this->keyStorage->getSystemUserKey($this->masterKeyId . '.' . $this->publicKeyId, Encryption::ID);
755+
}
756+
757+
/**
758+
* get public master key
759+
*
760+
* @return string
761+
*/
762+
public function getPrivateMasterKey() {
763+
return $this->keyStorage->getSystemUserKey($this->masterKeyId . '.' . $this->privateKeyId, Encryption::ID);
722764
}
723765
}

‎apps/encryption/lib/Users/Setup.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,8 @@ public function __construct(ILogger $logger,
7373
*/
7474
public function setupUser($uid, $password) {
7575
if (!$this->keyManager->userHasKeys($uid)) {
76-
return $this->keyManager->storeKeyPair($uid, $password,
77-
$this->crypt->createKeyPair());
76+
$keyPair = $this->crypt->createKeyPair();
77+
return is_array($keyPair) ? $this->keyManager->storeKeyPair($uid, $password, $keyPair) : false;
7878
}
7979
return true;
8080
}

‎apps/encryption/tests/KeyManagerTest.php‎

Lines changed: 49 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,9 @@
4343
use OCP\IConfig;
4444
use OCP\ILogger;
4545
use OCP\IUserSession;
46+
use OCP\Lock\ILockingProvider;
47+
use OCP\Lock\LockedException;
48+
use PHPUnit\Framework\MockObject\MockObject;
4649
use Test\TestCase;
4750

4851
class KeyManagerTest extends TestCase {
@@ -79,6 +82,9 @@ class KeyManagerTest extends TestCase {
7982
/** @var \OCP\IConfig|\PHPUnit\Framework\MockObject\MockObject */
8083
private $configMock;
8184

85+
/** @var ILockingProvider|MockObject */
86+
private $lockingProviderMock;
87+
8288
protected function setUp(): void {
8389
parent::setUp();
8490
$this->userId = 'user1';
@@ -99,6 +105,7 @@ protected function setUp(): void {
99105
$this->utilMock = $this->getMockBuilder(Util::class)
100106
->disableOriginalConstructor()
101107
->getMock();
108+
$this->lockingProviderMock = $this->createMock(ILockingProvider::class);
102109

103110
$this->instance = new KeyManager(
104111
$this->keyStorageMock,
@@ -107,7 +114,9 @@ protected function setUp(): void {
107114
$this->userMock,
108115
$this->sessionMock,
109116
$this->logMock,
110-
$this->utilMock);
117+
$this->utilMock,
118+
$this->lockingProviderMock
119+
);
111120
}
112121

113122
public function testDeleteShareKey() {
@@ -269,7 +278,8 @@ public function testInit($useMasterKey) {
269278
$this->userMock,
270279
$this->sessionMock,
271280
$this->logMock,
272-
$this->utilMock
281+
$this->utilMock,
282+
$this->lockingProviderMock
273283
]
274284
)->setMethods(['getMasterKeyId', 'getMasterKeyPassword', 'getSystemPrivateKey', 'getPrivateKey'])
275285
->getMock();
@@ -559,7 +569,8 @@ public function testValidateMasterKey($masterKey) {
559569
$this->userMock,
560570
$this->sessionMock,
561571
$this->logMock,
562-
$this->utilMock
572+
$this->utilMock,
573+
$this->lockingProviderMock
563574
]
564575
)->setMethods(['getPublicMasterKey', 'setSystemPrivateKey', 'getMasterKeyPassword'])
565576
->getMock();
@@ -578,6 +589,8 @@ public function testValidateMasterKey($masterKey) {
578589
$this->cryptMock->expects($this->once())->method('encryptPrivateKey')
579590
->with('private', 'masterKeyPassword', 'systemKeyId')
580591
->willReturn('EncryptedKey');
592+
$this->lockingProviderMock->expects($this->once())
593+
->method('acquireLock');
581594
$instance->expects($this->once())->method('setSystemPrivateKey')
582595
->with('systemKeyId', 'headerEncryptedKey');
583596
} else {
@@ -590,6 +603,39 @@ public function testValidateMasterKey($masterKey) {
590603
$instance->validateMasterKey();
591604
}
592605

606+
public function testValidateMasterKeyLocked() {
607+
/** @var \OCA\Encryption\KeyManager | \PHPUnit_Framework_MockObject_MockObject $instance */
608+
$instance = $this->getMockBuilder(KeyManager::class)
609+
->setConstructorArgs(
610+
[
611+
$this->keyStorageMock,
612+
$this->cryptMock,
613+
$this->configMock,
614+
$this->userMock,
615+
$this->sessionMock,
616+
$this->logMock,
617+
$this->utilMock,
618+
$this->lockingProviderMock
619+
]
620+
)->setMethods(['getPublicMasterKey', 'getPrivateMasterKey', 'setSystemPrivateKey', 'getMasterKeyPassword'])
621+
->getMock();
622+
623+
$instance->expects($this->once())->method('getPublicMasterKey')
624+
->willReturn('');
625+
$instance->expects($this->once())->method('getPrivateMasterKey')
626+
->willReturn('');
627+
628+
$instance->expects($this->any())->method('getMasterKeyPassword')->willReturn('masterKeyPassword');
629+
$this->cryptMock->expects($this->any())->method('generateHeader')->willReturn('header');
630+
631+
$this->lockingProviderMock->expects($this->once())
632+
->method('acquireLock')
633+
->willThrowException(new LockedException('encryption-generateMasterKey'));
634+
635+
$this->expectException(LockedException::class);
636+
$instance->validateMasterKey();
637+
}
638+
593639
public function dataTestValidateMasterKey() {
594640
return [
595641
['masterKey'],

‎apps/encryption/tests/Users/SetupTest.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,9 +90,9 @@ public function testSetupUser($hasKeys, $expected) {
9090
if ($hasKeys) {
9191
$this->keyManagerMock->expects($this->never())->method('storeKeyPair');
9292
} else {
93-
$this->cryptMock->expects($this->once())->method('createKeyPair')->willReturn('keyPair');
93+
$this->cryptMock->expects($this->once())->method('createKeyPair')->willReturn(['publicKey' => 'publicKey', 'privateKey' => 'privateKey']);
9494
$this->keyManagerMock->expects($this->once())->method('storeKeyPair')
95-
->with('uid', 'password', 'keyPair')->willReturn(true);
95+
->with('uid', 'password', ['publicKey' => 'publicKey', 'privateKey' => 'privateKey'])->willReturn(true);
9696
}
9797

9898
$this->assertSame($expected,

‎apps/settings/lib/Controller/ChangePasswordController.php‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,9 @@ public function changeUserPassword(string $username = null, string $password = n
188188
\OC::$server->getUserSession(),
189189
new \OCA\Encryption\Session(\OC::$server->getSession()),
190190
\OC::$server->getLogger(),
191-
$util);
191+
$util,
192+
\OC::$server->getLockingProvider()
193+
);
192194
$recovery = new \OCA\Encryption\Recovery(
193195
\OC::$server->getUserSession(),
194196
$crypt,

0 commit comments

Comments
 (0)