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.

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

Assigning to ircmaxell in hope that he can review this and apply the changes :)

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

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).

Maarten,

Sounds good to me. 

I would apply it for you, but I'd rather give credit where credit is due. Could 
you make the change on http://edit.php.net ? All you need to do is make a patch 
from that console (I believe you right click your work in progress tab, and 
there should be an option to submit a patch). Then just post back here with the 
username that you used and I'll commit it for you.

That way you get credit for the change instead of me.

If that's too much, let me know and I'll apply it for you. But I would rather 
you get the credit for it.

Thanks,

Anthony

I've tried to contact the dev. to apply this change for me. If you want I can apply it myself, but I quickly got lost in the interface where I should apply the change (and I don't have enough time to get fully acquainted with it). If anybody wants the new code applied, please do so, or guide me through the process.

Hello? Anybody? Are you just going to let this fester?

This bug has been fixed in the documentation's XML sources. Since the
online and downloadable versions of the documentation need some time
to get updated, we would like to ask you to be a bit patient.

Thank you for the report, and for helping us make our documentation better.

@maarten: I'd like to remove the utf8_encode() line from the sample code. For most people (who most likely are using UTF8, even if they are not aware of it) this will just result in double encoding. And if they are not using UTF8, then utf8_encode() may not be the most appropriate function to use (rather iconv or mb_convert_encoding with the encoding they are using.)

Are you okay with that?