Re: RFR: 8248268: Support KWP in addition to KW [v3]
On Tue, 23 Mar 2021 19:14:16 GMT, Greg Rubin
wrote:
>> 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
>
> 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?
In NISTWrapKAT.java, all of its test vectors do not use custom IV. So, the
default IV scenario for KW and KWP are covered there. I can add another
init(...) call which sets the IV based on the current IV to that test.
-
PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v3]
On Tue, 23 Mar 2021 17:16:04 GMT, Greg Rubin
wrote:
>> 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
>
> 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).
Sure, there are some more from the NIST zip.
-
PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v3]
On Tue, 23 Mar 2021 19:06:30 GMT, Greg Rubin
wrote:
>> 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/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.
Ok, I can update to make W() handles the IV semiblock overwrite.
-
PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v3]
On Tue, 23 Mar 2021 18:47:32 GMT, Greg Rubin
wrote:
>> 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 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).
Hmm, another alternative is to move this logic to the underlying key wrap impl,
i.e. AESKeyWrap and AESKeyWrapPadded, and start saving data after the first
8-byte in KeyWrapCipher class.
-
PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v3]
On Tue, 23 Mar 2021 20:09:23 GMT, Greg Rubin
wrote:
>> 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/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;`).
Right, I agree. Will look into it.
> 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?
Right, I will add additional checking on the Cipher.XXX_MODE. Agree with you
regarding reducing the code duplication.
-
PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v3]
On Tue, 23 Mar 2021 19:57:44 GMT, Greg Rubin wrote: >> 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/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`? Sure, good idea. - PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v3]
On Tue, 23 Mar 2021 19:18:14 GMT, Greg Rubin
wrote:
>> 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
>
> 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.
Sure, I can add some from the NIST zip which Michael StJohns mentioned to the
NISTWrapKAT.java.
-
PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v3]
On Tue, 23 Mar 2021 19:56:40 GMT, Greg Rubin
wrote:
>> 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/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.
This is just IV and length, not plaintext. So, I didn't use the constant time
array check. I can switch to the constant time version, it's trivial.
-
PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v3]
On Tue, 23 Mar 2021 18:41:26 GMT, Greg Rubin wrote: >> 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 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? Hmm, there are some inconsistencies in how the variables are named. I will remove this comment and update the method javadoc accordingly, i.e. ptOfs, ct, ctOfs have all been renamed to dummyN. - PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v3]
On Tue, 23 Mar 2021 18:39:27 GMT, Greg Rubin
wrote:
>> 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?
I chose to extend FeedbackCipher since we are supporting KW and KWP as cipher
modes and given that FeedbackCipher represents "a block cipher in one of its
modes", it seems a natural fit. As for the various unsupported operations,
those methods aren't currently used and instead of a no-op, I chose to throw
UnsupportedOperationException to prevent future accidental use.
-
PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v3]
On 3/22/2021 5:43 PM, Valerie Peng 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
PR: https://git.openjdk.java.net/jdk/pull/2404
KeyWrapCipher.java:
/**
212 * Sets the padding mechanism of this cipher. Currently, only
213 * "NoPadding" scheme is accepted for this cipher.
214 *
215 * @param padding the padding mechanism
216 *
217 * @exception NoSuchPaddingException if the requested padding mechanism
218 * does not match the supported padding scheme
219 */
220 @Override
221 protected void engineSetPadding(String padding)
222 throws NoSuchPaddingException {
223 if ((this.padding == null &&
!"NoPadding".equalsIgnoreCase(padding)) ||
224 this.padding instanceof PKCS5Padding &&
225 "PKCS5Padding".equalsIgnoreCase(padding)) {
226 throw new NoSuchPaddingException();
227 }
228 }
Shouldn't this be rejecting PKCS5Padding?
Actually, I keep wondering why this isn't AES/KW/NoPadding,
AES/KW/KWPPadding and AES/KW/AutoPadding where the latter doesn't take a
user provided IV, but figures out which of the two default IV values to
use based on whether the input is a multiple of the blocksize or not?
Decrypting is similar - do the decryption, and check which IV you get to
figure out padded or not padded.
Mike
Re: RFR: 8248268: Support KWP in addition to KW [v3]
On 3/23/2021 4:15 PM, Greg Rubin wrote:
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.
http://csrc.nist.gov/groups/STM/cavp/documents/mac/kwtestvectors.zip has
the NIST test vectors. See
https://csrc.nist.gov/CSRC/media/Projects/Cryptographic-Algorithm-Validation-Program/documents/mac/KWVS.pdf
for details.
Mike
Re: RFR: 8248268: Support KWP in addition to KW [v3]
On Mon, 22 Mar 2021 21:43:31 GMT, Valerie Peng 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:
Re: RFR: 8248268: Support KWP in addition to KW [v3]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/2404/files - new: https://git.openjdk.java.net/jdk/pull/2404/files/bc912f63..c90fdb1e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2404=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2404=01-02 Stats: 5 lines in 2 files changed: 1 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/2404.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2404/head:pull/2404 PR: https://git.openjdk.java.net/jdk/pull/2404
