3
\$\begingroup\$

I am creating one library for encryption/decryption using AES-256 with GCM Mode(With Random IV/Random Salt) for every request.

Code I have written is(reference : AES Encryption/Decryption with key) :

    public class AESGCMChanges {

    static String plainText1 = "DEMO text to be encrypted @1234";
    static String plainText2 = "999999999999";
    
   
    public static final int AES_KEY_SIZE = 256;
    public static final int GCM_IV_LENGTH = 12;
    public static final int GCM_TAG_LENGTH = 16;
    public static final int GCM_SALT_LENGTH = 32;
    
    private static final String FACTORY_ALGORITHM = "PBKDF2WithHmacSHA256";
    private static final String KEY_ALGORITHM = "AES";
    private static final int KEYSPEC_ITERATION_COUNT = 65536;
    private static final int KEYSPEC_LENGTH = 256;
    
    private static final String dataKey = "demoKey";

    public static void main(String[] args) throws Exception
    {
     
        byte[] cipherText = encrypt(plainText1.getBytes());
       
        String decryptedText = decrypt(cipherText);
        System.out.println("DeCrypted Text : " + decryptedText);
        
        cipherText = encrypt(plainText2.getBytes());
        
        decryptedText = decrypt(cipherText);
        System.out.println("DeCrypted Text : " + decryptedText);
    }

    public static byte[] encrypt(byte[] plaintext) throws Exception
    {
        byte[] IV = new byte[GCM_IV_LENGTH];
        SecureRandom random = new SecureRandom();
        random.nextBytes(IV);
        
        // Get Cipher Instance
        Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding");
        
        byte[] salt = generateSalt();
        
        // Generate Key
        SecretKey key = getDefaultSecretKey(dataKey, salt);
    
        
        // Create GCMParameterSpec
        GCMParameterSpec gcmParameterSpec = new GCMParameterSpec(GCM_TAG_LENGTH * 8, IV);
        
        // Initialize Cipher for ENCRYPT_MODE
        cipher.init(Cipher.ENCRYPT_MODE, key, gcmParameterSpec);
        
        // Perform Encryption
        byte[] cipherText = cipher.doFinal(plaintext);
        byte[] message = new byte[GCM_SALT_LENGTH + GCM_IV_LENGTH + plaintext.length + GCM_TAG_LENGTH];
        
        System.arraycopy(salt, 0, message, 0, GCM_SALT_LENGTH);
        System.arraycopy(IV, 0, message, GCM_SALT_LENGTH, GCM_IV_LENGTH);
        System.arraycopy(cipherText, 0, message, GCM_SALT_LENGTH+GCM_IV_LENGTH, cipherText.length);
     
        
        return message;
    }

    public static String decrypt(byte[] cipherText) throws Exception
    {

        if (cipherText.length < GCM_IV_LENGTH + GCM_TAG_LENGTH + GCM_SALT_LENGTH) throw new IllegalArgumentException();
        ByteBuffer buffer = ByteBuffer.wrap(cipherText);
    
        // Get Salt from Cipher
        byte[] salt = new byte[GCM_SALT_LENGTH];
        buffer.get(salt, 0, salt.length);
        
        // GET IV from cipher
        byte[] ivBytes1 = new byte[GCM_IV_LENGTH];
        buffer.get(ivBytes1, 0, ivBytes1.length);
        
        byte[] encryptedTextBytes = new byte[buffer.capacity() - salt.length - ivBytes1.length];
        buffer.get(encryptedTextBytes);
        
        // Get Cipher Instance
        Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding");
        
        // Generate Key
        SecretKey key = getDefaultSecretKey(dataKey, salt);
    
        // Create GCMParameterSpec
        GCMParameterSpec gcmParameterSpec = new GCMParameterSpec(GCM_TAG_LENGTH * 8, ivBytes1);
        
        // Initialize Cipher for DECRYPT_MODE
        cipher.init(Cipher.DECRYPT_MODE, key, gcmParameterSpec);
        
        // Perform Decryption
        byte[] decryptedText = cipher.doFinal(encryptedTextBytes);
        
        return new String(decryptedText);
    }
    
   

    
    private static SecretKey getDefaultSecretKey(final String password, final byte[] salt) throws NoSuchAlgorithmException, InvalidKeySpecException{
        return getSecretKey(password, salt, FACTORY_ALGORITHM, KEY_ALGORITHM, KEYSPEC_ITERATION_COUNT, KEYSPEC_LENGTH);
    }

    private static SecretKey getSecretKey(final String password, 
            final byte[] salt, 
            final String factoryAlgorithm, 
            final String keyAlgorithm, 
            final int iterationCount, 
            final int keyLength) throws NoSuchAlgorithmException, InvalidKeySpecException{
        SecretKeyFactory factory = SecretKeyFactory.getInstance(factoryAlgorithm);
        return new SecretKeySpec(factory.generateSecret(new PBEKeySpec(password.toCharArray(), salt, iterationCount, keyLength)).getEncoded(), keyAlgorithm); //thanks alot for the bug report
    }
    
    private static byte[] generateSalt()
    {
        final Random r = new SecureRandom();
        byte[] salt = new byte[32];
        r.nextBytes(salt);

        return salt;
    }
    
    

}

Now my question is:

  • Does it have any security flaws?
  • Does it follow best practices?
  • Can I do something to improve it?

All the lengths that I have taken for SALT, IV, Authentication tag are they OK? or they needs to be changed.

\$\endgroup\$
0
5
\$\begingroup\$

Protocol

Well, first the good news. I don't see anything particularly wrong with the algorithms or parameters used.

Class design

This is a badly designed class with a lot of copy / paste going on (and I've found a clear indication for that at the end, where you copy code directly from this site). I'm not a big fan of static classes, and this one doesn't lend itself particularly well for it. For instance, you don't want to use a separate password for each call, and you certainly don't want to derive a key from the same password multiple times.

By line code review

public class AESGCMChanges {

That's not a good name for a class. I presume this is testing only though.


public static final int GCM_IV_LENGTH = 12;

Some things are called _SIZE and others _LENGTH. It might be that one is in bits and the other one in bytes, but if you mix them I would indicate it in the name of the constants. Generally crypto-algorithm specifications define IV and tag sizes in bits, so probably best keep to that (and divide by Byte.SIZE where needed).

The constants have the correct sizes assigned to them though, although a salt of 32 bytes / 256 bits may be a bit overkill: 128 bits is a plenty.


private static final String FACTORY_ALGORITHM = "PBKDF2WithHmacSHA256";

Nope, that name doesn't do it for me. It's not a (generic?) factory you are naming, it is a name - and hash configuration - of the Password Based Key Derivation Algorithm or PBKDF.


private static final String dataKey = "demoKey";

For testing purposes it could be an idea to make such a key a constant in a testing class, but here it is really unwanted. The naming is incorrect as well, you'd expect all uppercase for constants.

Besides that, it's not the "data key" nor is it even a "key", it's a password or passphrase.


    byte[] cipherText = encrypt(plainText1.getBytes());

Always indicate the character encoding or you may see changes. Generally, I'd default to StandardCharsets.UTF_8 on Java (if you use the string directly then you will get an annoying checked exception to handle, something you can do without).


public static byte[] encrypt(byte[] plaintext) throws Exception

This is not a good method signature. I'd at least expect a password within it (as long as you keep to the current design anyway). What is good is that the plaintext and ciphertext is specified in bytes.

The exception handling is not well worked out; just throwing Exception is as bad as a catch-all. For a good idea of how to handle Java crypto-exceptions take a look here.

What about creating a class that has e.g. the number of iterations as configuration option, then initializes using the password and then allows for a set of plaintexts to be encrypted / decrypted?

Currently you don't allow any associated data for GCM mode. GCM mode specifies authenticated encryption with associated data or AEAD cipher. Not including associated data is not wrong, but it could be a consideration.


byte[] IV = new byte[GCM_IV_LENGTH];
SecureRandom random = new SecureRandom();
random.nextBytes(IV);

Normally I would not comment on this as it is not wrong or anything like that. It shows good use of the secure random class. However, I think it is not very symmetric with generateSalt; why not create a method for the IV as well?


SecretKey key = getDefaultSecretKey(dataKey, salt);

A "default" secret key? What's that? This is the key that's going to encrypt the data, right? Shouldn't this be called the "data key" in that case? We've already established that the other key is really a password. Besides that, I would not call a method that performs a deliberately heavy operation such as password based key derivation a "getter" either. It should be named e.g. deriveDataKey or something similar.

Missing from the call is the work factor / iteration count. I would certainly make that configurable and possibly store it with the ciphertext. You should use the highest number you can afford for the iteration count, and that number is going to shift to higher values in the future. Or so it should anyway.


System.arraycopy(cipherText, 0, message, GCM_SALT_LENGTH+GCM_IV_LENGTH, cipherText.length);

The salt and IV are relatively small, so buffering them in a separate array. However, Java has specific methods of writing data to an existing array using ByteBuffer. If your data is not that big then I would be OK with not using multiple update calls or streaming the data. I would however not recommend duplicating the ciphertext using System.arrayCopy.

And it is strange that this has not been implemented using ByteBuffer considering that you have used it for the decrypt method (again, the more symmetry the better).


return new SecretKeySpec(factory.generateSecret(new PBEKeySpec(password.toCharArray(), salt, iterationCount, keyLength)).getEncoded(), keyAlgorithm); //thanks alot for the bug report

The reason that the password is seen as character array is that you can destroy it's contents after you've handled it, in this case created a key from it. Array contents can be destroyed in Java (with reasonable certainty), String values cannot. So using password.toCharArray() here doesn't let you do this.

//thanks alot for the bug report

What bug report? What's the point of having a "thank you" here? Why not include a link if you decide to include such a comment?

In this case it seems you copied a small method without attribution.

This also shows the dangers of having end of line comments; they are easily missed. They are even less visible if too much is going on in that single line - as is the case here.

\$\endgroup\$
3
  • \$\begingroup\$ Thanks Maarten for a detailed look at it. I have one doubt though. You mentioned its not a data key/ not even key. Now say I am generating the data key using AWS KMS. Now do I need to use PBKDF or can I use it directly to encrypt the data. I do missed giving attribution, added it now \$\endgroup\$ Oct 18 '20 at 8:33
  • \$\begingroup\$ @AnkitBansal As an additional point to the "thanks" comment, there is no point in keeping history in the code. That's what a version control system is supposed to do. Keeping stuff like "this used to be an error", "there used to be a bug here", "this was added with bla" in the code is completely meaningless, especially if the comment does not give any further context. "We're using y instead of x because blablabla" is a good comment, especially when the first instinct would be to use x instead of y. But otherwise, I would not bother to annotate fixed bugs in the code in any way. \$\endgroup\$
    – Bobby
    Oct 18 '20 at 9:02
  • 1
    \$\begingroup\$ You can use AWS KMS for this, but the question is how you plan to use the keys. It would make more sense to me to use a single key and possibly derive the data keys from it. However, in that case you certainly don't need a PBKDF. A key based key derivation mechanism (KBKDF) makes more sense. However, I don't know if those are implemented for AWS. KDF's are often the last algorithms to be implemented, while they are pretty useful for key management - which what this topic is about. Directly using the AWS keys is not wrong or anything. Beware that GCM does have a per key message limit though \$\endgroup\$ Oct 18 '20 at 10:32

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.