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

[Greg Rubin]

On Sat, 27 Mar 2021 00:25:09 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:
> 
>   Refactor code to reduce code duplication
>   Address review comments
>   Add more test vectors

I'd advise against the AutoPadding scheme without more careful analysis and discussion. Have we seen either KW or KWP specifications which recommend that behavior?

My concern is that we've seen cases before where two different cryptographic algorithms could be selected transparently upon decryption and it lowers the security of the overall system. (A variant of in-band signalling.) The general consensus that I've been seeing in the (applied) cryptographic community is strongly away from in-band signalling and towards the decryptor fully specifying the algorithms and behavior prior to attempting decryption.

src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrapPadded.java line 71:

> 69:             match &= (ivAndLen[i] == iv[i]);
> 70:         }
> 71:         if (!match) {

True nitpick (thus ignorable): I believe that using bitwise math is slightly more resistant to compiler and/or CPU optimization to defend against timing-attacks. (Since I haven't even seen an attack against KW or KWP, this is simply a note in general rather than something which needs to be fixed.)

src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrapPadded.java line 78:

> 76:         for (int k = 5; k < SEMI_BLKSIZE; k++) {
> 77:             if (outLen != 0) {
> 78:                 outLen <<= SEMI_BLKSIZE;

While technically, this is correct (as it is shifting by 8 bits), it is pure coincidence that `SEMI_BLKSIZE` (8 bytes) happens to have the name integer value as the number of bits in one byte. It took me more reads than I care to admit to understand why this worked. Could we just replace this one with an `8` as it is clearer and more accurate?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2404