AES encryption class

Question

I am new to encryption and recently wanted to learn more about it. I researched how to do AES Encryption in java and assembled my own Java AES encryption class. I don't know if there are any major security holes, though.

Can someone experienced in Java encryption look at my code and tell me if there are any serious security vulnerabilities that I should know about?

import java.io.ByteArrayOutputStream;
import java.security.SecureRandom;
import java.util.Base64;

import javax.crypto.Cipher;
import javax.crypto.SecretKey;
import javax.crypto.SecretKeyFactory;
import javax.crypto.spec.IvParameterSpec;
import javax.crypto.spec.PBEKeySpec;
import javax.crypto.spec.SecretKeySpec;


public class AESEncryption {

    private SecureRandom random = new SecureRandom();
    private Base64.Encoder encoder = Base64.getEncoder();
    private Base64.Decoder decoder = Base64.getDecoder();
    private Cipher cipher = null;
    private ByteArrayOutputStream outputStream;

    /**
     * Initializes AES Encryption and sets Cipher transformation
     */
    public AESEncryption(){
        try{
            cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
        }catch(Exception e){
            e.printStackTrace();
        }
    }

    /**
     * Generates a random salt used to generate a key in encrypt()
     * 
     * @return The random salt
     */
    public String generateSalt(){
        byte bytes[] = new byte[16];
        random.nextBytes(bytes);
        return new String(bytes);
    }

    private byte[] generateIv(){
        byte bytes[] = new byte[16];
        random.nextBytes(bytes);
        return bytes;
    }

    /**
     * Encrypt the user supplied word
     * 
     * @param word The user supplied word
     * @param salt The salt generated by generateSalt()
     * @return The encrypted word with IV appended encoded
     */
    public String encrypt(String word, String salt) throws Exception{

        SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA1");
        PBEKeySpec spec = new PBEKeySpec("12345asdqwe".toCharArray(), salt.getBytes("UTF-8"), 65536, 128);
        SecretKey secretKey = factory.generateSecret(spec);
        SecretKeySpec secret = new SecretKeySpec(secretKey.getEncoded(), "AES");

        byte[] ivBytes = this.generateIv();

        cipher.init(Cipher.ENCRYPT_MODE, secret, new IvParameterSpec(ivBytes));

        byte[] encrypted = cipher.doFinal(word.getBytes("UTF-8"));
        outputStream = new ByteArrayOutputStream();
        outputStream.write(ivBytes);
        outputStream.write(encrypted);
        return encoder.encodeToString(outputStream.toByteArray());
    }

    /**
     * Decrypts the encrypted word with the salt and appended IV used to encrypt the word
     * 
     * @param encryptedWord The encrypted word returned from encrypt()
     * @param salt The salt used to encrypt the encrypted word
     * @return The decrypted word
     */
    public String decrypt(String encryptedWord, String salt) throws Exception{

        byte[] decodedBytes = decoder.decode(encryptedWord);

        SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA1");
        PBEKeySpec spec = new PBEKeySpec("12345asdqwe".toCharArray(), salt.getBytes("UTF-8"), 65536, 128);
        SecretKey secretKey = factory.generateSecret(spec);
        SecretKeySpec secret = new SecretKeySpec(secretKey.getEncoded(), "AES");

        cipher.init(Cipher.DECRYPT_MODE, secret, new IvParameterSpec(decodedBytes, 0, 16));

        return new String(cipher.doFinal(decodedBytes, 16, decodedBytes.length - 16));
    }

}

Answer 1

public class AESEncryption {

Make the class final unless you design for it to have child classes.

    private Cipher cipher = null;
    private ByteArrayOutputStream outputStream;

Cipher is a stateful object. It should not be used as a field. Imagine sharing an object instance between multiple threads or methods. Try and never assign null (if only because Java will do it itself).

outputStream is only necessary locally. Again, a stateful object, while there is otherwise no message specific state.

            e.printStackTrace();

Never ever do this. Use a // TODO tagging comment and then throw IllegalStateException(e); even configure your IDE to to this by default. Don't keep running with illegal state (and behold, if you do this, you can even make cipher final - which would have been useful if you'd needed it anyway).

        return new String(bytes);

Don't even convert random bytes to a string. Furthermore, if you decode bytes to a string, explicitly identify the encoding. You may lose data if you find invalid bytes (.e.g. anything above 0x7F for UTF-8).

        PBEKeySpec spec = new PBEKeySpec("12345asdqwe".toCharArray(), salt.getBytes("UTF-8"), 65536, 128);

Don't use static password values stuck in source code. Why do I even have to explain this? Note by the way that the reason that the password is a character array is to make it easier to destroy the password when it is not required any more. Beware that Java only uses the lowest 8 bits of each character - better check that you don't accept passwords that contain other values!

The 65537 value is a magic value, and should be constant. But generally you want to use a much higher value nowadays - unless your password is really complex. Making such a value configurable is very important. This however also means that you will have to indicate the value for the ciphertext somehow.

---

All in all, you are using stringified input / output and you simply throw Exception - which is simply telling your user to be screwed upon any error. Please look at my answer on how to use security exceptions on StackOverflow.

You don't specify any protocol that you are using. The user will have to guess the encodings of what you return. There is no warning of the mode, the inclusion of the IV or that the salt needs to be saved, the iteration count, the character encoding or the base-64 encoding.

You use CBC, which is not an authenticated mode such as GCM.

The salt is random, which means that the key is random. Since you don't include the salt, nobody will ever be able to decrypt. If the salt is saved otherwise, then it will probably get mangled because you forgot to base 64 encode it.

The key derivation is simply copied for the encrypt and decrypt methods. Why wasn't this put into a separate method?

And "mum's the word" apparently - this code seems to encrypt any strings, not just words?

Let's finish with a positive note: you do always perform UTF-8 encoding / decoding on the "word". Base 64 is not wrong either if a text interface is assumed. That's nice!