AES with password based SecretKeySpec vs PBE

Question

After reading through several stackoverflow questions about implementing AES I think I'm starting to understand the basics:

• Every single time I should generate a new IV
• When using PBE the iteration count should be around 1000-4000+
• Since I can't predict the amount of data to be encrypted I shouldn't use ECB cipher mode

My environment is quite simple:

• the passphrase should be secure, it's a securerandom generated random 32 character at the moment (i.e. not set by a user).
• the generated encrypted content may end up being stored as cookies, so they are somewhat public

Based on these I came up with the following Java code:

public class SecureEncryption {

private static final String CONTENT = "thisneedstobestoredverysecurely";
private static final String PASSPHRASE = "mysuperstrongpassword";
private static final int IV_LENGTH = 16;

public static void main(String[] args) throws Exception {
    MessageDigest digest = MessageDigest.getInstance("SHA-1");
    byte[] passphrase = digest.digest(PASSPHRASE.getBytes("UTF-8"));

    Cipher instance = Cipher.getInstance("AES/CFB/NoPadding");
    passphrase = Arrays.copyOf(passphrase, 16);
    SecretKeySpec secretKey = new SecretKeySpec(passphrase, "AES");
    byte[] iv = new byte[16];
    SecureRandom sr = new SecureRandom();
    sr.nextBytes(iv);
    instance.init(Cipher.ENCRYPT_MODE, secretKey, new IvParameterSpec(iv));
    byte[] encrypted = instance.doFinal(CONTENT.getBytes("UTF-8"));
    byte[] result = addIVtoEncrypted(iv, encrypted);

    System.arraycopy(result, 0, iv, 0, IV_LENGTH);
    System.arraycopy(result, IV_LENGTH, encrypted, 0, result.length - IV_LENGTH);
    instance.init(Cipher.DECRYPT_MODE, secretKey, new IvParameterSpec(iv));
    byte[] decrypted = instance.doFinal(encrypted);
    System.out.println(new String(decrypted, "UTF-8"));
}

private static byte[] addIVtoEncrypted(byte[] iv, byte[] encrypted) {
    byte[] ret = new byte[IV_LENGTH + encrypted.length];
    System.arraycopy(iv, 0, ret, 0, IV_LENGTH);
    System.arraycopy(encrypted, 0, ret, IV_LENGTH, encrypted.length);
    return ret;
}
}

While this works fine, I'm not sure if it's as secure as it can get.. I'm kind of lost at the moment regarding the following things:

• is PBE using AES+SHA more secure? Does the salt+iteration count adds considerably to security? Which exact combination of PBE should I use if so?
• Should I instead consider using salt for the encryptable content (rather than for the PBE key)?
• If using salt for the content, what is preferred: one static value, or different values, but appended/prepended to the encrypted result (just as it is done with IV)?

UPDATE: based on the recommendations received here I rewrote my implementation:

public class SecureEncryption {

private static final String CONTENT = "thisneedstobestoredverysecurely";
private static final String PASSPHRASE = "mysuperstrongpassword";
private static final int IV_LENGTH = 16;
private static final int AES_KEY_LENGTH = 16;
private static final int MAC_KEY_LENGTH = 16;
private static final int MAC_LENGTH = 20;
private static final int ITERATION_COUNT = 4096;
private static final String AES = "AES";
private static final String CIPHER_ALGORITHM = "AES/CFB/NoPadding";
private static final String SECRET_KEY_ALGORITHM = "PBKDF2WithHmacSHA1";
private static final String MAC_ALGORITHM = "HmacSHA1";

public static void main(String[] args) throws Exception {
    Cipher cipher = Cipher.getInstance(CIPHER_ALGORITHM);
    SecureRandom sr = new SecureRandom();
    byte[] salt = new byte[16];
    sr.nextBytes(salt);

    SecretKeyFactory factory = SecretKeyFactory.getInstance(SECRET_KEY_ALGORITHM);
    SecretKey secretKey = factory.generateSecret(new PBEKeySpec(PASSPHRASE.toCharArray(), salt, ITERATION_COUNT, 256));
    byte[] secretBytes = secretKey.getEncoded();

    byte[] iv = new byte[16];
    sr.nextBytes(iv);
    cipher.init(Cipher.ENCRYPT_MODE, new SecretKeySpec(secretBytes, 0, AES_KEY_LENGTH, AES), new IvParameterSpec(iv));
    byte[] encrypted = cipher.doFinal(CONTENT.getBytes("UTF-8"));
    byte[] result = concatArrays(iv, encrypted);

    byte[] macResult = getMAC(secretBytes, result);
    result = concatArrays(macResult, result);

    System.arraycopy(result, 0, macResult, 0, MAC_LENGTH);
    System.arraycopy(result, MAC_LENGTH, iv, 0, IV_LENGTH);
    System.arraycopy(result, MAC_LENGTH + IV_LENGTH, encrypted, 0, result.length - IV_LENGTH - MAC_LENGTH);

    if (!Arrays.equals(getDigest(getMAC(secretBytes, concatArrays(iv, encrypted))), getDigest(macResult))) {
        System.out.println("Invalid MAC");
    }
    cipher.init(Cipher.DECRYPT_MODE, new SecretKeySpec(secretBytes, 0, AES_KEY_LENGTH, AES), new IvParameterSpec(iv));
    byte[] decrypted = cipher.doFinal(encrypted);
    System.out.println(new String(decrypted, "UTF-8"));
}

private static byte[] getDigest(byte[] mac) throws Exception {
    MessageDigest digest = MessageDigest.getInstance("SHA1");
    return digest.digest(mac);
}

private static byte[] getMAC(byte[] secretBytes, byte[] data) throws Exception {
    Mac mac = Mac.getInstance(MAC_ALGORITHM);
    mac.init(new SecretKeySpec(secretBytes, AES_KEY_LENGTH, MAC_KEY_LENGTH, MAC_ALGORITHM));
    return mac.doFinal(data);
}

private static byte[] concatArrays(byte[] first, byte[] second) {
    byte[] ret = new byte[first.length + second.length];
    System.arraycopy(first, 0, ret, 0, first.length);
    System.arraycopy(second, 0, ret, first.length, second.length);
    return ret;
}
}

The plan will be to generate the salt installation time, and then it will remain the same for all encryption/decryption operations. I'm assuming that this should provide good enough protection against rainbow table attacks.

UPDATE 2: I had to realize that my MAC verification code wasn't quite optimal: the MAC already was SHA-1 hashed, so there was no point in creating a yet another SHA1 digest. I've also adjusted the MAC verification so it no longer uses Arrays.equals as that is vulnerable against timing attacks.

Solution

You should always use a key stretching algorithm when taking user input for a key such as a typed password. The key stretching does a few nice things. First it redistributes the entropy of your key (SHA1 does this as well) making the key appear more random (it isn't actually more random, the entropy remains the same), and second it makes brute forcing the key more computationally intensive (increasing with iterations obviously). The use of a random salt also precludes using precomputed lookup tables.

You should absolutely use a standard algorithm for this such as PBKDF2. In Java you can get a key factory for this via SecretKeyFactory.getInstance("PBKDF2WithHmacSHA1");

If your storing encrypted data in an environment you do not control you should also generate a MAC on your IV+Ciphertext and store it with your ciphertext. You can prepend it in the clear similar to the way you are storing the IV. Verify the MAC before decryption, you should verify indirectly by hashing the MAC first (a simple SHA1 works here) so as not to create a timing attack vector.

A MAC algorithm such as HMACSHA1 requires a secret key similar to a cipher. You should not use the same key for encrypting and generating the MAC. You can use the key stretching algorithm to generate a long enough key that you can use part for your cipher and part for your MAC.

ADDENDUM: If you are using Java 7 (or an external JCA provider that supports it) include a MAC with your AES cipher by using GCM mode. AES in GCM mode is a form of authenticated encryption that validates integrity as part of cipher. Implementing MAC generation and validation has various pitfalls that need to be avoided (such as the timing attack I mentioned or using separate keys) and rolling it in to the cipher is one less thing to screw up.

Creating secure crypto systems is not a trivial task, there are lots of ways to screw it up and make the entire process insecure. Instead of creating your own crypto system by putting together various crypto primitives it is generally a better idea to use a higher level library to handle things like cookie encryption and data storage or SSL/TLS for data in transit.

Comments

[ntoskrnl]

The MAC is critically important for cookie encryption. You could also consider an authenticated encryption mode such as GCM instead of HMAC, as it's faster and easier.

[Dev]

@ntoskrnl Excellent point, I added it to my answer. AEAD modes becoming a standard part of the Java library is an excellent advancement. I also forgot to include my usual warning about rolling ones own crytpo systems.

[Peter Major]

Thanks for the feedback, there is one point though that I don't quite understand yet: how to verify MAC indirectly before decryption? (unfortunately this code needs to work with Java 6, so I'm not sure I can use GCM yet, nonetheless I will read up on it as well, thanks for the pointer!)

[Dev]

@PeterMajor Don't compare MACs directly for equality, hash them first then compare equality of the hashes.

[Peter Major]

Thanks, but then the next question is: if I check the integrity first and return early without decrypting the known to be non-safe content, isn't that vulnerable for timing attacks? I should do the decryption regardless of the MAC verification result, but then ignore the results, right?

[Dev]

Generally speaking taking a different amount of time to do anything in a crypto system leaks information about its internal state. However, knowing that the MAC passed/failed and knowing how much of it was valid before it failed are of vastly different utility. I'd say you'd probably be ok, but I don't design crypto systems for a living.

[Peter Major]

@Dev thanks for the help, I've updated my initial post with your recommendations. Let me know if I've missed something.