RFR: 8248268: Support KWP in addition to KW [v3]

[Greg Rubin]

On Mon, 22 Mar 2021 21:43:31 GMT, Valerie Peng <valeriep at 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