From 4450757950b2546b285498d0cd26b9c85c107bcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elan=20Ruusam=C3=A4e?= Date: Wed, 14 Mar 2018 23:33:18 +0200 Subject: [PATCH 1/6] update to defuse/php-encryption 2.1 --- composer.json | 2 +- composer.lock | 44 +++++++++++++++++-------- src/Crypto/CryptoException.php | 4 +-- src/Crypto/CryptoKeyManager.php | 51 ++++++++++++++++++++++------- src/Crypto/CryptoManager.php | 48 +++++++++------------------ src/Crypto/CryptoUpgradeManager.php | 3 +- 6 files changed, 90 insertions(+), 62 deletions(-) diff --git a/composer.json b/composer.json index d7881c80e1..a4868881d3 100644 --- a/composer.json +++ b/composer.json @@ -65,7 +65,7 @@ "ext-session": "*", "ext-spl": "*", "cebe/markdown": "~1.1.1", - "defuse/php-encryption": "~1.2.1", + "defuse/php-encryption": "^2.1", "doctrine/dbal": "^2.5", "doctrine/orm": "^2.5", "enrise/urihelper": "^1.0", diff --git a/composer.lock b/composer.lock index f379a8ab24..7a6b245191 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#composer-lock-the-lock-file", "This file is @generated automatically" ], - "content-hash": "19c92800876dca53a9acc362c09bfd97", + "content-hash": "0bd2b3b06237c00086d0cda577bbe228", "packages": [ { "name": "cebe/markdown", @@ -99,28 +99,35 @@ }, { "name": "defuse/php-encryption", - "version": "v1.2.1", + "version": "v2.1.0", "source": { "type": "git", "url": "https://github.com/defuse/php-encryption.git", - "reference": "b87737b2eec06b13f025cabea847338fa203d1b4" + "reference": "5176f5abb38d3ea8a6e3ac6cd3bbb54d8185a689" }, "dist": { "type": "zip", - "url": "https://api.github.com/repos/defuse/php-encryption/zipball/b87737b2eec06b13f025cabea847338fa203d1b4", - "reference": "b87737b2eec06b13f025cabea847338fa203d1b4", + "url": "https://api.github.com/repos/defuse/php-encryption/zipball/5176f5abb38d3ea8a6e3ac6cd3bbb54d8185a689", + "reference": "5176f5abb38d3ea8a6e3ac6cd3bbb54d8185a689", "shasum": "" }, "require": { - "ext-mcrypt": "*", "ext-openssl": "*", + "paragonie/random_compat": "~2.0", "php": ">=5.4.0" }, + "require-dev": { + "nikic/php-parser": "^2.0|^3.0", + "phpunit/phpunit": "^4|^5" + }, + "bin": [ + "bin/generate-defuse-key" + ], "type": "library", "autoload": { - "files": [ - "Crypto.php" - ] + "psr-4": { + "Defuse\\Crypto\\": "src" + } }, "notification-url": "https://packagist.org/downloads/", "license": [ @@ -129,18 +136,29 @@ "authors": [ { "name": "Taylor Hornby", - "email": "havoc@defuse.ca" + "email": "taylor@defuse.ca", + "homepage": "https://defuse.ca/" + }, + { + "name": "Scott Arciszewski", + "email": "info@paragonie.com", + "homepage": "https://paragonie.com" } ], "description": "Secure PHP Encryption Library", "keywords": [ "aes", + "authenticated encryption", "cipher", + "crypto", + "cryptography", + "encrypt", "encryption", - "mcrypt", - "security" + "openssl", + "security", + "symmetric key cryptography" ], - "time": "2015-03-14T20:27:45+00:00" + "time": "2017-05-18T21:28:48+00:00" }, { "name": "doctrine/annotations", diff --git a/src/Crypto/CryptoException.php b/src/Crypto/CryptoException.php index 82b8fa9edf..23e6e90199 100644 --- a/src/Crypto/CryptoException.php +++ b/src/Crypto/CryptoException.php @@ -13,8 +13,8 @@ namespace Eventum\Crypto; -use RuntimeException; +use Defuse\Crypto\Exception\CryptoException as DefuseCryptoException; -class CryptoException extends RuntimeException +class CryptoException extends DefuseCryptoException { } diff --git a/src/Crypto/CryptoKeyManager.php b/src/Crypto/CryptoKeyManager.php index 83d957aab5..1231b51284 100644 --- a/src/Crypto/CryptoKeyManager.php +++ b/src/Crypto/CryptoKeyManager.php @@ -13,8 +13,7 @@ namespace Eventum\Crypto; -use Crypto; -use RandomLib; +use Defuse\Crypto\Key; use Symfony\Component\Filesystem\Exception\IOException; use Symfony\Component\Filesystem\Filesystem; @@ -26,6 +25,7 @@ final class CryptoKeyManager /** @var string */ private $keyfile; + /** @var Key */ private $key; public function __construct() @@ -40,6 +40,7 @@ public function regen() /** * Checks if key file can be updated + * @throws CryptoException */ public function canUpdate() { @@ -51,7 +52,8 @@ public function canUpdate() /** * Load or generate secret key used for crypt * - * @return string + * @throws CryptoException + * @return Key */ public function getKey() { @@ -66,16 +68,23 @@ public function getKey() return $this->key; } + /** + * @throws CryptoException + */ private function generateKey() { - // use RandomLib to get most compatible implementation - // Crypto uses mcrypt *ONLY* without any fallback - $factory = new RandomLib\Factory(); - $generator = $factory->getMediumStrengthGenerator(); - $this->key = $generator->generate(Crypto::KEY_BYTE_SIZE); - $this->storePrivateKey(); + try { + $this->key = Key::createNewRandomKey(); + $this->storePrivateKey(); + } catch (CryptoException $e) { + throw new CryptoException('Cannot perform operation: ' . $e->getMessage()); + } } + /** + * @throws CryptoException + * @return bool|null + */ private function loadPrivateKey() { if (!file_exists($this->keyfile) || !filesize($this->keyfile)) { @@ -84,22 +93,40 @@ private function loadPrivateKey() if (!is_readable($this->keyfile)) { throw new CryptoException("Secret file '{$this->keyfile}' not readable"); } - $key = trim(file_get_contents($this->keyfile)); + // load first to see that it's php script + // this would avoid printing secret key to output if in old or invalid format + $key = file_get_contents($this->keyfile); if (!$key) { throw new CryptoException("Unable to read secret file '{$this->keyfile}"); } - $this->key = $key; + if (substr($key, 0, 5) !== 'keyfile; + if (!$key) { + throw new CryptoException("Secret file corrupted: {$this->keyfile}"); + } + + try { + $this->key = Key::loadFromAsciiSafeString($key); + } catch (CryptoException $e) { + throw new CryptoException('Cannot perform operation: ' . $e->getMessage()); + } return true; } + /** + * @throws CryptoException + */ private function storePrivateKey() { $this->canUpdate(); try { $fs = new Filesystem(); - $fs->dumpFile($this->keyfile, $this->key); + $content = sprintf('<' . '?php return %s;', var_export($this->key->saveToAsciiSafeString(), 1)); + $fs->dumpFile($this->keyfile, $content); } catch (IOException $e) { throw new CryptoException("Unable to store secret file '{$this->keyfile}': {$e->getMessage()}"); } diff --git a/src/Crypto/CryptoManager.php b/src/Crypto/CryptoManager.php index 2ecc507fdf..eee70971a4 100644 --- a/src/Crypto/CryptoManager.php +++ b/src/Crypto/CryptoManager.php @@ -13,11 +13,10 @@ namespace Eventum\Crypto; -use CannotPerformOperationException; -use Crypto; -use CryptoTestFailedException; +use Defuse\Crypto\Crypto; +use Defuse\Crypto\Exception\EnvironmentIsBrokenException; +use Defuse\Crypto\RuntimeTests; use InvalidArgumentException; -use InvalidCiphertextException; use Setup; /** @@ -33,14 +32,16 @@ final class CryptoManager */ public static function encryptionEnabled() { - return Setup::get()->encryption == 'enabled'; + return Setup::get()->encryption === 'enabled'; } /** * Checks if system can perform encryption: - * - has mcrypt extension + * - has openssl extension * - some other tests performed by Crypto library * + * Checks for extensions presence because defuse/php-encryption is very cryptic about it errors. + * * @throws CryptoException if it can't be enabled * @return bool */ @@ -49,12 +50,9 @@ public static function canEncrypt() if (!function_exists('openssl_encrypt')) { throw new CryptoException('openssl extension not enabled'); } - if (!function_exists('mcrypt_create_iv')) { - throw new CryptoException('mcrypt extension not enabled'); - } try { - Crypto::RuntimeTest(); - } catch (CryptoTestFailedException $e) { + RuntimeTests::runtimeTest(); + } catch (EnvironmentIsBrokenException $e) { throw new CryptoException($e->getMessage(), $e->getCode(), $e); } @@ -82,14 +80,10 @@ public static function encrypt($plaintext, $key = null) } try { - $ciphertext = Crypto::Encrypt($plaintext, $key ?: self::getKey()); - } catch (CryptoTestFailedException $e) { - throw new CryptoException('Cannot safely perform encryption: Crypto test failed'); - } catch (CannotPerformOperationException $e) { - throw new CryptoException('Cannot safely perform encryption: Cannot perform operation: ' . $e->getMessage()); + return Crypto::encrypt($plaintext, $key ?: self::getKey()); + } catch (EnvironmentIsBrokenException $e) { + throw new CryptoException('Cannot perform operation: ' . $e->getMessage()); } - - return rtrim(base64_encode($ciphertext), '='); } /** @@ -107,22 +101,10 @@ public static function decrypt($ciphertext) } try { - $decrypted = Crypto::Decrypt(base64_decode($ciphertext), self::getKey()); - } catch (InvalidCiphertextException $e) { - // VERY IMPORTANT - // Either: - // 1. The ciphertext was modified by the attacker, - // 2. The key is wrong, or - // 3. $ciphertext is not a valid ciphertext or was corrupted. - // Assume the worst. - throw new CryptoException('The ciphertext has been tampered with'); - } catch (CryptoTestFailedException $e) { - throw new CryptoException('Cannot safely perform encryption: Crypto test failed'); - } catch (CannotPerformOperationException $e) { - throw new CryptoException('Cannot safely perform encryption: Cannot perform operation: ' . $e->getMessage()); + return Crypto::decrypt($ciphertext, self::getKey()); + } catch (EnvironmentIsBrokenException $e) { + throw new CryptoException('Cannot perform operation: ' . $e->getMessage()); } - - return $decrypted; } private static function getKey() diff --git a/src/Crypto/CryptoUpgradeManager.php b/src/Crypto/CryptoUpgradeManager.php index 0f90cec748..7caf84089d 100644 --- a/src/Crypto/CryptoUpgradeManager.php +++ b/src/Crypto/CryptoUpgradeManager.php @@ -32,11 +32,12 @@ public function __construct() * Perform few checks that enable/disable can be performed * * @param bool $enable TRUE if action is to enable, FALSE if action is to disable + * @throws CryptoException */ private function canPerform($enable) { $enabled = CryptoManager::encryptionEnabled(); - if ($enabled && $enable || (!$enabled && !$enable)) { + if (($enabled && $enable) || (!$enabled && !$enable)) { $state = $enabled ? 'enabled' : 'disabled'; throw new CryptoException("Can not perform, already $state"); } From a7487a232389d1f0a47d314862c932b704813b90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elan=20Ruusam=C3=A4e?= Date: Fri, 16 Mar 2018 19:44:54 +0200 Subject: [PATCH 2/6] add legacy keys support and migration to crypto2 --- ...20180316174428_eventum_crypto2_upgrade.php | 30 +++++++++++++++++++ src/Crypto/CryptoKeyManager.php | 8 ++++- src/Crypto/CryptoManager.php | 10 ++++++- src/Crypto/EncryptedValue.php | 9 +++--- 4 files changed, 51 insertions(+), 6 deletions(-) create mode 100644 db/migrations/20180316174428_eventum_crypto2_upgrade.php diff --git a/db/migrations/20180316174428_eventum_crypto2_upgrade.php b/db/migrations/20180316174428_eventum_crypto2_upgrade.php new file mode 100644 index 0000000000..4ea4a61155 --- /dev/null +++ b/db/migrations/20180316174428_eventum_crypto2_upgrade.php @@ -0,0 +1,30 @@ +regenerateKey(); + } + } +} diff --git a/src/Crypto/CryptoKeyManager.php b/src/Crypto/CryptoKeyManager.php index 1231b51284..458fe7077e 100644 --- a/src/Crypto/CryptoKeyManager.php +++ b/src/Crypto/CryptoKeyManager.php @@ -93,15 +93,21 @@ private function loadPrivateKey() if (!is_readable($this->keyfile)) { throw new CryptoException("Secret file '{$this->keyfile}' not readable"); } + // load first to see that it's php script // this would avoid printing secret key to output if in old or invalid format $key = file_get_contents($this->keyfile); if (!$key) { throw new CryptoException("Unable to read secret file '{$this->keyfile}"); } + + // support legacy key format if (substr($key, 0, 5) !== 'key = $key; + + return true; } + $key = require $this->keyfile; if (!$key) { throw new CryptoException("Secret file corrupted: {$this->keyfile}"); diff --git a/src/Crypto/CryptoManager.php b/src/Crypto/CryptoManager.php index eee70971a4..da380352db 100644 --- a/src/Crypto/CryptoManager.php +++ b/src/Crypto/CryptoManager.php @@ -15,6 +15,7 @@ use Defuse\Crypto\Crypto; use Defuse\Crypto\Exception\EnvironmentIsBrokenException; +use Defuse\Crypto\Key; use Defuse\Crypto\RuntimeTests; use InvalidArgumentException; use Setup; @@ -101,7 +102,14 @@ public static function decrypt($ciphertext) } try { - return Crypto::decrypt($ciphertext, self::getKey()); + $key = self::getKey(); + + if ($key instanceof Key) { + return Crypto::decrypt($ciphertext, $key); + } + + // support legacy decrypt + return Crypto::legacyDecrypt(base64_decode($ciphertext), $key); } catch (EnvironmentIsBrokenException $e) { throw new CryptoException('Cannot perform operation: ' . $e->getMessage()); } diff --git a/src/Crypto/EncryptedValue.php b/src/Crypto/EncryptedValue.php index 00e5bcf21c..4b03de66fd 100644 --- a/src/Crypto/EncryptedValue.php +++ b/src/Crypto/EncryptedValue.php @@ -13,8 +13,6 @@ namespace Eventum\Crypto; -use InvalidArgumentException; - /** * Class Encrypted Value * @@ -43,6 +41,7 @@ public function __construct($ciphertext = null) * The encrypted value is stored in object property. * * @param string $plaintext + * @throws CryptoException */ public function setValue($plaintext) { @@ -52,12 +51,13 @@ public function setValue($plaintext) /** * Return plain text value * + * @throws CryptoException * @return string */ public function getValue() { if ($this->ciphertext === null) { - throw new InvalidArgumentException('Value not initialized yet'); + throw new CryptoException('Value not initialized yet'); } return CryptoManager::decrypt($this->ciphertext); @@ -66,12 +66,13 @@ public function getValue() /** * Get encrypted value, for storing it to Database or Config * + * @throws CryptoException * @return string */ public function getEncrypted() { if ($this->ciphertext === null) { - throw new InvalidArgumentException('Value not initialized yet'); + throw new CryptoException('Value not initialized yet'); } return $this->ciphertext; From 4a08c8caacc3218c8752f90341f53367f6e085dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elan=20Ruusam=C3=A4e?= Date: Sat, 17 Mar 2018 23:55:06 +0200 Subject: [PATCH 3/6] update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b2710816a..8b62ad7aa2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ NOTE: Mail related Workflow methods changed signature, see #263 - Use filename-normalizer for attachment filenames extracted from emails (@glensc, #356) - Fix Open Redirect vulnerability found by NetSparker (https://www.netsparker.com/, @balsdorf) - Add experimental markdown rendering (@glensc, #291) +- Update to defuse/php-encryption 2.1 (@glensc, #358) [3.4.0]: https://github.com/eventum/eventum/compare/v3.3.5...master From 2c3193046fcd61507233f156ec0d098f99b52bea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elan=20Ruusam=C3=A4e?= Date: Sun, 18 Mar 2018 00:02:08 +0200 Subject: [PATCH 4/6] ensure all exceptions are caught --- src/Crypto/CryptoManager.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Crypto/CryptoManager.php b/src/Crypto/CryptoManager.php index da380352db..4fc456b466 100644 --- a/src/Crypto/CryptoManager.php +++ b/src/Crypto/CryptoManager.php @@ -53,7 +53,7 @@ public static function canEncrypt() } try { RuntimeTests::runtimeTest(); - } catch (EnvironmentIsBrokenException $e) { + } catch (CryptoException $e) { throw new CryptoException($e->getMessage(), $e->getCode(), $e); } @@ -73,7 +73,7 @@ public static function canEncrypt() public static function encrypt($plaintext, $key = null) { if ($plaintext === null || $plaintext === false) { - throw new InvalidArgumentException('Refusing to encrypt empty value'); + throw new CryptoException('Refusing to encrypt empty value'); } if (!self::encryptionEnabled()) { @@ -82,7 +82,7 @@ public static function encrypt($plaintext, $key = null) try { return Crypto::encrypt($plaintext, $key ?: self::getKey()); - } catch (EnvironmentIsBrokenException $e) { + } catch (CryptoException $e) { throw new CryptoException('Cannot perform operation: ' . $e->getMessage()); } } @@ -110,7 +110,7 @@ public static function decrypt($ciphertext) // support legacy decrypt return Crypto::legacyDecrypt(base64_decode($ciphertext), $key); - } catch (EnvironmentIsBrokenException $e) { + } catch (CryptoException $e) { throw new CryptoException('Cannot perform operation: ' . $e->getMessage()); } } From 2d337ad62915b539756672647cd865d1546d955c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elan=20Ruusam=C3=A4e?= Date: Sun, 18 Mar 2018 00:07:28 +0200 Subject: [PATCH 5/6] use our own base64 encoding to get shorter encrypted results --- src/Crypto/CryptoManager.php | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/Crypto/CryptoManager.php b/src/Crypto/CryptoManager.php index 4fc456b466..9708f50715 100644 --- a/src/Crypto/CryptoManager.php +++ b/src/Crypto/CryptoManager.php @@ -14,10 +14,8 @@ namespace Eventum\Crypto; use Defuse\Crypto\Crypto; -use Defuse\Crypto\Exception\EnvironmentIsBrokenException; use Defuse\Crypto\Key; use Defuse\Crypto\RuntimeTests; -use InvalidArgumentException; use Setup; /** @@ -81,10 +79,12 @@ public static function encrypt($plaintext, $key = null) } try { - return Crypto::encrypt($plaintext, $key ?: self::getKey()); + $ciphertext = Crypto::encrypt($plaintext, $key ?: self::getKey(), true); } catch (CryptoException $e) { throw new CryptoException('Cannot perform operation: ' . $e->getMessage()); } + + return rtrim(base64_encode($ciphertext), '='); } /** @@ -101,15 +101,20 @@ public static function decrypt($ciphertext) return $ciphertext; } + $ciphertext = base64_decode($ciphertext); + if (!$ciphertext) { + throw new CryptoException('Unable to decode ciphertext'); + } + try { $key = self::getKey(); if ($key instanceof Key) { - return Crypto::decrypt($ciphertext, $key); + return Crypto::decrypt($ciphertext, $key, true); } // support legacy decrypt - return Crypto::legacyDecrypt(base64_decode($ciphertext), $key); + return Crypto::legacyDecrypt($ciphertext, $key); } catch (CryptoException $e) { throw new CryptoException('Cannot perform operation: ' . $e->getMessage()); } From f38be2fc2c446cf07947b1f68d56367004612fda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Elan=20Ruusam=C3=A4e?= Date: Wed, 21 Mar 2018 21:15:45 +0200 Subject: [PATCH 6/6] disable encryption on downgrade --- .../20180316174428_eventum_crypto2_upgrade.php | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/db/migrations/20180316174428_eventum_crypto2_upgrade.php b/db/migrations/20180316174428_eventum_crypto2_upgrade.php index 4ea4a61155..a72e06357f 100644 --- a/db/migrations/20180316174428_eventum_crypto2_upgrade.php +++ b/db/migrations/20180316174428_eventum_crypto2_upgrade.php @@ -17,9 +17,6 @@ class EventumCrypto2Upgrade extends AbstractMigration { - /** - * There is no downgrade support. - */ public function up() { if (CryptoManager::encryptionEnabled()) { @@ -27,4 +24,16 @@ public function up() $cm->regenerateKey(); } } + + /** + * We can not migrate back to old key format, + * so just to keep installation usable, disable encryption. + */ + public function down() + { + if (CryptoManager::encryptionEnabled()) { + $cm = new CryptoUpgradeManager(); + $cm->disable(); + } + } }