On Wed, 2 Jun 2021 03:26:03 GMT, Anthony Scarpino <ascarp...@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
