Ciphertext Contains Key

[defuse]

This is a really bad idea:

$cipherText->key;

The interface should be changed so that the key is never stored in the ciphertext... because it will lead naive users to believe that it's ok to store the key with the ciphertext, and some people are going to serialize() without knowing the key is in there, etc.

[kaisellgren]

This is why I provided the save() method so that you can save its contents (without the key).

Maybe best to educate users to use it? Or we hook into the serialization process and make sure the key will never be there.

[defuse]

Thanks for putting up with my late-night github auditing. :-)

It would be best to keep the key separate. Have a key-generation function that returns a good key, then have the user pass that back to encrypt. From the user's perspective, it's 1 or 2 more lines of code. Something like this would be nice:

$encrypter = new Encrypter(MCRYPT_RIJNDAEL_128);
$key = $encrypter->randomKey();      or       $key = $encrypter->keyFromPassword($password);
$cipherText = $encrypter->encrypt("I am using PSL");
$plainText = $encrypter->decrypt($cipherText, $key);

To make things even easier, I would make the cipher parameter optional, defaulting to MCRYPT_RIJNDAEL_128, since most users will not know enough to make good cipher choices. You can format the key in a special way so that you're sure the key is either random, or derived from a password, making it much harder for a user to use a weak key (e.g. hard-coded ascii string) by accident.

Oh yeah, forgot to mention this one: MCRYPT_RIJNDAEL_256 isn't AES, it's the 256-bit block variant of Rijndael, which hasn't received as much cryptanalysis and doesn't have as good security margins as AES. AES is MCRYPT_RIJNDAEL_128 (this includes 128, 192, and 256-bit keys). I'm not even sure why the other one is in PHP, almost all PHP crypto makes this mistake.