On Tue, 18 May 2021 03:18:18 GMT, Anthony Scarpino <ascarp...@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
