Change encryption to CTR mode and use better signature

Okay, maybe I do see why it is valuable, you're using the 'ecb' block chaining mode on the encryption, you should probably be using 'cbc' at least.

This one is a little complex, but here's why the hash is currently needed:

Cookie encryption is done using AES in ECB mode, this means that each block of stuff encrypted is done so in isolation, so someone could take a cookie and swap an encrypted block from another cookie into it. It would be a hard attack to pull off, and I could only think that you would somehow swap out the timestamp of the cookie being valid, but the potential is there.

This 'attack' is currently stopped by adding the hash, so that even if some of the critical data is swapped out, then data in the hash won't be.

What should happen is the hash stays (does no harm really) and the encryption mode is changed to CBC, this would require adding an IV setting to the UI.

Does that make sense?

Attached is a patch that does the following:

Implement encryption/decryption in the style of:
http://www.itnewb.com/v/PHP-Encryption-Decryption-Using-the-MCrypt-Libra...

Changes the cypher mode to CTR.
Uses the users entered key to generate a stronger key.
Moves the signature for tamper prevention into the encryption/decryption function itself, which then validates everything in the payload.

This needs eyes on before anyone even thinks about committing it.

+++ bakery.module	21 Apr 2010 14:48:17 -0000
@@ -666,26 +672,89 @@ function _bakery_eat_cookie($type = 'CHO
+    // This is decryption:
+    // Get the IV:
+    $iv = substr($text, 0, mcrypt_enc_get_iv_size($td));
+    // Get the MAC:
+		$mac_location = strlen($text) - 32;
+		$message_mac = substr($text, $mo);
+		$encrypted_data = substr($text, mcrypt_enc_get_iv_size($td), strlen($text) - (32 + mcrypt_enc_get_iv_size($td)));
+    // Compute the MAC:
+		$mac = bakery_pbkdf2($iv . $encrypted_data, $computed_key, 1000, 32);
+
+    // Make sure that MAC in the message matches the computed $mac
+    if ($mac != $message_mac) {
+      return FALSE;
+    }

This section is just all wrong, tabs and a non-existent $mo variable.

+++ bakery.module	21 Apr 2010 14:48:17 -0000
@@ -666,26 +672,89 @@ function _bakery_eat_cookie($type = 'CHO
+
+/** PBKDF2 Implementation (as described in RFC 2898);
+ *
+ * Slightly modified version from:
+ * http://www.itnewb.com/v/PHP-Encryption-Decryption-Using-the-MCrypt-Library-libmcrypt
+ *
+ *  @param string p password
+ *  @param string s salt
+ *  @param int c iteration count (use 1000 or higher)
+ *  @param int kl derived key length
+ *  @param string a hash algorithm
+ *
+ *  @return string derived key
+*/

Doxygen doesn't meet coding standards.

Powered by Dreditor.

Attached patch addresses the issues above, and adds the following:

• Wrapper functions for encrypting and decrypting to make things clearer.
• Use of base64 encoding by default.
• A better salt. This will need to be configurable in a follow up patch.
• More documentation.

Should I be patching against 7.x or 6.x?

Yup it'll need testing in isolation to make sure the encryption is doing what it should, and in a master/slave environment.

I've added an 'unmix' function because I think it makes it clearer that bakery_mix and bakery_unmix are opposites of each other.

The patch from #6 actually still applies! But I suspect that there are now lots more places that need to be changed a little. I will work on it...

Here's a patch against 7.x-1.x (HEAD). Completely untested.

Will look at this when I get a chance.