2
\$\begingroup\$
  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;
        }
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ 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. \$\endgroup\$ – Peter Taylor Feb 12 '18 at 12:18
1
\$\begingroup\$

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())
\$\endgroup\$
  • \$\begingroup\$ i hope you also managed to get the "Things left" done, they are important =) @DON \$\endgroup\$ – Martin Frank Feb 14 '18 at 9:19
  • \$\begingroup\$ getDefaulCipher missing \$\endgroup\$ – mbharanidharan88 Feb 14 '18 at 9:22
  • \$\begingroup\$ java:63: error: incompatible types: SecretKey cannot be converted to byte[] return new SecretKeySpec(factory.generateSecret(new PBEKeySpec(password.toCharArray(), salt, iterationCount, keyLength)), keyAlgorithm); \$\endgroup\$ – mbharanidharan88 Feb 14 '18 at 9:25
  • 1
    \$\begingroup\$ 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!! \$\endgroup\$ – Martin Frank Feb 14 '18 at 10:13
  • \$\begingroup\$ and what about getDefaultCipher?, Sorry I could not figure that out \$\endgroup\$ – mbharanidharan88 Feb 14 '18 at 12:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.