ZF-6386: Zend_Filter_Encrypt_Mcrypt encodes key before encryption and decryption

Description

Zend_Filter_Encrypt_Mcrypt currently encodes the user provided key before using it to encrypt or decrypt data. This breaks the ability to use the encryption and decryption filters, using the Mcrypt adapter, with any data not encrypted using Zend_Filter_Encrypt_Mcrypt.

The code in question is:

    srand();
    $keysize = mcrypt_enc_get_key_size($cipher);
    $key     = substr(md5($this->_encryption['key']), 0, $keysize);

If you want to see the different results you can use these two pieces of code:

Key: 1234567890ABCDEFGHIJKLMNOPQRSTUV (32 byte, 256 bit) IV: 1234567890ABCDEF (16 byte, 128 bit) Algorithm: MCRYPT_RIJNDAEL_128 Mode: MCRYPT_MODE_CBC

PHP Mcrypt: $module = mcrypt_module_open(MCRYPT_RIJNDAEL_128, '', MCRYPT_MODE_CBC, ''); mcrypt_generic_init($module, '1234567890ABCDEFGHIJKLMNOPQRSTUV', '1234567890ABCDEF'); $encrypted = mcrypt_generic($module, 'test_string');

$encoded = base64_encode($encrypted);

encoded value is gFHQANviiEyF2LEy24Ws+w==

Zend_Filter_Encrypt: $encrypt = new Zend_Filter_Encrypt( array( 'adapter'=>'mcrypt', 'algorithm'=>MCRYPT_RIJNDAEL_128, 'mode'=>MCRYPT_MODE_CBC, 'key'=>'1234567890ABCDEFGHIJKLMNOPQRSTUV', 'vector'=>'1234567890ABCDEF' ) ); $encrypted = $encrypt->filter('test_string');

$encoded = base64_encode($encrypted);

encoded value is mkN6BBnQWHnwiPjnKe6zGg==

In this case, even though the keys are the same, Zend_Filter_Encrypt_Mcrypt uses md5 to encode the key before encryption. The resulting key (09c87b51e8bfad618e7bb82c538ac525) is actually used to perform the encryption.

At a minimum, it should be possible to disable the encoding of the key. If this is done, functionality needs to be added to ensure that the key length is valid for the cipher being used.

The documentation, both in the code and in the reference guide needs to be updated to explain that the key is encoded before being used to perform the encryption.

I will provide a patch in the next day or two. At this point I assume that we need to maintain the current functionality, adding the ability to turn it off, instead of just removing encoding entirely. Is that an accurate assumption?

Comments

New feature 'salt option' added with r15120. Allows the usage of salted or unsalted keys.

Attached would be my recommended changes. I went a step further and modified the code to allow/disallow padding of both the vector and the key. Also added helper methods and such. It's pretty self explanatory.

Why is this marked as fixed? I got the latest 1.10.8 and the patch is not appended to it.

The failure was about not turning off default salting. This has been fixed by r15120 which also included tests which showed the correct behaviour and that the mentioned failure was really fixed.

Therefor this issue was closed more than a year ago with the comment what has been done. Normally a patch is not used for an already fixed issue as long as there is no note of an failure or problem with the fix itself. James would have mentioned this.

Note that padding of key/vector would also be seen problematic because encoding/decoding would again only work with Zend_Filter_Encode instead of (as mentioned) other possible components beside ZF.

Sorry, I got the line numbers wrong.

Line 277: mcrypt_enc_get_supported_key_sizes($cipher) Line 278: if (empty($keysizes) || ($this->_encryption['salt'] == true))

Both in MCrypt.php