2
\$\begingroup\$

I'm implementing a PAKE-protocol as a university project and one step of the protocol involves sending encrypted data from a key created via a hash function (I'm using SHA-256). I want to use AES/GCM but do not have an IV so I'm using HKDF to expand the key into an IV and key with the following implementation:

import org.bouncycastle.crypto.digests.SHA256Digest;
import org.bouncycastle.crypto.generators.HKDFBytesGenerator;
import org.bouncycastle.crypto.params.HKDFParameters;
import org.bouncycastle.util.Arrays;

import javax.crypto.BadPaddingException;
import javax.crypto.Cipher;
import javax.crypto.IllegalBlockSizeException;
import javax.crypto.NoSuchPaddingException;
import javax.crypto.spec.GCMParameterSpec;
import javax.crypto.spec.SecretKeySpec;
import java.security.InvalidAlgorithmParameterException;
import java.security.InvalidKeyException;
import java.security.NoSuchAlgorithmException;

public class AesGcmSymmetricCipher {

    public static final String CIPHER_ALGORITHM = "AES/GCM/NoPadding";

    public static final int GCM_TAG_BITS = 128;
    public static final int IV_BYTES = 12;
    public static final int KEY_BYTES = 32; // AES-256

    private final Cipher cipher;

    public AesGcmSymmetricCipher() throws NoSuchPaddingException, NoSuchAlgorithmException {
        this.cipher = Cipher.getInstance(CIPHER_ALGORITHM);
    }

    public byte[] encrypt(byte[] key, byte[] clearData) throws InvalidAlgorithmParameterException, InvalidKeyException, BadPaddingException, IllegalBlockSizeException {

        var spec = expandKeys(key);
        cipher.init(Cipher.ENCRYPT_MODE, spec.getKeySpec(), spec.getParameterSpec());

        return cipher.doFinal(clearData);
    }

    public byte[] decrypt(byte[] key, byte[] cipherData) throws InvalidAlgorithmParameterException, InvalidKeyException, BadPaddingException, IllegalBlockSizeException {

        var spec = expandKeys(key);
        cipher.init(Cipher.DECRYPT_MODE, spec.getKeySpec(), spec.getParameterSpec());

        return cipher.doFinal(cipherData);
    }

    public String getAlgorithm() {
        return "AES-256/GCM/NoPadding using HKDF";
    }

    private KeyedMaterial expandKeys(byte[] initialKey) {
        var iv = new byte[IV_BYTES];
        var key = new byte[KEY_BYTES];

        var digest = new SHA256Digest();
        var kdf = new HKDFBytesGenerator(digest);
        var parameters = new HKDFParameters(initialKey, null, null);

        kdf.init(parameters);
        kdf.generateBytes(iv, 0, IV_BYTES);
        kdf.generateBytes(key, 0, KEY_BYTES);

        var gcmParameters = new GCMParameterSpec(GCM_TAG_BITS, iv);
        var keySpec = new SecretKeySpec(key, CIPHER_ALGORITHM);

        Arrays.clear(iv);
        Arrays.clear(key);

        return new KeyedMaterial(gcmParameters, keySpec);
    }

    private static class KeyedMaterial {

        private final GCMParameterSpec parameterSpec;
        private final SecretKeySpec keySpec;

        public KeyedMaterial(GCMParameterSpec parameterSpec, SecretKeySpec keySpec) {
            this.parameterSpec = parameterSpec;
            this.keySpec = keySpec;
        }

        public GCMParameterSpec getParameterSpec() {
            return parameterSpec;
        }

        public SecretKeySpec getKeySpec() {
            return keySpec;
        }
    }
}

I'm interested in feedback especially from a security perspective, if relevant the initial key passed to encrypt/decrypt is ephemeral and will only be used once for encryption and once for decryption.

\$\endgroup\$
2
\$\begingroup\$

First of all, thank you for using my Bouncy Castle additions:

Maarten Bodewes initial implementation of HKDF and NIST SP 800-108 MAC based KDF functions.

Design review:

You are trying to emulate Java's Cipher class. However, I don't think this is such a good idea if your encrypt methods and is so much different. The Cipher class in Java is reusable for the same key (and, unfortunately, same IV) and this one is clearly not. I would not try this hard. Better make this class more specific to the use case and avoid generic wrapper classes.

In itself, you may wonder if combining the key derivation and cipher is a good idea. You're removing the update functionality, for instance. This is less of a problem if this is for a specific use case.

Code review:

public class AesGcmSymmetricCipher {

Better made final.

The name doesn't fully capture what the class is about. Symmetric is already captured by Aes. The key derivation part is missing on the other hand.

public static final int KEY_BYTES = 32; // AES-256

I generally use 256 / Byte.SIZE so it is clear where the 32 comes from, that way you don't need the comment.

public byte[] encrypt(byte[] key, byte[] clearData) throws InvalidAlgorithmParameterException, InvalidKeyException, BadPaddingException, IllegalBlockSizeException {

clearData is generally called plaintext or simply message within the crypto community.

Other users will probably wonder what happened with the IV or nonce and where to insert it.

Arrays.clear(iv);
Arrays.clear(key);

Beware that Oracle made a huge mistakes by making the actual keys in software impossible to destroy. You can do this and fool yourself. It's great that you thought of this though - you're not in the wrong here.

return "AES-256/GCM/NoPadding using HKDF";

Nice but it doesn't capture all the little details - how is the IV calculated, for instance - so it doesn't seem to be of much use. You cannot specify it to anybody and they will know how to implement it.


Otherwise the class and design seems spot on to me, so well done.

\$\endgroup\$
  • \$\begingroup\$ Just to stay fair: the crypto API is so old that it was probably Sun who made the design mistake, not Oracle. \$\endgroup\$ – Roland Illig Feb 21 '20 at 7:21
  • \$\begingroup\$ I'm aware that the Array.clear(...) does not really clear the keys (I use them after the clear) and wondered when SecreteKeySpec clears its private data or if I forgot to do this. \$\endgroup\$ – recke96 Feb 21 '20 at 10:54
  • \$\begingroup\$ "You're removing the update functionality, for instance" I actually tried to implement it with update and varargs, but then the tag-verification always failed and through some googleing I found that there is apparently something wrong if you don't use doFinal(data) in GCM mode. \$\endgroup\$ – recke96 Feb 21 '20 at 10:55
  • \$\begingroup\$ @RolandIllig I don't know, the Destroyable interface is a late addition that was only introduced in version 8 - it's relatively new. Of course, before that it was missing entirely, so there is that. \$\endgroup\$ – Maarten Bodewes Feb 21 '20 at 12:30
  • \$\begingroup\$ @recke96 Yeah, no, it is always multiple update() calls followed by a doFinal, even for other modes (CBC mode only removed padding within one of the overloaded doFinal methods for instance.. \$\endgroup\$ – Maarten Bodewes Feb 21 '20 at 12:34

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.