AES Encryption/Decryption with key

Question

1. Is this the best way to encrypt/decrypt data?
2. And Is it good to store Generated InitVector and CipherText in database as Base64 String which is converted from byte[]?

---

import javax.crypto.SecretKeyFactory;
import javax.crypto.spec.SecretKeySpec;
import javax.crypto.spec.PBEKeySpec;
import javax.crypto.*;
import java.util.Random;
import java.security.SecureRandom;
import java.security.AlgorithmParameters;
import javax.crypto.spec.IvParameterSpec;
import javax.crypto.Cipher;
import java.nio.charset.StandardCharsets;
import java.util.Base64;
public class MyClass {
 public static void main(String args[]) {
 //Encrypt
 EncryptHelper helper = new EncryptHelper(); 
 EncryptedData a = helper.encrypt("key123", "This is the text to encrypt");
 //Decrypt
 EncryptHelper helper1 = new EncryptHelper(); 
 String b = helper1.decrypt("key123", a);
 System.out.println(b);
 }
 public static class EncryptedData
 {
 String _saltString;
 String _initVector;
 String _cipherText;
 public EncryptedData(String saltString, String initVector, String cipherText)
 {
 _saltString = saltString;
 _initVector = initVector;
 _cipherText = cipherText;
 }
 public String getSaltString() { return _saltString; }
 public String getInitVector() { return _initVector; }
 public String getCipherText() { return _cipherText; }
 }
 public static class EncryptHelper
 { 
 public EncryptedData encrypt(String password, String data)
 {
 try
 {
 byte[] salt = generateSalt();
 SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256");
 PBEKeySpec spec = new PBEKeySpec(password.toCharArray(), salt, 65536, 256);
 SecretKey tmp = factory.generateSecret(spec);
 SecretKey secret = new SecretKeySpec(tmp.getEncoded(), "AES");
 /* Encrypt the message. */
 Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
 cipher.init(Cipher.ENCRYPT_MODE, secret);
 AlgorithmParameters params = cipher.getParameters();
 byte[] initVector = params.getParameterSpec(IvParameterSpec.class).getIV();
 byte[] ciphertext = cipher.doFinal(data.getBytes("UTF-8"));
 return new EncryptedData(getBase64String(salt), getBase64String(initVector), getBase64String(ciphertext));
 }
 catch(Exception ex)
 {
 System.out.println(ex.getMessage());
 }
 return null;
 }
 public String decrypt(String password, EncryptedData b)
 {
 try
 {
 byte[] salt = getBase64Bytes(b.getSaltString());
 SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA256");
 PBEKeySpec spec = new PBEKeySpec(password.toCharArray(), salt, 65536, 256);
 SecretKey tmp = factory.generateSecret(spec);
 SecretKey secret = new SecretKeySpec(tmp.getEncoded(), "AES");
 //Get from EncryptedData
 byte[] initVector = getBase64Bytes(b.getInitVector());
 byte[] cipherText = getBase64Bytes(b.getCipherText());
 Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
 cipher.init(Cipher.DECRYPT_MODE, secret, new IvParameterSpec(initVector));
 String plaintext = new String(cipher.doFinal(cipherText), "UTF-8");
 return plaintext;
 }
 catch(Exception ex)
 {
 return ex.getMessage();
 }
 }
 private String getBase64String(byte[] data)
 {
 byte[] encoded = Base64.getEncoder().encode(data);
 return new String(encoded); 
 }
 private byte[] getBase64Bytes(String str)
 {
 return Base64.getDecoder().decode(str);
 }
 private byte[] generateSalt()
 {
 final Random r = new SecureRandom();
 byte[] salt = new byte[32];
 r.nextBytes(salt);
 return salt;
 }
 }
}

• Peter Taylor: The easiest way to fix the formatting is probably to put a horizontal rule after the questions ( ---- ), delete the code, paste it in cleanly, select it, and press Ctrl-K.

Answer 1

One approach might surely be to remove redundant code for the SecretKey and to add a default getter:

private static final String FACTORY_ALGORITHM = "PBKDF2WithHmacSHA256";
private static final String KEY_ALGORITHM = "AES";
private static final int KEYSPEC_ITERATION_COUNT = 65536;
private static final int KEYSPEC_LENGTH = 256;
//TODO describe your default implementation or use better member names (mine are a bit too plain - this would make this javadoc obsolete)
private SecretKey getDefaultSecretKey(final String password, final byte[] salt){
 return getSecretKey(password, salt, FACTORY_ALGORITHM, KEY_ALGORITHM, KEYSPEC_ITERATION_COUNT, KEYSPEC_LENGTH);
}
private SecretKey getSecretKey(final String password, 
 final byte[] salt, 
 final String factoryAlgorithm, 
 final String keyAlgorithm, 
 final int iterationCount, 
 final int keyLength){
 SecretKeyFactory factory = SecretKeyFactory.getInstance(factoryAlgorithm);
 return new SecretKeySpec(factory.generateSecret(new PBEKeySpec(password.toCharArray(), salt, iterationCount, keyLength)).getEncoded(), keyAlgorithm); //thanks alot for the bug report
}

that would lead to a very readable version of your code:

public EncryptedData encrypt(String password, String data) throws Exception {
//you DONT handle Exceptions - so you let other people do the work you're supposed to do
//thats ok for me =)
 SecretKey secret = getDefaultSecretKey(password, generateSalt());
 Cipher cipher = getDefaulCipher(); //analogy towards getDefaultSecretKey - now you know how it would work
 cipher.init(Cipher.ENCRYPT_MODE, secret);
 AlgorithmParameters params = cipher.getParameters();
 byte[] initVector = params.getParameterSpec(IvParameterSpec.class).getIV();
 byte[] ciphertext = cipher.doFinal(data.getBytes("UTF-8")); //FIXME: no hardcoded Byte-Conversion
 return new EncryptedData(getBase64String(salt), getBase64String(initVector), getBase64String(ciphertext));
}
public String decrypt(String password, EncryptedData b) throws Exception { 
//yeah exceptions handling is other peoples duty, not mine 
 SecretKey secret = getDefaultSecretKey(password, getBase64Bytes(b.getSaltString()));
 byte[] initVector = getBase64Bytes(b.getInitVector());
 byte[] cipherText = getBase64Bytes(b.getCipherText());
 Cipher cipher = getDefaulCipher(); //as mentioned above
 cipher.init(Cipher.DECRYPT_MODE, secret, new IvParameterSpec(initVector));
 return new String(cipher.doFinal(cipherText), "UTF-8"); //FIXME: no hardcoded Byte-Conversion
}

Things left (see comments in the code above):

• Improved exception handling;
• No hardcoded Byte-Conversion;
• Default getter for Cipher (following the example from getDefaultSecretKey())

• Martin Frank: i hope you also managed to get the "Things left" done, they are important =) @DON
• Hary: getDefaulCipher missing
• Hary: java:63: error: incompatible types: SecretKey cannot be converted to byte[] return new SecretKeySpec(factory.generateSecret(new PBEKeySpec(password.toCharArray(), salt, iterationCount, keyLength)), keyAlgorithm)
• Martin Frank: it's always hard for me, i don't copy&paste this code into an IDE i just write from brain into codereview.stackexchange.com - thank you for taking my adviceds for real!!
• Hary: and what about getDefaultCipher?, Sorry I could not figure that out