8248268: Support KWP in addition to KW #2404
Conversation
Updated existing AESWrap support with AES/KW/NoPadding cipher transformation. Added support for AES/KWP/NoPadding and AES/KW/PKCS5Padding support to SunJCE provider.
|
👋 Welcome back valeriep! A progress list of the required criteria for merging this PR into |
|
/csr |
|
@valeriepeng The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
openjdk
bot
added
security
|
@valeriepeng has indicated that a compatibility and specification (CSR) request is needed for this pull request. |
Webrevs
|
|
@valeriepeng This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
Ping, anyone has time to review this? |
| throw new IllegalBlockSizeException("data should" + | ||
| " be at least 16 bytes and multiples of 8"); | ||
| } | ||
| // assert ptOfs == 0; ct == pt; ctOfs == 0; |
There was a problem hiding this comment.
Is this a missing code assertion?
There was a problem hiding this comment.
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.
| byte[] ivOut = new byte[ICV1.length]; | ||
| W_INV(ct, ctLen, ivOut, embeddedCipher); | ||
| ctLen -= SEMI_BLKSIZE; | ||
|
|
||
| // check against icv and fail if not match | ||
| if (!Arrays.equals(ivOut, 0, ICV1.length, this.iv, 0, ICV1.length)) { | ||
| throw new IllegalBlockSizeException("Integrity check failed"); | ||
| } |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
src/java.base/share/classes/com/sun/crypto/provider/KWUtil.java
Outdated
Show resolved
Hide resolved
| if (!Arrays.equals(ivAndLen, 0, ICV2.length, icv, 0, ICV2.length)) { | ||
| throw new IllegalBlockSizeException("Integrity check failed"); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrapPadded.java
Show resolved
Hide resolved
| if (!decrypting && dataIdx == 0) { | ||
| writeIvSemiBlock(dataBuf); | ||
| } | ||
|
|
||
| if (inLen > 0) { | ||
| store(in, inOfs, inLen); | ||
| } | ||
|
|
||
| // if enc, add padding | ||
| if (!decrypting) { | ||
| int actualLen = dataIdx - SEMI_BLKSIZE; | ||
| if (padding != null) { | ||
| int paddingLen = padding.padLength(actualLen); | ||
| // check and re-size dataBuf if needed | ||
| store(null, 0, paddingLen); | ||
| try { | ||
| padding.padWithLen(dataBuf, dataIdx, paddingLen); | ||
| dataIdx += paddingLen; | ||
| } catch (ShortBufferException sbe) { | ||
| // should never happen | ||
| throw new AssertionError(); | ||
| } | ||
| } | ||
| } | ||
| try { | ||
| int outLen; | ||
| if (decrypting) { | ||
| outLen = cipher.decryptFinal(dataBuf, 0, dataIdx, null, -1); | ||
| // unpad if padding is used | ||
| if (padding != null) { | ||
| int padIdx = padding.unpad(dataBuf, 0, outLen); | ||
| if (padIdx <= 0) { | ||
| throw new BadPaddingException("Bad Padding: " + padIdx); | ||
| } | ||
| outLen = padIdx; | ||
| } | ||
| } else { | ||
| outLen = cipher.encryptFinal(dataBuf, 0, dataIdx, null, -1); | ||
| } |
There was a problem hiding this comment.
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;).
There was a problem hiding this comment.
Right, I agree. Will look into it.
|
Mailing list message from Michael StJohns on security-dev: On 3/23/2021 4:15 PM, Greg Rubin wrote:
http://csrc.nist.gov/groups/STM/cavp/documents/mac/kwtestvectors.zip has Mike -------------- next part -------------- |
|
Mailing list message from Michael StJohns on security-dev: On 3/22/2021 5:43 PM, Valerie Peng wrote:
KeyWrapCipher.java:
Shouldn't this be rejecting PKCS5Padding? Actually, I keep wondering why this isn't? AES/KW/NoPadding, Decrypting is similar - do the decryption, and check which IV you get to Mike -------------- next part -------------- |
Sure, I will add some, thanks Mike for the pointers. |
|
Hi Mike, Please find comments in line below.
Right, will fix this and update the javadoc above as well.
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,
|
Address review comments Add more test vectors
There was a problem hiding this comment.
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.
| boolean match = true; | ||
| for (int i = 0; i < ICV2.length; i++) { | ||
| match &= (ivAndLen[i] == iv[i]); | ||
| } | ||
| if (!match) { |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Sure, I can change to below:
| boolean match = true; | |
| for (int i = 0; i < ICV2.length; i++) { | |
| match &= (ivAndLen[i] == iv[i]); | |
| } | |
| if (!match) { | |
| int match = 0; | |
| for (int i = 0; i < ICV2.length; i++) { | |
| match |= (ivAndLen[i] ^ iv[i]); | |
| } | |
| if (match != 0) { | |
| throw new IllegalBlockSizeException("Integrity check failed"); | |
| } | |
Is this what you have in mind?
src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrapPadded.java
Outdated
Show resolved
Hide resolved
|
Mailing list message from Michael StJohns on security-dev: On 4/3/2021 11:35 AM, Greg Rubin wrote:
I think this is in response to my comment? The wrap function can take a Key as an input and can have the unwrap The key thing to remember here is that the IV (initial value - RFC FWIW, the actual cryptographic operations between padded data and Obviously, this doesn't work if someone provides their own IV - but If an AutoPadding mode were implemented, I'd throw exceptions if someone Later, Mike |
|
Mailing list message from Greg Rubin on security-dev: Mike, Yes, this was in response to your comment. I'm aware that the IV really serves more as an integrity check and mode The proposed "AutoPadding" mode is an example of in-band signalling in Thank you, On Sat, Apr 3, 2021 at 4:38 PM Michael StJohns <mstjohns at comcast.net> wrote:
-------------- next part -------------- |
|
Mailing list message from Michael StJohns on security-dev: On 4/7/2021 1:28 PM, Greg Rubin wrote:
I sent a note off to the original mode inventor - Russ Housley:
I got back:
Which sort of confirms what I thought, but added a question:? Are there Does there need to be an autopad OID? If it were me, I'd be avoiding implementing the PKCS5 padding mode WRT to HSM uncertainty, I ran into problems especially trying to wrap At some point, there needs to be an RFC written that specifies the Later, Mike
-------------- next part -------------- |
|
Mailing list message from Michael StJohns on security-dev: *sigh* Minor correction in line. On 4/7/2021 2:49 PM, Michael StJohns wrote:
"if" not "is"
-------------- next part -------------- |
|
Mailing list message from Greg Rubin on security-dev: I agree that the response from Housley certainly supports that For KW+PKCS5, I have (unfortunately) seen this deployed in the real world As for OIDs, that seems somewhat unrelated and I don't think we need I've also encountered HSMs which added PKCS5 padding prior to KW so that Finally, I believe that the encoding question is separate from the wrapping Greg On Wed, Apr 7, 2021 at 11:51 AM Michael StJohns <mstjohns at comcast.net>
-------------- next part -------------- |
KWParameters and KWPParameters.
|
@valeriepeng This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
|
@valeriepeng this pull request can not be integrated into git checkout JDK-8248268
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
the
merge-conflict
openjdk
bot
added
rfr
src/java.base/share/classes/com/sun/crypto/provider/BlockCipherParamsCore.java
Show resolved
Hide resolved
| core = new BlockCipherParamsCore(AESConstants.AES_BLOCK_SIZE); | ||
| core = new BlockCipherParamsCore(AESConstants.AES_BLOCK_SIZE, 4, 8); |
There was a problem hiding this comment.
A cipher object may not take different IV sizes at the same time. I was just wondering how it could be used in practice. Maybe something like:
AlgorithmParameters algParams = AlgorithmParameters.getInstance("AES");
algParams.init(ivParameterSpec);
The IV parameter is given with the init() method. Then, it may be not necessary to construct the BlockCipherParamsCore object will all potential IV sizes. See the comments in BlockCipherParamsCore.
There was a problem hiding this comment.
Cipher objects normally takes just one iv size. BlockCipherParamsCore is used by various impls of AlgorithmParametersSpi class which may be used with different block cipher algorithms. The iv parameter is given with the AlgorithmParametersSpi.init() method and invalid iv will lead to exceptions. Since there are iv size checks built in BlockCipherParamsCore already, it seems safer to relax the check a bit for AES (4 and 8 for KWP and KW). The other choice is to just remove the size check from BlockCipherParamsCore for AES algorithm, but then we'd not be able to reject invalid iv lengths as before.
| @@ -48,8 +49,11 @@ | |||
| private int block_size = 0; | |||
| private byte[] iv = null; | |||
|
|
|||
| BlockCipherParamsCore(int blksize) { | |||
| private int[] moreSizes = null; | |||
There was a problem hiding this comment.
The moreSizes is not used other than the init() method field. It may be not easy to check the specific size if we cache all supported sized in the object. For example, if the required IV size if 8 bytes, it may be a problem about how to make sure the iv size is 8 bytes exactly for a specific algorithm.
Maybe, we could just have a ivSize filed. The default value is block_size, which coupe be set with the init(ivParameterSpec) method.
....
private int ivSize;
...
BlockCipherParamsCore(int blkSize) {
block_size = blkSize;
ivSize = blkSize;
}
...
void init(AlgorithmParameterSpec paramSpec) {
ivSize = ...; // reset IV size.
}
// use ivSize while encoding and decoding.
There was a problem hiding this comment.
The problem with this is that this assumes that the specified paramSpec argument of the init() method is always valid.
src/java.base/share/classes/com/sun/crypto/provider/ConstructKeys.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/com/sun/crypto/provider/ConstructKeys.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/com/sun/crypto/provider/KeyWrapCipher.java
Outdated
Show resolved
Hide resolved
src/java.base/share/classes/com/sun/crypto/provider/ConstructKeys.java
Outdated
Show resolved
Hide resolved
|
Mailing list message from Michael StJohns on security-dev: In line On 5/21/2021 5:01 PM, Xue-Lei Andrew Fan wrote:
The mode is KW - it has a fixed length 8 byte non-iv integrity tag.??? I'd treat KWP as a final (in the Java final sense) extension to KW with This is sort of one reason I was arguing for AES/KW/KWPPadding rather Mike |
1 similar comment
|
Mailing list message from Michael StJohns on security-dev: In line On 5/21/2021 5:01 PM, Xue-Lei Andrew Fan wrote:
The mode is KW - it has a fixed length 8 byte non-iv integrity tag.??? I'd treat KWP as a final (in the Java final sense) extension to KW with This is sort of one reason I was arguing for AES/KW/KWPPadding rather Mike |
|
Good points, Mike! Thank you!
Hm, I missed this point. KW (RFC 3394) supports Alternative IV (AIV), while KWP is just a case about how to construct the alternative IV for KWP operations.
I agreed. Maybe, we could do:
I thought of the "AES/KW/KWPPadding" style as well. The "AES/KWP/NoPadding" looks a little bit weird to me because there is padding while we call it with "NoPadding". KWP is not exactly like the traditional AES/CBC/PKCS5Padding, where the components could be treated separately. The padding scheme of KWP impact the IV as well. So it was arguable to me. Finally I feel better of the "AES/KWP/NoPadding" when I read the NIST SP title again, "Recommendation for Block Cipher Modes of Operation: Methods for Key Wrapping". Maybe, it is an industry accepted notation to treat KWP as a block cipher mode. |
|
Mailing list message from Michael StJohns on security-dev: On 5/22/2021 1:57 PM, Xue-Lei Andrew Fan wrote:
Yes.? Sorry if I wasn't clearer in earlier emails.
I think that's acceptable.? Hmm... is it possible to make the set ICV
Works.? Or see the above.
Yes.
We keep referring to it as an IV? - but it isn't - it's an Integrity
Step 5 of the KWP-AE algorithm basically passes the A65959A6 value The equivalent for KW-AE is step 2 which passes the ICV1 value in The underlying function W() is doing what it does without any reference So the differences between KWP and KW are actually in how the plain text Think of it this way:? W() and W'() are the actual cryptographic actions Another way of looking at that - KW-AE can't use a variant ICV. (Or at *shrug* I don't think that Java nomenclature and NIST nomenclature have to match Mike -------------- next part -------------- |
1 similar comment
|
Mailing list message from Michael StJohns on security-dev: On 5/22/2021 1:57 PM, Xue-Lei Andrew Fan wrote:
Yes.? Sorry if I wasn't clearer in earlier emails.
I think that's acceptable.? Hmm... is it possible to make the set ICV
Works.? Or see the above.
Yes.
We keep referring to it as an IV? - but it isn't - it's an Integrity
Step 5 of the KWP-AE algorithm basically passes the A65959A6 value The equivalent for KW-AE is step 2 which passes the ICV1 value in The underlying function W() is doing what it does without any reference So the differences between KWP and KW are actually in how the plain text Think of it this way:? W() and W'() are the actual cryptographic actions Another way of looking at that - KW-AE can't use a variant ICV. (Or at *shrug* I don't think that Java nomenclature and NIST nomenclature have to match Mike -------------- next part -------------- |
|
Mailing list message from Michael StJohns on security-dev: Some more general comments - related to the restructuring. In AESKeyWrap at 152-155 - that check probably should be moved to W().?? AESKeyWrap at 158 - shouldn't you be returning the cipher text length?? The length of the final ciphertext data should be 8 bytes longer than The call in KeyWrapCipher.engineGetOutputSize() should probably also be KWUtil.W() - should probably check that in.length >= inLen + 8 and throw Would it be useful to add a comment in KeyWrapCipher that? warns Mike On 5/24/2021 7:01 PM, Valerie Peng wrote: |
1 similar comment
|
Mailing list message from Michael StJohns on security-dev: Some more general comments - related to the restructuring. In AESKeyWrap at 152-155 - that check probably should be moved to W().?? AESKeyWrap at 158 - shouldn't you be returning the cipher text length?? The length of the final ciphertext data should be 8 bytes longer than The call in KeyWrapCipher.engineGetOutputSize() should probably also be KWUtil.W() - should probably check that in.length >= inLen + 8 and throw Would it be useful to add a comment in KeyWrapCipher that? warns Mike On 5/24/2021 7:01 PM, Valerie Peng wrote: |
|
Current impl takes account into RFC 3394 (Sept 2002), RFC 5649 (Aug 2009), NIST 800-38F (Dec 2012) and PKCS#11 v3.0 Current Mech Spec (June 2020) and tries to support them all. If solely basing on RFC 3394/5649, then AES KW and KWP cipher should just supports key wrap/unwrap but not enc/dec. The main reason that I added the support for alternative iv and enc/dec is more for NIST 800-38F which states that KW/KWP can be used for general data protection as well and PKCS#11 v3.0 which lists 3 mechanisms mapping to java's AES using KW, AES using KW and PKCS5Padding and AES using KWP. These 3 mechanisms supports all 4, i.e. enc/dec/wrap/unwrap, as well custom IVs (8-byte for KW and 4-byte for KWP). Naming can be a very subjective matter and putting it aside, I don't see why KW and KWP should not allow custom IVs? Isn't hardcoded values less secure or fragile in general? |
Hi Mike, Thanks for chiming in and review. Please find my comments below:
I need to do a system update and will get to your other comments once it's finished. |
|
In the latest commit, I have made some minor optimizations regrading the in-place encryption as suggested in Mike's earlier comments. However, the optimizations are limited to when the output offset == 0 case. |
|
@valeriepeng This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 269 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
…y and updated tests accordingly.
|
Hi Mike, Thanks for the feedbacks, |
|
/integrate |
openjdk
bot
added
integrated
|
@valeriepeng Since your change was applied there have been 273 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 136badb. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
This change updates SunJCE provider as below:
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
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/2404/head:pull/2404$ git checkout pull/2404Update a local copy of the PR:
$ git checkout pull/2404$ git pull https://git.openjdk.java.net/jdk pull/2404/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2404View PR using the GUI difftool:
$ git pr show -t 2404Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/2404.diff