employing AES algorithm in Java

Question

I have employed an AES algorithm in order to encrypt files within a GUI. I was wondering if my algorithm is secure. Could I change anything to improve it? Is my IV secure and is it producing different random numbers each time? Am I transferring the salt successfully?

I know that I could improve this by making separate classes for the different methods however I am just doing this within the main for now. I'm quite new to cryptography so any feedback would be greatly appreciated! // author @Alex Matheakis public class AESFileEncryption {

// password to encrypt the file - how long should password be?
private static final String password = "UxIpOqSdNmSTuxZaShPu";
public static void main(String args[]) throws Exception {
 // file to be encrypted
 FileInputStream inF = new FileInputStream(GUI.AESinFile); // 'AESinFile' is a JFileChooser method from my GUI class
 // encrypted file
 FileOutputStream outF = new FileOutputStream("encrypted_file.des");
 // generate and write the salt
 SecureRandom sr = new SecureRandom();
 byte[] salt = new byte[16];
 sr.nextBytes(salt);
 outF.write(salt);
 // generate key
 SecretKeyFactory skf = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA1");
 KeySpec keySpec = new PBEKeySpec(password.toCharArray(), salt, 65536, 256); // salt, iteration count, key strength
 SecretKey tmp = skf.generateSecret(keySpec);
 SecretKey secretKey = new SecretKeySpec(tmp.getEncoded(), "AES"); // returns key
 // initialise the cipher with secure padding
 Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
 cipher.init(Cipher.ENCRYPT_MODE, secretKey);
 AlgorithmParameters p = cipher.getParameters();
 // iv used when initializing the cipher to make text more random
 byte[] iv = p.getParameterSpec(IvParameterSpec.class).getIV();
 outF.write(iv);
 // file encryption
 byte[] input = new byte[64];
 int bytesRead;
 while ((bytesRead = inF.read(input)) != -1) {
 byte[] output = cipher.update(input, 0, bytesRead);
 if (output != null)
 outF.write(output);
 }
 byte[] output = cipher.doFinal();
 if (output != null)
 outF.write(output);
 System.out.println("file encrypted");
 inF.close();
 outF.flush();
 outF.close();
 // inputScanner.close();
}
}

• ferada: @adot710 I rolled back that edit since it looked like a mistake, maybe try again if it was formatting related.
• Vogel612: Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers .

Answer 1

Line 105 is a typo. The indentation suggests its only executed if output is not null.

But it's actually unrelated to the if statement.

You should always use curly braces even if execution is only 1 line long. This goes for all statements: while, if, do, etc.

• adot710: Good spot didn't see that thanks, does the rest of my code seem okay?