Fix and implement cross engine decryption.

Sort out the nasty problems that were preventing Mcrypt and OpenSSL
backends from decrypting each others ciphertext.

Add a test to show how each engine can decode its own and the other's
cipher texts.

src/Utility/Crypto/Mcrypt.php

@@ -16,6 +16,11 @@ namespace Cake\Utility\Crypto;
 
 /**
  * Mcrypt implementation of crypto features for Cake\Utility\Security
+ *
+ * This class is not intended to be used directly and should only
+ * be used in the context of Cake\Utility\Security.
+ *
+ * @internal
  */
 class Mcrypt {
 
@@ -55,15 +60,18 @@ class Mcrypt {
  * @return string Encrypted data.
  * @throws \InvalidArgumentException On invalid data or key.
  */
-	public static function encrypt($plain, $key, $hmacSalt = null) {
+	public static function encrypt($plain, $key) {
 		$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);
-		$hmac = hash_hmac('sha256', $ciphertext, $key);
-		return $hmac . $ciphertext;
+
+		// Pad out plain to make it AES compatible.
+		$pad = ($ivSize - (strlen($plain) % $ivSize));
+		$plain .= str_repeat(chr($pad), $pad);
+
+		return $iv . mcrypt_encrypt($algorithm, $key, $plain, $mode, $iv);
 	}
 
 /**
@@ -75,17 +83,6 @@ class Mcrypt {
  * @throws InvalidArgumentException On invalid data or key.
  */
 	public static function decrypt($cipher, $key) {
-		// Split out hmac for comparison
-		$macSize = 64;
-		$hmac = substr($cipher, 0, $macSize);
-		$cipher = substr($cipher, $macSize);
-
-		$compareHmac = hash_hmac('sha256', $cipher, $key);
-		// TODO time constant comparison?
-		if ($hmac !== $compareHmac) {
-			return false;
-		}
-
 		$algorithm = MCRYPT_RIJNDAEL_128;
 		$mode = MCRYPT_MODE_CBC;
 		$ivSize = mcrypt_get_iv_size($algorithm, $mode);
@@ -93,6 +90,10 @@ class Mcrypt {
 		$iv = substr($cipher, 0, $ivSize);
 		$cipher = substr($cipher, $ivSize);
 		$plain = mcrypt_decrypt($algorithm, $key, $cipher, $mode, $iv);
-		return rtrim($plain, "\0");
+
+		// Remove PKCS#7 padding
+		$padChar = substr($plain, -1);
+		$padLen = ord($padChar);
+		return substr($plain, 0, -$padLen);
 	}
 }

src/Utility/Crypto/OpenSsl.php

@@ -19,6 +19,11 @@ namespace Cake\Utility\Crypto;
  *
  * OpenSSL should be favored over mcrypt as it is actively maintained and
  * more widely available.
+ *
+ * This class is not intended to be used directly and should only
+ * be used in the context of Cake\Utility\Security.
+ *
+ * @internal
  */
 class OpenSsl {
 
@@ -49,12 +54,11 @@ class OpenSsl {
  * @throws \InvalidArgumentException On invalid data or key.
  */
 	public static function encrypt($plain, $key, $hmacSalt = null) {
-		$method = 'AES-128-CBC';
+		$method = 'AES-256-CBC';
 		$ivSize = openssl_cipher_iv_length($method);
+
 		$iv = openssl_random_pseudo_bytes($ivSize);
-		$ciphertext = $iv . openssl_encrypt($plain, $method, $key, 0, $iv);
-		$hmac = hash_hmac('sha256', $ciphertext, $key);
-		return $hmac . $ciphertext;
+		return $iv . openssl_encrypt($plain, $method, $key, true, $iv);
 	}
 
 /**
@@ -66,23 +70,13 @@ class OpenSsl {
  * @throws InvalidArgumentException On invalid data or key.
  */
 	public static function decrypt($cipher, $key) {
-		// Split out hmac for comparison
-		$macSize = 64;
-		$hmac = substr($cipher, 0, $macSize);
-		$cipher = substr($cipher, $macSize);
-
-		$compareHmac = hash_hmac('sha256', $cipher, $key);
-		// TODO time constant comparison?
-		if ($hmac !== $compareHmac) {
-			return false;
-		}
-		$method = 'AES-128-CBC';
+		$method = 'aes-256-cbc';
 		$ivSize = openssl_cipher_iv_length($method);
 
 		$iv = substr($cipher, 0, $ivSize);
+
 		$cipher = substr($cipher, $ivSize);
-		$plain = openssl_decrypt($cipher, $method, $key, 0, $iv);
-		return rtrim($plain, "\0");
+		return openssl_decrypt($cipher, $method, $key, true, $iv);
 	}
 }
 

src/Utility/Security.php

@@ -98,6 +98,8 @@ class Security {
 /**
  * Get the crypto implementation based on the loaded extensions.
  *
+ * You can use this method to forcibly decide between mcrypt/openssl/custom implementations.
+ *
  * @param object $instance The crypto instance to use.
  * @return object Crypto instance.
  */
@@ -115,7 +117,9 @@ class Security {
 		if (isset(static::$_instance)) {
 			return static::$_instance;
 		}
-		throw new InvalidArgumentException('No compatible crypto engine loaded. Load either mcrypt or openssl');
+		throw new InvalidArgumentException(
+			'No compatible crypto engine available. ' .
+			'Load either the openssl or mcrypt extensions');
 	}
 
 /**
@@ -164,7 +168,9 @@ class Security {
 		$key = substr(hash('sha256', $key . $hmacSalt), 0, 32);
 
 		$crypto = static::engine();
-		return $crypto->encrypt($plain, $key);
+		$ciphertext = $crypto->encrypt($plain, $key);
+		$hmac = hash_hmac('sha256', $ciphertext, $key);
+		return $hmac . $ciphertext;
 	}
 
 /**
@@ -204,6 +210,16 @@ class Security {
 		// Generate the encryption and hmac key.
 		$key = substr(hash('sha256', $key . $hmacSalt), 0, 32);
 
+		// Split out hmac for comparison
+		$macSize = 64;
+		$hmac = substr($cipher, 0, $macSize);
+		$cipher = substr($cipher, $macSize);
+
+		$compareHmac = hash_hmac('sha256', $cipher, $key);
+		if ($hmac !== $compareHmac) {
+			return false;
+		}
+
 		$crypto = static::engine();
 		return $crypto->decrypt($cipher, $key);
 	}

tests/TestCase/Utility/SecurityTest.php

@@ -15,6 +15,8 @@
 namespace Cake\Test\TestCase\Utility;
 
 use Cake\TestSuite\TestCase;
+use Cake\Utility\Crypto\Mcrypt;
+use Cake\Utility\Crypto\OpenSsl;
 use Cake\Utility\Security;
 
 /**
@@ -82,21 +84,23 @@ class SecurityTest extends TestCase {
  */
 	public function testRijndael() {
 		$this->skipIf(!function_exists('mcrypt_encrypt'));
+		$engine = Security::engine();
+
+		Security::engine(new Mcrypt());
 		$txt = 'The quick brown fox jumped over the lazy dog.';
 		$key = 'DYhG93b0qyJfIxfs2guVoUubWwvniR2G0FgaC9mi';
 
 		$result = Security::rijndael($txt, $key, 'encrypt');
 		$this->assertEquals($txt, Security::rijndael($result, $key, 'decrypt'));
 
-		$result = Security::rijndael($key, $txt, 'encrypt');
-		$this->assertEquals($key, Security::rijndael($result, $txt, 'decrypt'));
-
 		$result = Security::rijndael('', $key, 'encrypt');
 		$this->assertEquals('', Security::rijndael($result, $key, 'decrypt'));
 
 		$key = 'this is my key of over 32 chars, yes it is';
 		$result = Security::rijndael($txt, $key, 'encrypt');
 		$this->assertEquals($txt, Security::rijndael($result, $key, 'decrypt'));
+
+		Security::engine($engine);
 	}
 
 /**
@@ -246,6 +250,32 @@ class SecurityTest extends TestCase {
 	}
 
 /**
+ * Test that values encrypted with open ssl can be decrypted with mcrypt and the reverse.
+ *
+ * @return void
+ */
+	public function testEngineEquivalence() {
+		$restore = Security::engine();
+		$txt = "Obi-wan you're our only hope";
+		$key = 'This is my secret key phrase it is quite long.';
+		$salt = 'A tasty salt that is delicious';
+
+		Security::engine(new Mcrypt());
+		$cipher = Security::encrypt($txt, $key, $salt);
+		$this->assertEquals($txt, Security::decrypt($cipher, $key, $salt));
+
+		Security::engine(new OpenSsl());
+		$this->assertEquals($txt, Security::decrypt($cipher, $key, $salt));
+
+		Security::engine(new OpenSsl());
+		$cipher = Security::encrypt($txt, $key, $salt);
+		$this->assertEquals($txt, Security::decrypt($cipher, $key, $salt));
+
+		Security::engine(new Mcrypt());
+		$this->assertEquals($txt, Security::decrypt($cipher, $key, $salt));
+	}
+
+/**
  * Tests that the salt can be set and retrieved
  *
  * @return void