Encrypting a string using AES/CBC/PKCS5PADDING

Question

The code below takes a String and encrypts it using AES/CBC/PKCS5PADDING as transformation. I am learning as I go and I have a few questions about my code.

1. Is SecureRandom ok for generating my KEY and my IV?
2. What's up with all these exceptions?
3. Is my code creating any vulnerabilities in the encryption process? (mistakes maybe?)
4. Am I seeding SecureRandom properly?

I'm hopping to incorporate this into a larger project or build on this. Any suggestions for making the code easier to work with multiple classes?

import java.io.UnsupportedEncodingException;
import java.security.InvalidAlgorithmParameterException;
import java.security.InvalidKeyException;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import javax.crypto.BadPaddingException;
import javax.crypto.Cipher;
import javax.crypto.IllegalBlockSizeException;
import javax.crypto.KeyGenerator;
import javax.crypto.NoSuchPaddingException;
import javax.crypto.SecretKey;
import javax.crypto.spec.IvParameterSpec;
public class AESCrypt {
 private SecureRandom r = new SecureRandom();
 private Cipher c;
 private IvParameterSpec IV;
 private SecretKey s_KEY;
 // Constructor
 public AESCrypt() throws NoSuchAlgorithmException, NoSuchPaddingException {
 this.c = Cipher.getInstance("AES/CBC/PKCS5PADDING");
 this.IV = generateIV();
 this.s_KEY = generateKEY();
 }
 // COnvert the String to bytes..Should I be using UTF-8? I dont think it
 // messes with the encryption and this way any pc can read it ?
 // Initialize the cipher
 // Encrypt the String of bytes
 // Return encrypted bytes
 protected byte[] encrypt(String strToEncrypt) throws InvalidKeyException,
 InvalidAlgorithmParameterException, IllegalBlockSizeException,
 BadPaddingException, UnsupportedEncodingException {
 byte[] byteToEncrypt = strToEncrypt.getBytes("UTF-8");
 this.c.init(Cipher.ENCRYPT_MODE, this.s_KEY, this.IV, this.r);
 byte[] encryptedBytes = this.c.doFinal(byteToEncrypt);
 return encryptedBytes;
 }
 // Initialize the cipher in DECRYPT_MODE
 // Decrypt and store as byte[]
 // Convert to plainText and return
 protected String decrypt(byte[] byteToDecrypt) throws InvalidKeyException,
 InvalidAlgorithmParameterException, IllegalBlockSizeException,
 BadPaddingException {
 this.c.init(Cipher.DECRYPT_MODE, this.s_KEY, this.IV);
 byte[] plainByte = this.c.doFinal(byteToDecrypt);
 String plainText = new String(plainByte);
 return plainText;
 }
 // Create the IV.
 // Create a Secure Random Number Generator and an empty 16byte array. Fill
 // the array.
 // Returns IV
 private IvParameterSpec generateIV() {
 byte[] newSeed = r.generateSeed(16);
 r.setSeed(newSeed);
 byte[] byteIV = new byte[16];
 r.nextBytes(byteIV);
 IV = new IvParameterSpec(byteIV);
 return IV;
 }
 // Create a "KeyGenerator" that takes in 'AES' as parameter
 // Create a "SecureRandom" Object and use it to initialize the
 // "KeyGenerator"
 // keyGen.init(256, sRandom); Initialize KeyGenerator with parameters
 // 256bits AES
 private SecretKey generateKEY() throws NoSuchAlgorithmException {
 // byte[] bytKey = AES_KEY.getBytes(); // Converts the Cipher Key to
 // Byte format
 // Should I use SHA-2 to get a random key or is this better?
 byte[] newSeed = r.generateSeed(32);
 r.setSeed(newSeed);
 KeyGenerator keyGen = KeyGenerator.getInstance("AES"); // A
 // "KEyGenerator"
 // object,
 SecureRandom sRandom = r.getInstanceStrong(); // A "SecureRandom" object
 // used to init the
 // keyGenerator
 keyGen.init(256, sRandom); // Initialize RAndom Number Generator
 s_KEY = keyGen.generateKey();
 return s_KEY;
 }
 public String byteArrayToString(byte[] s) {
 String string = new String(s);
 return string;
 }
 // Get Methods for all class variables
 public Cipher getCipher() {
 return c;
 }
 public IvParameterSpec getIV() {
 return IV;
 }
 public SecretKey getSecretKey() {
 return s_KEY;
 }
}

Answer 1

1. Is SecureRandom ok for generating my KEY and my IV?

   I would generally advise that you don't specify your own SecureRandom for the key generator, unless you have a specific reason to do so. By default, it will select the highest priority implementation it finds amongst the installed providers.

   Also, if your code is used with a hardware security module (HSM) in the future, it will either completely ignore your request or it will even throw an exception to tell you that you mustn't try to specify an alternative source of randomness.

   Using it to generate an IV value is fine.

2. What's up with all these exceptions?

   Yeah, irritating isn't it? The security APIs are peppered with checked exceptions. Fortunately, many of them extend GeneralSecurityException, so you can just throw that if you have no intention of acting upon the individual exceptions.

   As in all code, throw exceptions that are appropriate to the abstraction of your API layer.

3. Is my code creating any vulnerabilities in the encryption process? (mistakes maybe?)

   No, it generally looks fine. You should specify "UTF-8" when converting your plaintext bytes to a string, but that's about it.

   Obviously you'll need to store your IV along with your ciphertext when you eventually use this in anger.

4. Am I seeding SecureRandom properly?

   There's not really any need to seed a SecureRandom object. Many implementations of SecureRandom ignore the seeds they are supplied. Just create it using:

   SecureRandom random = new SecureRandom();
   

   You are currently using SecureRandom::generateSeed() which is actually intended for seeding other PRNGs. There's no need to use it to re-seed your existing SecureRandom instance. Just use the basic no-arg constructor as I suggest above.

• Patrick: Many implementations of SecureRandom ignore the seeds they are supplied Really? This would break the API, wouldn't it? Why would you prevent seeding the secureRandom (if the implementation is so that it will never decrease entropy)?

Answer 2

Just a few notes. Perhaps this answer is not so perfect like Duncan's one. But I try it anyway.

1. do not use SecureRandom as class member. Create SecureRandom as local variable if you need it.

From Proper use of Java’s SecureRandom:

• Periodically throw away the existing java.security.SecureRandom instance and create a new one. This will generate a new instance with a new seed.
• Periodically add new random material to the PRNG seed by making a call to java.security.SecureRandom.setSeed(java.security.SecureRandom.generateSeed(int)).

2. Use SecretKeyFactory and PBEKeySpec to generate your secret key.

3. Some times it can be a good idea to use Base64 encoding/decoding. (Debuging, binary data encryption, etc...)

4. throws Exception will make your code more readable. You can always create a MyCryptException(cause) but this would be useless since MyCryptException is just a wrapper for the real exception and provide no father functionality.

5. Are you really need the getter methods for Cipher, IvParameterSpec, SecretKey ?

6. Define an Interface for your AESCrypt class., Every algorithm or service should implement a interface

Look at this example:

public interface ICrypt {
 String encode(String plainText) throws Exception;
 String decode(String encodedText) throws Exception;
}
public class AESCrypt implements ICrypt {
 private static final int PASSWORD_ITERATIONS = 65536; // vs brute force
 private static final int KEY_LENGTH = 256;
 private char[] pass = "password".toCharArray(); // hardcoded or read me from a file
 private byte[] salt = new byte[20]; // for more confusion
 private byte[] ivBytes = null;
 public AESCrypt() {
 //
 // INIT SALT
 //
 SecureRandom secureRandom = new SecureRandom(); // seed is 0
 secureRandom.setSeed(secureRandom.generateSeed(16));
 secureRandom.nextBytes(salt);
 }
 private Cipher createCipher(boolean encryptMode) throws Exception {
 if (!encryptMode && ivBytes == null) {
 throw new IllegalStateException("ivBytes is null");
 }
 SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA1");
 PBEKeySpec spec = new PBEKeySpec(pass, salt, PASSWORD_ITERATIONS, KEY_LENGTH);
 SecretKey secretKey = factory.generateSecret(spec);
 SecretKeySpec secret = new SecretKeySpec(secretKey.getEncoded(), "AES");
 Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
 int mode = encryptMode ? Cipher.ENCRYPT_MODE : Cipher.DECRYPT_MODE;
 if (ivBytes == null) {
 cipher.init(mode, secret);
 AlgorithmParameters params = cipher.getParameters();
 ivBytes = params.getParameterSpec(IvParameterSpec.class).getIV();
 } else {
 cipher.init(mode, secret, new IvParameterSpec(ivBytes));
 }
 return cipher;
 }
 @Override
 public String encode(String plainText) throws Exception {
 Cipher cipher = createCipher(true);
 byte[] encryptedBytes = cipher.doFinal(plainText.getBytes("UTF-8"));
 return new String(encryptedBytes);
 }
 @Override
 public String decode(String encodedText) throws Exception {
 Cipher cipher = createCipher(false);
 return new String(cipher.doFinal(encodedText.getBytes()), "UTF-8");
 }
}

If you want to use base64 encoding/decoding, you do not need to change your AESCrypt implementation. Instead implement a decorator for this purpose:

public class Base64Decorator implements ICrypt {
 private ICrypt realCrypt;
 private Base64Decorator(ICrypt crypt) {
 this.realCrypt = crypt;
 }
 public static ICrypt wrap(ICrypt real) {
 return new Base64Decorator(real);
 }
 @Override
 public String encode(String plainText) throws Exception {
 String encoded = realCrypt.encode(plainText);
 return Base64.getEncoder().encodeToString(encoded.getBytes());
 }
 @Override
 public String decode(String encodedText) throws Exception {
 byte[] encodedBytes = Base64.getDecoder().decode(encodedText);
 return realCrypt.decode(new String(encodedBytes));
 }
}

Now you can use it this way:

public static void main(String[] args) throws Exception {
 ICrypt crypt = Base64Decorator.wrap(new AESCrypt());
 // ICrypt crypt = new AESCrypt();
 String encoded = crypt.encode("hello");
 String decoded = crypt.decode(encoded);
 System.out.println(encoded);
 System.out.println(decoded);
}

Output - Base64Decorator + AESCrypt:

eOXeBjEYzsUgnuHQDnZxXQ==

hello

Output - AESCrypt:

¬ ̈b;J¤yÇ@^Ž ́Ä

hello

• Duncan Jones: I've down-voted you for two bad pieces of advice. Firstly, you mention PBEKeySpec, but this has no relevance to the OP's code. You also say that throws Exception is more readable, but that's not good practice. I don't really understand either why you are promoting base64 encoding/decoding; there's nothing wrong with it, of course, but it seems like unhelpful filler in your answer.
• devops: this is code review, but ok
• Duncan Jones: Exceptions should be thrown at a level that suits the abstraction of the API layer (see Effective Java for more advice on that). I can't imagine a situation, outside of test code, where throws Exception is desired. And you seem to misunderstand what PBEKeySpec is for - this is used to produce good quality key material from a user password. The OP is not doing that - he is creating random new key material.
• Duncan Jones: I've read Effective Java many times. He certainly does advise against needlessly creating your own exception classes, but there are times where it's useful. We can't really tell in this example, without more information about the OP's overall system. However, throwing Exception is never a good idea. If you look around in common Java libraries, you'll almost never see that being done. It just offers so little information for the calling application.
• Duncan Jones: @dit I've removed my down-vote, since I think your good advice outweighs the bad. But I recommend you remove the reference to PBEKeySpec, as that's definitely wrong.

Answer 3

Your code has several flaws:

You use a random number generator to seed itself. That's useless. Just get the random numbers out, and you're done.

Your code declares exceptions that can never be thrown. Since you are using standard algorithms, your code will never throw UnknownAlgorithmException. Therefore, wrap the code like this:

public byte[] encrypt(String str) {
 try {
 ...
 return bytes;
 } catch (GeneralSecurityException e) {
 throw new IllegalStateException(e);
 }
}

As for the UnsupportedEncodingException: use new String(bytes, StandardCharsets.UTF_8) instead of new String(bytes, "UTF-8").

Whenever you convert between bytes and characters, you must specify the encoding. Your IDE should warn whenever you use the unsafe conversion methods. (If it doesn't, your IDE isn't as helpful as it could and should be.)

The byteArrayToString method is unused. Remove it.

Carefully look at all the comments you wrote. Most of them repeat what the code already says. Remove those. Then, check whether the comments say something wrong. Remove those as well. After that, there are only few comments left, which is good.