Hi, Your Encryption Sucks

This is a result of programmers trying to reinvent the wheel, more often than not, when existing tools are more that sufficient to get the job done. But since these are efforts by those who either don’t understand what they’re doing or think they can do better, let me clarify one glaringly obvious fact that a lot of them overlook.

Anything you create will be worse

Not that someone isn’t clever to come across a better algorithm or a better technique. It’s all down to a simple matter of logistics and probability (another thing inherent to encryption). Your own concoctions will be seen by your eyes, your boss’ (maybe) and a few colleagues.

This is vastly below the scrutiny existing libraries receive. There are countless eyes scanning existing libraries whereas yours won’t and it won’t be a fair comparison. Which do you think is more likely to expose bugs?

So how do you do it properly?

There are countless forum threads out there complaining about decryption being corrupted. This is usually because block ciphers need their input strings in… ready? Specific size blocks. The solution is to pad the data to match the block size of the cipher before encryption and the remove the pad after encryption.

Here are two functions in PHP that do just that when given the string to pad and the block size.

function _pad( $str, $bsize ) {

	// Find the pad size
	$pad = $bsize - ( strlen( $str ) % $bsize );

	// Repeat the equivalent character up to the pad size
	$str .= str_repeat( chr( $pad ), $pad );

	return $str;
}

function _unpad( $str, $bsize ) {

	// Michael Corleone says hello
	$len = mb_strlen( $str );

	// Find the pad character ( last one )
	$pad = ord( $str[$len - 1] );

	// If padding would have been applied to the string...
	if ($pad && $pad < $bsize) {

		// ...find the pad
		$pm = preg_match( '/' . chr( $pad ) . '{' . $pad . '}$/', $str );

		// Pad found, strip it.
		if( $pm )
			return mb_substr($str, 0, $len - $pad);
	}

	return $str;
}

And now of course, you need an encryption function that uses it and gets back the expected result. I’ve seen many examples of multiple functions doing this, but I don’t see why one can’t accomplish both encryption and decryption. The critical aspect of IV based (initialization vector) encryption is packaging it along with the encrypted data and making sure both are in a format that’s not prone to corruption, yet is still easy to decrypt. Base64 will work nicely for this.

So here’s how you don’t reinvent the wheel…

Keep in mind that I may have been suffering from mild sleep-deprivation when I wrote this.

function _crypt( $str, $key, $encrypt = true ) {

	// Open Rijndael-256 in CBC mode (was CFB; code fixed 12/08/2012, comment changed 12/13/2012).
	// Change the encryption mode to 'ecb' if you enjoy Herpes
	$td = mcrypt_module_open('rijndael-256', '', 'cbc', '');

	// Find the IV size for the mcrypt module
	$size = mcrypt_enc_get_iv_size( $td );

	// Block sizes are needed to calculate the padding ( further down )
	$bsize = mcrypt_enc_get_block_size( $td );

	// Hash the key for consistency and use only the key size needed
	$key = substr( hash( 'ripemd160', $key ), 0, mcrypt_enc_get_key_size( $td ) );

	if( $encrypt ) {

		/*
			We're encrypting. Create a new initialization vector.

		 	If you change this to MCRYPT_RAND for any reason other than
			for testing or because your platform doesn't support it,
			I'll hunt you down and kick you in the groin.
		*/
		$iv = mcrypt_create_iv( $size, MCRYPT_DEV_URANDOM);

	} else {

		// Unwrap the combined IV and data packet
		$str = base64_decode( $str );

		// Extract the IV from the front (using IV size)
		$iv = mb_substr( $str, 0, $size );

		/*
			Isolate the data by removing the IV altogether.

			There's some controversy about using str_replace on multibyte
			characters, but as far as I can tell, there seem to be no ill effects.
			Besides, there's no equivalent "mb_replace" in PHP unless it's DIY.
		*/
		$str = str_replace( $iv, '', $str );

		// Decode in preparation for decryption
		$str = base64_decode( $str );
	}

	// Initialize mcrypt
	// I keep calling this "MyCrypt" in my head. Probably because of "MySQL"
	mcrypt_generic_init( $td, $key, $iv );

	if ( $encrypt ) {

		// Prepare string by padding to match block size
		$str = _pad( $str, $bsize );

		/*
			Encrypt the data.

			I'm supposed to say "this is where the magic happens" here, except I'll never say,
			"this is where the magic happens". That is a tired and old cliche with no useful
			purpose other than to engender ire and abject contempt to whoever says it. If
			"this is where the magic happens", then why are you trying desperately to point it
			out to everyone? Shouldn't you be hiding the location "where the magic happens"?
			The phrase "this is where the magic happens" was created to let everyone know that
			the speaker is constantly getting sex and this is their favorite
			[ bed / hooker / other object ] on which they receive it. No one realy believes it
			because people who say it are either never seeing this "magic" or are date-rapists.
			Now, despite the dubious origin of the phrase "this is where the magic happens",
			people have started using it at every inappropriate instance. E.G. While pointing
			to a chair in the office "this is where the magic happens". Unless the tool who says
			it is looking at porn at work, he's not performing "magic". In fact, that's not
			"magic", it's just disturbing. Not just because it's a public place, but people
			will be borrowing his office supplies without knowing he's touched them after he's
			touched something else... repeatedly.

			BTW, this is where the magic happens.
		*/
		$enc = mcrypt_generic( $td, $str );

		// Base64 the encrypted data, add the IV to the front and base64 again.
		$out = base64_encode( $iv . base64_encode( $enc ) );
		/*
			This ensures that the data remains portable because both the IV and the encrypted
			data contain special chracters ( unicode / multibyte ) that may be lost if handled
			as-is. This is especially true if the functions you're passing the data to can only
			handle ASCII data.
		*/

	} else {

		// Decrypt the data ( we removed the IV and decoded above )
		$str = mdecrypt_generic( $td, $str );

		// Remove the padding added to fit the block size
		$out = _unpad( $str, $bsize );
	}

	// Clean up
	mcrypt_generic_deinit( $td );
	mcrypt_module_close( $td );

	// Return encrypted/decrypted string
	return $out;
}

Very simple way to test this is to put it in a loop.

for( $i = 0; $i < 30; $i++ ) {

	$key = md5( $i ); // Simple key generation
	$data = "This is some test data";

	$enc = _crypt( $data, $key );
	$dec = _crypt( $enc, $key, false );

	echo "Test data :  $data <br />";
	echo "Key : $key; <br />";
	echo "Encrypted data : $enc <br />";
	echo "Decrypted data : $dec <br />";
	echo "Match : " . ( ( $data == $dec )? "True" : "False" ) . "<br /><br /><br />";
}

I’ve tested this a couple of times and it worked. I don’t have the time or the energy to keep testing, but trust me, this is how you do encryption properly. If there are bugs, scan through and see where I’ve missed a semicolon or something, but rest assured, this is “standard”.

Now… bedtime!

5 thoughts on “Hi, Your Encryption Sucks

  1. Pingback: ID Obfuscation Part II | This page intentionally left ugly

  2. It’s close, but despite the description of mcrypt_create_iv, your IV should also be encrypted; if mcrypt doesn’t do this for you internally, you should do it yourself.

    The reason is that the most likely attack against a strong ciphertext is either chosen ciphertext or chosen plaintext. If your IV is readily available, it’s effectively useless as it can just be factored into the chosen *text attack. However, such an attack is much less likely to succeed against an encrypted chunk of crypto-random IV.

    Also you mention CFB in the comments, but appear to instead be using the CBC mode.

    • Hi there, yes, I was using CFB, but switched it CBC (mentioned bottom of that post), but I forgot to change that in the comments. Er… yeah, sleep-deprivation ;) . I’ll fix it now.

      It was my understanding that IV was in essence being used a bit like a Public Key for decryption only in that it must be included with the message and there’s no harm in it being public. But unlike a Public/Private Key pair, it must never again be reused with any other message and must be sufficiently random, hence the new key generation per new message. Is this interpretation wrong? If it is, I’ll have to re-read the spec again.

      Thanks for dropping by!

  3. Part of the motivation behind CFB/CBC is trying to ensure that encrypting the same text with the same key is unlikely to produce the same cipher text. However, there’s an additional layer of security in the fact that, even if an attacker is able to determine the message that was directly encrypted (e.g. by identifying a cipher text), because the message at this stage is XOR’d against some other unknown value (imagine a less secure one-time pad), the previous block in the chain must also be broken to be able to reverse the XOR – and this goes all the way back to the first block. As your IV is the first link in the chain, this layer of security is invalidated if the previous block is readily available. As mentioned in the earlier post, statistically speaking, an IV generated using a CRNG is less likely to be matched using a brute force attack.

    Many sources will tell you that there’s no harm in making the IV public – and this isn’t entirely untrue – but there’s undeniably an extra layer of security if it’s kept encrypted. I imagine this is why the description of mcrypt_create_iv states that keeping the IV secret may be “desirable.”

    • So then there’s an element of security through obscurity by transmitting the IV even if random. But then, to encrypt the IV without generating another IV (endless spiral?), we’ll be stuck using something like ECB. I mean, I guess it’s a better alternative to just base64, which is essentially nothing. Still, is that acceptable?

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s