Symmetric decryption with javax.crypto

I'm trying to determine if this code could be a memory hog with huge files. Can anyone tell me if there is anything I can do to make this more memory efficient, or is this how java.crypto works?

```
private byte[] symmetricDecryptAttachment(byte[] encryptedAttachmentBytes, byte[] keyBytes) throws IOException, GeneralSecurityException, EIPException {
byte[] decryptedValue = null;

// TODO: change to use non-deprecated method ConfigurationManager:getParsedStringValue() instead of getStringValue() which is deprecated
String decryptionAlgorithm = manager.getStringValue(SymmetricDecryptionAlgorithm);
String cipherBlockMode = manager.getStringValue(SymmetricDecryptionBlockCipherMode);
String padding = manager.getStringValue(SymmetricDecryptionPadding);

// Key bytes are only the first 16 bytes
final int KEY_BYTES_STARTING_BYTE = 0;
final int KEY_BYTES_ENDING_BYTE = 16;

// IV bytes are only the first 16 bytes
final int IV_BYTES_STARTING_BYTE = 0;
final int IV_BYTES_ENDING_BYTE = 16;

// Cipher bytes are everything after first 16 bytes (iv bytes)
final int CIPHER_BYTES_STARTING_BYTE = IV_BYTES_ENDING_BYTE;

try {
// 1. Get the cipher ready to start doing the AES transformation
Cipher cipher = Cipher.getInstance(decryptionAlgorithm + "/" + cipherBlockMode + "/" + padding);

// 2. Only take the first 16 bytes so that it is 128 bits as we are doing AES 128 bit decryption
// *Note: if this is not 128 bit decryption then you might have to change this ...
byte[] trimmedKeyBytes = Arrays.copyOfRange(keyBytes, KEY_BYTES_STARTING_BYTE, KEY_BYTES_ENDING_BYTE);

// 3. Start the decryption process by creating a key spec object
SecretKeySpec keySpec = new SecretKeySpec(trimmedKeyBytes, decryptionAlgorithm);

// 4. The initialization vector (IV) is the first 16 bytes/octets of the cipher value in the body.
// 4.1 To recover the IV, Base64 decode the Ciphe

String decryptionAlgorithm = manager.getStringValue(SymmetricDecryptionAlgorithm);

What manager is this, and why does the method have to perform its own configuration?

// Key bytes are only the first 16 bytes
final int KEY_BYTES_STARTING_BYTE = 0;
final int KEY_BYTES_ENDING_BYTE = 16;

final int IV_BYTES_STARTING_BYTE = 0;
final int IV_BYTES_ENDING_BYTE = 16;

Entirely not needed, and if they were they should be private static class variables, the Java equivalent of constants. What about using Cipher#getBlockSize()?

// 1. Get the cipher ready to start doing the AES transformation
Cipher cipher = Cipher.getInstance(decryptionAlgorithm + "/" + cipherBlockMode + "/" + padding);

The comment is dangerous, as it mentions AES but you configure the decryptionAlgorithm. I'm not sure why you would need 3 separate configuration fields for this.

byte[] trimmedKeyBytes = Arrays.copyOfRange(keyBytes, KEY_BYTES_STARTING_BYTE, KEY_BYTES_ENDING_BYTE);

So if the user supplies a 256 bit key, then just use the 128 bits and do not throw error? That's not good practice at all.

byte[] encryptedBytes = null;
byte[] ivBytes = null;

You are already assigning the references in the if statement, no need to initialize them to null. You can even make them final so that you know that each branch will initialize the variables!

//      -do +2 from start because we want to remove \r\n at the beginning and -2 to remove \r\n from the end
// Remove the new line and carriage returns from beginning and end, if they are there

You have got binary data, which has been base 64 decoded, and your originally binary ciphertext could maybe include a carriage return + line feed? Well, a random IV or ciphertext will probably contain that with a chance of 1/65536, which means that your decryption will fail once in about 4 billion times. You'll just have to wait long enough for it to happen.

encryptedBytes = Arrays.copyOfRange(encryptedAttachmentBytes, CIPHER_BYTES_STARTING_BYTE, encryptedAttachmentBytes.length);

First of all, I have no clue why you would copy the encrypted data and then the IV bytes, as that makes no sense at all - you never receive or need them in that order.

But note that both the IvParameterSpec() constructor and Cipher#doFinal() method have forms that already let you define an offset and amount of bytes to use. For the IV that doesn't matter much, for the ciphertext it adds insult to injury.

private byte[] symmetricDecryptAttachment(byte[] encryptedAttachmentBytes, byte[] keyBytes) throws IOException, GeneralSecurityException, EIPException {
    try {
        ...
    } catch (Exception e) {
        System.err.println("Exception occurred in symmetricDecryptAttachment() of SOAPDecryptionProcessor: \n" + e.getMessage());
        throw new EIPException(e.getMessage());
    }

Pokemon exception handling, using System.out, removing the exception from the stack trace, not specifying which exceptions can be thrown, no distinguishing between different types of exception (NoSuchAlgorithmException is different from a bad ciphertext, right?).

That kind of exception handling has to go.

Be warned that CBC is only useful for confidentiality, and then preferably only over authenticated messages or for in place encryption / decryption. I'm not sure if you're using that, but using GCM is probably a better option (within an IV of 12 bytes).

As for your question: yes, use streaming. Incremental updates are also possible, but given that your ciphertext is likely arriving per stream, using CipherInputStream makes most sense. Note that the Base64 encoder class can also be wrapped over a stream.

StackExchange Code Review Q#152450, answer score: 3