AES Java Multithreading exceptions

Question

I noticed that an application I was running was throwing exceptions upon simultaneous decryption. I wrote the following to test this:

public void run() {
    for(int i=0; i<100000; i++){
        String encrypted = Crypt.encrypt(
                "Lorem ipsum dolor sit amet.",
                "password"
        );

        String decrypted = Crypt.decrypt(encrypted, "password")[0];
        System.out.println(decrypted);
    }
}


public static void main(String[] args) {

    Thread t1 = new Thread(new Main());
    Thread t2 = new Thread(new Main());

    t1.start();
    t2.start();

}

The Crypt methods are as follows:

public static String encrypt(String input, String key){

    try {

        byte[] ivBytes = new byte[16];
        SecureRandom.getInstance("SHA1PRNG").nextBytes(ivBytes);

        IvParameterSpec ips = new IvParameterSpec(ivBytes);
        byte[] keybytes = md5(key);//This isn't final. Don't worry ;)
        byte[] crypted = null;
        SecretKeySpec skey = new SecretKeySpec(keybytes, "AES");
        Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
        cipher.init(Cipher.ENCRYPT_MODE, skey, ips);
        byte[] ptext = input.getBytes("UTF-8");
        crypted = cipher.doFinal(ptext);

        return Base64.encodeBase64String(ivBytes)+Base64.encodeBase64String(crypted);
    }catch(Exception e){
        e.printStackTrace();
    }
    return null;
}

public static String[] decrypt(String input, String key){

    String iv = input.substring(0, 24);
    String encrypted = input.substring(24);
    try {
        IvParameterSpec ips = new IvParameterSpec(Base64.decodeBase64(iv));
        byte[] keybytes = md5(key);//This isn't final. Don't worry ;)
        byte[] output = null;
        SecretKeySpec skey = new SecretKeySpec(keybytes, "AES");
        Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
        cipher.init(Cipher.DECRYPT_MODE, skey, ips);
        output = cipher.doFinal(Base64.decodeBase64(encrypted));
        if(output==null){
            throw new Exception();
        }

        return new String[]{new String(output),iv};
    }catch(Exception e){
        e.printStackTrace();
    }
}

Sure enough the first attempt on both threads fail:

javax.crypto.BadPaddingException: Given final block not properly padded
at com.sun.crypto.provider.SunJCE_f.b(DashoA13*..)
at com.sun.crypto.provider.SunJCE_f.b(DashoA13*..)
at com.sun.crypto.provider.AESCipher.engineDoFinal(DashoA13*..)
at javax.crypto.Cipher.doFinal(DashoA13*..)
at common.Crypt.decrypt(Crypt.java:122)
at bootstrap.Main.run(Main.java:427)
at java.lang.Thread.run(Thread.java:680)

There are successes for approximately 20 attempts on each thread (presumably where the unsafe calls don't intersect) and then exceptions are thrown. This pattern seems to continue.

I'm running this on OS X 10.7.2 if that helps.

If I understand correctly this could simply be a vendor issue as it's using the Sun JDK which I could easily swap out, but I thought it would be best to get some more experienced opinions on this and put it up in case someone having the same issue stumbles across it.

If someone could point me in the direction of a thread safe encryption+decryption scheme which achieves the same results I'd be really grateful.

Thanks, Marcus

You have a bug (unrelated though): `new String(output)` should be `new String(output, "UTF-8")`. I don't reproduce it on my Windows 7 box, JDK 1.6.0_26, with md5 doing nothing more than key.getBytes("UTF-8), and Base64 from commons-codec. Maybe the problem comes from your md5 or Base64 implementation, which you don't talk about. (I use "passwordpassword" as key) — JB Nizet, Jan 24 '12 at 17:23
The exception is thrown by the line javax.crypto.Cipher.doFinal in my Crypt class. It's definitely an issue on a lower level than my code. If I insert a valid encrypted string rather than creating one with a random IV using encrypt() (which might have been the issue) the problem still occurs intermittently which rules out the possibility of a rogue Base64 string or md5 issues. I tried running this on a Ubuntu machine I have lying around and the same issue occurs. — Marcus, Jan 24 '12 at 17:58
It turns out that my md5() method breaks randomly which was leading to the error. Thanks for pointing out that the problem could have been lying there all along. — Marcus, Jan 24 '12 at 18:34
Dupe of http://stackoverflow.com/questions/6957406/is-cipher-thread-safe ?? — Tails, Jan 25 '12 at 02:43
Not quite. The issue in that question is the use of a single instance between threads, I used one instance per thread. Worth noting for those that find this question though. Thanks. — Marcus, Jan 26 '12 at 15:02

score 3 · Accepted Answer · answered Feb 03 '12 at 23:53

The issue was caused by this line which was being used to generate a key from the password.

byte[] keybytes = md5(key);

The md5 function which used the MessageDigest class was prone to returning garbage which was then being fed into the decryption which proceeded to fail.

score 1 · Answer 2 · edited Jan 31 '12 at 10:18

public static String[] decrypt(String input, String key){

    String iv = input.substring(0, 24);
    String encrypted = input.substring(24);
    try {
        IvParameterSpec ips = new IvParameterSpec(Base64.decodeBase64(iv));
        byte[] keybytes = md5(key);//This isn't final. Don't worry ;)
        byte[] output = null;
        SecretKeySpec skey = new SecretKeySpec(keybytes, "AES");
        Cipher cipher = Cipher.getInstance("AES/CBC/NoPadding"); //Change here
        cipher.init(Cipher.DECRYPT_MODE, skey, ips);
        output = cipher.doFinal(Base64.decodeBase64(encrypted));
        if(output==null){
            throw new Exception();
        }

        return new String[]{new String(output),iv};
    }catch(Exception e){
        e.printStackTrace();
    }
}

How is changing the padding going to affect the stability? The issue ended up being a faulty MD5 function I was using anyway. — Marcus, Feb 03 '12 at 23:50

AES Java Multithreading exceptions

2 Answers2