AES Encryption/Decryption

Question

Where I currently work I'm tasked with writing a good encryption and decryption code which will have DEK as well as KEK. Haven't started on KEK yet. I have already told them, that we need expert help, but I don't think I am going to get any. Any tips on KEK how to generate them and where to store them? (HSM ?) would be appreciated.

After researching over the week I've written this code below.

Looking for feedback on what is wrong in the code below and how it can be fixed. Also, I'm using a GUID as authentication tag which is binding to a user to whom the information belongs. The encrypted text and tag is stored in separate database at separate location. Am I on the right path? if not please show me.

import java.io.File;
import java.io.FileInputStream;
import java.io.FileOutputStream;
import java.io.IOException;
import java.nio.charset.Charset;
import java.security.GeneralSecurityException;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.util.Base64;

import javax.crypto.Cipher;
import javax.crypto.KeyGenerator;
import javax.crypto.SecretKey;
import javax.crypto.spec.GCMParameterSpec;
import javax.crypto.spec.SecretKeySpec;

public enum AES {

    INSTANCE;

    private static final String CIPHER_ALGORITHM = "AES";
    private static final String ALGORITHM_MODE_PADDING = "AES/GCM/NoPadding";

    private static final int IV_LENGTH_BYTES = 16;
    private static final int AES_KEY_SIZE = 128;
    private static final int GCM_TAG_LENGTH = 128;

    public String encrypt(String plainText, String authenticationTag) throws GeneralSecurityException, IOException {
        createKeyFile(authenticationTag);
        Cipher cipher = Cipher.getInstance(ALGORITHM_MODE_PADDING);
        final int blockSize = cipher.getBlockSize();
        SecretKey secretKey = readKeyFile(authenticationTag);
        byte[] plainTextByte = plainText.trim().getBytes(Charset.forName("UTF-8"));
        byte[] ivBytes = generateIV();
        cipher.init(Cipher.ENCRYPT_MODE, secretKey, new GCMParameterSpec(GCM_TAG_LENGTH, ivBytes));
        cipher.updateAAD(authenticationTag.getBytes(Charset.forName("UTF-8")));
        byte[] encryptedMessage = cipher.doFinal(plainTextByte);
        byte iv_and_encryptedMessage[] = concatIvAndCipher(ivBytes, encryptedMessage, blockSize);
        Base64.Encoder encoder = Base64.getEncoder().withoutPadding();
        String iv_and_encryptedText = encoder.encodeToString(iv_and_encryptedMessage);
        return iv_and_encryptedText;
    }

    public String decrypt(String iv_and_encryptedTextBase64, String authenticationTag)
            throws GeneralSecurityException, IOException {
        Cipher cipher = Cipher.getInstance(ALGORITHM_MODE_PADDING);
        final int blockSize = cipher.getBlockSize();
        Base64.Decoder decoder = Base64.getDecoder();
        byte[] iv_and_encryptedMessage = decoder.decode(iv_and_encryptedTextBase64.trim());
        byte[] ivBytes = extractIvFrom(iv_and_encryptedMessage, blockSize);
        byte[] encryptedTextBytes = extractEncryptedMessageFrom(iv_and_encryptedMessage, blockSize);
        SecretKey secretKey = readKeyFile(authenticationTag);
        cipher.init(Cipher.DECRYPT_MODE, secretKey, new GCMParameterSpec(GCM_TAG_LENGTH, ivBytes));
        cipher.updateAAD(authenticationTag.getBytes(Charset.forName("UTF-8")));
        byte[] decryptedByte = cipher.doFinal(encryptedTextBytes);
        String decryptedMessage = new String(decryptedByte, Charset.forName("UTF-8"));
        return decryptedMessage;
    }

    private SecretKey readKeyFile(String tag) {

    }

    private void createKeyFile(String tag) {

    }

    public byte[] generateIV() throws NoSuchAlgorithmException {
        SecureRandom random = new SecureRandom();
        byte[] ivBytes = new byte[IV_LENGTH_BYTES];
        random.nextBytes(ivBytes);
        return ivBytes;
    }

    public byte[] concatIvAndCipher(byte[] iv, byte[] cipher, int blockSize) {
        final byte[] ivAndEncryptedMessage = new byte[iv.length + cipher.length];
        System.arraycopy(iv, 0, ivAndEncryptedMessage, 0, blockSize);
        System.arraycopy(cipher, 0, ivAndEncryptedMessage, blockSize, cipher.length);
        return ivAndEncryptedMessage;
    }

    public byte[] extractIvFrom(byte[] iv_and_encryptedMessage, int blockSize) {
        byte[] ivBytes = new byte[blockSize];
        System.arraycopy(iv_and_encryptedMessage, 0, ivBytes, 0, blockSize);
        return ivBytes;
    }

    public byte[] extractEncryptedMessageFrom(byte[] iv_and_encryptedMessage, int blockSize) {
        byte[] encryptedTextBytes = new byte[iv_and_encryptedMessage.length - blockSize];
        System.arraycopy(iv_and_encryptedMessage, blockSize, encryptedTextBytes, 0, encryptedTextBytes.length);
        return encryptedTextBytes;
    }
}

Answer 1

I won't say anything about the cryptographic aspects (as in, what I see looks okay to me (but I too am not an expert, take it to IT Security please, or actually pay someone for it), but there's no indication except for the HSM bit what this is going to be used for).

• I guess readKeyFile and createKeyFile were left out as irrelevant.
• At least the extractIvFrom and extractEncryptedMessageFrom methods aren't necessary, take a look at the other constructors for GCMParameterSpec and the other methods for doFinal which allow specifying the offset and length of the part of the array to use. Concatenating things is also a bit wasteful, but I guess punting on precalculating the necessary buffer size is fine for the moment.
• Some values are repeatedly used, just move them into constants, like the Charset value, or, even better, use java.nio.charset.StandardCharsets.UTF_8 instead.
• In terms of API I probably wouldn't use an actual String for the plain text as that's quite limiting in what can be passed in, but if that's the use case, sure.
• There are random finals in the methods that are inconsistent with the fact that nothing else is marked final.
• Finally, you'll probably hear different advice about singletons, though I find that it will probably not help too much with testing if everything refers to a single instance of this. At least it's thread-safe, so that's something, but that could've been achieved with static methods too.