Thursday, July 21, 2011

BerrySync: Untested code is broken code

TLDR - What did I learn?

In the irc channels I lurk, a phrase reigns true. "Untested code is broken code". I overlooked this bug because my tests didn't keep up with the code. Don't be lazy, you will not remember you took short cuts a month ago.



BerrySync's Show Stopper
 
So I'm very quick to point fingers, So naturally when our program stops decrypting 75% of the I went straight to http://bit.ly/r9vKFS. You should note RIM is abiding by this document http://bit.ly/ey4Fg and if you read further down you'll note that this algorithm padding standard does not mention AES encryption which is tragic because its the only symmetric key unformatter engine that RIM supplies, meaning you'll need it for AES encryption! For example BerrySync's crypto module sans error control:

AESKey key = new AESKey(keyData);
InitializationVector iv = new InitializationVector(ivData);
ByteArrayOutputStream outputStream = new ByteArrayOutputStream();
BlockEncryptor encryptor = new BlockEncryptor(
     new PKCS5FormatterEngine(
          new AESCBCEncryptorEngine(key, iv)), outputStream);
encryptor.write(plainText);
encryptor.close();
outputStream.close();
return outputStream.toByteArray(); 

I And you should check out http://bit.ly/rplM0s. Here you'll see that Mozilla is abiding to http://bit.ly/piPqcJ. The 2.1 standard includes AES encryption in the spec, hold on a minute. RIM must be doing something totally off the wall here. All of that combined with the knowledge that using third party and unlicensed encryption and getting your app into AppWorld would involve many headaches and much money ala ECCN.



Wrong

As with most things my finger pointing was wrong, the bug actually lived on this line
data = data.replace('-', '\0'); 
Where my lazy attempt at removing characters from a string totally backfired. The interesting thing is that during rare cases, my sync account and about 25% of others tested it didn't actually make a difference and everything worked as intended. Where the other 75% of accounts saw a Bad Padding Exception during decryption.

1 comment:

  1. "Don't be lazy, you will not remember you took short cuts a month ago."

    Haha so true man.

    ReplyDelete