RFR: 8255557: Decouple GCM from CipherCore [v4]
Valerie Peng
valeriep at openjdk.java.net
Fri May 21 02:54:52 UTC 2021
On Wed, 19 May 2021 20:21:23 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:
>
> Fix perf problem by reorganizing doLastBlock()
Here are comments on the remaining src changes. Will continue looking at the tests.
Thanks,
Valerie
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 628:
> 626: }
> 627:
> 628: int mergeBlock(byte[] buffer, int bufOfs, byte[] in, int inOfs,
Can be made 'static'? No javadoc for this one, maybe move the javadoc for the other one up and use "Methods" in the javadoc to cover both?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 637:
> 635: * The method takes two buffers to create one block of data
> 636: *
> 637: * This in only called when buffer length is less that a blockSize
nit: typo in->is, that->than
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 640:
> 638: * @return number of bytes used from 'in'
> 639: */
> 640: int mergeBlock(byte[] buffer, int bufOfs, int bufLen, byte[] in,
Can be made 'static'?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 648:
> 646:
> 647: System.arraycopy(buffer, bufOfs, block, 0, bufLen);
> 648: int inUsed = Math.min(block.length - bufLen,
Seems equivalent to `int inUsed = Math.min((block.length - bufLen), inLen);`?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 741:
> 739: } else {
> 740: // If the remaining in buffer + data does not fill a
> 741: // block, complete the gctr operation
This comment seems more suited for the else block below at line 753. In addition, the criteria for completing the gctr operation should be whether src still has remaining bytes left. It's possible that the (src.remaining() == blockSize - over) and happen to fill up the block, right? The current flow for this particular case would be an op.update(...) then continue down to line 770-778, maybe you can adjust the if-check on line748 so this would go through line 754-759 and return, i.e. the else block?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 752:
> 750: if (dst != null) {
> 751: dst.put(block, 0, blockSize);
> 752: }
not counting this blockSize value into "processed"?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 752:
> 750: if (dst != null) {
> 751: dst.put(block, 0, blockSize);
> 752: }
Why not just do `op.update(block, 0, blockSize, dst); `?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 757:
> 755: if (dst != null) {
> 756: dst.put(block, 0, Math.min(block.length, len));
> 757: }
Would this method work correctly if dst is null? Shouldn't this be checked in the beginning of this method?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 805:
> 803: /**
> 804: * This segments large data into smaller chunks so hotspot will start
> 805: * using CTR and GHASH intrinsics sooner. This is a problem for app
nit: typo: CTR->GCTR? This is to improve performance rather than a problem, no? Same comment applies to the other throttleData() method above.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 874:
> 872: } else if (!src.isDirect() && !dst.isDirect()) {
> 873: // if src is read only, then we need a copy
> 874: if (!src.isReadOnly()) {
Do you mean if (src.hasArray() && dst.hasArray())? Even if src is not read only, but array()/arrayOffset() methods are optional and can only be safely invoked after the hasArray() call.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 908:
> 906: * If an intermediate array is needed, the whole original out array
> 907: * length is allocated because it's simpler than keep the same offset
> 908: * and hope the expected output is
The comment seems incomplete? Perhaps you mean "if an intermediate array is needed, it will be allocated using the whole original out array length, so that the same out array offset can be used for code simplicity."
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 942:
> 940:
> 941: System.arraycopy(out, originalOutOfs, originalOut, originalOutOfs,
> 942: len);
Don't you need to do `originalOut = null;` after copying the bytes over? Otherwise, if you have two overlapping calls with the same engine, the 2nd restoreOut(...) call may lead to data corruption, i.e. it will duplicate the output bytes to the original output buffer (in the 1st overlapping call).
Same applies for the ByteBuffer case, i.e. restoreDst(...).
If time permits, please add a regression test to cover this.
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...
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1014:
> 1012: len += gctrghash.update(buffer, 0, bLen, out, outOfs);
> 1013: outOfs += bLen;
> 1014: }
For encryption, would the ibuffer contain more than one blocksize of data? Isn't the existing impl only put the remaining input (less than a block) into it? Line 1013: the `outOfs += bLen;`, shouldn't 'bLen' be 'len'?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1016:
> 1014: }
> 1015:
> 1016: // blen is now the offset for 'buffer'
nit: typo, blen->bLen
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'?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1033:
> 1031: ibuffer.reset();
> 1032: // Code below will write the remainder from 'in' to ibuffer
> 1033: } else if (remainder > 0) {
If bLen == 0, there is no need to put the rest of 'buffer' into 'ibuffer'.
It looks strange that the remaining buffer data is stored back into 'ibuffer', then the code proceeds to encrypt 'in' from line 1043-1046. This looks incorrect as all prior buffered input should be processed before process current input.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1036:
> 1034: // If a block or more was encrypted from 'buffer' only, but
> 1035: // the rest of 'buffer' with 'in' could not construct a
> 1036: // block, then put the rest of 'buffer' back into ibuffer.
If 'ibuffer' is always less than a block long, then some of these are dead code and can be trimmed. Same goes for the doUpdate() w/ ByteBuffer arguments.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1044:
> 1042:
> 1043: // Encrypt the remaining blocks inside of 'in'
> 1044: if (inLen > 0) {
If encrypting the remaining blocks, should use (inLen >= blockSize)?
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.
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.
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?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1184:
> 1182: block = new byte[bufLen + inLen];
> 1183: System.arraycopy(buffer, 0, block, 0, bufLen);
> 1184: System.arraycopy(in, inOfs, block, bufLen, inLen);
Not needed if inLen == 0.
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?
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?
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'?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1412:
> 1410: }
> 1411:
> 1412: checkDataLength(len - tagLenBytes);
The impl of checkDataLength(...) is based on the "processed" variable which is always 0 for doUpdate() calls. This suits encryption better, but does not suit decryption? It seems that the doUpdate() calls would just keep buffering the data into 'ibuffer' without checking the size. It seems to me that this checkDataLength() call on line 1412 would always pass.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1437:
> 1435: } catch (ArrayIndexOutOfBoundsException aiobe) {
> 1436: throw new ShortBufferException("Output buffer invalid");
> 1437: }
I think this should be moved to the very beginning before all the processing and if the output capacity is less than 'len-tagLenBytes' value, then no need to proceed? IIRC, the save/restore is more for algorithms which use padding, may not be needed for GCM?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1442:
> 1440: throw new ShortBufferException("Output buffer too small, must" +
> 1441: "be at least " + (len - tagLenBytes) + " bytes long");
> 1442: }
This looks like a half-baked save/restore functionality?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1475:
> 1473:
> 1474: // Length of the input
> 1475: ByteBuffer tag;
Is the comment obsolete? I don't quite follow how it's related to 'tag'.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1503:
> 1501: tag.put(ct);
> 1502: tag.flip();
> 1503: // Limit is how much of the ibuffer has been chopped off.
The comment seems incorrect? Limit is not how much the ibuffer has been chopped off, but rather what has not been chopped off, no?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1510:
> 1508:
> 1509: // 'len' contains the length in ibuffer and src
> 1510: checkDataLength(len);
Is this really useful given 'processed' is 0 and there is only one argument 'len'. Should always pass?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1515:
> 1513: // the dst buffer is too short. Context will be restored so the
> 1514: // method can be called again with the proper sized dst buffer.
> 1515: if (len > dst.remaining()) {
We should check the dst capacity in the beginning of the method, if its capacity is too small, i.e. less than the overall length - tag length, don't proceed further.
As in the doFinal() w/ byte[] arguments, I am not sure if the save/restore is really necessary.
Maybe add a test if it's not already covered by existing tests if time permits.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1557:
> 1555:
> 1556: // Decrypt the all the input data and put it into dst
> 1557: //decryptBlocks(buffer, ct, dst);
nit: remove obsolete commented out code?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1610:
> 1608: // update the input parameters for what was taken out of 'in'
> 1609: inOfs += inUsed;
> 1610: inLen -= inUsed;
This merge block code won't be needed if inLen == 0, i.e. can just assign in to be buffer, inOfs to 0, and inLen to bufRemaining.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1615:
> 1613: if (inLen > 0) {
> 1614: int resultLen;
> 1615: resultLen = op.update(block, 0, blockSize,
nit: can just do `int resultLen = op.update(...);`
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1665:
> 1663: /**
> 1664: * This class is for encryption operations when both GCTR and GHASH
> 1665: * can operation in parallel.
nit: typo "operation"->"operate"
src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java line 255:
> 253: attrs.put("SupportedKeyFormats", "RAW");
> 254: ps("Cipher", "DESedeWrap",
> 255: "com.sun.crypto.provider.DESedeWrapCipher", null, attrs);
fix indentation to be consistent?
src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java line 264:
> 262: "com.sun.crypto.provider.ARCFOURCipher", attrs);
> 263: ps("Cipher", "AESWrap", "com.sun.crypto.provider.AESWrapCipher$General",
> 264: null, attrs);
fix indentation to be consistent?
-------------
PR: https://git.openjdk.java.net/jdk/pull/4072
More information about the security-dev
mailing list