Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v2]
On Thu, 8 Apr 2021 01:33:56 GMT, Weijun Wang wrote:
>> This code change does not intend to support multiple byte tags. Instead, it
>> aims to fail more gracefully when such a tag is encountered. For `DerValue`
>> constructors from an encoding (type I), an `IOException` will be thrown
>> since it's already in the throws clause. For constructors from tag and value
>> (type II), an `IllegalArgumentException` will be thrown. All existing type
>> II callers inside JDK use tag numbers smaller than 31.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> make sure test fails before code change
src/java.base/share/classes/sun/security/util/DerValue.java line 322:
> 320: tag = buf[pos++];
> 321: if ((tag & 0x1f) == 0x1f) {
> 322: throw new IOException("Tag number cannot exceed 30");
It may be safe if not support multiple bytes tag in the current implementation
of JDK, especially the ASN.1 implementation is private. However, multiple
bytes tag is a legal form of ASN.1 encoding, I think. It would be nice to have
a comment to state that this form is not support yet, and we may consider it in
the future if needed. It may be helpful for future code maintenance.
The exception message, "Tag number cannot exceed 30", may be not accuracy. I
think tag number can exceed 30 per the specification, but JDK does not support
it yet because we did not run into such tags in practice. I may use some words
like: "Tag number exceed 30 is not supported".
-
PR: https://git.openjdk.java.net/jdk/pull/3391
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v6]
On Thu, 8 Apr 2021 01:48:20 GMT, Hai-May Chao wrote: >> Please review the changes that adds the -signer option to keytool >> -genkeypair command. As key agreement algorithms do not have a signing >> algorithm, the specified signer's private key will be used to sign and >> generate a key agreement certificate. >> CSR review is at: https://bugs.openjdk.java.net/browse/JDK-8264325 > > Hai-May Chao has updated the pull request incrementally with one additional > commit since the last revision: > > test update with comment Well done. - Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v5]
On Thu, 8 Apr 2021 01:42:16 GMT, Hai-May Chao wrote:
>> test/jdk/sun/security/tools/keytool/GenKeyPairSigner.java line 96:
>>
>>> 94:
>>> 95: Certificate[] certChain = kstore.getCertificateChain("e1");
>>> 96: if (certChain.length != 2) {
>>
>> Try using `Asserts` class in `/test/lib` to make code simpler. Also, why not
>> throw an exception but call `System.exit(1)`? We usually do not call this
>> method in a test because the test framework must take great care so that
>> itself does not get terminated.
>
> Changed to throw the exception for errors. Meanwhile, the test is pretty
> straightforward/simple, and using if comparison should serve its testing need
> and it does not make the code complicated.
You can choose your style, but `Asserts.assertEquals(certChain.length, 2,
"Generated cert chain is in error")` is definitely simpler and will give you
more info when it fails.
-
PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v5]
On Thu, 8 Apr 2021 01:48:20 GMT, Hai-May Chao wrote: >> In testSignerJKS() has made sure that the new entry created with new key >> password can *only* be accessed when a correct key password is provided in >> order to retrieve the corresponding signer’s private key. > > The new entry protected by the new key password is an existing function, and > its testing should have been covered. OK. - PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v5]
On Thu, 8 Apr 2021 01:42:18 GMT, Hai-May Chao wrote: >> test/jdk/sun/security/tools/keytool/GenKeyPairSigner.java line 299: >> >>> 297: System.exit(1); >>> 298: } >>> 299: >> >> Since you are here, you can check if the new entry is indeed protected by >> the new key password. > > In testSignerJKS() has made sure that the new entry created with new key > password can *only* be accessed when a correct key password is provided in > order to retrieve the corresponding signer’s private key. The new entry protected by the new key password is an existing function, and its testing should have been covered. - PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v5]
On Thu, 8 Apr 2021 00:01:57 GMT, Weijun Wang wrote:
>> Hai-May Chao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update with review comments
>
> src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java
> line 88:
>
>> 86: */
>> 87: public CertAndKeyGen (String keyType, String sigAlg, String
>> providerName)
>> 88: throws NoSuchAlgorithmException, NoSuchProviderException
>
> Indent the line 8 spaces further.
Done.
> test/jdk/sun/security/tools/keytool/GenKeyPairSigner.java line 96:
>
>> 94:
>> 95: Certificate[] certChain = kstore.getCertificateChain("e1");
>> 96: if (certChain.length != 2) {
>
> Try using `Asserts` class in `/test/lib` to make code simpler. Also, why not
> throw an exception but call `System.exit(1)`? We usually do not call this
> method in a test because the test framework must take great care so that
> itself does not get terminated.
Changed to throw the exception for errors. Meanwhile, the test is pretty
straightforward/simple, and using if comparison should serve its testing need
and it does not make the code complicated.
> test/jdk/sun/security/tools/keytool/GenKeyPairSigner.java line 299:
>
>> 297: System.exit(1);
>> 298: }
>> 299:
>
> Since you are here, you can check if the new entry is indeed protected by the
> new key password.
In testSignerJKS() has made sure that the new entry created with new key
password can *only* be accessed when a correct key password is provided in
order to retrieve the corresponding signer’s private key.
-
PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v6]
> Please review the changes that adds the -signer option to keytool -genkeypair > command. As key agreement algorithms do not have a signing algorithm, the > specified signer's private key will be used to sign and generate a key > agreement certificate. > CSR review is at: https://bugs.openjdk.java.net/browse/JDK-8264325 Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision: test update with comment - Changes: - all: https://git.openjdk.java.net/jdk/pull/3281/files - new: https://git.openjdk.java.net/jdk/pull/3281/files/3be5bc98..63f6a4ac Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3281=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3281=04-05 Stats: 36 lines in 2 files changed: 0 ins; 17 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/3281.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3281/head:pull/3281 PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding [v2]
> This code change does not intend to support multiple byte tags. Instead, it > aims to fail more gracefully when such a tag is encountered. For `DerValue` > constructors from an encoding (type I), an `IOException` will be thrown since > it's already in the throws clause. For constructors from tag and value (type > II), an `IllegalArgumentException` will be thrown. All existing type II > callers inside JDK use tag numbers smaller than 31. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: make sure test fails before code change - Changes: - all: https://git.openjdk.java.net/jdk/pull/3391/files - new: https://git.openjdk.java.net/jdk/pull/3391/files/d25d3fb2..46b3700b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3391=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3391=00-01 Stats: 11 lines in 1 file changed: 6 ins; 1 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/3391.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3391/head:pull/3391 PR: https://git.openjdk.java.net/jdk/pull/3391
RFR: 8264864: Multiple byte tag not supported by ASN.1 encoding
This code change does not intend to support multiple byte tags. Instead, it aims to fail more gracefully when such a tag is encountered. For `DerValue` constructors from an encoding (type I), an `IOException` will be thrown since it's already in the throws clause. For constructors from tag and value (type II), an `IllegalArgumentException` will be thrown. All existing type II callers inside JDK use tag numbers smaller than 31. - Commit messages: - 8264864: Multiple byte tag not supported by ASN.1 encoding Changes: https://git.openjdk.java.net/jdk/pull/3391/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3391=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264864 Stats: 78 lines in 2 files changed: 77 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3391.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3391/head:pull/3391 PR: https://git.openjdk.java.net/jdk/pull/3391
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v5]
On Wed, 7 Apr 2021 23:17:53 GMT, Hai-May Chao wrote:
>> Please review the changes that adds the -signer option to keytool
>> -genkeypair command. As key agreement algorithms do not have a signing
>> algorithm, the specified signer's private key will be used to sign and
>> generate a key agreement certificate.
>> CSR review is at: https://bugs.openjdk.java.net/browse/JDK-8264325
>
> Hai-May Chao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update with review comments
No comment on src side.
src/java.base/share/classes/sun/security/tools/keytool/CertAndKeyGen.java line
88:
> 86: */
> 87: public CertAndKeyGen (String keyType, String sigAlg, String
> providerName)
> 88: throws NoSuchAlgorithmException, NoSuchProviderException
Indent the line 8 spaces further.
test/jdk/sun/security/tools/keytool/GenKeyPairSigner.java line 96:
> 94:
> 95: Certificate[] certChain = kstore.getCertificateChain("e1");
> 96: if (certChain.length != 2) {
Try using `Asserts` class in `/test/lib` to make code simpler. Also, why not
throw an exception but call `System.exit(1)`? We usually do not call this
method in a test because the test framework must take great care so that itself
does not get terminated.
test/jdk/sun/security/tools/keytool/GenKeyPairSigner.java line 299:
> 297: System.exit(1);
> 298: }
> 299:
Since you are here, you can check if the new entry is indeed protected by the
new key password.
-
PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v5]
> Please review the changes that adds the -signer option to keytool -genkeypair > command. As key agreement algorithms do not have a signing algorithm, the > specified signer's private key will be used to sign and generate a key > agreement certificate. > CSR review is at: https://bugs.openjdk.java.net/browse/JDK-8264325 Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision: update with review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/3281/files - new: https://git.openjdk.java.net/jdk/pull/3281/files/157601f2..3be5bc98 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3281=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3281=03-04 Stats: 28 lines in 3 files changed: 0 ins; 12 del; 16 mod Patch: https://git.openjdk.java.net/jdk/pull/3281.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3281/head:pull/3281 PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v4]
On Fri, 2 Apr 2021 11:52:16 GMT, Weijun Wang wrote: >>> Maybe we don't need to resolve it in this code change. If we look carefully >>> at RFC 8410 Sections 10.1 and 10.2, it shows the X25519 certificate in 10.2 >>> is using the signer's SKID in 10.1 as its own SKID and it has no AKID. >>> Currently, keytool will generate a new SKID and use signer's SKID as AKID. >>> If we really want to generate a certificate that's identical to the one in >>> the RFC, we'll need a way to tell keytool to omit the AKID (something like >>> "-ext akid=none"). >> >> Better to use AKID for certification path building. > > I don't mean we will remove it by default. Just think there needs a way to > remove either AKID or SKID, because we always generate them automatically. The support for omitting AKID will be tracked by different PR if needed. - PR: https://git.openjdk.java.net/jdk/pull/3281
Re: RFR: 8260693: Provide the support for specifying a signer in keytool -genkeypair [v4]
On Fri, 2 Apr 2021 01:40:16 GMT, Weijun Wang wrote:
>> Hai-May Chao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update with review comments
>
> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 1978:
>
>> 1976: keypair.getPublicKeyAnyway(),
>> 1977: signerSubjectKeyId);
>> 1978: } else {
>
> No need for an if-else block? `signerSubjectKeyId` is null in this case.
Removed.
> src/java.base/share/classes/sun/security/tools/keytool/Main.java line 2013:
>
>> 2011: x500Name};
>> 2012: System.err.println(form.format(source));
>> 2013: }
>
> Either put the declaration of `source` outside the if-else block and move the
> println line outside; or, move the declaration of `form` inside. It looks
> confusing that `form` is declared outside and `source` inside.
Done.
> src/java.base/share/classes/sun/security/tools/keytool/Resources.java line
> 310:
>
>> 308: "Generating {0} bit {1} key pair and self-signed
>> certificate ({2}) with a validity of {3} days\n\tfor: {4}"},
>> 309:
>> {"Generating.keysize.bit.keyAlgName.key.pair.and.a.certificate.sigAlgName.issued.by.an.entry.signerAlias.with.a.validity.of.validality.days.for",
>> 310: "Generating {0} bit {1} key pair and a certificate
>> ({2}) issued by an entry <{3}> with a validity of {4} days\n\tfor: {5}"},
>
> I feel "issued by <{3}>" sounds more normal. No need to say "an entry". You
> decide it.
Removed "an entry".
-
PR: https://git.openjdk.java.net/jdk/pull/3281
RE: [11u] RFR: 8226374: Restrict TLS signature schemes and named groups
Hmm, could have sworn... Thanks, Paul -Original Message- From: "Langer, Christoph" Date: Wednesday, April 7, 2021 at 3:16 PM To: "Hohensee, Paul" , "Doerr, Martin" , jdk-updates-dev , security-dev Cc: "Lindenmaier, Goetz" Subject: RE: [11u] RFR: 8226374: Restrict TLS signature schemes and named groups Hi Paul, thanks for the review. The CSR that Martin mentions is the one that Oracle has filed for 11.0.12-oracle. so we can simply reuse it. As for 13, there exists a CSR as well: JDK-8256335 Best regards Christoph > -Original Message- > From: Hohensee, Paul > Sent: Mittwoch, 7. April 2021 23:42 > To: Doerr, Martin ; jdk-updates-dev d...@openjdk.java.net>; security-dev > Cc: Lindenmaier, Goetz ; Langer, Christoph > > Subject: Re: [11u] RFR: 8226374: Restrict TLS signature schemes and named > groups > > The backport looks fine, except there's a missing blank line after FFDHE_2048 > in NamedGroup.java. :) Thanks for filing a CSR (there doesn't seem to be one > for the 13u backport: perhaps Yan will add one after the fact). I'm not a > security person, so it would be great if someone who is reviews the CSR to > see if there are any 11u-specific issues with it. > > Thanks, > Paul > > -Original Message- > From: jdk-updates-dev on > behalf of "Doerr, Martin" > Date: Wednesday, April 7, 2021 at 9:10 AM > To: jdk-updates-dev , security-dev > > Cc: "Lindenmaier, Goetz" , "Langer, > Christoph" > Subject: [11u] RFR: 8226374: Restrict TLS signature schemes and named > groups > > Hi, > > JDK-8226374 is backported to 11.0.12-oracle. I'd like to backport it for > parity. > It doesn't apply cleanly. I've taken the 13u backport as source because it > resolves the wrong backport order with JDK-8242141. > > Bug: > https://bugs.openjdk.java.net/browse/JDK-8226374 > > 11u CSR: > https://bugs.openjdk.java.net/browse/JDK-8264555 > > Original change (JDK14): > https://hg.openjdk.java.net/jdk/jdk/rev/a93b7b28f644 > > 13u backport: > https://github.com/openjdk/jdk13u-dev/commit/384445d2 > > 11u rejected hunks (integrated manually): > http://cr.openjdk.java.net/~mdoerr/8226374_TLS_11u/8226374_TLS_rej.txt > > my new 11u backport: > http://cr.openjdk.java.net/~mdoerr/8226374_TLS_11u/webrev.00/ > > Please review. > > Best regards, > Martin >
RE: [11u] RFR: 8226374: Restrict TLS signature schemes and named groups
Hi Paul, thanks for the review. The CSR that Martin mentions is the one that Oracle has filed for 11.0.12-oracle. so we can simply reuse it. As for 13, there exists a CSR as well: JDK-8256335 Best regards Christoph > -Original Message- > From: Hohensee, Paul > Sent: Mittwoch, 7. April 2021 23:42 > To: Doerr, Martin ; jdk-updates-dev d...@openjdk.java.net>; security-dev > Cc: Lindenmaier, Goetz ; Langer, Christoph > > Subject: Re: [11u] RFR: 8226374: Restrict TLS signature schemes and named > groups > > The backport looks fine, except there's a missing blank line after FFDHE_2048 > in NamedGroup.java. :) Thanks for filing a CSR (there doesn't seem to be one > for the 13u backport: perhaps Yan will add one after the fact). I'm not a > security person, so it would be great if someone who is reviews the CSR to > see if there are any 11u-specific issues with it. > > Thanks, > Paul > > -Original Message- > From: jdk-updates-dev on > behalf of "Doerr, Martin" > Date: Wednesday, April 7, 2021 at 9:10 AM > To: jdk-updates-dev , security-dev > > Cc: "Lindenmaier, Goetz" , "Langer, > Christoph" > Subject: [11u] RFR: 8226374: Restrict TLS signature schemes and named > groups > > Hi, > > JDK-8226374 is backported to 11.0.12-oracle. I'd like to backport it for > parity. > It doesn't apply cleanly. I've taken the 13u backport as source because it > resolves the wrong backport order with JDK-8242141. > > Bug: > https://bugs.openjdk.java.net/browse/JDK-8226374 > > 11u CSR: > https://bugs.openjdk.java.net/browse/JDK-8264555 > > Original change (JDK14): > https://hg.openjdk.java.net/jdk/jdk/rev/a93b7b28f644 > > 13u backport: > https://github.com/openjdk/jdk13u-dev/commit/384445d2 > > 11u rejected hunks (integrated manually): > http://cr.openjdk.java.net/~mdoerr/8226374_TLS_11u/8226374_TLS_rej.txt > > my new 11u backport: > http://cr.openjdk.java.net/~mdoerr/8226374_TLS_11u/webrev.00/ > > Please review. > > Best regards, > Martin >
Re: [11u] RFR: 8226374: Restrict TLS signature schemes and named groups
The backport looks fine, except there's a missing blank line after FFDHE_2048 in NamedGroup.java. :) Thanks for filing a CSR (there doesn't seem to be one for the 13u backport: perhaps Yan will add one after the fact). I'm not a security person, so it would be great if someone who is reviews the CSR to see if there are any 11u-specific issues with it. Thanks, Paul -Original Message- From: jdk-updates-dev on behalf of "Doerr, Martin" Date: Wednesday, April 7, 2021 at 9:10 AM To: jdk-updates-dev , security-dev Cc: "Lindenmaier, Goetz" , "Langer, Christoph" Subject: [11u] RFR: 8226374: Restrict TLS signature schemes and named groups Hi, JDK-8226374 is backported to 11.0.12-oracle. I'd like to backport it for parity. It doesn't apply cleanly. I've taken the 13u backport as source because it resolves the wrong backport order with JDK-8242141. Bug: https://bugs.openjdk.java.net/browse/JDK-8226374 11u CSR: https://bugs.openjdk.java.net/browse/JDK-8264555 Original change (JDK14): https://hg.openjdk.java.net/jdk/jdk/rev/a93b7b28f644 13u backport: https://github.com/openjdk/jdk13u-dev/commit/384445d2 11u rejected hunks (integrated manually): http://cr.openjdk.java.net/~mdoerr/8226374_TLS_11u/8226374_TLS_rej.txt my new 11u backport: http://cr.openjdk.java.net/~mdoerr/8226374_TLS_11u/webrev.00/ Please review. Best regards, Martin
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v3]
On Tue, 6 Apr 2021 18:30:31 GMT, Martin Balao wrote: >> Hi, >> >> I'd like to propose a fix for JDK-8261355 [1]. >> >> The scheme used for holding data and padding while performing encryption >> operations is almost the same than the existing one for decryption. The only >> difference is that encryption does not require a block-sized buffer to be >> always held because there is no need, upon an update call, to determine >> which bytes are real output for the caller and which are padding -as it's >> required for decryption-. I added a couple of comments in implUpdate to >> explain this. >> >> No regressions observed in jdk/sun/security/pkcs11. >> >> Thanks, >> Martin.- >> >> -- >> [1] - https://bugs.openjdk.java.net/browse/JDK-8261355 > > Martin Balao has updated the pull request incrementally with one additional > commit since the last revision: > > Bug fixes and improvements as discussed in the PR Marked as reviewed by valeriep (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2510
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding
On Wed, 7 Apr 2021 16:42:53 GMT, Valerie Peng wrote: >> @valeriepeng please take a look at my comments in-line and the new proposal >> here: >> https://github.com/openjdk/jdk/pull/2510/commits/b47c03edff1f48b925a67203102385470ac1afdc >> >> Thanks, >> Martin.- > > Sure, will take another look. Thanks! > Valerie Rest of changes look good. - PR: https://git.openjdk.java.net/jdk/pull/2510
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding [v2]
On Tue, 6 Apr 2021 14:26:00 GMT, Martin Balao wrote: >> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java >> line 265: >> >>> 263: // NSS requires block-sized updates in multi-part >>> operations. >>> 264: reqBlockUpdates = ((tokenLabel[0] == 'N' && >>> tokenLabel[1] == 'S' >>> 265: && tokenLabel[2] == 'S') ? true : false); >> >> IIRC, depending on how the impl is registered, engineSetPadding(String) may >> not always be called. It's probably safer to set this in engineInit(...)? > > Looks to me that engineSetPadding is always called from the P11Cipher > constructor. I thought that was a good location to set the reqBlockUpdates > variable because it's next to the paddingObj initialization; which is a > pre-requisite for reqBlockUpdates to be used. In other words, if we have no > Java-side padding (paddingObj == null), reqBlockUpdates won't be used and we > don't even pay the price of setting it. Ok. - PR: https://git.openjdk.java.net/jdk/pull/2510
Re: RFR: 8248268: Support KWP in addition to KW [v4]
I agree that the response from Housley certainly supports that
"AutoPadding" is likely a safe mode to use. I still would prefer not to see
it (keeping things simple) but don't really have any objections to it.
For KW+PKCS5, I have (unfortunately) seen this deployed in the real world
and had to assist Java developers in manually removing PKCS5 padding from
the result of KW decryption. (No OIDs were used in any parts of these
designs, so I cannot say what would have been used.) So, since I had to
help multiple people write this code already, I really cannot object to
adding support for it. The JCA supports many algorithms which shouldn't be
used but exist for compatibility and interactions with other systems (DES,
RC4, etc.). This would be yet another algorithm of that type. (Arguably,
both KW and KWP should probably be replaced by AES-GCM for modern systems,
but that is an entirely different discussion.)
As for OIDs, that seems somewhat unrelated and I don't think we need
something new. I've rarely needed to use the OIDs for KW or KWP and
suspect that we could simply choose the one that corresponds to the
algorithm we actually used.
I've also encountered HSMs which added PKCS5 padding prior to KW so that
all output was 8-byte aligned. That was very frustrating to deal with as it
was not clearly documented at the time.
Finally, I believe that the encoding question is separate from the wrapping
question. Each key type should (and generally does) define how to encode it
as an octet string. Then you apply the relevant wrapping/unwrapping
algorithm to that encoding. KW/KWP should not define how to encode keys any
more than RSA should define how to wrap a serialized RSA key. (However, I
may have misunderstood your comment "... RFC written that specifies the
default encodings for keys wrapped by this algorithm.")
Greg
On Wed, Apr 7, 2021 at 11:51 AM Michael StJohns
wrote:
> *sigh* Minor correction in line.
>
> On 4/7/2021 2:49 PM, Michael StJohns wrote:
>
> On 4/7/2021 1:28 PM, Greg Rubin wrote:
>
> Mike,
>
> Yes, this was in response to your comment.
>
> I'm aware that the IV really serves more as an integrity check and mode
> signalling mechanism than anything else. My concern is that in the past few
> years I've seen various issues related to "in band signalling" where
> something about the ciphertext (or directly associated metadata) changes
> how the data is decrypted and authenticated. This has reached the level
> where several cryptographic forums I participate in are starting to
> consider it a full anti-pattern.
>
> The proposed "AutoPadding" mode is an example of in-band signalling in
> which an externally provided ciphertext changes how it is interpreted.
> While I cannot personally think of a specific risk in this case, I would be
> inclined not to include this mode unless there is a strong driving need
> from our users. While I have definitely seen people not knowing if their
> data was encrypted with KW or KW+PKCS5/7, I haven't personally seen
> uncertainty between KW and KWP. (I also haven't worked with all possible
> HSMs, just a few of them.) So, from a position of caution, I'd avoid
> "AutoPadding", but this is a preference based on current best-practice
> rather than a strong objection based on specific concerns or risks.
>
>
> I sent a note off to the original mode inventor - Russ Housley:
>
> Can you think of any reason why there might be an issue with providing an
> autopadding mode for KW/KWP (e.g. select which to use based on the input
> data for encrypt and determine which was used after running the unwrap
> function but before removing the initial block and any padding)?
>
> I got back:
>
> As long as every party supports both modes, you could use KW id [sic - I
> think he meant "is"]
>
> "if" not "is"
>
> the inout is a multiple of 64 bits, otherwise use KWP. Of course, the
> algorithm identifier needs to be set appropriately.
>
> Which sort of confirms what I thought, but added a question: Are there
> algorithm OIDs for KW with PKCS5 padding or do people just use the KW OID(
> 2.16.840.1.101.3.4.1.{5,25,45}? As far as I can tell, there are no OIDs
> for KW with PKCS5.
>
> Does there need to be an autopad OID?
>
> If it were me, I'd be avoiding implementing the PKCS5 padding mode here.
> I can't actually find a specification that includes it and it looks like a
> hack that was fixed by the specification of KWP. I'd prefer not to extend
> the hack's lifetime, given that RFC5649 is 10+ years old.
>
> WRT to HSM uncertainty, I ran into problems especially trying to wrap RSA
> private keys. Turned out that some encoded as 8 byte multiples and some
> did not. In any event, I mentioned HSMs, but I really care about the
> general model for the JCE. I'd *really* like to avoid having to have to
> first figure out the private key encoding length (which may be difficult as
> a provider may not choose to export an unwrapped private key even if its a
> software provider) before
Re: RFR: 8248268: Support KWP in addition to KW [v4]
*sigh* Minor correction in line.
On 4/7/2021 2:49 PM, Michael StJohns wrote:
On 4/7/2021 1:28 PM, Greg Rubin wrote:
Mike,
Yes, this was in response to your comment.
I'm aware that the IV really serves more as an integrity check and
mode signalling mechanism than anything else. My concern is that in
the past few years I've seen various issues related to "in band
signalling" where something about the ciphertext (or directly
associated metadata) changes how the data is decrypted and
authenticated. This has reached the level where several cryptographic
forums I participate in are starting to consider it a full anti-pattern.
The proposed "AutoPadding" mode is an example of in-band signalling
in which an externally provided ciphertext changes how it is
interpreted. While I cannot personally think of a specific risk in
this case, I would be inclined not to include this mode unless there
is a strong driving need from our users. While I have definitely seen
people not knowing if their data was encrypted with KW or KW+PKCS5/7,
I haven't personally seen uncertainty between KW and KWP. (I also
haven't worked with all possible HSMs, just a few of them.) So, from
a position of caution, I'd avoid "AutoPadding", but this is a
preference based on current best-practice rather than a strong
objection based on specific concerns or risks.
I sent a note off to the original mode inventor - Russ Housley:
Can you think of any reason why there might be an issue with
providing an autopadding mode for KW/KWP (e.g. select which to use
based on the input data for encrypt and determine which was used
after running the unwrap function but before removing the initial
block and any padding)?
I got back:
As long as every party supports both modes, you could use KW id [sic
- I think he meant "is"]
"if" not "is"
the inout is a multiple of 64 bits, otherwise use KWP. Of course,
the algorithm identifier needs to be set appropriately.
Which sort of confirms what I thought, but added a question: Are there
algorithm OIDs for KW with PKCS5 padding or do people just use the KW
OID( 2.16.840.1.101.3.4.1.{5,25,45}? As far as I can tell, there are
no OIDs for KW with PKCS5.
Does there need to be an autopad OID?
If it were me, I'd be avoiding implementing the PKCS5 padding mode
here. I can't actually find a specification that includes it and it
looks like a hack that was fixed by the specification of KWP. I'd
prefer not to extend the hack's lifetime, given that RFC5649 is 10+
years old.
WRT to HSM uncertainty, I ran into problems especially trying to wrap
RSA private keys. Turned out that some encoded as 8 byte multiples
and some did not. In any event, I mentioned HSMs, but I really care
about the general model for the JCE. I'd *really* like to avoid having
to have to first figure out the private key encoding length (which may
be difficult as a provider may not choose to export an unwrapped
private key even if its a software provider) before choosing the
wrapping algorithm. Doing it that way just fits the JCE model better.
At some point, there needs to be an RFC written that specifies the
default encodings for keys wrapped by this algorithm.
Later, Mike
Thank you,
Greg
On Sat, Apr 3, 2021 at 4:38 PM Michael StJohns > wrote:
On 4/3/2021 11:35 AM, Greg Rubin wrote:
> 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.
I think this is in response to my comment?
The wrap function can take a Key as an input and can have the unwrap
method produce a Key as an output - indeed it should be used
primarily
for this rather than the more general encrypt/decrypt functions.
The
problem is that the encoding of the key may not be known prior to
the
attempt to wrap it - hence it's not known whether or not padding
need be
applied. This is especially problematic with HSMs. Providing an
AutoPadding mode would allow the wrapping algorithm to decide
whether to
use either of the RFC 3394 (AKA KW) Integrity Check Value (ICV)
or the
RFC5649 (aka KWP) value and padding length.
The key thing to remember here is that the IV (initial value - RFC
language) /ICV (integrity check value - NIST language)actually
isn't an
IV(initialization vector) in the ordinary meaning, it's a flag,
Re: RFR: 8248268: Support KWP in addition to KW [v4]
On 4/7/2021 1:28 PM, Greg Rubin wrote:
Mike,
Yes, this was in response to your comment.
I'm aware that the IV really serves more as an integrity check and
mode signalling mechanism than anything else. My concern is that in
the past few years I've seen various issues related to "in band
signalling" where something about the ciphertext (or directly
associated metadata) changes how the data is decrypted and
authenticated. This has reached the level where several cryptographic
forums I participate in are starting to consider it a full anti-pattern.
The proposed "AutoPadding" mode is an example of in-band signalling in
which an externally provided ciphertext changes how it is
interpreted. While I cannot personally think of a specific risk in
this case, I would be inclined not to include this mode unless there
is a strong driving need from our users. While I have definitely seen
people not knowing if their data was encrypted with KW or KW+PKCS5/7,
I haven't personally seen uncertainty between KW and KWP. (I also
haven't worked with all possible HSMs, just a few of them.) So, from a
position of caution, I'd avoid "AutoPadding", but this is a preference
based on current best-practice rather than a strong objection based on
specific concerns or risks.
I sent a note off to the original mode inventor - Russ Housley:
Can you think of any reason why there might be an issue with providing
an autopadding mode for KW/KWP (e.g. select which to use based on the
input data for encrypt and determine which was used after running the
unwrap function but before removing the initial block and any padding)?
I got back:
As long as every party supports both modes, you could use KW id [sic -
I think he meant "is"] the inout is a multiple of 64 bits, otherwise
use KWP. Of course, the algorithm identifier needs to be set
appropriately.
Which sort of confirms what I thought, but added a question: Are there
algorithm OIDs for KW with PKCS5 padding or do people just use the KW
OID( 2.16.840.1.101.3.4.1.{5,25,45}? As far as I can tell, there are no
OIDs for KW with PKCS5.
Does there need to be an autopad OID?
If it were me, I'd be avoiding implementing the PKCS5 padding mode
here. I can't actually find a specification that includes it and it
looks like a hack that was fixed by the specification of KWP. I'd
prefer not to extend the hack's lifetime, given that RFC5649 is 10+
years old.
WRT to HSM uncertainty, I ran into problems especially trying to wrap
RSA private keys. Turned out that some encoded as 8 byte multiples and
some did not. In any event, I mentioned HSMs, but I really care about
the general model for the JCE. I'd *really* like to avoid having to
have to first figure out the private key encoding length (which may be
difficult as a provider may not choose to export an unwrapped private
key even if its a software provider) before choosing the wrapping
algorithm. Doing it that way just fits the JCE model better.
At some point, there needs to be an RFC written that specifies the
default encodings for keys wrapped by this algorithm.
Later, Mike
Thank you,
Greg
On Sat, Apr 3, 2021 at 4:38 PM Michael StJohns > wrote:
On 4/3/2021 11:35 AM, Greg Rubin wrote:
> 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.
I think this is in response to my comment?
The wrap function can take a Key as an input and can have the unwrap
method produce a Key as an output - indeed it should be used
primarily
for this rather than the more general encrypt/decrypt functions. The
problem is that the encoding of the key may not be known prior to the
attempt to wrap it - hence it's not known whether or not padding
need be
applied. This is especially problematic with HSMs. Providing an
AutoPadding mode would allow the wrapping algorithm to decide
whether to
use either of the RFC 3394 (AKA KW) Integrity Check Value (ICV) or
the
RFC5649 (aka KWP) value and padding length.
The key thing to remember here is that the IV (initial value - RFC
language) /ICV (integrity check value - NIST language)actually
isn't an
IV(initialization vector) in the ordinary meaning, it's a flag,
padding
and integrity indicator and will be fixed for all keys of the same
length that use the
Integrated: 8262316: Reducing locks in RSA Blinding
On Wed, 31 Mar 2021 21:47:24 GMT, Anthony Scarpino wrote: > Hi, > > I need a review of the locking change to the RSA blinding code. The problem > was reported that multithreaded performance suffered because there was one > global lock on the many blindings operation. The change reduces locking by > using a ConcurrentLinkedQueue to store the different BlindingParameters that > other threads maybe using. The queue size is limited to prevent sudden > surges in stored BlindingParameters and a WeakHashMap is still used so the > objects can be freed when no longer used. Performance doubles under high > load. > > thanks > > Tony This pull request has now been integrated. Changeset: 7a99a987 Author:Anthony Scarpino URL: https://git.openjdk.java.net/jdk/commit/7a99a987 Stats: 80 lines in 1 file changed: 37 ins; 17 del; 26 mod 8262316: Reducing locks in RSA Blinding Reviewed-by: jnimeh - PR: https://git.openjdk.java.net/jdk/pull/3296
Re: RFR: 8248268: Support KWP in addition to KW [v4]
Mike, Yes, this was in response to your comment. I'm aware that the IV really serves more as an integrity check and mode signalling mechanism than anything else. My concern is that in the past few years I've seen various issues related to "in band signalling" where something about the ciphertext (or directly associated metadata) changes how the data is decrypted and authenticated. This has reached the level where several cryptographic forums I participate in are starting to consider it a full anti-pattern. The proposed "AutoPadding" mode is an example of in-band signalling in which an externally provided ciphertext changes how it is interpreted. While I cannot personally think of a specific risk in this case, I would be inclined not to include this mode unless there is a strong driving need from our users. While I have definitely seen people not knowing if their data was encrypted with KW or KW+PKCS5/7, I haven't personally seen uncertainty between KW and KWP. (I also haven't worked with all possible HSMs, just a few of them.) So, from a position of caution, I'd avoid "AutoPadding", but this is a preference based on current best-practice rather than a strong objection based on specific concerns or risks. Thank you, Greg On Sat, Apr 3, 2021 at 4:38 PM Michael StJohns wrote: > On 4/3/2021 11:35 AM, Greg Rubin wrote: > > 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. > > I think this is in response to my comment? > > The wrap function can take a Key as an input and can have the unwrap > method produce a Key as an output - indeed it should be used primarily > for this rather than the more general encrypt/decrypt functions. The > problem is that the encoding of the key may not be known prior to the > attempt to wrap it - hence it's not known whether or not padding need be > applied. This is especially problematic with HSMs. Providing an > AutoPadding mode would allow the wrapping algorithm to decide whether to > use either of the RFC 3394 (AKA KW) Integrity Check Value (ICV) or the > RFC5649 (aka KWP) value and padding length. > > The key thing to remember here is that the IV (initial value - RFC > language) /ICV (integrity check value - NIST language)actually isn't an > IV(initialization vector) in the ordinary meaning, it's a flag, padding > and integrity indicator and will be fixed for all keys of the same > length that use the specified values. E.g. unlike other modes that > require an initialization vector, you don't need to know the ICV to > decrypt the underlying key stream, but you can (and for that matter > MUST) easily test the recovered first block against the expected ICV to > determine whether the output needs padding removed or not. > > FWIW, the actual cryptographic operations between padded data and > non-padded data (of the right multiple length) are identical. It's only > the pre or post processing that's looking for different data. > > Obviously, this doesn't work if someone provides their own IV - but > that's fairly unlikely. CF CCM and its non-normative example formatting > function appendix A - each and every implementation I've seen uses that > formatting function, even though it isn't actually required by the > standard. I'd be surprised if anyone decided to use a different set of > non-standard IV values. > > If an AutoPadding mode were implemented, I'd throw exceptions if someone > tried to set the IV. > > Later, Mike > > >
Re: RFR: 8261355: No data buffering in SunPKCS11 Cipher encryption when the underlying mechanism has no padding
On Tue, 6 Apr 2021 18:29:56 GMT, Martin Balao wrote: >> I will take a look. >> Thanks~ > > @valeriepeng please take a look at my comments in-line and the new proposal > here: > https://github.com/openjdk/jdk/pull/2510/commits/b47c03edff1f48b925a67203102385470ac1afdc > > Thanks, > Martin.- Sure, will take another look. Thanks! Valerie - PR: https://git.openjdk.java.net/jdk/pull/2510
[11u] RFR: 8226374: Restrict TLS signature schemes and named groups
Hi, JDK-8226374 is backported to 11.0.12-oracle. I'd like to backport it for parity. It doesn't apply cleanly. I've taken the 13u backport as source because it resolves the wrong backport order with JDK-8242141. Bug: https://bugs.openjdk.java.net/browse/JDK-8226374 11u CSR: https://bugs.openjdk.java.net/browse/JDK-8264555 Original change (JDK14): https://hg.openjdk.java.net/jdk/jdk/rev/a93b7b28f644 13u backport: https://github.com/openjdk/jdk13u-dev/commit/384445d2 11u rejected hunks (integrated manually): http://cr.openjdk.java.net/~mdoerr/8226374_TLS_11u/8226374_TLS_rej.txt my new 11u backport: http://cr.openjdk.java.net/~mdoerr/8226374_TLS_11u/webrev.00/ Please review. Best regards, Martin
