php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #68238 mcrypt_encode tests are broken
Submitted: 2014-10-15 17:16 UTC Modified: 2014-10-15 18:30 UTC
From: gm dot outside+php at gmail dot com Assigned:
Status: Not a bug Package: Testing related
PHP Version: 5.6.1 OS: Linux
Private report: No CVE-ID: None
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If this is not your bug, you can add a comment by following this link.
If this is your bug, but you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: gm dot outside+php at gmail dot com
New email:
PHP Version: OS:

 

 [2014-10-15 17:16 UTC] gm dot outside+php at gmail dot com
Description:
------------
There was a recent commit (http://git.php.net/?p=php-src.git;a=commit;h=a861a3a93d89a50ce58e1ab1abef1eb501f97483) that changed behaviour of the mcrypt_encode() function.  After that commit the key is required to be at least the expected key length long, otherwise a warning message is issued and the mcrypt_encode() routine returns a failure.

The corresponding test in ext/mcrypt/tests/bug62102_rfc2144.php supplies 10 bytes key instead of 16 for cast-128 80-bit encryption and 5 bytes key instead of 10 for cast-128 40-bit encryption.

A quick fix to the test would be to pad the keys with '\0' manually (RFC-2144 B.1), e.g.

mcrypt_encrypt('cast-128', "\x01\x23\x45\x67\x12\x34\x56\x78\x23\x45\0\0\0\0\0\0", $plaintext, 'ecb')

but unfortunately due to the way changed code treats key data (as a null terminated string) and due to calculating the key size as strlen() of that string there is no way to satisfy the RFC-2144 B.1 since all trailing '\0' will be ignored.


Expected result:
----------------
That the RFC-2144 test would be passed with the explicitly specified vector and that mcrypt_encrypt() would honour the key argument as a binary string that can include '\0' anywhere in the string.

Actual result:
--------------
All trailing '\0' in the key argument are ignored, therefore it's impossible to pass RFC-2144 test to match section B.1.

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2014-10-15 17:32 UTC] gm dot outside+php at gmail dot com
I did more testing, and I was a bit wrong about the strlen() part.  Manually padding the keys with '\0' actually works, but the result does not match the ciphertext provided in RFC-2144 B.1 anymore.  Additionally to that I was wrong re: the keysize requirement for that cipher should be 16 bytes (128-bit) as cipher's name 'cast-128' suggests.

Once the keys are properly padded with '\0' to be 128-bit the test returns the following differences:

002+ 80-bit: 753de29f5d167d03
003+ 40-bit: f00b0530833d7444
002- 80-bit: eb6a711a2c02271b
003- 40-bit: 7ac816d16e9b302e

So, something else was also changed that the mcrypt extension no longer conforms to RFC-2144 B.1.
 [2014-10-15 18:12 UTC] nikic@php.net
-Status: Open +Status: Not a bug
 [2014-10-15 18:12 UTC] nikic@php.net
This is a bug in your mcrypt library, which does not properly support Cast-128 with keys <= 80 bits. Padding the keys with NUL bytes is not the same, because  Cast-128 uses only 12 rounds (instead of 16) if the key is <= 80 bits.

Updating your libmcrypt version will probably fix this.
 [2014-10-15 18:30 UTC] gm dot outside+php at gmail dot com
@nikic, I'm running the latest available mcrypt, which is 2.5.8.  The library passed all its internal tests.  Also, PHP 5.5.12 is working on the same box and does not have any issues with mcrypt_encode().

Additionally to this there are other bug reports pointing to the same issue, e.g. bug #67286 , so I would not dismiss this bug this easy.

Finally, the gcov test at php.net itself is failing the test on mcrypt.  The following tests was performed less than 2 days ago: http://gcov.php.net/viewer.php?version=PHP_HEAD&func=tests&file=ext%2Fmcrypt%2Ftests%2Fbug62102_rfc2144.phpt

So, let's try to pinpoint the issue? :)  Or at least let's fix the test case...
 [2014-10-15 18:43 UTC] nikic@php.net
The bug you linked has nothing to do with issue, it is a generic complaint from someone using broken crypto code, which is rejected by the stricter input requirements in PHP 5.6.

When you say that mcrypt_encrypt on your PHP 5.5.12 build works correctly, do you mean that it provides the correct output for this test script (whereas PHP 5.6 does not)? This test was only added in PHP 5.6, so just running `make test` will not be enough to check it - you have to manually run the file from the 5.6 tree.
 [2014-10-15 19:11 UTC] gm dot outside+php at gmail dot com
Re: bug #67286 - well, the change to the behaviour as it is now is incompatible with the documentation at http://php.net/manual/en/function.mcrypt-encrypt.php, so there people will report bugs against this behaviour.  Moreover, the change is breaking things -- a warning message is one thing, but the function was changed to bail out with a failure upon a discovery of incorrect key size.  This is the problem, actually.

Re: test case.  Yes, the test case was added on Mar 5th (you committed it 2 days later), but it's currently broken in the HEAD, so I'm curious how one could do proper unit testing there :).

Re: 5.5.12 - no, I didn't run the exact same test, but OctoberCMS I'm running on that box stopped to work after switching to 5.6.1.  By default that application was using the MCRYPT_RIJNDAEL_128 cipher and that cipher was no longer working (the same application, the same mcrypt library, and the only change was PHP).  This is the reason I started to investigate.

Finally, I'm going to run the same test case on my PHP 5.5.12 and will report back shortly.  Still, it's strange to have a broken test for something that important as encryption.
 [2014-10-15 20:15 UTC] nikic@php.net
Thank you for pointing out the outdated information in the documentation. I have just pushed some updates to the mcrypt_encrypt and mcrypt_decrypt docs; however it will take a few days to propagate to mirrors.

The test works on my machine (using libmcrypt 2.5.8-3.1 on Ubuntu) and also works on Travis (which is our CI platform). As libmcrypt is currently not maintained, it might be that the Cast-128 fixes have been applied by the distro vendor and may not be present in libmcrypt packages of other distros. There is nothing we (PHP) can do about this.

That your code using Rijndael-128 no longer works is unrelated to this problem (which only applies to Cast-128 with <= 80 bit keys, not to other ciphers like Rijndael). Presumably your code uses incorrect key sizes. If that is the case, you should either modify it to explicitly emulate the old \0 byte padding behavior or - which I would strongly recommend - fix your encryption code to use correctly sized keys.
 [2014-10-15 20:31 UTC] gm dot outside+php at gmail dot com
I've just confirmed that libmcrypt 2.5.8 (compile from sources) has an issue with key sizes <= 80-bits.  I was getting exactly the same encrypted as PHP was reporting in the test.  I'll look into Ubuntu patches, thanks for the pointers!
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Fri Apr 19 21:01:30 2024 UTC