On Mon, 22 Mar 2021 21:43:31 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>> This change updates SunJCE provider as below:
>> - updated existing AESWrap support with AES/KW/NoPadding cipher
>> transformation.
>> - added support for AES/KWP/NoPadding and AES/KW/PKCS5Padding.
>>
>> Existing AESWrap impl, i.e. AESWrapCipher class, is re-factored and renamed
>> to KeyWrapCipher class. The W and W_inverse functions are moved to KWUtil
>> class. The KW and KWP support are in the new AESKeyWrap and AESKeyWrapPadded
>> classes which extend FeedbackCipher and used in KeyWrapCipher class. To
>> minimize data copying, AESKeyWrap and AESKeyWrapPadded will do the crypto
>> operation over the same input buffer which is allocated and managed by
>> KeyWrapCipher class.
>>
>> Also note that existing AESWrap impl does not take IV. However, the
>> corresponding PKCS#11 mechanisms do, so I added support for accepting IVs to
>> both KW and KWP.
>>
>> Thanks,
>> Valerie
>
> Valerie Peng has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Changed AlgorithmParameters impls to register under AES/KW/NoPadding and
> AES/KWP/NoPadding
src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrap.java line 41:
> 39: * and represents AES cipher in KW mode.
> 40: */
> 41: class AESKeyWrap extends FeedbackCipher {
I see lots of unsupported operations and `encrypt/decryptFinal` ignores their
output parameters. Should we be extending `FeedbackCipher` if so much doesn't
seem to quite fit?
src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrap.java line 155:
> 153: " be at least 16 bytes and multiples of 8");
> 154: }
> 155: // assert ptOfs == 0; ct == pt; ctOfs == 0;
Is this a missing code assertion?
src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrap.java line 193:
> 191: if (!Arrays.equals(ivOut, 0, ICV1.length, this.iv, 0,
> ICV1.length)) {
> 192: throw new IllegalBlockSizeException("Integrity check
> failed");
> 193: }
It feels like an odd asymmetry that we validate the IV upon decryption in
`AESKeyWrap` but the IV is prepended in `KeyWrapCipher.writeIvSemiBlock`. I'm
worried that by separating this logic like this it is easier for us to get it
wrong (or break it in the future).
src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrapPadded.java line
69:
> 67: if (!Arrays.equals(ivAndLen, 0, ICV2.length, icv, 0,
> ICV2.length)) {
> 68: throw new IllegalBlockSizeException("Integrity check failed");
> 69: }
While I cannot find any public discussion of this, I'm always uncomfortable
checking the plaintext (prior to authentication) against a known value in
non-constant time. I'm worried that this (and the equivalent in the unpadded
version) might be a problem in the future.
src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrapPadded.java line
246:
> 244: int outLen = validateIV(ivAndLen, this.iv);
> 245: // check padding bytes
> 246: int padLen = ctLen - outLen;
Can we add an explicit check that `0 <= padLen < SEMI_BLKSIZE`?
src/java.base/share/classes/com/sun/crypto/provider/KWUtil.java line 87:
> 85: */
> 86: static final void W_INV(byte[] in, int inLen, byte[] ivOut,
> 87: SymmetricCipher cipher) {
The asymmetry between `W` not taking an IV but `W_INV` returning an IV also
bothers me and seems to make this harder to use safely.
src/java.base/share/classes/com/sun/crypto/provider/KeyWrapCipher.java line 507:
> 505: } else {
> 506: outLen = cipher.encryptFinal(dataBuf, 0, dataIdx, null,
> -1);
> 507: }
Can we extract this shared logic (among the different versions of
`engineDoFinal`) into a separate helper method so that the different
`engineDoFinal` methods just need to handle their specific differences (such as
returning `Arrays.copyOf(dataBuf, outLen)` or calling
`System.arraycopy(dataBuf, 0, out, outOfs, outLen); return outLen;`).
src/java.base/share/classes/com/sun/crypto/provider/KeyWrapCipher.java line 660:
> 658: @Override
> 659: protected byte[] engineWrap(Key key)
> 660: throws IllegalBlockSizeException, InvalidKeyException {
Is it okay that we can call `cipher.init(Cipher.ENCRYPT_MODE, ...)` and then
use it with `cipher.wrap()`?
Also, can we simply delegate the heavy lifting to `engineDoFinal()` (or the new
suggested helper method) rather than duplicating this logic here?
test/jdk/com/sun/crypto/provider/Cipher/KeyWrap/NISTWrapKAT.java line 105:
> 103:
> 104: @DataProvider
> 105: public Object[][] testData() {
Can we please add test cases for `AES/KWP/NoPadding` where the input is an even
multiple of 8? We've [seen bugs in this case
before](https://github.com/pyca/cryptography/issues/4156).
test/jdk/com/sun/crypto/provider/Cipher/KeyWrap/TestGeneral.java line 50:
> 48:
> 49: System.out.println("input len: " + inLen);
> 50: c.init(Cipher.ENCRYPT_MODE, KEY, new IvParameterSpec(in, 0,
> ivLen));
Do we have tests for no IV (and thus the default)?
Do we have tests that the null (default) IV matches the results from an
explicitly provided default ID?
test/jdk/com/sun/crypto/provider/Cipher/KeyWrap/TestGeneral.java line 179:
> 177: System.out.println("Testing " + ALGO);
> 178: c = Cipher.getInstance(ALGO, "SunJCE");
> 179: for (int i = 0; i < MAX_KWP_PAD_LEN; i++) {
I see that here (and earlier) we do test all padding lengths. I'd still like
some KATs generated by a known good implementation to ensure that we are not
just compatible with ourselves.
-------------
PR: https://git.openjdk.java.net/jdk/pull/2404
