From 62bc0e5264af50be48dbcbb720b7bd16e8d88df5 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Fri, 7 Aug 2015 14:04:17 +0200 Subject: [PATCH] use password hash instead of the plain password to encrypt the private key --- .../controller/settingscontroller.php | 2 +- apps/encryption/hooks/userhooks.php | 6 +- apps/encryption/lib/crypto/crypt.php | 143 ++++++++++++++++-- apps/encryption/lib/keymanager.php | 9 +- apps/encryption/lib/recovery.php | 2 +- .../controller/SettingsControllerTest.php | 2 +- apps/encryption/tests/hooks/UserHooksTest.php | 2 +- apps/encryption/tests/lib/KeyManagerTest.php | 2 +- apps/encryption/tests/lib/RecoveryTest.php | 2 +- .../encryption/tests/lib/crypto/cryptTest.php | 111 +++++++++++++- apps/encryption/vendor/pbkdf2fallback.php | 87 +++++++++++ 11 files changed, 337 insertions(+), 31 deletions(-) create mode 100644 apps/encryption/vendor/pbkdf2fallback.php diff --git a/apps/encryption/controller/settingscontroller.php b/apps/encryption/controller/settingscontroller.php index 641c97e1d6..dbd960bb78 100644 --- a/apps/encryption/controller/settingscontroller.php +++ b/apps/encryption/controller/settingscontroller.php @@ -103,7 +103,7 @@ class SettingsController extends Controller { $decryptedKey = $this->crypt->decryptPrivateKey($encryptedKey, $oldPassword); if ($decryptedKey) { - $encryptedKey = $this->crypt->symmetricEncryptFileContent($decryptedKey, $newPassword); + $encryptedKey = $this->crypt->encryptPrivateKey($decryptedKey, $newPassword); $header = $this->crypt->generateHeader(); if ($encryptedKey) { $this->keyManager->setPrivateKey($uid, $header . $encryptedKey); diff --git a/apps/encryption/hooks/userhooks.php b/apps/encryption/hooks/userhooks.php index a86b866278..71c0435d20 100644 --- a/apps/encryption/hooks/userhooks.php +++ b/apps/encryption/hooks/userhooks.php @@ -220,8 +220,7 @@ class UserHooks implements IHook { if ($user && $params['uid'] === $user->getUID() && $privateKey) { // Encrypt private key with new user pwd as passphrase - $encryptedPrivateKey = $this->crypt->symmetricEncryptFileContent($privateKey, - $params['password']); + $encryptedPrivateKey = $this->crypt->encryptPrivateKey($privateKey, $params['password']); // Save private key if ($encryptedPrivateKey) { @@ -259,8 +258,7 @@ class UserHooks implements IHook { $this->keyManager->setPublicKey($user, $keyPair['publicKey']); // Encrypt private key with new password - $encryptedKey = $this->crypt->symmetricEncryptFileContent($keyPair['privateKey'], - $newUserPassword); + $encryptedKey = $this->crypt->encryptPrivateKey($keyPair['privateKey'], $newUserPassword); if ($encryptedKey) { $this->keyManager->setPrivateKey($user, $this->crypt->generateHeader() . $encryptedKey); diff --git a/apps/encryption/lib/crypto/crypt.php b/apps/encryption/lib/crypto/crypt.php index f3cf38fb96..eef16e5144 100644 --- a/apps/encryption/lib/crypto/crypt.php +++ b/apps/encryption/lib/crypto/crypt.php @@ -30,6 +30,7 @@ use OC\Encryption\Exceptions\DecryptionFailedException; use OC\Encryption\Exceptions\EncryptionFailedException; use OCA\Encryption\Exceptions\MultiKeyDecryptException; use OCA\Encryption\Exceptions\MultiKeyEncryptException; +use OCA\Encryption\Vendor\PBKDF2Fallback; use OCP\Encryption\Exceptions\GenericEncryptionException; use OCP\IConfig; use OCP\ILogger; @@ -42,6 +43,10 @@ class Crypt { // default cipher from old ownCloud versions const LEGACY_CIPHER = 'AES-128-CFB'; + // default key format, old ownCloud version encrypted the private key directly + // with the user password + const LEGACY_KEY_FORMAT = 'password'; + const HEADER_START = 'HBEGIN'; const HEADER_END = 'HEND'; /** @@ -57,6 +62,11 @@ class Crypt { */ private $config; + /** + * @var array + */ + private $supportedKeyFormats; + /** * @param ILogger $logger * @param IUserSession $userSession @@ -66,6 +76,7 @@ class Crypt { $this->logger = $logger; $this->user = $userSession && $userSession->isLoggedIn() ? $userSession->getUser() : false; $this->config = $config; + $this->supportedKeyFormats = ['hash', 'password']; } /** @@ -161,10 +172,23 @@ class Crypt { /** * generate header for encrypted file + * + * @param string $keyFormat (can be 'hash' or 'password') + * @return string + * @throws \InvalidArgumentException */ - public function generateHeader() { + public function generateHeader($keyFormat = 'hash') { + + if (in_array($keyFormat, $this->supportedKeyFormats, true) === false) { + throw new \InvalidArgumentException('key format "' . $keyFormat . '" is not supported'); + } + $cipher = $this->getCipher(); - $header = self::HEADER_START . ':cipher:' . $cipher . ':' . self::HEADER_END; + + $header = self::HEADER_START + . ':cipher:' . $cipher + . ':keyFormat:' . $keyFormat + . ':' . self::HEADER_END; return $header; } @@ -211,6 +235,25 @@ class Crypt { return $cipher; } + /** + * get key size depending on the cipher + * + * @param string $cipher supported ('AES-256-CFB' and 'AES-128-CFB') + * @return int + * @throws \InvalidArgumentException + */ + protected function getKeySize($cipher) { + if ($cipher === 'AES-256-CFB') { + return 32; + } else if ($cipher === 'AES-128-CFB') { + return 16; + } + + throw new \InvalidArgumentException( + 'Wrong cipher defined only AES-128-CFB and AES-256-CFB are supported.' + ); + } + /** * get legacy cipher * @@ -237,6 +280,63 @@ class Crypt { return $data . 'xx'; } + /** + * generate password hash used to encrypt the users private key + * + * @param string $password + * @param string $cipher + * @return string + */ + protected function generatePasswordHash($password, $cipher) { + $instanceId = $this->config->getSystemValue('instanceid'); + $instanceSecret = $this->config->getSystemValue('secret'); + $salt = hash('sha256', $instanceId . $instanceSecret, true); + $keySize = $this->getKeySize($cipher); + + if (function_exists('hash_pbkdf2')) { + $hash = hash_pbkdf2( + 'sha256', + $password, + $salt, + 100000, + $keySize, + true + ); + } else { + // fallback to 3rdparty lib for PHP <= 5.4. + // FIXME: Can be removed as soon as support for PHP 5.4 was dropped + $fallback = new PBKDF2Fallback(); + $hash = $fallback->pbkdf2( + 'sha256', + $password, + $salt, + 100000, + $keySize, + true + ); + } + + return $hash; + } + + /** + * encrypt private key + * + * @param string $privateKey + * @param string $password + * @return bool|string + */ + public function encryptPrivateKey($privateKey, $password) { + $cipher = $this->getCipher(); + $hash = $this->generatePasswordHash($password, $cipher); + $encryptedKey = $this->symmetricEncryptFileContent( + $privateKey, + $hash + ); + + return $encryptedKey; + } + /** * @param string $privateKey * @param string $password @@ -252,6 +352,16 @@ class Crypt { $cipher = self::LEGACY_CIPHER; } + if (isset($header['keyFormat'])) { + $keyFormat = $header['keyFormat']; + } else { + $keyFormat = self::LEGACY_KEY_FORMAT; + } + + if ($keyFormat === 'hash') { + $password = $this->generatePasswordHash($password, $cipher); + } + // If we found a header we need to remove it from the key we want to decrypt if (!empty($header)) { $privateKey = substr($privateKey, @@ -263,20 +373,31 @@ class Crypt { $password, $cipher); - // Check if this is a valid private key - $res = openssl_get_privatekey($plainKey); - if (is_resource($res)) { - $sslInfo = openssl_pkey_get_details($res); - if (!isset($sslInfo['key'])) { - return false; - } - } else { + if ($this->isValidPrivateKey($plainKey) === false) { return false; } return $plainKey; } + /** + * check if it is a valid private key + * + * @param $plainKey + * @return bool + */ + protected function isValidPrivateKey($plainKey) { + $res = openssl_get_privatekey($plainKey); + if (is_resource($res)) { + $sslInfo = openssl_pkey_get_details($res); + if (isset($sslInfo['key'])) { + return true; + } + } + + return true; + } + /** * @param $keyFileContents * @param string $passPhrase @@ -358,7 +479,7 @@ class Crypt { * @param $data * @return array */ - private function parseHeader($data) { + protected function parseHeader($data) { $result = []; if (substr($data, 0, strlen(self::HEADER_START)) === self::HEADER_START) { diff --git a/apps/encryption/lib/keymanager.php b/apps/encryption/lib/keymanager.php index 8c8c1f8fd7..47a8e7d391 100644 --- a/apps/encryption/lib/keymanager.php +++ b/apps/encryption/lib/keymanager.php @@ -146,7 +146,7 @@ class KeyManager { Encryption::ID); // Encrypt private key empty passphrase - $encryptedKey = $this->crypt->symmetricEncryptFileContent($keyPair['privateKey'], ''); + $encryptedKey = $this->crypt->encryptPrivateKey($keyPair['privateKey'], ''); $header = $this->crypt->generateHeader(); $this->setSystemPrivateKey($this->publicShareKeyId, $header . $encryptedKey); } @@ -203,8 +203,8 @@ class KeyManager { // Save Public Key $this->setPublicKey($uid, $keyPair['publicKey']); - $encryptedKey = $this->crypt->symmetricEncryptFileContent($keyPair['privateKey'], - $password); + $encryptedKey = $this->crypt->encryptPrivateKey($keyPair['privateKey'], $password); + $header = $this->crypt->generateHeader(); if ($encryptedKey) { @@ -226,8 +226,7 @@ class KeyManager { $keyPair['publicKey'], Encryption::ID); - $encryptedKey = $this->crypt->symmetricEncryptFileContent($keyPair['privateKey'], - $password); + $encryptedKey = $this->crypt->encryptPrivateKey($keyPair['privateKey'], $password); $header = $this->crypt->generateHeader(); if ($encryptedKey) { diff --git a/apps/encryption/lib/recovery.php b/apps/encryption/lib/recovery.php index e31a14fba6..e82ecca9d4 100644 --- a/apps/encryption/lib/recovery.php +++ b/apps/encryption/lib/recovery.php @@ -136,7 +136,7 @@ class Recovery { public function changeRecoveryKeyPassword($newPassword, $oldPassword) { $recoveryKey = $this->keyManager->getSystemPrivateKey($this->keyManager->getRecoveryKeyId()); $decryptedRecoveryKey = $this->crypt->decryptPrivateKey($recoveryKey, $oldPassword); - $encryptedRecoveryKey = $this->crypt->symmetricEncryptFileContent($decryptedRecoveryKey, $newPassword); + $encryptedRecoveryKey = $this->crypt->encryptPrivateKey($decryptedRecoveryKey, $newPassword); $header = $this->crypt->generateHeader(); if ($encryptedRecoveryKey) { $this->keyManager->setSystemPrivateKey($this->keyManager->getRecoveryKeyId(), $header . $encryptedRecoveryKey); diff --git a/apps/encryption/tests/controller/SettingsControllerTest.php b/apps/encryption/tests/controller/SettingsControllerTest.php index ed8135b9c4..d985c7d7d2 100644 --- a/apps/encryption/tests/controller/SettingsControllerTest.php +++ b/apps/encryption/tests/controller/SettingsControllerTest.php @@ -188,7 +188,7 @@ class SettingsControllerTest extends TestCase { $this->cryptMock ->expects($this->once()) - ->method('symmetricEncryptFileContent') + ->method('encryptPrivateKey') ->willReturn('encryptedKey'); $this->cryptMock diff --git a/apps/encryption/tests/hooks/UserHooksTest.php b/apps/encryption/tests/hooks/UserHooksTest.php index 921c924d01..aa16a4d870 100644 --- a/apps/encryption/tests/hooks/UserHooksTest.php +++ b/apps/encryption/tests/hooks/UserHooksTest.php @@ -114,7 +114,7 @@ class UserHooksTest extends TestCase { ->willReturnOnConsecutiveCalls(true, false); $this->cryptMock->expects($this->exactly(4)) - ->method('symmetricEncryptFileContent') + ->method('encryptPrivateKey') ->willReturn(true); $this->cryptMock->expects($this->any()) diff --git a/apps/encryption/tests/lib/KeyManagerTest.php b/apps/encryption/tests/lib/KeyManagerTest.php index 0bac5e0341..71b00cf254 100644 --- a/apps/encryption/tests/lib/KeyManagerTest.php +++ b/apps/encryption/tests/lib/KeyManagerTest.php @@ -260,7 +260,7 @@ class KeyManagerTest extends TestCase { ->method('setSystemUserKey') ->willReturn(true); $this->cryptMock->expects($this->any()) - ->method('symmetricEncryptFileContent') + ->method('encryptPrivateKey') ->with($this->equalTo('privateKey'), $this->equalTo('pass')) ->willReturn('decryptedPrivateKey'); diff --git a/apps/encryption/tests/lib/RecoveryTest.php b/apps/encryption/tests/lib/RecoveryTest.php index 8d5d31af0b..c0624a36be 100644 --- a/apps/encryption/tests/lib/RecoveryTest.php +++ b/apps/encryption/tests/lib/RecoveryTest.php @@ -96,7 +96,7 @@ class RecoveryTest extends TestCase { ->method('decryptPrivateKey'); $this->cryptMock->expects($this->once()) - ->method('symmetricEncryptFileContent') + ->method('encryptPrivateKey') ->willReturn(true); $this->assertTrue($this->instance->changeRecoveryKeyPassword('password', diff --git a/apps/encryption/tests/lib/crypto/cryptTest.php b/apps/encryption/tests/lib/crypto/cryptTest.php index 14ed1513e1..3c7767a890 100644 --- a/apps/encryption/tests/lib/crypto/cryptTest.php +++ b/apps/encryption/tests/lib/crypto/cryptTest.php @@ -98,18 +98,41 @@ class cryptTest extends TestCase { /** - * test generateHeader + * test generateHeader with valid key formats + * + * @dataProvider dataTestGenerateHeader */ - public function testGenerateHeader() { + public function testGenerateHeader($keyFormat, $expected) { $this->config->expects($this->once()) ->method('getSystemValue') ->with($this->equalTo('cipher'), $this->equalTo('AES-256-CFB')) ->willReturn('AES-128-CFB'); - $this->assertSame('HBEGIN:cipher:AES-128-CFB:HEND', - $this->crypt->generateHeader() - ); + if ($keyFormat) { + $result = $this->crypt->generateHeader($keyFormat); + } else { + $result = $this->crypt->generateHeader(); + } + + $this->assertSame($expected, $result); + } + + /** + * test generateHeader with invalid key format + * + * @expectedException \InvalidArgumentException + */ + public function testGenerateHeaderInvalid() { + $this->crypt->generateHeader('unknown'); + } + + public function dataTestGenerateHeader() { + return [ + [null, 'HBEGIN:cipher:AES-128-CFB:keyFormat:hash:HEND'], + ['password', 'HBEGIN:cipher:AES-128-CFB:keyFormat:password:HEND'], + ['hash', 'HBEGIN:cipher:AES-128-CFB:keyFormat:hash:HEND'] + ]; } /** @@ -262,4 +285,82 @@ class cryptTest extends TestCase { } + /** + * test return values of valid ciphers + * + * @dataProvider dataTestGetKeySize + */ + public function testGetKeySize($cipher, $expected) { + $result = $this->invokePrivate($this->crypt, 'getKeySize', [$cipher]); + $this->assertSame($expected, $result); + } + + /** + * test exception if cipher is unknown + * + * @expectedException \InvalidArgumentException + */ + public function testGetKeySizeFailure() { + $this->invokePrivate($this->crypt, 'getKeySize', ['foo']); + } + + public function dataTestGetKeySize() { + return [ + ['AES-256-CFB', 32], + ['AES-128-CFB', 16], + ]; + } + + /** + * @dataProvider dataTestDecryptPrivateKey + */ + public function testDecryptPrivateKey($header, $privateKey, $expectedCipher, $isValidKey, $expected) { + /** @var \OCA\Encryption\Crypto\Crypt | \PHPUnit_Framework_MockObject_MockObject $crypt */ + $crypt = $this->getMockBuilder('OCA\Encryption\Crypto\Crypt') + ->setConstructorArgs( + [ + $this->logger, + $this->userSession, + $this->config + ] + ) + ->setMethods( + [ + 'parseHeader', + 'generatePasswordHash', + 'symmetricDecryptFileContent', + 'isValidPrivateKey' + ] + ) + ->getMock(); + + $crypt->expects($this->once())->method('parseHeader')->willReturn($header); + if (isset($header['keyFormat']) && $header['keyFormat'] === 'hash') { + $crypt->expects($this->once())->method('generatePasswordHash')->willReturn('hash'); + $password = 'hash'; + } else { + $crypt->expects($this->never())->method('generatePasswordHash'); + $password = 'password'; + } + + $crypt->expects($this->once())->method('symmetricDecryptFileContent') + ->with('privateKey', $password, $expectedCipher)->willReturn('key'); + $crypt->expects($this->once())->method('isValidPrivateKey')->willReturn($isValidKey); + + $result = $crypt->decryptPrivateKey($privateKey, 'password'); + + $this->assertSame($expected, $result); + } + + public function dataTestDecryptPrivateKey() { + return [ + [['cipher' => 'AES-128-CFB', 'keyFormat' => 'password'], 'HBEGIN:HENDprivateKey', 'AES-128-CFB', true, 'key'], + [['cipher' => 'AES-256-CFB', 'keyFormat' => 'password'], 'HBEGIN:HENDprivateKey', 'AES-256-CFB', true, 'key'], + [['cipher' => 'AES-256-CFB', 'keyFormat' => 'password'], 'HBEGIN:HENDprivateKey', 'AES-256-CFB', false, false], + [['cipher' => 'AES-256-CFB', 'keyFormat' => 'hash'], 'HBEGIN:HENDprivateKey', 'AES-256-CFB', true, 'key'], + [['cipher' => 'AES-256-CFB'], 'HBEGIN:HENDprivateKey', 'AES-256-CFB', true, 'key'], + [[], 'privateKey', 'AES-128-CFB', true, 'key'], + ]; + } + } diff --git a/apps/encryption/vendor/pbkdf2fallback.php b/apps/encryption/vendor/pbkdf2fallback.php new file mode 100644 index 0000000000..ca579f8e7d --- /dev/null +++ b/apps/encryption/vendor/pbkdf2fallback.php @@ -0,0 +1,87 @@ +