RFR: 8255557: Decouple GCM from CipherCore [v4]

[Valerie Peng]

On Wed, 2 Jun 2021 03:26:03 GMT, Anthony Scarpino <ascarpino at openjdk.org> wrote:

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 975:
>> 
>>> 973:                 doUpdate(in, inOff, inLen, output, 0);
>>> 974:             } catch (ShortBufferException e) {
>>> 975:                 // update decryption has no output
>> 
>> The comment does not seems to make sense? This is GCMEncrypt class. The right reason should be that the output array is allocated by the provider and should have the right size. However, it seems safer to throw AssertionException() instead of swallowing the SBE...
>
> Yeah the comment isn't right.  I think it's excessive to throw an exception that should never happen, but I'll add a ProviderException.

Sure.

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1030:
>> 
>>> 1028:                     inOfs += inLenUsed;
>>> 1029:                     inLen -= inLenUsed;
>>> 1030:                     outOfs += blockSize;
>> 
>> 'blockSize' should be 'len'?
>
> Either is fine because len and blockSize will be the same.

Hmm, re-reading the code, I see why you chose to use blockSize since line 1055 uses "len += ..." so whether len==blockSize depends on the code in line 1022-1054.

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1055:
>> 
>>> 1053:                 // remainder offset is based on original buffer length
>>> 1054:                 ibuffer.write(in, inOfs + inLen, remainder);
>>> 1055:             }
>> 
>> I wonder if these update(byte[], int, int, byte[], int) calls (such as the one on line 1045) should take ibuffer and stores the unprocessed bytes into it. This way seems more robust and you won't need separate logic. Same comment for the doUpdate() taking ByteBuffer arguments.
>
> Do you mean having all the GCM interface implementations have an argument that takes ibuffer and adds any unprocessed data into?  Maybe it would save a copy of the code, but I'm not sure I like GCTR or GHASH adding data into the ibuffer.

Maybe there are other alternatives than making GCTR/GHASH to write into ibuffer. Or, perhaps we can figure out the right input length when passing them to GCTR or GHASH? This is again more for readability and robustness. What you have works, just need to be very careful and require detailed tracking/knowledge on GCTR/GHASH level since all inputs are passed down, but not all are used.

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1163:
>> 
>>> 1161:                     r = doUpdate(buffer, 0, bufLen, out, outOfs);
>>> 1162:                     bufLen -= r;
>>> 1163:                     inOfs += r;
>> 
>> Would bufLen >= blockSize? Regardless, 'inOfs' should not be touched as 'in' is not used in the above doUpdate() call.
>
> removing it

Ok.

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1174:
>> 
>>> 1172:                     inLen -= r;
>>> 1173:                     r = gctrghash.update(block, 0, blockSize, out,
>>> 1174:                         outOfs + resultLen);
>> 
>> I don't follow why you don't update the 'outOfs' after the line 1161 doUpdate() call and then add the resultLen when calling gctrhash.update(...) here. Seems fragile and difficult to maintain?
>
> i cleaned it up.. all the += or -+ are annoying, but not there is much i can do

Ok, will wait for the commit.

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1213:
>> 
>>> 1211: 
>>> 1212:             // copy the tag to the end of the buffer
>>> 1213:             System.arraycopy(block, 0, out, resultLen + outOfs, tagLenBytes);
>> 
>> Now that the tag is copied to the output, why not increment resultLen w/ tagLenBytes? This way, you don't have to keep repeating the (resultLen + tagLenBytes) for another two times?
>
> yeah, that got changed after this comment

Ok.

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1317:
>> 
>>> 1315:          * If tagOfs = 0, 'in' contains only the tag
>>> 1316:          * if tagOfs = blockSize, there is no data in 'in' and all the tag
>>> 1317:          *   is in ibuffer
>> 
>> Is this correct? mergeBlock() returns the number of used bytes from 'in', if no data is in 'in' and all the tag is from 'ibuffer', tagOfs should equal to -tagLenBytes. The next line should be moved up as the tag position gradually shifts from 'in' toward 'ibuffer'. The tagOfs for the next line should be -tagLenBytes < tagOfs < 0?
>
> Yeah, I reworkded it

Ok, will wait for the commit.

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1401:
>> 
>>> 1399:             ShortBufferException {
>>> 1400:             GHASH save = null;
>>> 1401: 
>> 
>> Should do ArrayUtil.nullAndBoundsCheck() on 'in'?
>
> that was done in engineDoFinal

Right. Would be nice to place the same checks at the same level for consistency.

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

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