RFR: 8255557: Decouple GCM from CipherCore [v3]

[Valerie Peng]

On Tue, 18 May 2021 03:18:18 GMT, Anthony Scarpino <ascarpino at openjdk.org> wrote:

>> Hi,
>> 
>> I need a review of this rather large change to GCM.  GCM will no longer use CipherCore, and AESCrypt  to handle it's buffers and other objects.  It is also a major code redesign limits the amount of data copies and make some performance-based decisions.
>> 
>> Thanks
>> 
>> Tony
>
> Anthony Scarpino has updated the pull request incrementally with one additional commit since the last revision:
> 
>   cleanup

src/java.base/share/classes/com/sun/crypto/provider/GHASH.java line 181:

> 179: 
> 180:     // Will process as many blocks it can and will leave the remaining.
> 181:     int update(ByteBuffer ct, int inLen) {

Throughout this method, comments still have "src", but the variable is renamed to "ct". Fix this inconsistency?

src/java.base/share/classes/com/sun/crypto/provider/GHASH.java line 187:

> 185:             int processed = inLen - (inLen % AES_BLOCK_SIZE);
> 186:             processBlocksDirect(ct, inLen);
> 187:             ct.position(ct.position());

ct.position(ct.position())? looks redundant?

src/java.base/share/classes/com/sun/crypto/provider/GHASH.java line 189:

> 187:             ct.position(ct.position());
> 188:             return processed;
> 189:         } else if (!ct.isReadOnly()) {

Maybe you meant "ct.hasArray()" instead of "!ct.isReadOnly()"?

src/java.base/share/classes/com/sun/crypto/provider/GHASH.java line 202:

> 200:         if (inLen == 0) {
> 201:             return 0;
> 202:         }

Seems that this should be moved to the very beginning of this method, i.e. before the if-check on line 184?

src/java.base/share/classes/com/sun/crypto/provider/GHASH.java line 338:

> 336:     public int doFinal(ByteBuffer src, ByteBuffer dst) {
> 337:         return doFinal(src, src.remaining());
> 338:     }

Have you considered changing the argument list of existing update/doFinal(...) methods? Less calls.

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 75:

> 73:     private GCMEngine engine;
> 74: 
> 75:     private boolean encryption = true;

Move these non-static fields to where they belong?

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 92:

> 90: 
> 91:     private boolean initialized = false;
> 92:     // Default value is 128bits, this in stores bytes.

nit: "this in stores bytes" -> "this is in bytes"?

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 94:

> 92:     // Default value is 128bits, this in stores bytes.
> 93:     int tagLenBytes = 16;
> 94:     // Key size if the value is passed

nit: add info such as this is in bits or bytes to comment.

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 98:

> 96:     // Prevent reuse of iv or key
> 97:     boolean reInit = false;
> 98:     final static byte[] emptyBuf = new byte[0];

Move this to where the rest of static variables are? "final static" -> "static final". emptyBuf -> EMPTY_BUF or else?

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 133:

> 131:             throw new InvalidKeyException("The key must be " +
> 132:                 keySize + " bytes");
> 133:         }

Set the keyValue to all 0s before throwing exception, i.e. try-finally.

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 137:

> 135:         // Check for reuse
> 136:         if (encryption) {
> 137:             if (Arrays.compare(keyValue, lastKey) == 0 && Arrays.compare(iv,

Per the earlier impl, it uses MessageDigest.isEqual(...) instead of Arrays.compare(...).

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 144:

> 142: 
> 143:             // Both values are already clones
> 144:             lastKey = keyValue;

Per the earlier impl, it does a check-n-erase before this assignment, i.e.
`if (lastKey != null) { Arrays.fill(lastKey, (byte) 0); }`

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 149:

> 147:             if (spec == null) {
> 148:                 throw new InvalidKeyException("No GCMParameterSpec specified");
> 149:             }

This seems redundant as it's already been checked in engineInit() before calling this method. Line 154 directly calls spec.getTLen() without checking spec != null.

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 168:

> 166:     // return tag length in bytes
> 167:     int getTagLen() {
> 168:         return this.tagLenBytes;

Doesn't seem too useful if all it does is just returning the 'tagLenBytes' field? With your current code, tagLenBytes is initialized with 16 and is set in init(). When a GCMParameterSpec is not specified, it uses the tagLenBytes value from earlier init() instead of a fixed default. This seems incorrect?

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 199:

> 197:     @Override
> 198:     protected int engineGetKeySize(Key key) throws InvalidKeyException {
> 199:         return super.engineGetKeySize(key);

CipherSpi.engineGetKeySize(...) throws UnsupportedOperationException(). You will need to copy the impl from AESCipher.engineGetKeySize(...).

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 204:

> 202:     @Override
> 203:     protected byte[] engineGetIV() {
> 204:         return iv.clone();

Check for iv != null, otherwise NPE.

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 220:

> 218:             random.nextBytes(iv);
> 219:         } else {
> 220:             new SecureRandom().nextBytes(iv);

Consider using JCAUtil.getDefSecureRandom()?

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 225:

> 223:     }
> 224: 
> 225:     SecureRandom random = null;

Is this field used? I didn't find any assignment. If it's needed, can you please move it to where the rest of fields are?

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 230:

> 228:         GCMParameterSpec spec;
> 229:         spec = new GCMParameterSpec(getTagLen() * 8,
> 230:             iv == null ? createIv(random) : iv.clone());

Use default tag length if (iv == null)?

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 259:

> 257:     @Override
> 258:     protected void engineInit(int opmode, Key key,
> 259:         AlgorithmParameterSpec params, SecureRandom random)

The specified "random" is not saved to internal "random". Perhaps an oversight?

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 266:

> 264:         if (params == null) {
> 265:             iv = createIv(random);
> 266:             spec = new GCMParameterSpec(getTagLen() * 8, iv);

Same, use default tag length?

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 279:

> 277:             if (iv.length == 0) {
> 278:                 throw new InvalidAlgorithmParameterException("IV is empty");
> 279:             }

Why separating the validation of iv and tag length in separate methods, i.e. engineInit() vs init()? Consider consolidating them?

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 402:

> 400:         }
> 401:         try {
> 402:             ArrayUtil.nullAndBoundsCheck(input, inputOffset, inputLen);

Why is only this ArrayUtil.nullAndBoundsCheck(...) present in this engineDoFinal(...)? There are other engineUpdate/engineDoFinal() calls which also have input array, offset, and length. Shouldn't this check be added there as well? If the crypto engine check is separated out into a separate method, e.g. checkEngine(), then you don't have to explicitly release the crypto engine (as on line 405) and can just call checkEngine() after all the input validation has passed.

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 446:

> 444:         } finally {
> 445:             // Release crypto engine
> 446:             engine = null;

Should also do `Arrays.fill(encodedKey, (byte)0);`

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 468:

> 466:                 "The wrapped key does not have the correct length");
> 467:         }
> 468:         return ConstructKeys.constructKey(encodedKey, wrappedKeyAlgorithm,

should try-finally this call and do `Arrays.fill(encodedKey, (byte)0);`

-------------

PR: https://git.openjdk.java.net/jdk/pull/4072