On Sat, 27 Mar 2021 00:25:09 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:
>
> 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
