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

[Valerie Peng]

On Tue, 25 May 2021 20:33:55 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:
> 
>   Address review feedbacks

> 
> 
> _Mailing list message from [Michael StJohns](mailto:mstjohns at comcast.net) on [security-dev](mailto:security-dev at mail.openjdk.java.net):_
> 
> Some more general comments - related to the restructuring.
> 
> In AESKeyWrap at 152-155 - that check probably should be moved to W().??
> KWP should do the formatting prior to passing the data to W().? Also at
> 185-187 - move that to W_INV().
> 
> AESKeyWrap at 158 - shouldn't you be returning the cipher text length??
> That's what the comment in FeedbackCipher says.? W() should probably be
> returning the final length.
> 
> The length of the final ciphertext data should be 8 bytes longer than
> the plaintext. decryptFinal() seems to do the right thing by decreasing
> the length returned.?? But again - shouldn't that be the purview of W_INV()?
> 
> The call in KeyWrapCipher.engineGetOutputSize() should probably also be
> passed into KWUtil rather than being? done in KeyWrapCipher.? And the
> more I look at this, the more I think that all of the engineUpdate
> operations should throw UnsupportedOperationException - it would
> certainly simplify the logic.? That would make the call return either?
> inputLength + 8 or inputLength - 8 depending on mode.
> 
> KWUtil.W() - should probably check that in.length >= inLen + 8 and throw
> a ShortBufferException if not.
> 
> Would it be useful to add a comment in KeyWrapCipher that? warns
> maintainers that? AESKeyWrap(Padded).encryptFinal() and decryptFinal()
> uses the input buffer as the output buffer? That's a reasonable approach
> for decryption, but I'm wondering if you might want to support in-place
> encryption as there's no spec prohibition requiring data to be held
> until the encryption is complete.
> 
> Mike
> 
> On 5/24/2021 7:01 PM, Valerie Peng wrote:

Hi Mike,

Thanks for chiming in and review. Please find my comments below:
1) regarding the data length checks in AESKeyWrap/AESKeyWrapPadded class, it's better to check them before formatting the data and passing them to W(). This way, the error message can be clear and fail fast. I added assert statement to W()/W_INV() so the assumption on data sizes are clear.
2) regarding the returned text length of encryptFinal()/decryptFinal() calls, the returned value is correct in that 'ptLen' is the length of the formatted input to W() which contains both the first semiblock and user-supplied data. The name of the variable probably caused the confusion. For better readability, I changed W()/W_INV() to return an int. The javadoc explains what the returned int means. Hope this is clearer now.
3) regarding KWUtil, it's just a state-less class holding utility methods which are used by both AESKeyWrap (KW impl) and AESKeyWrapPadded (KWP impl). So I don't see why it should handle engineGetOutputSize() calls.
4) as for engineUpdate(...) throwing UnsupportedOperationException, I see the point that you are trying to make. Currently we are bound by the Cipher/CipherSpi javadoc spec. Another concern is that existing apps which are using multi-part enc/dec, may be caught off guard with this. So, that's how it is.

I need to do a system update and will get to your other comments once it's finished.
Thanks,
Valerie

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

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