Doc #62453 [Asn]: Terrible sample code for mcrypt_encrypt

Edit report at https://bugs.php.net/bug.php?id=62453&edit=1 ID: 62453 User updated by: maarten dot bodewes at gmail dot com Reported by: maarten dot bodewes at gmail dot com Summary: Terrible sample code for mcrypt_encrypt Status: Assigned Type: Documentation Problem Package: Documentation problem PHP Version: 5.4.4 Assigned To: ircmaxell Block user comment: N Private report: N New Comment: The requirements to add integrity protection and correct padding are very much dependent on the use case. If you store encrypted strings in a database (which is a pretty common use case) then neither are required. That said, adding a integrity validation or PKCS#7 padding makes a lot of sense and will seldom hurt; it's probably better regarding security than using 256 bit keys over 128 bit ones. The following observations can be made regarding PKCS#7: - PKCS#7 padding, if programmed correctly, will allow for any length messages with any content (including bytes valued 00h at the end, which is where zero byte padding fails). - PKCS#7 will *always* pad the message, so it may grow it with 16 bytes if the plain text is a multiple of 16. So approx. 1 out of 16 times it will take 16 bytes more than zero padding now deployed. - padding/unpadding should be almost instantaneous and can be performed after decryption - PKCS#7 should *not* be implemented without taking the block size in mind, and should check all the padding bytes, exiting with an error if both restraints are not met The following observations can be made regarding integrity checks: - currently the only integrity check that seems to be present in PHP is HMAC in http://php.net/manual/en/function.hash-hmac.php so it would currently make sense to point in that direction (if you need integrity checks, see HMAC). HMAC is still a viable solution. - AES CMAC and implementing GCM or possibly EAX modes of encryption would make a lot of sense; GCM will be used more and more, e.g. in upcoming TLS and XML encryption standards. These will add integrity checking and authentication within the encryption protocol itself (and won't require padding). Note that stream ciphers do have more stringent requirements on the IV though (well, actually the Nonce, but they play a similar role). - MAC / HMAC require the use of a second key (!), the key used for encryption should not be used. - MAC / HMAC require a full pass (using either the cipher or the hash algorithm) over the cipher text. This will approx. double the CPU time required for the encryption (for larger messages). - The MAC/HMAC should be over the cipher text, not the plain text (even though this has its drawbacks too, more reason to switch to GCM mode) if only to stop padding oracle attacks. - Padding oracle attacks (and other attacks in this regard), if applicable, can only be stopped by using a stream cipher (no padding) or indeed integrity protection of the cipher text. The PKCS#7 padding algorithms does not seem to be present in PHP - currently it seems that users are required to copy or write their own code (incorrectly of course, see notes above). Note that most other API's integrate padding within their encrypt/decrypt implementations. PKCS#7 is pretty much standard for CBC/ECB. I think this is a bit much to put into the sample code :) In the end, you cannot apply cryptographic algorithms without knowing at least the cornerstones of cryptography. My advice would be to point to the PHP HMAC functionality with a warning to not use the same key used for encryption. As for PKCS#7, I would add (the relatively simple) implementation to PHP and point to that when present - keep the warning in for now. Regards, Maarten PS although I have 10 years experience in the practical *application* of cryptography I am not a professor in cryptography, feel free to have this advice looked at by more knowledgeable persons in the community (http://crypto.stackexchange.com seems to host a surprising amount of them, or try an actual professor in cryptography). Previous Comments: ------------------------------------------------------------------------ [2012-07-01 03:12:23] ircmaxell@php.net Maarten, I like the patch in general, and completely agree with it. The only thing I'm a bit concerned about is the warning: > # === WARNING === > > # Resulting cipher text has no integrity or authenticity added > # and is not protected against padding oracle attacks. While I completely understand *why* the warning is there, perhaps is there a way that it can be demonstrated how to protect those issues (integrate HMAC, etc). Remember that the documentation is not targeted towards security experts, and many who will use it have no idea of the difference between integrity and authenticity. Would it be worth while to briefly demonstrate a method to do so? Or do you think that's too deep for this example? Additionally, there are several mentions of PKCS#7 padding, yet I don't see an example of that in the patch provided. Is that something worth adding in? Anthony ------------------------------------------------------------------------ [2012-06-30 23:55:18] nikic@php.net Assigning to ircmaxell in hope that he can review this and apply the changes :) ------------------------------------------------------------------------ [2012-06-30 14:08:11] maarten dot bodewes at gmail dot com Created patch on the original sample code (including PHP header/footer) using the following requirements. Tested against Java code (send to Nikic). Requirements: * use MCRYPT_RIJNDAEL_128 (AES) instead of MCRYPT_RIJNDAEL_256 * use CBC mode encryption * use a 128 bit (16 byte), 192 (24 bit) or 256 bit (32 byte) key specified in hexadecimals consisting of random bytes * use a random IV (already present), and prepend that to the cipher text (so it can be used during decryption) * perform an explicit encoding (UTF-8 would come to mind) on the plain text, or specify that UTF-8 encoding is being used for compatability reasons * base64 encode the resulting cipher text, as most of web programmers use strings Furthermore the following warnings should be present: * PKCS#7 padding should be used on binary plain "text" * this CBC encryption mode only provided confidentiality, not integrity or authentication * padding oracle attacks may reveal the plain text in client/server setups ------------------------------------------------------------------------ [2012-06-30 11:14:01] nikic@php.net Could you maybe provide a better example for the function? I don't know anything about the topic at hand, so I probably can't come up with a good example on my own. ------------------------------------------------------------------------ [2012-06-29 23:49:49] maarten dot bodewes at gmail dot com Description: ------------ --- From manual page: http://www.php.net/function.mcrypt-encrypt#refsect1-function.mcrypt-encrypt-examples --- Hi, I'm a security professional (+10 years experience). I'm wondering which arse wrote that PHP sample this bug is pointing at. The following mistakes are present (at the minimum): * using an IV with ECB encoding * using ECB at all for non random plain text * using ECB within an example in the first place * mistaking a passphrase with a key (keys should be random bytes of data, or at least generated using e.g. PBKDF2, bcrypt or scrypt) * supplying an incorrect number of characters for the key (25 if I'm not mistaken) * using MCRYPT_RIJNDAEL_256 instead of MCRYPT_RIJNDAEL_128 (AES) * not performing PKCS#7 padding by default If this sample is to coax unsuspecting people in writing insecure code which is not compatible with any crypto library out there, keep it in. Otherwise toss it out and start over again. ------------------------------------------------------------------------ -- Edit this bug report at https://bugs.php.net/bug.php?id=62453&edit=1