1
\$\begingroup\$

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();

}

}
\$\endgroup\$
  • \$\begingroup\$ @adot710 I rolled back that edit since it looked like a mistake, maybe try again if it was formatting related. \$\endgroup\$ – ferada May 6 at 23:38
  • \$\begingroup\$ 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. \$\endgroup\$ – Vogel612 May 21 at 15:23
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Good spot didn't see that thanks, does the rest of my code seem okay? \$\endgroup\$ – adot710 May 2 at 11:39

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.