RFR: 8248268: Support KWP in addition to KW

classic Classic list List threaded Threaded
25 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFR: 8248268: Support KWP in addition to KW

Valerie Peng-2
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

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

Commit messages:
 - 8248268: Support KWP in addition to KW

Changes: https://git.openjdk.java.net/jdk/pull/2404/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2404&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8248268
  Stats: 2534 lines in 14 files changed: 1923 ins; 536 del; 75 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248268: Support KWP in addition to KW [v2]

Valerie Peng-2
> 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:

  Restored Iv algorithm parameters impl.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2404/files
  - new: https://git.openjdk.java.net/jdk/pull/2404/files/fa468921..bc912f63

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2404&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2404&range=00-01

  Stats: 330 lines in 4 files changed: 177 ins; 147 del; 6 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248268: Support KWP in addition to KW

Valerie Peng-2
In reply to this post by Valerie Peng-2
On Thu, 4 Feb 2021 10:51:12 GMT, Valerie Peng <[hidden email]> 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

Ping, anyone has time to review this?

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

PR: https://git.openjdk.java.net/jdk/pull/2404
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248268: Support KWP in addition to KW [v3]

Valerie Peng-2
In reply to this post by Valerie Peng-2
> 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&pr=2404&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2404&range=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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248268: Support KWP in addition to KW [v3]

Greg Rubin
On Mon, 22 Mar 2021 21:43:31 GMT, Valerie Peng <[hidden email]> 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:         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?

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.

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

PR: https://git.openjdk.java.net/jdk/pull/2404
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248268: Support KWP in addition to KW [v3]

Michael StJohns
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


Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248268: Support KWP in addition to KW [v3]

Michael StJohns
In reply to this post by Valerie Peng-2
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


Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248268: Support KWP in addition to KW [v3]

Valerie Peng-2
In reply to this post by Greg Rubin
On Tue, 23 Mar 2021 18:39:27 GMT, Greg Rubin <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248268: Support KWP in addition to KW [v3]

Valerie Peng-2
In reply to this post by Greg Rubin
On Tue, 23 Mar 2021 18:41:26 GMT, Greg Rubin <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248268: Support KWP in addition to KW [v3]

Valerie Peng-2
In reply to this post by Greg Rubin
On Tue, 23 Mar 2021 19:56:40 GMT, Greg Rubin <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248268: Support KWP in addition to KW

Valerie Peng-2
In reply to this post by Valerie Peng-2
On Mon, 22 Mar 2021 18:48:47 GMT, Valerie Peng <[hidden email]> 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
>
> Ping, anyone has time to review this?

>
>
> _Mailing list message from [Michael StJohns](mailto:[hidden email]) on [security-dev](mailto:[hidden email]):_
>
> 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
>
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20210323/e1a400db/attachment.htm>

Sure, I will add some, thanks Mike for the pointers.

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

PR: https://git.openjdk.java.net/jdk/pull/2404
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248268: Support KWP in addition to KW [v3]

Valerie Peng-2
In reply to this post by Greg Rubin
On Tue, 23 Mar 2021 19:18:14 GMT, Greg Rubin <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248268: Support KWP in addition to KW [v3]

Valerie Peng-2
In reply to this post by Greg Rubin
On Tue, 23 Mar 2021 19:57:44 GMT, Greg Rubin <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248268: Support KWP in addition to KW [v3]

Valerie Peng-2
In reply to this post by Greg Rubin
On Tue, 23 Mar 2021 20:09:23 GMT, Greg Rubin <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248268: Support KWP in addition to KW [v3]

Valerie Peng-2
In reply to this post by Greg Rubin
On Tue, 23 Mar 2021 18:47:32 GMT, Greg Rubin <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248268: Support KWP in addition to KW [v3]

Valerie Peng-2
In reply to this post by Greg Rubin
On Tue, 23 Mar 2021 19:06:30 GMT, Greg Rubin <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248268: Support KWP in addition to KW [v3]

Valerie Peng-2
In reply to this post by Greg Rubin
On Tue, 23 Mar 2021 17:16:04 GMT, Greg Rubin <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8248268: Support KWP in addition to KW

Valerie Peng-2
In reply to this post by Valerie Peng-2
On Thu, 25 Mar 2021 02:18:06 GMT, Valerie Peng <[hidden email]> wrote:

>> Ping, anyone has time to review this?
>
>>
>>
>> _Mailing list message from [Michael StJohns](mailto:[hidden email]) on [security-dev](mailto:[hidden email]):_
>>
>> 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
>>
>> -------------- next part --------------
>> An HTML attachment was scrubbed...
>> URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20210323/e1a400db/attachment.htm>
>
> Sure, I will add some, thanks Mike for the pointers.

Hi Mike,

Please find comments in line below.

> 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?

Right, will fix this and update the javadoc above as well.
 
> 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.

The way I view it, both KW and KWP are modes. For interoperability, I referenced the corresponding mechanism definition from PKCS#11 v3.0 and base the java transformation on the corresponding PKCS#11 mechanisms. As for AES/KW/AutoPadding, it's an interesting idea, is there any other provider or library support it? To encrypt data of any size, we can just use AES/KWP/NoPadding. Is there additional benefit for this AutoPadding scheme?

Thanks,
Valerie
>
> Mike
>
> -------------- next part --------------
> An HTML attachment was scrubbed...
> URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20210323/0372393b/attachment-0001.htm>

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

PR: https://git.openjdk.java.net/jdk/pull/2404
Reply | Threaded
Open this post in threaded view
|

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

Valerie Peng-2
In reply to this post by Valerie Peng-2
> 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

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2404/files
  - new: https://git.openjdk.java.net/jdk/pull/2404/files/c90fdb1e..e4fd6ea2

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2404&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2404&range=02-03

  Stats: 465 lines in 6 files changed: 202 ins; 107 del; 156 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
Reply | Threaded
Open this post in threaded view
|

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

Greg Rubin
On Sat, 27 Mar 2021 00:25:09 GMT, Valerie Peng <[hidden email]> 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
12