trjhtr

I've looked at multiple examples of Java AES CBC mode encryption but I couldn't find a good answer that was safe to use. I ended up writing my own util code. 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!

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.

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.

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".