Skip to content
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

use password hash to encrypt private key #18121

Merged
merged 2 commits into from
Aug 24, 2015
Merged

use password hash to encrypt private key #18121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
62bc0e5
use password hash instead of the plain password to encrypt the privat…
Aug 7, 2015
854fd63
use uid as additional information for salt
Aug 7, 2015
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
4 changes: 2 additions & 2 deletions apps/encryption/controller/settingscontroller.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,10 @@ public function updatePrivateKeyPassword($oldPassword, $newPassword) {

if ($passwordCorrect !== false) {
$encryptedKey = $this->keyManager->getPrivateKey($uid);
$decryptedKey = $this->crypt->decryptPrivateKey($encryptedKey, $oldPassword);
$decryptedKey = $this->crypt->decryptPrivateKey($encryptedKey, $oldPassword, $uid);

if ($decryptedKey) {
$encryptedKey = $this->crypt->symmetricEncryptFileContent($decryptedKey, $newPassword);
$encryptedKey = $this->crypt->encryptPrivateKey($decryptedKey, $newPassword, $uid);
$header = $this->crypt->generateHeader();
if ($encryptedKey) {
$this->keyManager->setPrivateKey($uid, $header . $encryptedKey);
Expand Down
6 changes: 2 additions & 4 deletions apps/encryption/hooks/userhooks.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,8 +220,7 @@ public function setPassphrase($params) {
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'], $params['uid']);

// Save private key
if ($encryptedPrivateKey) {
Expand Down Expand Up @@ -259,8 +258,7 @@ public function setPassphrase($params) {
$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, $user);

if ($encryptedKey) {
$this->keyManager->setPrivateKey($user, $this->crypt->generateHeader() . $encryptedKey);
Expand Down
144 changes: 134 additions & 10 deletions apps/encryption/lib/crypto/crypt.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
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;
Expand All @@ -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';
/**
Expand All @@ -57,6 +62,11 @@ class Crypt {
*/
private $config;

/**
* @var array
*/
private $supportedKeyFormats;

/**
* @param ILogger $logger
* @param IUserSession $userSession
Expand All @@ -66,6 +76,7 @@ public function __construct(ILogger $logger, IUserSession $userSession, IConfig
$this->logger = $logger;
$this->user = $userSession && $userSession->isLoggedIn() ? $userSession->getUser() : false;
$this->config = $config;
$this->supportedKeyFormats = ['hash', 'password'];
}

/**
Expand Down Expand Up @@ -161,10 +172,23 @@ public function symmetricEncryptFileContent($plainContent, $passPhrase) {

/**
* 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;
}
Expand Down Expand Up @@ -211,6 +235,25 @@ public function getCipher() {
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
*
Expand Down Expand Up @@ -238,11 +281,71 @@ private function addPadding($data) {
}

/**
* generate password hash used to encrypt the users private key
*
* @param string $password
* @param string $cipher
* @param string $uid only used for user keys
* @return string
*/
protected function generatePasswordHash($password, $cipher, $uid = '') {
$instanceId = $this->config->getSystemValue('instanceid');
$instanceSecret = $this->config->getSystemValue('secret');
$salt = hash('sha256', $uid . $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
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback already does this on it's own. I think we can remove the conditional statement here thus.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I saw this too. I just did it this way for convenience reasons. So that at day X we just have to remove the fallback class and the if-statement and doesn't have to change the function call. Thought that this might be easier a few years down the road when we didn't touched the method for a long time and are not as familiar with it as we are today. But it doesn't make a big difference, we can also always use the fallback class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your call. :-)


return $hash;
}

/**
* encrypt private key
*
* @param string $privateKey
* @param string $password
* @param string $uid for regular users, empty for system keys
* @return bool|string
*/
public function decryptPrivateKey($privateKey, $password = '') {
public function encryptPrivateKey($privateKey, $password, $uid = '') {
$cipher = $this->getCipher();
$hash = $this->generatePasswordHash($password, $cipher, $uid);
$encryptedKey = $this->symmetricEncryptFileContent(
$privateKey,
$hash
);

return $encryptedKey;
}

/**
* @param string $privateKey
* @param string $password
* @param string $uid for regular users, empty for system keys
* @return bool|string
*/
public function decryptPrivateKey($privateKey, $password = '', $uid = '') {

$header = $this->parseHeader($privateKey);

Expand All @@ -252,6 +355,16 @@ public function decryptPrivateKey($privateKey, $password = '') {
$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, $uid);
}

// If we found a header we need to remove it from the key we want to decrypt
if (!empty($header)) {
$privateKey = substr($privateKey,
Expand All @@ -263,18 +376,29 @@ public function decryptPrivateKey($privateKey, $password = '') {
$password,
$cipher);

// Check if this is a valid private key
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 false;
if (isset($sslInfo['key'])) {
return true;
}
} else {
return false;
}

return $plainKey;
return true;
}

/**
Expand Down Expand Up @@ -358,7 +482,7 @@ private function decrypt($encryptedContent, $iv, $passPhrase = '', $cipher = sel
* @param $data
* @return array
*/
private function parseHeader($data) {
protected function parseHeader($data) {
$result = [];

if (substr($data, 0, strlen(self::HEADER_START)) === self::HEADER_START) {
Expand Down
15 changes: 6 additions & 9 deletions apps/encryption/lib/keymanager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ public function validateShareKey() {
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);
}
Expand Down Expand Up @@ -184,8 +184,7 @@ public function getRecoveryKeyId() {
*/
public function checkRecoveryPassword($password) {
$recoveryKey = $this->keyStorage->getSystemUserKey($this->recoveryKeyId . '.privateKey', Encryption::ID);
$decryptedRecoveryKey = $this->crypt->decryptPrivateKey($recoveryKey,
$password);
$decryptedRecoveryKey = $this->crypt->decryptPrivateKey($recoveryKey, $password);

if ($decryptedRecoveryKey) {
return true;
Expand All @@ -203,8 +202,8 @@ public function storeKeyPair($uid, $password, $keyPair) {
// Save Public Key
$this->setPublicKey($uid, $keyPair['publicKey']);

$encryptedKey = $this->crypt->symmetricEncryptFileContent($keyPair['privateKey'],
$password);
$encryptedKey = $this->crypt->encryptPrivateKey($keyPair['privateKey'], $password, $uid);

$header = $this->crypt->generateHeader();

if ($encryptedKey) {
Expand All @@ -226,8 +225,7 @@ public function setRecoveryKey($password, $keyPair) {
$keyPair['publicKey'],
Encryption::ID);

$encryptedKey = $this->crypt->symmetricEncryptFileContent($keyPair['privateKey'],
$password);
$encryptedKey = $this->crypt->encryptPrivateKey($keyPair['privateKey'], $password);
$header = $this->crypt->generateHeader();

if ($encryptedKey) {
Expand Down Expand Up @@ -308,8 +306,7 @@ public function init($uid, $passPhrase) {

try {
$privateKey = $this->getPrivateKey($uid);
$privateKey = $this->crypt->decryptPrivateKey($privateKey,
$passPhrase);
$privateKey = $this->crypt->decryptPrivateKey($privateKey, $passPhrase, $uid);
} catch (PrivateKeyMissingException $e) {
return false;
} catch (DecryptionFailedException $e) {
Expand Down
5 changes: 2 additions & 3 deletions apps/encryption/lib/recovery.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ public function enableAdminRecovery($password) {
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);
Expand Down Expand Up @@ -263,8 +263,7 @@ private function removeRecoveryKeys($path) {
public function recoverUsersFiles($recoveryPassword, $user) {
$encryptedKey = $this->keyManager->getSystemPrivateKey($this->keyManager->getRecoveryKeyId());

$privateKey = $this->crypt->decryptPrivateKey($encryptedKey,
$recoveryPassword);
$privateKey = $this->crypt->decryptPrivateKey($encryptedKey, $recoveryPassword);

$this->recoverAllFiles('/' . $user . '/files/', $privateKey, $user);
}
Expand Down
2 changes: 1 addition & 1 deletion apps/encryption/tests/controller/SettingsControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ public function testUpdatePrivateKeyPassword() {

$this->cryptMock
->expects($this->once())
->method('symmetricEncryptFileContent')
->method('encryptPrivateKey')
->willReturn('encryptedKey');

$this->cryptMock
Expand Down
2 changes: 1 addition & 1 deletion apps/encryption/tests/hooks/UserHooksTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ public function testSetPassphrase() {
->willReturnOnConsecutiveCalls(true, false);

$this->cryptMock->expects($this->exactly(4))
->method('symmetricEncryptFileContent')
->method('encryptPrivateKey')
->willReturn(true);

$this->cryptMock->expects($this->any())
Expand Down
2 changes: 1 addition & 1 deletion apps/encryption/tests/lib/KeyManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ public function testSetRecoveryKey() {
->method('setSystemUserKey')
->willReturn(true);
$this->cryptMock->expects($this->any())
->method('symmetricEncryptFileContent')
->method('encryptPrivateKey')
->with($this->equalTo('privateKey'), $this->equalTo('pass'))
->willReturn('decryptedPrivateKey');

Expand Down
2 changes: 1 addition & 1 deletion apps/encryption/tests/lib/RecoveryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ public function testChangeRecoveryKeyPassword() {
->method('decryptPrivateKey');

$this->cryptMock->expects($this->once())
->method('symmetricEncryptFileContent')
->method('encryptPrivateKey')
->willReturn(true);

$this->assertTrue($this->instance->changeRecoveryKeyPassword('password',
Expand Down
Loading