Re: RFR: 8248268: Support KWP in addition to KW [v7]
On Fri, 14 May 2021 00:33:12 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 with a new target base due to a > merge or a rebase. The pull request now contains seven commits: > > - Merge master into JDK-8248268 > - Minor update to address review comments. > - Changed AESParameters to allow 4-byte, 8-byte IVs and removed >KWParameters and KWPParameters. > - Refactor code to reduce code duplication >Address review comments >Add more test vectors > - Changed AlgorithmParameters impls to register under AES/KW/NoPadding and >AES/KWP/NoPadding > - Restored Iv algorithm parameters impl. > - 8248268: Support KWP in addition to KW > >Updated existing AESWrap support with AES/KW/NoPadding cipher >transformation. Added support for AES/KWP/NoPadding and >AES/KW/PKCS5Padding support to SunJCE provider. 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? - PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v7]
On Sat, 22 May 2021 01:02:50 GMT, Xue-Lei Andrew Fan wrote:
>> Valerie Peng has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains seven commits:
>>
>> - Merge master into JDK-8248268
>> - Minor update to address review comments.
>> - Changed AESParameters to allow 4-byte, 8-byte IVs and removed
>>KWParameters and KWPParameters.
>> - Refactor code to reduce code duplication
>>Address review comments
>>Add more test vectors
>> - Changed AlgorithmParameters impls to register under AES/KW/NoPadding and
>>AES/KWP/NoPadding
>> - Restored Iv algorithm parameters impl.
>> - 8248268: Support KWP in addition to KW
>>
>>Updated existing AESWrap support with AES/KW/NoPadding cipher
>>transformation. Added support for AES/KWP/NoPadding and
>>AES/KW/PKCS5Padding support to SunJCE provider.
>
> src/java.base/share/classes/com/sun/crypto/provider/ConstructKeys.java line
> 176:
>
>> 174: return ConstructKeys.constructPublicKey(encoding,
>> keyAlgorithm);
>> 175: default:
>> 176: throw new RuntimeException("Unsupported key type");
>
> Good improvement to thrown an exception rather than return "null"!
>
> Is NoSuchAlgorithmException fitted better the specification? See also the
> comment in KeyWrapCipher.engineUnwrap().
Yes, will change.
> src/java.base/share/classes/com/sun/crypto/provider/KeyWrapCipher.java line
> 710:
>
>> 708: outLen = helperDecrypt(dataBuf, dataIdx);
>> 709: return ConstructKeys.constructKey(dataBuf, 0, outLen,
>> 710: wrappedKeyAlgorithm, wrappedKeyType);
>
> Per the specification of engineUnwrap, maybe, the
> ConstructKeys.constructKey() implementation could throw
> NoSuchAlgorithmException if wrappedKeyType is unknown. See the common in the
> the ConstructKeys.constructKey() implementation.
Sure, makes sense.
-
PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v7]
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:
On Fri, 21 May 2021 20:44:57 GMT, Xue-Lei Andrew Fan wrote:
Valerie Peng has updated the pull request with a new target base due to a merge
or a rebase. The pull request now contains seven commits:
- Merge master into JDK-8248268
- Minor update to address review comments.
- Changed AESParameters to allow 4-byte, 8-byte IVs and removed
KWParameters and KWPParameters.
- Refactor code to reduce code duplication
Address review comments
Add more test vectors
- Changed AlgorithmParameters impls to register under AES/KW/NoPadding and
AES/KWP/NoPadding
- Restored Iv algorithm parameters impl.
- 8248268: Support KWP in addition to KW
Updated existing AESWrap support with AES/KW/NoPadding cipher
transformation. Added support for AES/KWP/NoPadding and
AES/KW/PKCS5Padding support to SunJCE provider.
src/java.base/share/classes/com/sun/crypto/provider/AESParameters.java line 50:
48:
49: public AESParameters() {
50: core = new BlockCipherParamsCore(AESConstants.AES_BLOCK_SIZE, 4, 8);
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.
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.
src/java.base/share/classes/com/sun/crypto/provider/BlockCipherParamsCore.java
line 52:
50: private byte[] iv = null;
51:
52: private int[] moreSizes = null;
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.
The problem with this is that this assumes that the specified paramSpec
argument of the init() method is always valid.
-
PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v7]
On Sat, 22 May 2021 00:45:27 GMT, Xue-Lei Andrew Fan wrote:
>> Valerie Peng has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains seven commits:
>>
>> - Merge master into JDK-8248268
>> - Minor update to address review comments.
>> - Changed AESParameters to allow 4-byte, 8-byte IVs and removed
>>KWParameters and KWPParameters.
>> - Refactor code to reduce code duplication
>>Address review comments
>>Add more test vectors
>> - Changed AlgorithmParameters impls to register under AES/KW/NoPadding and
>>AES/KWP/NoPadding
>> - Restored Iv algorithm parameters impl.
>> - 8248268: Support KWP in addition to KW
>>
>>Updated existing AESWrap support with AES/KW/NoPadding cipher
>>transformation. Added support for AES/KWP/NoPadding and
>>AES/KW/PKCS5Padding support to SunJCE provider.
>
> src/java.base/share/classes/com/sun/crypto/provider/ConstructKeys.java line
> 190:
>
>> 188: return constructKey(encoding, keyAlgorithm, keyType);
>> 189: } else {
>> 190: byte[] encoding2 = Arrays.copyOfRange(encoding, ofs,
>> ofs + len);
>
> The array will be copied again in the X509EncodedKeySpec and
> PKCS8EncodedKeySpec. It is out of this scope, but it may be a performance
> improvement if adding X509EncodedKeySpec and PKCS8EncodedKeySpec constructors
> that accept array offset, so that the array copy here could be avoid.
Right, maybe there is no constructor which takes offset and length due to
trying to avoid the cost of validating them. I recall seeing other *Spec
classes whose list of constructors are similar to
X509EncodedKeySpec/PKCS8EncodedKeySpec as well.
> src/java.base/share/classes/com/sun/crypto/provider/ConstructKeys.java line
> 192:
>
>> 190: byte[] encoding2 = Arrays.copyOfRange(encoding, ofs,
>> ofs + len);
>> 191: try {
>> 192: return constructKey(encoding2, keyAlgorithm,
>> keyType);
>
> The two constructKey methods are depended on each other. It may improve the
> readability if only having one-way dependence, by moving the switch-block
> logic from the former constructKey() to the later one.
>
>
> static final Key constructKey(byte[] encoding, String keyAlgorithm,
>int keyType) throws InvalidKeyException, NoSuchAlgorithmException {
> return constructKey(encoding, 0. encoding.length, keyAlgorithm, keyType);
> }
>
>
> With this re-org, the array copy could be avoid if the key type is unknown.
Yes, I will refactor the two constructKey() methods as you suggested.
-
PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v7]
On Fri, 21 May 2021 20:44:57 GMT, Xue-Lei Andrew Fan wrote:
>> Valerie Peng has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains seven commits:
>>
>> - Merge master into JDK-8248268
>> - Minor update to address review comments.
>> - Changed AESParameters to allow 4-byte, 8-byte IVs and removed
>>KWParameters and KWPParameters.
>> - Refactor code to reduce code duplication
>>Address review comments
>>Add more test vectors
>> - Changed AlgorithmParameters impls to register under AES/KW/NoPadding and
>>AES/KWP/NoPadding
>> - Restored Iv algorithm parameters impl.
>> - 8248268: Support KWP in addition to KW
>>
>>Updated existing AESWrap support with AES/KW/NoPadding cipher
>>transformation. Added support for AES/KWP/NoPadding and
>>AES/KW/PKCS5Padding support to SunJCE provider.
>
> src/java.base/share/classes/com/sun/crypto/provider/AESParameters.java line
> 50:
>
>> 48:
>> 49: public AESParameters() {
>> 50: core = new BlockCipherParamsCore(AESConstants.AES_BLOCK_SIZE, 4,
>> 8);
>
> 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.
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.
> src/java.base/share/classes/com/sun/crypto/provider/BlockCipherParamsCore.java
> line 52:
>
>> 50: private byte[] iv = null;
>> 51:
>> 52: private int[] moreSizes = null;
>
> 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.
The problem with this is that this assumes that the specified paramSpec
argument of the init() method is always valid.
-
PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v7]
On Fri, 21 May 2021 19:15:49 GMT, Xue-Lei Andrew Fan wrote: >> Valerie Peng has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains seven commits: >> >> - Merge master into JDK-8248268 >> - Minor update to address review comments. >> - Changed AESParameters to allow 4-byte, 8-byte IVs and removed >>KWParameters and KWPParameters. >> - Refactor code to reduce code duplication >>Address review comments >>Add more test vectors >> - Changed AlgorithmParameters impls to register under AES/KW/NoPadding and >>AES/KWP/NoPadding >> - Restored Iv algorithm parameters impl. >> - 8248268: Support KWP in addition to KW >> >>Updated existing AESWrap support with AES/KW/NoPadding cipher >>transformation. Added support for AES/KWP/NoPadding and >>AES/KW/PKCS5Padding support to SunJCE provider. > > src/java.base/share/classes/com/sun/crypto/provider/BlockCipherParamsCore.java > line 81: > >> 79: expectedLen + " bytes long"); >> 80: } >> 81: iv = tmpIv.clone(); > > The moreSizes is not used after initialization. The iv/tmpIv could be a > value other than the block_size. The getEncoded() method would use the iv > value for the encoding. While in the decoding method init(byte[]) method, > the IV sizes other block_size is not considered, and IOE will be thrown. > Could this be a problem? Right, good catch, I will fix it. - PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v7]
On 5/22/2021 1:57 PM, Xue-Lei Andrew Fan wrote:
On Fri, 14 May 2021 00:33:12 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 with a new target base due to a merge
or a rebase. The pull request now contains seven commits:
- Merge master into JDK-8248268
- Minor update to address review comments.
- Changed AESParameters to allow 4-byte, 8-byte IVs and removed
KWParameters and KWPParameters.
- Refactor code to reduce code duplication
Address review comments
Add more test vectors
- Changed AlgorithmParameters impls to register under AES/KW/NoPadding and
AES/KWP/NoPadding
- Restored Iv algorithm parameters impl.
- 8248268: Support KWP in addition to KW
Updated existing AESWrap support with AES/KW/NoPadding cipher
transformation. Added support for AES/KWP/NoPadding and
AES/KW/PKCS5Padding support to SunJCE provider.
Good points, Mike! Thank you!
_Mailing list message from [Michael StJohns](mailto:mstjo...@comcast.net) on
[security-dev](mailto:security-...@mail.openjdk.java.net):_
src/java.base/share/classes/com/sun/crypto/provider/AESParameters.java line 50:
48:
49: public AESParameters() {
50: core = new BlockCipherParamsCore(AESConstants.AES_BLOCK_SIZE, 4, 8);
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:
The mode is KW - it has a fixed length 8 byte non-iv integrity tag.???
KWP is a special case of KW where there's still an 8 byte tag, but part
of it is interpreted by KWP to figure out how much padding was
included.?? KW (AKA RFC3394) permits user (actually specification
specified) IV values.? KWP (aka RFC5649) does not.
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.
Yes. Sorry if I wasn't clearer in earlier emails.
I'd treat KWP as a final (in the Java final sense) extension to KW with
a fixed AIV flag value and a defined interpretation for the 8 byte AIV
tag.? E.g. if you try to specify an IV for KWP, it should fail.?? If
someone else wants to do something like KWP or even twiddle with the 4
byte AIV, let them do their own KW wrap around - which they should be
able to do that via the KW/NoPadding model by specifying their own AIV.?
That should improve interoperability by preventing some monkey see
monkey do errors.
I agreed. Maybe, we could do:
1. Support Alternative IV for KW, for the flexibility as described in section
2.2.3.2 of RFC 3394.
I think that's acceptable. Hmm... is it possible to make the set ICV
call protected rather than public? Generally, the defined ICVs match
up with specific algorithm OIDS. You shouldn't see a third one unless
someone defines a brand new variant. But you want to make it possible
for someone to do an extension.
2. Use default IV if no AIV specified for KW, as described in section 2.2.3.1
of RFC 3394.
Works. Or see the above.
3. Use a fixed IV for KWP, as described in section 3of RFC 5649.
Yes.
This is sort of one reason I was arguing for AES/KW/KWPPadding rather
than AES/KWP/NoPadding.
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.
We keep referring to it as an IV - but it isn't - it's an Integrity
Check Value. Unlike an IV, it doesn't inform the decryption stage, it's
only checked after the decryption is complete. (E.g. in a mode with an
IV, you need to know the IV before you encrypt and before you decrypt).
Then there's this from Section 6.3 of SP800-38F.
Let S = ICV2|| [len(P)/8]32 || P|| PAD
Step 5 of the KWP-AE algorithm basically passes the A65959A6 value
pre-prepended to the pad length both pre-pended to the padded data
through to W().
The equivalent for KW-AE is step 2
Re: RFR: 8248268: Support KWP in addition to KW [v7]
On Fri, 14 May 2021 00:33:12 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 with a new target base due to a
> merge or a rebase. The pull request now contains seven commits:
>
> - Merge master into JDK-8248268
> - Minor update to address review comments.
> - Changed AESParameters to allow 4-byte, 8-byte IVs and removed
>KWParameters and KWPParameters.
> - Refactor code to reduce code duplication
>Address review comments
>Add more test vectors
> - Changed AlgorithmParameters impls to register under AES/KW/NoPadding and
>AES/KWP/NoPadding
> - Restored Iv algorithm parameters impl.
> - 8248268: Support KWP in addition to KW
>
>Updated existing AESWrap support with AES/KW/NoPadding cipher
>transformation. Added support for AES/KWP/NoPadding and
>AES/KW/PKCS5Padding support to SunJCE provider.
Good points, Mike! Thank you!
> _Mailing list message from [Michael StJohns](mailto:mstjo...@comcast.net) on
> [security-dev](mailto:security-...@mail.openjdk.java.net):_
> > src/java.base/share/classes/com/sun/crypto/provider/AESParameters.java line
> > 50:
> > > 48:
> > > 49: public AESParameters() {
> > > 50: core = new BlockCipherParamsCore(AESConstants.AES_BLOCK_SIZE,
> > > 4, 8);
> > > 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:
>
> The mode is KW - it has a fixed length 8 byte non-iv integrity tag.???
> KWP is a special case of KW where there's still an 8 byte tag, but part
> of it is interpreted by KWP to figure out how much padding was
> included.?? KW (AKA RFC3394) permits user (actually specification
> specified) IV values.? KWP (aka RFC5649) does not.
>
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'd treat KWP as a final (in the Java final sense) extension to KW with
> a fixed AIV flag value and a defined interpretation for the 8 byte AIV
> tag.? E.g. if you try to specify an IV for KWP, it should fail.?? If
> someone else wants to do something like KWP or even twiddle with the 4
> byte AIV, let them do their own KW wrap around - which they should be
> able to do that via the KW/NoPadding model by specifying their own AIV.?
> That should improve interoperability by preventing some monkey see
> monkey do errors.
>
I agreed. Maybe, we could do:
1. Support Alternative IV for KW, for the flexibility as described in section
2.2.3.2 of RFC 3394.
2. Use default IV if no AIV specified for KW, as described in section 2.2.3.1
of RFC 3394.
3. Use a fixed IV for KWP, as described in section 3of RFC 5649.
> This is sort of one reason I was arguing for AES/KW/KWPPadding rather
> than AES/KWP/NoPadding.
>
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.
-
PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v7]
In line
On 5/21/2021 5:01 PM, Xue-Lei Andrew Fan wrote:
On Fri, 14 May 2021 00:33:12 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 with a new target base due to a merge
or a rebase. The pull request now contains seven commits:
- Merge master into JDK-8248268
- Minor update to address review comments.
- Changed AESParameters to allow 4-byte, 8-byte IVs and removed
KWParameters and KWPParameters.
- Refactor code to reduce code duplication
Address review comments
Add more test vectors
- Changed AlgorithmParameters impls to register under AES/KW/NoPadding and
AES/KWP/NoPadding
- Restored Iv algorithm parameters impl.
- 8248268: Support KWP in addition to KW
Updated existing AESWrap support with AES/KW/NoPadding cipher
transformation. Added support for AES/KWP/NoPadding and
AES/KW/PKCS5Padding support to SunJCE provider.
src/java.base/share/classes/com/sun/crypto/provider/AESParameters.java line 50:
48:
49: public AESParameters() {
50: core = new BlockCipherParamsCore(AESConstants.AES_BLOCK_SIZE, 4, 8);
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:
The mode is KW - it has a fixed length 8 byte non-iv integrity tag.
KWP is a special case of KW where there's still an 8 byte tag, but part
of it is interpreted by KWP to figure out how much padding was
included. KW (AKA RFC3394) permits user (actually specification
specified) IV values. KWP (aka RFC5649) does not.
I'd treat KWP as a final (in the Java final sense) extension to KW with
a fixed AIV flag value and a defined interpretation for the 8 byte AIV
tag. E.g. if you try to specify an IV for KWP, it should fail. If
someone else wants to do something like KWP or even twiddle with the 4
byte AIV, let them do their own KW wrap around - which they should be
able to do that via the KW/NoPadding model by specifying their own AIV.
That should improve interoperability by preventing some monkey see
monkey do errors.
This is sort of one reason I was arguing for AES/KW/KWPPadding rather
than AES/KWP/NoPadding.
Mike
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.
src/java.base/share/classes/com/sun/crypto/provider/BlockCipherParamsCore.java
line 52:
50: private byte[] iv = null;
51:
52: private int[] moreSizes = null;
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.
src/java.base/share/classes/com/sun/crypto/provider/BlockCipherParamsCore.java
line 81:
79: expectedLen + " bytes long");
80: }
81: iv = tmpIv.clone();
The moreSizes is not used after initialization. The iv/tmpIv could be a value
other than the block_size. The getEncoded() method would use the iv value for
the encoding. While in the decoding method init(byte[]) method, the IV sizes
other block_size is not considered, and IOE will be thrown. Could this be a
problem?
-
PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v7]
On Fri, 14 May 2021 00:33:12 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 with a new target base due to a
> merge or a rebase. The pull request now contains seven commits:
>
> - Merge master into JDK-8248268
> - Minor update to address review comments.
> - Changed AESParameters to allow 4-byte, 8-byte IVs and removed
>KWParameters and KWPParameters.
> - Refactor code to reduce code duplication
>Address review comments
>Add more test vectors
> - Changed AlgorithmParameters impls to register under AES/KW/NoPadding and
>AES/KWP/NoPadding
> - Restored Iv algorithm parameters impl.
> - 8248268: Support KWP in addition to KW
>
>Updated existing AESWrap support with AES/KW/NoPadding cipher
>transformation. Added support for AES/KWP/NoPadding and
>AES/KW/PKCS5Padding support to SunJCE provider.
src/java.base/share/classes/com/sun/crypto/provider/ConstructKeys.java line 176:
> 174: return ConstructKeys.constructPublicKey(encoding,
> keyAlgorithm);
> 175: default:
> 176: throw new RuntimeException("Unsupported key type");
Good improvement to thrown an exception rather than return "null"!
Is NoSuchAlgorithmException fitted better the specification? See also the
comment in KeyWrapCipher.engineUnwrap().
src/java.base/share/classes/com/sun/crypto/provider/ConstructKeys.java line 190:
> 188: return constructKey(encoding, keyAlgorithm, keyType);
> 189: } else {
> 190: byte[] encoding2 = Arrays.copyOfRange(encoding, ofs, ofs
> + len);
The array will be copied again in the X509EncodedKeySpec and
PKCS8EncodedKeySpec. It is out of this scope, but it may be a performance
improvement if adding X509EncodedKeySpec and PKCS8EncodedKeySpec constructors
that accept array offset, so that the array copy here could be avoid.
src/java.base/share/classes/com/sun/crypto/provider/ConstructKeys.java line 192:
> 190: byte[] encoding2 = Arrays.copyOfRange(encoding, ofs, ofs
> + len);
> 191: try {
> 192: return constructKey(encoding2, keyAlgorithm,
> keyType);
The two constructKey methods are depended on each other. It may improve the
readability if only having one-way dependence, by moving the switch-block logic
from the former constructKey() to the later one.
static final Key constructKey(byte[] encoding, String keyAlgorithm,
int keyType) throws InvalidKeyException, NoSuchAlgorithmException {
return constructKey(encoding, 0. encoding.length, keyAlgorithm, keyType);
}
With this re-org, the array copy could be avoid if the key type is unknown.
src/java.base/share/classes/com/sun/crypto/provider/KeyWrapCipher.java line 710:
> 708: outLen = helperDecrypt(dataBuf, dataIdx);
> 709: return ConstructKeys.constructKey(dataBuf, 0, outLen,
> 710: wrappedKeyAlgorithm, wrappedKeyType);
Per the specification of engineUnwrap, maybe, the ConstructKeys.constructKey()
implementation could throw NoSuchAlgorithmException if wrappedKeyType is
unknown. See the common in the the ConstructKeys.constructKey() implementation.
-
PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v7]
On Fri, 14 May 2021 00:33:12 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 with a new target base due to a
> merge or a rebase. The pull request now contains seven commits:
>
> - Merge master into JDK-8248268
> - Minor update to address review comments.
> - Changed AESParameters to allow 4-byte, 8-byte IVs and removed
>KWParameters and KWPParameters.
> - Refactor code to reduce code duplication
>Address review comments
>Add more test vectors
> - Changed AlgorithmParameters impls to register under AES/KW/NoPadding and
>AES/KWP/NoPadding
> - Restored Iv algorithm parameters impl.
> - 8248268: Support KWP in addition to KW
>
>Updated existing AESWrap support with AES/KW/NoPadding cipher
>transformation. Added support for AES/KWP/NoPadding and
>AES/KW/PKCS5Padding support to SunJCE provider.
src/java.base/share/classes/com/sun/crypto/provider/AESParameters.java line 50:
> 48:
> 49: public AESParameters() {
> 50: core = new BlockCipherParamsCore(AESConstants.AES_BLOCK_SIZE, 4,
> 8);
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.
src/java.base/share/classes/com/sun/crypto/provider/BlockCipherParamsCore.java
line 52:
> 50: private byte[] iv = null;
> 51:
> 52: private int[] moreSizes = null;
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.
src/java.base/share/classes/com/sun/crypto/provider/BlockCipherParamsCore.java
line 81:
> 79: expectedLen + " bytes long");
> 80: }
> 81: iv = tmpIv.clone();
The moreSizes is not used after initialization. The iv/tmpIv could be a value
other than the block_size. The getEncoded() method would use the iv value for
the encoding. While in the decoding method init(byte[]) method, the IV sizes
other block_size is not considered, and IOE will be thrown. Could this be a
problem?
-
PR: https://git.openjdk.java.net/jdk/pull/2404
Re: RFR: 8248268: Support KWP in addition to KW [v7]
> 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 with a new target base due to a merge or a rebase. The pull request now contains seven commits: - Merge master into JDK-8248268 - Minor update to address review comments. - Changed AESParameters to allow 4-byte, 8-byte IVs and removed KWParameters and KWPParameters. - Refactor code to reduce code duplication Address review comments Add more test vectors - Changed AlgorithmParameters impls to register under AES/KW/NoPadding and AES/KWP/NoPadding - Restored Iv algorithm parameters impl. - 8248268: Support KWP in addition to KW Updated existing AESWrap support with AES/KW/NoPadding cipher transformation. Added support for AES/KWP/NoPadding and AES/KW/PKCS5Padding support to SunJCE provider. - Changes: https://git.openjdk.java.net/jdk/pull/2404/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2404=06 Stats: 2592 lines in 16 files changed: 1930 ins; 555 del; 107 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
