On Tue, 25 May 2021 20:33:55 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: > > Address review feedbacks > > > _Mailing list message from [Michael StJohns](mailto:mstjo...@comcast.net) on > [security-dev](mailto:security-...@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
