Problem
I wrote my first file encryption program, that encrypts a file with AES-256 GCM and stores IV and salt prepended to the file content, so it’s likely that I did something worse than possible.
I would like you to look at my code and point out errors or places where it is possible to make better.
import javax.crypto.*;
import javax.crypto.spec.GCMParameterSpec;
import javax.crypto.spec.PBEKeySpec;
import javax.crypto.spec.SecretKeySpec;
import java.io.*;
import java.security.GeneralSecurityException;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.security.spec.KeySpec;
import java.util.Base64;
import java.util.logging.Level;
import java.util.logging.Logger;
public class FileCryptor implements Serializable {
private static final Logger LOGGER = Logger.getLogger(FileCryptor.class.getName());
private static final int DEFAULT_GCM_AUTHENTICATION_TAG_SIZE_BITS = 128;
private static final int DEFAULT_GCM_IV_NONCE_SIZE_BYTES = 12;
private static final int DEFAULT_PBKDF2_ITERATIONS = 65536;
private static final int DEFAULT_PBKDF2_SALT_SIZE_BYTES = 32;
private static final int DEFAULT_AES_KEY_LENGTH_BITS = 256;
private static final String DEFAULT_CIPHER = "AES";
private static final String DEFAULT_CIPHERSCHEME = "AES/GCM/NoPadding";
private static final String DEFAULT_PBKDF2_SCHEME = "PBKDF2WithHmacSHA256";
private int gcmAuthenticationTagSizeBits = DEFAULT_GCM_AUTHENTICATION_TAG_SIZE_BITS;
private int gcmIvNonceSizeBytes = DEFAULT_GCM_IV_NONCE_SIZE_BYTES;
private int pbkdf2Iterations = DEFAULT_PBKDF2_ITERATIONS;
private int pbkdf2SaltSizeBytes = DEFAULT_PBKDF2_SALT_SIZE_BYTES;
private int aesKeyLengthBits = DEFAULT_AES_KEY_LENGTH_BITS;
private String cipher = DEFAULT_CIPHER;
private String cipherscheme = DEFAULT_CIPHERSCHEME;
private String pbkdf2Scheme = DEFAULT_PBKDF2_SCHEME;
/**
* Creates a new empty EncryptedFile object
*/
public FileCryptor() {
}
/**
* Generates a randomly filled byte array
*
* @param sizeInBytes length of the array in bytes
* @return byte array containing random values
* @throws NoSuchAlgorithmException
*/
private static byte[] generateRandomArry(int sizeInBytes) throws NoSuchAlgorithmException {
/* generate random salt */
final byte[] salt = new byte[sizeInBytes];
SecureRandom random = SecureRandom.getInstanceStrong();
random.nextBytes(salt);
return salt;
}
/**
* Encrypts the provided file using the provided password and writes it
* in a file with extension "enc"
*
* @param password password which is used to generate the key
* @param path path to a writeable file (may already exist)
* @throws GeneralSecurityException
*/
public void encrypt(String password, String path) throws GeneralSecurityException {
/* Derive the key*/
SecretKeyFactory factory = SecretKeyFactory.getInstance(pbkdf2Scheme);
byte[] newSalt = generateRandomArry(pbkdf2SaltSizeBytes);
KeySpec keyspec = new PBEKeySpec(password.toCharArray(), newSalt, pbkdf2Iterations, aesKeyLengthBits);
SecretKey tmp = factory.generateSecret(keyspec);
SecretKey key = new SecretKeySpec(tmp.getEncoded(), cipher);
Cipher myCipher = Cipher.getInstance(cipherscheme);
byte[] newNonce = generateRandomArry(gcmIvNonceSizeBytes);
GCMParameterSpec spec = new GCMParameterSpec(gcmAuthenticationTagSizeBits, newNonce);
myCipher.init(Cipher.ENCRYPT_MODE, key, spec);
try (
FileInputStream fileInputStream = new FileInputStream(path);
FileOutputStream fileOutputStream = new FileOutputStream(path + ".enc");
CipherOutputStream encryptedOutputStream = new CipherOutputStream(fileOutputStream, myCipher);
) {
// write IV/nonce
fileOutputStream.write(newNonce);
// write salt
fileOutputStream.write(newSalt);
byte[] buffer = new byte[32];
while (fileInputStream.read(buffer) > 0) {
encryptedOutputStream.write(buffer);
}
} catch (IOException e) {
LOGGER.log(Level.SEVERE, e.getMessage(), e);
throw new SecurityException(e.getMessage(), e);
}
}
/**
* Decrypts the encrypted file using the provided password.
*
* @param password password which is used to generate the key
* @param path path to a previously encrypted file to be decrypted
* @throws GeneralSecurityException
*/
public void decrypt(String password, String path) throws GeneralSecurityException {
// Read configuration from file
byte[] myNonce = new byte[gcmIvNonceSizeBytes];
byte[] mySalt = new byte[pbkdf2SaltSizeBytes];
try (
FileInputStream fileInputStream = new FileInputStream(path);
) {
int countReadBytesNonce = fileInputStream.read(myNonce);
int countReadBytesSalt = fileInputStream.read(mySalt);
} catch (IOException e) {
LOGGER.log(Level.SEVERE, e.getMessage(), e);
throw new SecurityException(e.getMessage(), e);
}
/* Derive the key*/
SecretKeyFactory factory = SecretKeyFactory.getInstance(pbkdf2Scheme);
KeySpec keyspec = new PBEKeySpec(password.toCharArray(), mySalt, pbkdf2Iterations, aesKeyLengthBits);
SecretKey tmp = factory.generateSecret(keyspec);
SecretKey key = new SecretKeySpec(tmp.getEncoded(), cipher);
Cipher myCipher = Cipher.getInstance(cipherscheme);
GCMParameterSpec spec = new GCMParameterSpec(gcmAuthenticationTagSizeBits, myNonce);
myCipher.init(Cipher.DECRYPT_MODE, key, spec);
try (
FileOutputStream fileOutputStream = new FileOutputStream(path.substring(0, path.length() - 4));
FileInputStream fileInputStream = new FileInputStream(path);
CipherInputStream cipherInputStream = new CipherInputStream(fileInputStream, myCipher);
) {
// offset the stream by the bytes already read previosly
byte[] skipped = new byte[gcmIvNonceSizeBytes+pbkdf2SaltSizeBytes];
int read = fileInputStream.read(skipped);
byte[] buffer = new byte[32];
while (cipherInputStream.read(buffer) > 0) {
fileOutputStream.write(buffer);
}
} catch (IOException e) {
LOGGER.log(Level.SEVERE, e.getMessage(), e);
throw new SecurityException(e.getMessage(), e);
}
}
}
Solution
Elephant in the room
Let’s first try and identify the elephant in the room. There is one big problem with using GCM and file decryption: tag validation. The problem is that for GCM there are two choices: either you output unvalidated chunks of data, or you buffer all data until the tag is validated. The first option should be followed by any reasonable low level API designer, leaving it up to the user to handle invalid tags.
In Java, those will result in AEADBadTagException (or just BadPaddingException in some older / third party implementations). Unfortunately Java devs had some struggle with this as well, so there have been multiple implementations of both the cipher and the CipherInputStream & CipherOutputStream if I remember correctly. To be honest, I lost track and most of my knowledge of this issue some time ago.
Anyway, you should really test how the current Java versions handle the situation. But it is very important that for your program you make sure that:
- not all data is buffered (for large files);
- you indicate to the user that the encryption has failed and that cleanup is necessary.
Because if the data is not all buffered, you don’t want to leave the user with partial files with content that was not valid. Some kind of strategy is required.
Design
The class requires one password per file. That’s OK, but please mind you that this gets cumbersome fast for multiple files, especially since the PBKDF2 function needs to be run each time – even if you’d use the same password. Creating a key and then using that makes more sense, and it simplifies your methods. You still have a random IV anyway.
The parameters of the function say that you need to provide a path to a writable file. However, that’s not how path is actually used. All side effects should be made clear to the user, and the format of the encrypted file should be documented as well.
The design is relatively OK. The streams seem to be used correctly. The security parameters seem correct as well, kudos. You are using streaming with a small (too small?) buffer size – which isn’t configurable though and stated as a literal instead of a constant. There is precious little encoding / decoding going on, which is exactly how file handling should be; little to no stringified code to be found except maybe for the parameters (char[] and File make more sense to me).
The iteration count is already at the small side; it is very much expected to change per system, so I would write it to file as well. Including a versioning header is always a good idea.
There is too little checking on the parameters and on the files themselves, especially when it comes to error generation. Java does have more extensive file libraries since the NIO libraries were introduced.
The exception handling is not very good. There is too little distinction made between various exception classes, especially when it comes to e.g. ciphers not being found and input / output errors.
FileCryptor class
public class FileCryptor implements Serializable {
Why would this kind of class be serializable? That just doesn’t make sense. Besides that, you should include a serialization constant in case your inner class design is changed.
private int gcmAuthenticationTagSizeBits = DEFAULT_GCM_AUTHENTICATION_TAG_SIZE_BITS;
That’s nice, you can reprogram or upgrade you class and you don’t have to create your fields. However, a subclass cannot use them because they are private. And if you upgrade then you can always introduce them. Don’t increase the state / fields unless necessary and directly use the constants. The constants will get inlined, speeding up the Java code.
On the other hand:
private int pbkdf2Iterations = DEFAULT_PBKDF2_ITERATIONS;
is something you do immediately want to be upgradable, so much so that it could be saved with the file.
generateRandomArry method
Method name is missing an “a” for “Arry”
final byte[] salt = new byte[sizeInBytes];
Sorry? What salt? If you just return it as a generic array then you should not name the variable salt.
SecureRandom random = SecureRandom.getInstanceStrong();
That’s overdoing it, getInstanceStrong() is for long term key generation. Just use new SecureRandom().
encrypt method
password.toCharArray(),
The whole idea of char[] is that you can zero it out. So passing it as a string doesn’t make a whole lot of sense.
Note too that the PBKDF2 function implementation of the standard JVM only uses the lower 8 bits of each char (rather stupidly if you ask me). I would make sure that the characters are not outside that range, or you may end up with something that is encrypted with a different password than you might have thunk. This is particularly “fun” if somebody uses a Chinese password or if you try and get compatibility with other runtimes.
From the PBEKeySpec class documentation:
You convert the password characters to a PBE key by creating an instance of the appropriate secret-key factory. For example, a secret-key factory for PKCS #5 will construct a PBE key from only the low order 8 bits of each password character, whereas a secret-key factory for PKCS #12 will take all 16 bits of each character.
And, as PBKDF2 is specified in the PKCS#5 Password Based Encryption (PBE) standards, this is from the algorithms page of Java:
Password-based key-derivation algorithm defined in PKCS #5: Password-Based Cryptography Specification, Version 2.1 using the specified pseudo-random function (). Example:
PBKDF2WithHmacSHA256.
byte[] buffer = new byte[32];
while (fileInputStream.read(buffer) > 0) {
encryptedOutputStream.write(buffer);
}
InputStream has a transferTo method since Java 9.
throw new SecurityException(e.getMessage(), e);
No, that cannot be right. Think of your own message and possibly throw your own checked exception. A security exception “file cannot be opened” doesn’t make sense.
From the documentation:
Thrown by the security manager to indicate a security violation.
… that’s not it. Probably you wanted to reuse GeneralSecurityException, but that would also be wrong.
decrypt method
byte[] myNonce = new byte[gcmIvNonceSizeBytes];
Oh, my, don’t start with the my prefix now, just use the same names as in the encryption method.
int countReadBytesNonce = fileInputStream.read(myNonce);
Please use readFully. This won’t fail on files, but will fail for other input streams.
/* Derive the key*/
If you have to write that down the you need to introduce a method. Duplicate code.
path.length() - 4
Oh, right, that’s never going to fail right? Try running that on "LPT1.enc" on Windows. Please check your input before doing that.
byte[] skipped = new byte[gcmIvNonceSizeBytes+pbkdf2SaltSizeBytes];
Just keep the file open please.