2
\$\begingroup\$

I've been working on my first "big" project in java, an AES encryption and decryption class, it's now working but i don't know how to improve my code. Also i'm not the best in security and I have no idea if there's something that make my code vulnerable.

I don't have a lot of experience in java so if you have any tips to make the project better that would be great.

What i'm thinking: -I shouldn't receive a SecretKey type in my main, maybe create a private SecretKey in my AES class to keep it. -Maybe i should abstract my class more, never tried it in java though. -I should improve my generateKey method.

This is how the class works:

  • You generate an AES key with the generateKey method which call the saveKey method to keep the key in a file.
  • I then loadKey the file to get a SecretKey to use later.
  • I can then call my methods encrypt/decrypt file/string

Here is the class:



    package encryptUtils;

    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 java.io.File;
    import java.io.FileInputStream;
    import java.io.FileNotFoundException;
    import java.io.FileOutputStream;
    import java.io.IOException;
    import java.io.UnsupportedEncodingException;

    import java.security.cert.CertificateException;
    import java.security.InvalidKeyException;
    import java.security.KeyStore;
    import java.security.KeyStoreException;
    import java.security.NoSuchAlgorithmException;
    import java.security.UnrecoverableKeyException;

    public class AES {
        public static void generateKey(
                String password,
                String userName)
                throws CryptoException {
            KeyGenerator KeyGen;

            try {
                KeyGen = KeyGenerator.getInstance("AES");
            } catch (NoSuchAlgorithmException ex) {
                throw new CryptoException("Error generateKey | KeyGenerator.getInstance", ex);
            }

            KeyGen.init(128);
            SecretKey SecKey = KeyGen.generateKey();
            saveKey(SecKey, password, userName);
        }

        private static void saveKey(
                SecretKey key,
                String password,
                String userName)
                throws CryptoException {
            KeyStore keyStore;

            try {
                keyStore = KeyStore.getInstance("JCEKS");
            } catch (KeyStoreException ex) {
                throw new CryptoException("Error saveKey | KeyStore.getInstance", ex);
            }

            try {
                keyStore.load(null, null);
            } catch (IOException | NoSuchAlgorithmException | CertificateException ex) {
                throw new CryptoException("Error saveKey | keyStore.load", ex);
            }

            try {
                keyStore.setKeyEntry(userName, key, password.toCharArray(), null);
            } catch (KeyStoreException ex) {
                throw new CryptoException("Error saveKey | keyStore.setKeyEntry", ex);
            }

            try {
                keyStore.store(new FileOutputStream(userName + ".jceks"), password.toCharArray());
            } catch (KeyStoreException | IOException | NoSuchAlgorithmException | CertificateException ex) {
                throw new CryptoException("Error saveKey | keyStore.store", ex);
            }
        }

        public static SecretKey loadKey(
                String password,
                String userName)
                throws CryptoException {
            KeyStore keyStore;

            try {
                keyStore = KeyStore.getInstance("JCEKS");
            } catch (KeyStoreException ex) {
                throw new CryptoException("Error loadKey | KeyStore.getInstance", ex);
            }

            try {
                keyStore.load(new FileInputStream(userName + ".jceks"), password.toCharArray());
            } catch (IOException | NoSuchAlgorithmException | CertificateException ex) {
                throw new CryptoException("Error loadKey | keyStore.load", ex);
            }

            SecretKey key;

            try {
                key = (SecretKey) keyStore.getKey(userName, password.toCharArray());
            } catch (KeyStoreException | NoSuchAlgorithmException | UnrecoverableKeyException ex) {
                throw new CryptoException("Error loadKey | key = (SecretKey) keyStore.getKey", ex);
            }

            return key;
        }

        public static byte[] encryptString(
                SecretKey key,
                String toEncrypt)
                throws CryptoException {
            byte[] toEncryptBytes;

            try {
                toEncryptBytes = toEncrypt.getBytes("UTF8");
            } catch (UnsupportedEncodingException ex) {
                throw new CryptoException("Error encryptString | String.getBytes", ex);
            }

            byte[] outputBytes = doCrypto(Cipher.ENCRYPT_MODE, key, toEncryptBytes);
            return outputBytes;
        }

        public static String decryptString(
                SecretKey key,
                byte[] toDecryptBytes)
                throws CryptoException {
            byte[] outputBytes = doCrypto(Cipher.DECRYPT_MODE, key, toDecryptBytes);

            String ciphtx = new String(outputBytes);
            return ciphtx;
        }

        public static File encryptFile(
                SecretKey key,
                File inputFile)
                throws CryptoException {
            FileInputStream inputStream;

            try {
                inputStream = new FileInputStream(inputFile);
            } catch (FileNotFoundException ex) {
                throw new CryptoException("Error encryptFile | new FileInputStream", ex);
            }

            byte[] inputBytes = new byte[(int) inputFile.length()];

            try {
                inputStream.read(inputBytes);
            } catch (IOException ex) {
                throw new CryptoException("Error encryptFile | FileInputStream.read", ex);
            }

            byte[] outputBytes = doCrypto(Cipher.ENCRYPT_MODE, key, inputBytes);

            FileOutputStream outputStream;
            File outputFile = new File(inputFile.getName() + ".enc");

            try {
                outputStream = new FileOutputStream(outputFile);
            } catch (FileNotFoundException ex) {
                throw new CryptoException("Error encryptFile | new FileOutputStream", ex);
            }

            try {
                outputStream.write(outputBytes);
            } catch (IOException ex) {
                throw new CryptoException("Error encryptFile | FileOutputStream.write", ex);
            }

            try {
                inputStream.close();
            } catch (IOException ex) {
                throw new CryptoException("Error encryptFile | FileInputStream.close", ex);
            }

            try {
                outputStream.close();
            } catch (IOException ex) {
                throw new CryptoException("Error encryptFile | FileOutputStream.close", ex);
            }

            return outputFile;
        }

        public static File decryptFile(
                SecretKey key,
                File inputFile)
                throws CryptoException {
            FileInputStream inputStream;

            try {
                inputStream = new FileInputStream(inputFile);
            } catch (FileNotFoundException ex) {
                throw new CryptoException("Error decryptFile | new FileInputStream", ex);
            }

            byte[] inputBytes = new byte[(int) inputFile.length()];

            try {
                inputStream.read(inputBytes);
            } catch (IOException ex) {
                throw new CryptoException("Error decryptFile | FileInputStream.read", ex);
            }

            byte[] outputBytes = doCrypto(Cipher.DECRYPT_MODE, key, inputBytes);
            FileOutputStream outputStream;
            File outputFile = new File("dec" + inputFile.getName().substring(0, inputFile.getName().length() - 4));

            try {
                outputStream = new FileOutputStream(outputFile);
            } catch (FileNotFoundException ex) {
                throw new CryptoException("Error decryptFile | new FileOutputStream", ex);
            }

            try {
                outputStream.write(outputBytes);
            } catch (IOException ex) {
                throw new CryptoException("Error decryptFile | FileOutputStream.write", ex);
            }

            try {
                inputStream.close();
            } catch (IOException ex) {
                throw new CryptoException("Error decryptFile | FileInputStream.close", ex);
            }

            try {
                outputStream.close();
            } catch (IOException ex) {
                throw new CryptoException("Error decryptFile | FileOutputStream.close", ex);
            }

            return outputFile;
        }

        private static byte[] doCrypto(
                int cipherMode,
                SecretKey key,
                byte[] inputBytes)
                throws CryptoException {
            Cipher cipher;

            try {
                cipher = Cipher.getInstance("AES");
            } catch (NoSuchAlgorithmException | NoSuchPaddingException ex) {
                throw new CryptoException("Error doCrypto | cipher.getInstance", ex);
            }

            try {
                cipher.init(cipherMode, key);
            } catch (InvalidKeyException ex) {
                throw new CryptoException("Error doCrypto | cipher.init", ex);
            }

            byte[] outputBytes;

            try {
                outputBytes = cipher.doFinal(inputBytes);
            } catch (IllegalBlockSizeException | BadPaddingException ex) {
                throw new CryptoException("Error doCrypto | cipher.doFinal", ex);
            }

            return outputBytes;
        }
    }

Here is the main to test it:



    package encryptUtils;

    import java.io.File;

    import javax.crypto.SecretKey;

    public class Main {

        public static void main(
                String[] args)
                throws Exception {

            /*
            *   AES
            */
            System.out.println( "----- AES -----" );

            //change from File to String and give path to method
            File inputFile = new File("pic.jpg");
            //File inputFile = new File("cleartext.txt");

            //Should be char array to empty when finished with
            String password = "password";
            String userName = "bob";

            try {
                System.out.println("\n----- Generate Key -----");
                AES.generateKey(password, userName);
                System.out.println("----- ---------- -----\n----- Load Key -----");
                SecretKey AESkey = AES.loadKey(password, userName);

                System.out.println("----- ---------- -----\n----- File Encryption -----\ninputFileName: " + inputFile.getName());
                File encryptedFile = AES.encryptFile(AESkey, inputFile);
                System.out.println( "encryptedFileName: " + encryptedFile.getName() + "\n----- ---------- -----\n----- File Decryption -----");
                File decryptedFile = AES.decryptFile(AESkey, encryptedFile);

                System.out.println( "decryptedFileName: " + decryptedFile.getName() + "\n----- ---------- -----\n----- String Encryption -----");
                byte[]outputBytes = AES.encryptString(AESkey, "Lorem Ipsum is simply dummy text of the printing and typesetting industry. Lorem Ipsum has been the industry's standard dummy text ever since the 1500s, when an unknown printer took a galley of type and scrambled it to make a type specimen book. It has survived not only five centuries, but also the leap into electronic typesetting, remaining essentially unchanged. It was popularised in the 1960s with the release of Letraset sheets containing Lorem Ipsum passages, and more recently with desktop publishing software like Aldus PageMaker including versions of Lorem Ipsum.");
                System.out.println(new String(outputBytes) + "\n----- ---------- -----\n----- String Decryption -----");
                String decryptedString = AES.decryptString(AESkey, outputBytes);
                System.out.println(decryptedString + "\n----- ---------- -----\n");
            } catch (CryptoException ex) {
                System.out.println(ex.getMessage());
                ex.printStackTrace();
            }

            System.out.println("\nEnd");
        }
    }

\$\endgroup\$
3
\$\begingroup\$

I am not quite sure, if it is a good idea, but I personally would insert log entries, that tell you at what point your application crashed and insert a single, but large try-multi catch block at the end of you class. For example:

        try {
            keyStore = KeyStore.getInstance("JCEKS");
        } catch (KeyStoreException ex) {
            throw new CryptoException("Error saveKey | KeyStore.getInstance", ex);
        }

        try {
            keyStore.load(null, null);
        } catch (IOException | NoSuchAlgorithmException | CertificateException ex) {
            throw new CryptoException("Error saveKey | keyStore.load", ex);
        }

        try {
            keyStore.setKeyEntry(userName, key, password.toCharArray(), null);
        } catch (KeyStoreException ex) {
            throw new CryptoException("Error saveKey | keyStore.setKeyEntry", ex);
        }

        try {
            keyStore.store(new FileOutputStream(userName + ".jceks"), password.toCharArray());
        } catch (KeyStoreException | IOException | NoSuchAlgorithmException | CertificateException ex) {
            throw new CryptoException("Error saveKey | keyStore.store", ex);
        }

would convert to

try {
        log.info ("Get an instance of the keyStore");
        keyStore = KeyStore.getInstance("JCEKS");

        log.info ("load (null,null)");
        keyStore.load(null, null);

        log.infor ("Set the key entry for the user");
        keyStore.setKeyEntry(userName, key, password.toCharArray(), null);

        log.info("store in the keystore");
        keyStore.store(new FileOutputStream(userName + ".jceks"), password.toCharArray());
} catch (Exception e) {
        throw new CryptoException ("Execution failed! See logfile for more info", e);
}

I think this is far better readable, making it easier to understand and/or improve you algorythm.

EDIT

Viewing this question on SO, you should convert the String parameter to a char[] parameter

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your help. ;) I'll change my try/catch accordingly. And for the string that's what i thought, thanks for the confirmation. \$\endgroup\$ – moKa Jan 28 '17 at 9:01

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.