6
\$\begingroup\$

I've looked at multiple examples of Java AES CBC mode encryption but I couldn't find a proper solution that's safe to use. I ended up writing my own utility class. My questions is: Is this code safe to use? I've read some stuff about padding oracle attacks but I don't really understand how they work.

I've tested the code and it seems to work properly:

CryptoUtils.removeCryptoRestriction();

String string = "Hello world! This is a test string. Have a nice day!";
byte[] iv = CryptoUtils.generateIv();
byte[] key = CryptoUtils.generateKey();

byte[] encrypted = CryptoUtils.doCrypto(Cipher.ENCRYPT_MODE, key, iv, string.getBytes("UTF-8"));
System.out.println(new String(encrypted));

byte[] decrypted = CryptoUtils.doCrypto(Cipher.DECRYPT_MODE, key, iv, encrypted);
System.out.println(new String(decrypted));

Output:

��zI Fğ��O>��_�@3�ܷ+|ZI�m���M�4�;���ygr8G�ܪ8�6u���M
Hello world! This is a test string. Have a nice day!

Here's the util code:

public static byte[] doCrypto(int mode, byte[] keyBytes, byte[] ivBytes, byte[] bytes) throws GeneralSecurityException {
    Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5PADDING");
    SecretKeySpec secretKeySpec = new SecretKeySpec(keyBytes, 0, keyBytes.length, "AES");
    IvParameterSpec ivParameterSpec = new IvParameterSpec(ivBytes);
    cipher.init(mode, secretKeySpec, ivParameterSpec);
    return cipher.doFinal(bytes);
}

public static void doCrypto(int mode, byte[] keyBytes, byte[] ivBytes, File in, File out) throws GeneralSecurityException, IOException {
    Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5PADDING");
    SecretKeySpec secretKeySpec = new SecretKeySpec(keyBytes, 0, keyBytes.length, "AES");
    IvParameterSpec ivParameterSpec = new IvParameterSpec(ivBytes);
    cipher.init(mode, secretKeySpec, ivParameterSpec);

    try (FileInputStream fileInputStream = new FileInputStream(in); FileOutputStream fileOutputStream = new FileOutputStream(out)) {
        byte[] buffer = new byte[1024];

        for (int i = 0; i != -1; i = fileInputStream.read(buffer)) {
            byte[] updateBytes = cipher.update(buffer, 0, i);
            if (updateBytes != null) fileOutputStream.write(updateBytes);
        }
        byte[] finalBytes = cipher.doFinal();
        if (finalBytes != null) fileOutputStream.write(finalBytes);
    }
}

public static byte[] generateIv() {
    SecureRandom secureRandom = new SecureRandom();
    byte[] iv = new byte[16];
    secureRandom.nextBytes(iv);
    return iv;
}

public static byte[] generateKey() throws NoSuchAlgorithmException {
    KeyGenerator keyGenerator = KeyGenerator.getInstance("AES");
    keyGenerator.init(256);
    SecretKey secretKey = keyGenerator.generateKey();
    return secretKey.getEncoded();
}

public static void removeCryptoRestriction() {
    Security.setProperty("crypto.policy", "unlimited");
}

The file encryption works too.

Thanks in advance!

\$\endgroup\$
2
\$\begingroup\$

There are several ways to prevent a Padding Oracle attack use one or more of the following:

  1. Use authenticated encryption such as GCM mode or encrypt-then-MAC.
  2. Do not report padding errors.
  3. Do not allow an attacker to use your code to decrypt.
  4. If the attacker can use your decryption seriously rate-limit access.
\$\endgroup\$
2
\$\begingroup\$

You should move to an authenticated mode of encryption like CCM, EAX, OCB or GCM. The fact that you are generating a random sequence of bytes from the OS's PRNG is sound. It looks like the argument provided to the crypto object is already padding the resulting cipher text of the file stream.

The top bit about removing the restrictions is suspect.

Padding attacks are a side channel attack in crypto that allows an attacker to perform cryptanalysis on a block of cipher text because the plain text input would determine the resulting cipher texts length. By padding the resulting cipher text it makes that side channel more difficult.

\$\endgroup\$
  • \$\begingroup\$ How do you think cryptanalysis works exactly? \$\endgroup\$ – jas- Feb 7 '18 at 7:56
  • \$\begingroup\$ Typo, was supposed to be CCM \$\endgroup\$ – jas- Feb 7 '18 at 16:58
  • \$\begingroup\$ In many cases the authentication modes are not available, in particular Apple unfortunately does not provide the mentioned modes for iOS. A Padding Oracle requires padding which allows the attacker to recover the text (less the first block) but not the key if padding errors are returned and sufficient access is allowed. Modes that do not require padding such as GCM can not suffer from such an attack. \$\endgroup\$ – zaph Feb 7 '18 at 17:50
1
\$\begingroup\$

Not directly related to the security of this (I'm not remotely qualified to talk about it anyways):

CryptoUtils.doCrypto(Cipher.ENCRYPT_MODE, ...);
CryptoUtils.doCrypto(Cipher.DECRYPT_MODE, ...);

^^ this is an example of a state machine in a method. You want to avoid these in high-level languages like java. They don't make for a nice interface in usage. Instead you want an encrypt and a decrypt. This pushes the "mode" parameter into the name of your method.

If it's easier to implement the code itself as a state-machine, that's just fine, as long as you do so internally.

public static void encrypt(byte[] keyBytes, byte[] ivBytes, byte[] cleartext) throws GeneralSecurityException {
    return doCrypto(Cipher.ENCRYPT_MODE, keyBytes, ivBytes, cleartext);
}

If you were to aim for a more general-purpose cryptography utility class, you could expose an overload that allows the user to specify which cipher they want to use.

If you decide to do so, you can pass the cipher.getAlgorithm() to the SecretKeySpec constructor.


I like how you used try-with-resources to handle the IO in the file-encryption. But there is a somewhat cleaner way than manually handling the bytes you have there.

Consider the following methods:

public static void encrypt(byte[] keyBytes, byte[] ivBytes, Path in, Path out) throws GeneralSecurityException, IOException {
    Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5PADDING");
    SecretKeySpec secretKeySpec = new SecretKeySpec(keyBytes, cipher.getAlgorithm());
    IvParameterSpec ivParameterSpec = new IvParameterSpec(ivBytes);
    cipher.init(Cipher.ENCRYPT_MODE, secretKeySpec, ivParameterSpec);

    try (OutputStream os = new CipherOutputStream(Files.newOutputStream(out), cipher)) {
        Files.copy(in, os);
    }
}

public static void decrypt(byte[] keyBytes, byte[] ivBytes, Path in, Path out) throws GeneralSecurityException, IOException {
    Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5PADDING");
    SecretKeySpec secretKeySpec = new SecretKeySpec(keyBytes, cipher.getAlgorithm());
    IvParameterSpec ivParameterSpec = new IvParameterSpec(ivBytes);
    cipher.init(Cipher.DECRYPT_MODE, secretKeySpec, ivParameterSpec);

    try (InputStream is = new CipherInputStream(Files.newInputStream(in), cipher)) {
        Files.copy(is, out);
    }
}

Here we can notice some duplication and extract the Cipher initialization routine into a separate method to simplify the code to:

public static void encrypt(byte[] keyBytes, byte[] ivBytes, Path in, Path out) throws GeneralSecurityException, IOException {
    Cipher cipher = initializeCipher(Cipher.ENCRYPTION_MODE, keyBytes, ivBytes);
    try (OutputStream os = new CipherOutputStream(Files.newOutputStream(out), cipher)) {
        Files.copy(in, os);
    }
}

Note that I explicitly changed from File to the newer (and more modern Path from the java.nio package)

Be aware that even here you can allow the user to specify the Cipher by allowing them to pass a String as cipherSpec or something along these lines.

On that note: you can use that initialization method even in the code that en/decrypts byte[] directly.


Lastly I wanted to be a bit annoyed that neither generateIv nor generateKey provide any means to specify the number of bytes used. While 256-bit AES is generally considered secure, some users might want to opt for the extra security of using a 512-bit key. Also I think a 16-bit IV is probably a bit too short.

Additionally I'd recommend using SecureRandom.getInstanceStrong() over new SecureRandom() because this gets you a definitively strong SecureRandom instead of "the first that was available".

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy