1

I'm in the process of porting our Java application to OS X (10.8). One of our unit tests fails when doing encryption (it works on Windows). Both are running Java 7 Update 21 but the Windows version is using the 32 bit JDK and the Mac version is using the 64 bit JDK.

When running it on Mac I get the following exception when trying to decrypt the encrypted data:

Caused by: javax.crypto.BadPaddingException: Given final block not properly padded at com.sun.crypto.provider.CipherCore.doFinal(CipherCore.java:811) at com.sun.crypto.provider.CipherCore.doFinal(CipherCore.java:676) at com.sun.crypto.provider.AESCipher.engineDoFinal(AESCipher.java:313) at javax.crypto.Cipher.doFinal(Cipher.java:2087) at com.degoo.backend.security.Crypto.processCipher(Crypto.java:56) ... 25 more

Here's the encryption class.

import javax.crypto.Cipher;
import javax.crypto.KeyGenerator;
import javax.crypto.SecretKey;
import javax.crypto.spec.IvParameterSpec;
import javax.crypto.spec.SecretKeySpec;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;

public final class Crypto {

    private final static String CIPHER_ALGORITHM = "AES";
    private final static String CIPHER_TRANSFORMATION = "AES/CBC/PKCS5Padding";

    public final static int CRYPTO_KEY_SIZE = 16;    

    public static byte[] encryptByteArray(byte[] blockToEncrypt, int maxLengthToEncrypt, byte[] encryptionKey, byte[] ivBytes) {
        return processCipher(blockToEncrypt, maxLengthToEncrypt, Cipher.ENCRYPT_MODE, ivBytes, encryptionKey);
    }

    public static byte[] decryptByteArray(byte[] encryptedData, byte[] encryptionKey, byte[] ivBytes) {
        return processCipher(encryptedData, encryptedData.length, Cipher.DECRYPT_MODE, ivBytes, encryptionKey);
    }

    private static byte[] processCipher(byte[] blockToEncrypt, int maxLength, int cryptionMode, byte[] ivBytes, byte[] encryptionKey) {
        try {
            IvParameterSpec iv = new IvParameterSpec(ivBytes);
            final Cipher cipher = initCipher(cryptionMode, iv, encryptionKey);
            return cipher.doFinal(blockToEncrypt, 0, maxLength);
        } catch (Exception e) {
            throw new RuntimeException("Failure", e);
        }
    }

    private static Cipher initCipher(int cryptionMode, IvParameterSpec iv, byte[] encryptionKey) {
        KeyGenerator keyGen;
        try {
            keyGen = KeyGenerator.getInstance(CIPHER_ALGORITHM);

            final SecureRandom randomSeed = new SecureRandom();
            randomSeed.setSeed(encryptionKey);
            keyGen.init(CRYPTO_KEY_SIZE * 8, randomSeed);

            // Generate the secret key specs.
            final SecretKey secretKey = keyGen.generateKey();

            final SecretKeySpec secretKeySpec = new SecretKeySpec(secretKey.getEncoded(), CIPHER_ALGORITHM);

            // Instantiate the cipher
            final Cipher cipher = Cipher.getInstance(CIPHER_TRANSFORMATION);

            cipher.init(cryptionMode, secretKeySpec, iv);
            return cipher;

        } catch (Exception e) {
            throw new RuntimeException("Failure", e);
        }
    }
}

The test code looks like this:

public void testEncryption() throws Exception {
        int dataLength = TestUtil.nextInt(applicationParameters.getDataBlockMinSize());
        byte[] dataToEncrypt = new byte[dataLength];
        TestUtil.nextBytes(dataToEncrypt);

        int keyLength = 16;
        byte[] key = new byte[keyLength];
        TestUtil.nextBytes(key);

        byte[] ivBytes = new byte[16];
        TestUtil.nextBytes(key);

        long startTime = System.nanoTime();
        byte[] encryptedBlock = Crypto.encryptByteArray(dataToEncrypt, dataToEncrypt.length, key, ivBytes);
        long endTime = System.nanoTime();
        System.out.println("Encryption-speed: " + getMBPerSecond(dataLength, startTime, endTime));

        startTime = System.nanoTime();
        byte[] decryptedData = Crypto.decryptByteArray(encryptedBlock, key, ivBytes);
        endTime = System.nanoTime();
        System.out.println("Decryption-speed: " + getMBPerSecond(dataLength, startTime, endTime));

        if (encryptedBlock.length == decryptedData.length) {
            boolean isEqual = true;
            //Test that the encrypted data is not equal to the decrypted data.
            for (int i = 0; i < encryptedBlock.length; i++) {
                if (encryptedBlock[i] != decryptedData[i]) {
                    isEqual = false;
                    break;
                }
            }
            if (isEqual) {
                throw new RuntimeException("Encrypted data is equal to decrypted data!");
            }
        }

        Assert.assertArrayEquals(dataToEncrypt, decryptedData);
    }
Yrlec
  • 3,401
  • 6
  • 39
  • 75

1 Answers1

2

I think I've found it. For some reason the code above derives an encryption-key by seeding a SecureRandom instance with the existing encryption key to get a new byte[] (don't ask me why, it was a long time ago it was written). This is then fed to the SecretKeySpec constructor. If I skip all this and just feed the SecretKeySpec constructor the encryption key that we already have then the unit test passes. The code that does the encryption now looks like this:

final SecretKeySpec secretKeySpec = new SecretKeySpec(encryptionKey, CIPHER_ALGORITHM);

// Instantiate the cipher
final Cipher cipher = Cipher.getInstance(CIPHER_TRANSFORMATION);

cipher.init(cryptionMode, secretKeySpec, iv);
return cipher;

The odd thing is that it has worked on Windows. Looks like the SecureRandom implementations behave differently on OS X and on Windows. Calling setSeed on OS X appends to the seed whereas Windows replaces it.

Update: found some more details on the implementation differences of SecureRandom: http://www.cigital.com/justice-league-blog/2009/08/14/proper-use-of-javas-securerandom/

Yrlec
  • 3,401
  • 6
  • 39
  • 75
  • 1
    Yes, that code is correct. It is possible that the author tried (in vain) to create a Key Based Key Derivation Function. This would be useful if the `encryptionKey` array has enough entropy but is not fully random, or if the `encryptionKey` is not of the correct size. In that case you should really use a real KDF such as HKDF (implemented, e.g. in Bouncy Castle, latest version) or one of the NIST KDF's (implemented e.g. in Bouncy Castle, but only in the latest source code). – Maarten Bodewes Sep 11 '13 at 21:57
  • Oh, and thanks for reporting back. That answer was the one I would have given you otherwise. Note that there is a notorious example on [AndroidSnippets](http://www.androidsnippets.com/encryptdecrypt-strings) that uses this "key derivation function" and lots of code is based on it. I cannot get it removed, and I cannot login on that site (Google openID fails because of implementation error on their server, typical). Read the comments on the bottom of the page and [this blog](http://android-developers.blogspot.co.uk/2013/02/using-cryptography-to-store-credentials.html) and weep. – Maarten Bodewes Sep 11 '13 at 22:04
  • Thanks for the clarification! Yeah, encryptionKey already has enough entropy and is of correct size so that code is definitely not needed. I find it fascinating how much really bad examples of encryption code there are on the Internet. I think I've seen more example code using ECB than CBC. :) – Yrlec Sep 12 '13 at 07:37
  • It's also interesting that documentation explicitly states "The given seed supplements, rather than replaces, the existing seed. Thus, repeated calls are guaranteed never to reduce randomness." http://docs.oracle.com/javase/7/docs/api/java/security/SecureRandom.html#setSeed(byte[]). Odd that the Windows implementation of such a fundamental and important feature doesn't match the documentation. – Yrlec Sep 12 '13 at 07:40
  • It's not incorrect, in the sense that it is the one and only seed. The question however then becomes if the RNG has been automatically seeded. You can trigger this by getting random data. Using a PRNG this way can be useful, but in my opinion they should have simply used a `"SHA1PRNG/UNSEEDED"` option instead, instead of some magic, provider specific trick. – Maarten Bodewes Sep 12 '13 at 20:33
  • I agree. I suspect that this has caused lots of security issues for Java apps on Windows. Just look at what happened with BitCoin when Android recreated the same random numbers with SecureRandom. – Yrlec Sep 13 '13 at 07:36