From d7ed0339b1d8c00b528b177ddf375c6383b841d0 Mon Sep 17 00:00:00 2001 From: chinpei215 Date: Fri, 19 Jan 2018 23:35:39 +0900 Subject: [PATCH 1/4] Make mcrypt optional Now Security::encrypt() and Security::decrypt() works with openssl if the mcrypt extension is unavailable. Note that Security::rijndael() doesn't work with openssl. --- composer.json | 7 +- lib/Cake/Test/Case/Utility/SecurityTest.php | 87 +++++++++++++++++++++ lib/Cake/Utility/Security.php | 71 ++++++++++++++--- 3 files changed, 151 insertions(+), 14 deletions(-) diff --git a/composer.json b/composer.json index dfaee3a63..f807f6612 100644 --- a/composer.json +++ b/composer.json @@ -18,8 +18,11 @@ "source": "https://github.com/cakephp/cakephp" }, "require": { - "php": ">=5.3.0", - "ext-mcrypt": "*" + "php": ">=5.3.0" + }, + "suggest": { + "ext-openssl": "You need to install ext-openssl or ext-mcrypt to use AES-256 encryption", + "ext-mcrypt": "You need to install ext-openssl or ext-mcrypt to use AES-256 encryption" }, "require-dev": { "phpunit/phpunit": "<6.0.0", diff --git a/lib/Cake/Test/Case/Utility/SecurityTest.php b/lib/Cake/Test/Case/Utility/SecurityTest.php index fc7c25bdd..545ba4b0b 100644 --- a/lib/Cake/Test/Case/Utility/SecurityTest.php +++ b/lib/Cake/Test/Case/Utility/SecurityTest.php @@ -29,6 +29,45 @@ class SecurityTest extends CakeTestCase { */ public $sut = null; +/** + * setUp method + * + * @return void + */ + public function setUp() { + parent::setUp(); + Security::engine(null); + } + +/** + * tearDown method + * + * @return void + */ + public function tearDown() { + parent::tearDown(); + Security::engine(null); + } + +/** + * Tests that Security::engine() works + * + * @return void + */ + public function testEngine() { + if (extension_loaded('mcrypt')) { + $this->assertEquals('mcrypt', Security::engine()); + } + + $this->assertContains(Security::engine(), array('mcrypt', 'openssl')); + + Security::engine('mcrypt'); + $this->assertEquals('mcrypt', Security::engine()); + + Security::engine('openssl'); + $this->assertEquals('openssl', Security::engine()); + } + /** * testInactiveMins method * @@ -337,6 +376,54 @@ class SecurityTest extends CakeTestCase { $this->assertEquals($txt, Security::decrypt($result, $key)); } +/** + * Tests that encrypted strings are comatible between the mcrypt and openssl engine. + * + * @dataProvider plainTextProvider + * @param string $txt Plain text to be encrypted. + * @return void + */ + public function testEncryptDecryptCompatibility($txt) { + $this->skipIf(!extension_loaded('mcrypt'), 'This test requires mcrypt to be installed'); + $this->skipIf(!extension_loaded('openssl'), 'This test requires oepnssl to be installed'); + $this->skipIf(version_compare(PHP_VERSION, '5.3.3', '<'), 'This test requires PHP 5.3.3 or grater'); + + $key = '12345678901234567890123456789012'; + + Security::engine('mcrypt'); + $mcrypt = Security::encrypt($txt, $key); + + Security::engine('openssl'); + $openssl = Security::encrypt($txt, $key); + + $this->assertEquals(strlen($mcrypt), strlen($openssl)); + + Security::engine('mcrypt'); + $this->assertEquals($txt, Security::decrypt($mcrypt, $key)); + $this->assertEquals($txt, Security::decrypt($openssl, $key)); + + Security::engine('openssl'); + $this->assertEquals($txt, Security::decrypt($mcrypt, $key)); + $this->assertEquals($txt, Security::decrypt($openssl, $key)); + } + +/** + * Data provider for testEncryptDecryptCompatibility + * + * @return array + */ + public function plainTextProvider() { + return array( + array(''), + array('abcdefg'), + array('1234567890123456'), + array('The quick brown fox'), + array('12345678901234567890123456789012'), + array('The quick brown fox jumped over the lazy dog.'), + array('何らかのマルチバイト文字列'), + ); + } + /** * Test that changing the key causes decryption to fail. * diff --git a/lib/Cake/Utility/Security.php b/lib/Cake/Utility/Security.php index 75c5ad4c9..1c5db6482 100644 --- a/lib/Cake/Utility/Security.php +++ b/lib/Cake/Utility/Security.php @@ -25,6 +25,13 @@ App::uses('CakeText', 'Utility'); */ class Security { +/** + * The encryption engine + * + * @var string + */ + protected static $_engine = null; + /** * Default hash method * @@ -206,7 +213,7 @@ class Security { * for sensitive data. Additionally this method does *not* work in environments * where suhosin is enabled. * - * Instead you should use Security::rijndael() when you need strong + * Instead you should use Security::encrypt() when you need strong * encryption. * * @param string $text Encrypted string to decrypt, normal string to encrypt @@ -349,12 +356,24 @@ class Security { // Generate the encryption and hmac key. $key = substr(hash('sha256', $key . $hmacSalt), 0, 32); - $algorithm = MCRYPT_RIJNDAEL_128; - $mode = MCRYPT_MODE_CBC; + if (static::engine() === 'openssl') { + $method = 'AES-256-CBC'; + $ivSize = openssl_cipher_iv_length($method); + $iv = openssl_random_pseudo_bytes($ivSize); + $padLength = (int)ceil((strlen($plain) ?: 1) / $ivSize) * $ivSize; + $ciphertext = openssl_encrypt(str_pad($plain, $padLength, "\0"), $method, $key, true, $iv); + // Remove the PKCS#7 padding block for compatibility with mcrypt. + // Since we have padded the provided data with \0, the final block contains only padded bytes. + // So it can be removed safely. + $ciphertext = $iv . substr($ciphertext, 0, -$ivSize); + } else { + $algorithm = MCRYPT_RIJNDAEL_128; + $mode = MCRYPT_MODE_CBC; + $ivSize = mcrypt_get_iv_size($algorithm, $mode); + $iv = mcrypt_create_iv($ivSize, MCRYPT_DEV_URANDOM); + $ciphertext = $iv . mcrypt_encrypt($algorithm, $key, $plain, $mode, $iv); + } - $ivSize = mcrypt_get_iv_size($algorithm, $mode); - $iv = mcrypt_create_iv($ivSize, MCRYPT_DEV_URANDOM); - $ciphertext = $iv . mcrypt_encrypt($algorithm, $key, $plain, $mode, $iv); $hmac = hash_hmac('sha256', $ciphertext, $key); return $hmac . $ciphertext; } @@ -404,14 +423,42 @@ class Security { return false; } - $algorithm = MCRYPT_RIJNDAEL_128; - $mode = MCRYPT_MODE_CBC; - $ivSize = mcrypt_get_iv_size($algorithm, $mode); + if (static::engine() === 'openssl') { + $method = 'AES-256-CBC'; + $ivSize = openssl_cipher_iv_length($method); + $iv = substr($cipher, 0, $ivSize); + $cipher = substr($cipher, $ivSize); + // Regenerate PKCS#7 padding block + $padding = openssl_encrypt('', $method, $key, true, substr($cipher, -$ivSize)); + $plain = openssl_decrypt($cipher . $padding, $method, $key, true, $iv); + } else { + $algorithm = MCRYPT_RIJNDAEL_128; + $mode = MCRYPT_MODE_CBC; + $ivSize = mcrypt_get_iv_size($algorithm, $mode); + $iv = substr($cipher, 0, $ivSize); + $cipher = substr($cipher, $ivSize); + $plain = mcrypt_decrypt($algorithm, $key, $cipher, $mode, $iv); + } - $iv = substr($cipher, 0, $ivSize); - $cipher = substr($cipher, $ivSize); - $plain = mcrypt_decrypt($algorithm, $key, $cipher, $mode, $iv); return rtrim($plain, "\0"); } +/** + * Set or get the encryption engine + * + * @param string $engine The encryption engine to use + * @return string + */ + public static function engine($engine = null) { + if (func_num_args() > 0) { + static::$_engine = $engine; + } elseif (static::$_engine === null) { + static::$_engine = 'mcrypt'; + if (!extension_loaded('mcrypt') && extension_loaded('openssl') && version_compare(PHP_VERSION, '5.3.3', '>=')) { + static::$_engine = 'openssl'; + } + } + return static::$_engine; + } + } From 5289aae64ed3b2e3029245aad960d9d7cc35d5d5 Mon Sep 17 00:00:00 2001 From: chinpei215 Date: Fri, 19 Jan 2018 23:50:14 +0900 Subject: [PATCH 2/4] Change Security::randomBytes() to fallback to mcrypt_create_iv() --- lib/Cake/Utility/Security.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/Cake/Utility/Security.php b/lib/Cake/Utility/Security.php index 1c5db6482..e6ac1e849 100644 --- a/lib/Cake/Utility/Security.php +++ b/lib/Cake/Utility/Security.php @@ -191,9 +191,12 @@ class Security { if (function_exists('openssl_random_pseudo_bytes')) { return openssl_random_pseudo_bytes($length); } + if (function_exists('mcrypt_create_iv')) { + return mcrypt_create_iv($length); + } trigger_error( 'You do not have a safe source of random data available. ' . - 'Install either the openssl extension, or paragonie/random_compat. ' . + 'Install either the openssl extension, the mcrypt extension, or paragonie/random_compat. ' . 'Falling back to an insecure random source.', E_USER_WARNING ); From fc397bd4812a8f7e29c6f12ca5cd103d719ea4fd Mon Sep 17 00:00:00 2001 From: chinpei215 Date: Sat, 20 Jan 2018 00:25:35 +0900 Subject: [PATCH 3/4] Pass MCRYPT_DEV_URANDOM to mcrypt_create_iv() explicitly --- lib/Cake/Utility/Security.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Cake/Utility/Security.php b/lib/Cake/Utility/Security.php index e6ac1e849..739874c09 100644 --- a/lib/Cake/Utility/Security.php +++ b/lib/Cake/Utility/Security.php @@ -192,7 +192,7 @@ class Security { return openssl_random_pseudo_bytes($length); } if (function_exists('mcrypt_create_iv')) { - return mcrypt_create_iv($length); + return mcrypt_create_iv($length, MCRYPT_DEV_URANDOM); } trigger_error( 'You do not have a safe source of random data available. ' . From a6b0271560a41f5dbc52b4484491a6b3700bac52 Mon Sep 17 00:00:00 2001 From: chinpei215 Date: Sat, 24 Feb 2018 12:17:51 +0900 Subject: [PATCH 4/4] Remove Security::engine() We disscussed and decided to avoid auto selecting which extension to use. Instead, call Configure::write('Security.useOpenSsl', true) manually. --- lib/Cake/Test/Case/Utility/SecurityTest.php | 35 +++++---------------- lib/Cake/Utility/Security.php | 29 ++--------------- 2 files changed, 10 insertions(+), 54 deletions(-) diff --git a/lib/Cake/Test/Case/Utility/SecurityTest.php b/lib/Cake/Test/Case/Utility/SecurityTest.php index 545ba4b0b..2b1fe1ab7 100644 --- a/lib/Cake/Test/Case/Utility/SecurityTest.php +++ b/lib/Cake/Test/Case/Utility/SecurityTest.php @@ -36,7 +36,7 @@ class SecurityTest extends CakeTestCase { */ public function setUp() { parent::setUp(); - Security::engine(null); + Configure::delete('Security.useOpenSsl'); } /** @@ -46,26 +46,7 @@ class SecurityTest extends CakeTestCase { */ public function tearDown() { parent::tearDown(); - Security::engine(null); - } - -/** - * Tests that Security::engine() works - * - * @return void - */ - public function testEngine() { - if (extension_loaded('mcrypt')) { - $this->assertEquals('mcrypt', Security::engine()); - } - - $this->assertContains(Security::engine(), array('mcrypt', 'openssl')); - - Security::engine('mcrypt'); - $this->assertEquals('mcrypt', Security::engine()); - - Security::engine('openssl'); - $this->assertEquals('openssl', Security::engine()); + Configure::delete('Security.useOpenSsl'); } /** @@ -385,24 +366,24 @@ class SecurityTest extends CakeTestCase { */ public function testEncryptDecryptCompatibility($txt) { $this->skipIf(!extension_loaded('mcrypt'), 'This test requires mcrypt to be installed'); - $this->skipIf(!extension_loaded('openssl'), 'This test requires oepnssl to be installed'); - $this->skipIf(version_compare(PHP_VERSION, '5.3.3', '<'), 'This test requires PHP 5.3.3 or grater'); + $this->skipIf(!extension_loaded('openssl'), 'This test requires openssl to be installed'); + $this->skipIf(version_compare(PHP_VERSION, '5.3.3', '<'), 'This test requires PHP 5.3.3 or greater'); $key = '12345678901234567890123456789012'; - Security::engine('mcrypt'); + Configure::write('Security.useOpenSsl', false); $mcrypt = Security::encrypt($txt, $key); - Security::engine('openssl'); + Configure::write('Security.useOpenSsl', true); $openssl = Security::encrypt($txt, $key); $this->assertEquals(strlen($mcrypt), strlen($openssl)); - Security::engine('mcrypt'); + Configure::write('Security.useOpenSsl', false); $this->assertEquals($txt, Security::decrypt($mcrypt, $key)); $this->assertEquals($txt, Security::decrypt($openssl, $key)); - Security::engine('openssl'); + Configure::write('Security.useOpenSsl', true); $this->assertEquals($txt, Security::decrypt($mcrypt, $key)); $this->assertEquals($txt, Security::decrypt($openssl, $key)); } diff --git a/lib/Cake/Utility/Security.php b/lib/Cake/Utility/Security.php index 739874c09..322b29d9e 100644 --- a/lib/Cake/Utility/Security.php +++ b/lib/Cake/Utility/Security.php @@ -25,13 +25,6 @@ App::uses('CakeText', 'Utility'); */ class Security { -/** - * The encryption engine - * - * @var string - */ - protected static $_engine = null; - /** * Default hash method * @@ -359,7 +352,7 @@ class Security { // Generate the encryption and hmac key. $key = substr(hash('sha256', $key . $hmacSalt), 0, 32); - if (static::engine() === 'openssl') { + if (Configure::read('Security.useOpenSsl')) { $method = 'AES-256-CBC'; $ivSize = openssl_cipher_iv_length($method); $iv = openssl_random_pseudo_bytes($ivSize); @@ -426,7 +419,7 @@ class Security { return false; } - if (static::engine() === 'openssl') { + if (Configure::read('Security.useOpenSsl')) { $method = 'AES-256-CBC'; $ivSize = openssl_cipher_iv_length($method); $iv = substr($cipher, 0, $ivSize); @@ -446,22 +439,4 @@ class Security { return rtrim($plain, "\0"); } -/** - * Set or get the encryption engine - * - * @param string $engine The encryption engine to use - * @return string - */ - public static function engine($engine = null) { - if (func_num_args() > 0) { - static::$_engine = $engine; - } elseif (static::$_engine === null) { - static::$_engine = 'mcrypt'; - if (!extension_loaded('mcrypt') && extension_loaded('openssl') && version_compare(PHP_VERSION, '5.3.3', '>=')) { - static::$_engine = 'openssl'; - } - } - return static::$_engine; - } - }