Re: Code Review Request: TLS 1.3 Implementation

[Xuelei Fan]

> http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.00

AlpnExtension.java
--
Looks fine to me.


CertificateRequest.java
---
Looks fine to me.


CertificateStatus.java
--
- 42  * Pack of the CertificateStatus handshake message.
+ 42  * Pack of the CertificateStatus handshake message. [RFC 6066]

May be nice if adding the RFC number that defines this handshake 
message.  Otherwise, looks fine to me.



CertificateVerify.java
--
-129  if (x509Credentials == null) {
+129  if (x509Credentials == null ||
 x509Credentials.popPublicKey == null) {
 130  shc.conContext.fatal(Alert.HANDSHAKE_FAILURE,
 131 "No X509 credentials negotiated for CertificateVerify");
 132  }

May be safe to check the x509Credentials.popPublicKey as well.  Similar 
to line 357-360, 607-610, 916-919.



-233  if (x509Possession == null) {
+233  if (x509Possession == null ||
  x509Possession.popPrivateKey == null) {
 234  if (SSLLogger.isOn && SSLLogger.isOn("ssl,handshake")) {
 235 SSLLogger.fine(
 236 "No X.509 credentials negotiated for CertificateVerify");
 237  }
 238
 239 return null;
 240  }

May be safe to check the x509Possession.popPrivateKey as well. Similar 
to line 458-466, 697-704, 1021-1027.


Otherwise, looks fine to me.

CertStatusExtension.java

 326 private final int encodedLen;

Looks like this field is not used.  Remove it?

 425   if (!responderIds.isEmpty()) {
 426   ridStr = responderIds.toString();
-427
 428  }
One unnecessary blank line.  Otherwise, looks fine to me.

Xuelei

On 5/25/2018 4:45 PM, Xuelei Fan wrote:

Hi,

I'd like to invite you to review the TLS 1.3 implementation.  I 
appreciate it if I could have compatibility and specification feedback 
before May 31, 2018, and implementation feedback before June 7, 2018.


Here is the webrev:
     http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.00

The formal TLS 1.3 specification is not finalized yet, although it had 
been approved to be a standard.  The implementation is based on the 
draft version 28:

     https://tools.ietf.org/html/draft-ietf-tls-tls13-28

For the overall description of this enhancement, please refer to JEP 332:
     http://openjdk.java.net/jeps/332

For the compatibility and specification update, please refer to CSR 
8202625:

     https://bugs.openjdk.java.net/browse/JDK-8202625

Note that we are using the sandbox for the development right now.  For 
more information, please refer to Bradford's previous email:


http://mail.openjdk.java.net/pipermail/security-dev/2018-May/017139.html

Thanks & Regards,
Xuelei

RFR: 8196141: Add GoDaddy root certificates

[Rajan Halade]

Please review this fix to add GoDaddy root certificates to cacerts.

Webrev: http://cr.openjdk.java.net/~rhalade/8196141/webrev.00/

Thanks,
Rajan

Re: [RFR]: JDK-8203182 - Release session if initialization of SunPKCS11 Signature fails

[Valerie Peng]

Hi Martin,

Thanks for covering additional scenarios.

The updated webrev looks good to me. I will sponsor your patch.

Thanks,
Valerie

On 5/24/2018 12:13 PM, Martin Balao wrote:

Hi Valerie,

Thanks for your review.

These are the cases I identified:

 * engineSign and engineVerify include a try-finally block that 
releases the session under any circumstances.


 * engineUpdate may leak. Session release added if failure occurs.

 * cancelOperation may leak. If session has no objects, it's killed. 
If it has, a "cancel operation by finishing it" code is executed. This 
code can fail on sign/verify operations, and a session may be leaked. 
It's not clear to me why this "cancel operation by finishing it" code 
is needed, though. Added a try-finally block anyways.


Webrev 01:

 * 
http://cr.openjdk.java.net/~mbalao/webrevs/8203182/8203182.webrev.01/ 

 * 
http://cr.openjdk.java.net/~mbalao/webrevs/8203182/8203182.webrev.01.zip 



Look forward to your answer.

Kind regards,
Martin.-

On Tue, May 15, 2018 at 7:30 PM, Valerie Peng > wrote:


Hi Martin,

Your fix only addresses the initialization case. Have you
considered fixing the same problem under difference calls?

Regards,
Valerie

On 5/14/2018 3:25 PM, Martin Balao wrote:

Hi,

Can I have a review for "JDK-8203182 - Release session if
initialization of SunPKCS11 Signature fails" [1]?

 *
http://cr.openjdk.java.net/~mbalao/webrevs/8203182/8203182.webrev.00/


>
 *
http://cr.openjdk.java.net/~mbalao/webrevs/8203182/8203182.webrev.00.zip



>

Thanks in advanced!

Martin.-

--
[1] - https://bugs.openjdk.java.net/browse/JDK-8203182

Re: [8u-dev] Request for Review and Approval to Backport 8189760 : sun/security/ssl/CertPathRestrictions/TLSRestrictions.java failed with unexpected Exception intermittently

[Ivan Gerasimov]

Thanks Seán!


On 5/31/18 12:13 PM, Seán Coffey wrote:

Looks fine Ivan.

Approved for jdk8u-dev.

regards,
Sean. 

Re: RFR: 8191031: Remove several Symantec Root CAs

[Sean Mullan]

Looks good.

--Sean

On 5/30/18 3:05 PM, Rajan Halade wrote:
May I request you to review this fix to remove listed Symantec root 
certificates.


Webrev: http://cr.openjdk.java.net/~rhalade/8191031/webrev.00/

Thanks,
Rajan

[8u-dev] Request for Review and Approval to Backport 8189760 : sun/security/ssl/CertPathRestrictions/TLSRestrictions.java failed with unexpected Exception intermittently

[Ivan Gerasimov]

Hello!

The test failure (in the subj. line) was observed in JDK 8u, so I'd like 
to backport the fix.


The patch did not apply cleanly due to the difference in couple of 
lines, thus the re-review request.


(Specifically, the difference is in TLSRestrictions.java, lines 216 and 
221 of patched version; otherwise, the fix is essentially the same.)


The patched test passed well on all supported platforms.

Bug: https://bugs.openjdk.java.net/browse/JDK-8189760

Jdk 8u webrev: http://cr.openjdk.java.net/~igerasim/8189760/00/webrev/

Jdk 11 change: 
http://closedjdk.us.oracle.com/jdk/jdk-cpu/open/rev/2d250a0174a6


Jdk 11 review: 
http://mail.openjdk.java.net/pipermail/security-dev/2018-January/016675.html


Thanks in advance!

--
With kind regards,
Ivan Gerasimov

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

[Sean Mullan]

Looks fine to me.

--Sean

On 5/30/18 5:39 PM, Jamil Nimeh wrote:

Hopefully the final set of updates before committing this feature:

  * Updated the ChaChaEngine implementations in ChaCha20Cipher.java to
properly conform to Cipher's specification when the output buffer
used in doUpdate is too small.  Instead of throwing
IndexOutOfBoundsException like it was doing, it will now throw a
ShortBufferException per the spec.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.09

Thanks,

--Jamil


On 05/22/2018 01:43 PM, Jamil Nimeh wrote:

A couple updates:

  * Touched up the comments surrounding init/wrap/unwrap methods. 
None of these affect public javadocs.

  * Added an implementation for engineGetParameters.  This was
something that just got forgotten from the initial development of
the cipher when there were no AlgorithmParameter implementations.
  o There are a couple tests added to ChaCha20Poly1305ParamTest to
exercise these new code paths.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.08

Thanks,

--Jamil


On 5/21/2018 11:36 AM, Jamil Nimeh wrote:


And the fun just keeps going!  Round 7.

This revision removes wrap/unwrap support for ChaCha20 and 
ChaCha20-Poly1305 until there is a standardized key wrapping approach 
for these ciphers (similar to how AES has its own key wrapping scheme 
in RFC 3394).


Initializing the cipher with WRAP/UNWRAP mode will throw 
UnsupportedOperationException and likewise the wrap/unwrap methods 
will throw IllegalStateException.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.07/

Thanks,

--Jamil


On 05/16/2018 12:05 PM, Jamil Nimeh wrote:


Round 6.

This brings ChaCha20/ChaCha20-Poly1305 into conformance with 
Cipher's specification when forms of init that take 
AlgorithmParameters or AlgorithmParameterSpec are used. Previously, 
a non-null AP or APS object was required.  In order to better 
conform to the specification, if a null AP or APS is provided for 
these ciphers, a random nonce will be generated and the counter will 
be set to 1, just as is currently done with valid forms of init that 
don't take an AP or APS object (e.g. Cipher.init(int, Key, 
SecureRandom) ).  Per the spec in Cipher, this is only true for 
ENCRYPT_MODE and will throw InvalidKeyException when done in 
DECRYPT_MODE.


I also added a few test cases that exercise these code paths in the 
ChaCha20Poly1305Parameters.java test case.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.06/

Thanks,

--Jamil


On 05/04/2018 07:06 AM, Jamil Nimeh wrote:


Round 5.

This adds Sean's comments.  Sean, I was never able to execute a 
case on init where a half-baked object would fail.  In most cases 
it would fail in checks in javax.crypto.Cipher before it ever made 
it down to my code.  I'm pretty confident the init sequence is OK.  
I did move the setting of a few data members toward the end of the 
init sequence but setting the key and nonce is necessary before 
creating the initial state, which is then used for generating an 
authentication key for AEAD mode and generating keystream.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.05

Also the CSR has been finalized and can be found here:
https://bugs.openjdk.java.net/browse/JDK-8198925

--Jamil

On 04/27/2018 02:21 PM, Jamil Nimeh wrote:


Round 4 of updates for ChaCha20 and ChaCha20-Poly1305, minor stuff 
mostly:


  * Added words in the description of javax.crypto.Cipher
recommending callers reinitialize the Cipher to use different
nonces after each complete encryption or decryption (similar
language to what exists already for AES-GCM encryption).
  * Added an additional test case for ChaCha20NoReuse
  * Made accessor methods for ChaCha20ParameterSpec final and
cleaned up the code a bit based on comments from the field.

http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.04/

Thanks!

--Jamil


On 04/13/2018 11:59 AM, Jamil Nimeh wrote:

Round 3 of updates for ChaCha20 and ChaCha20-Poly1305:

* Removed the key field in ChaCha20 and Poly1305 implementations 
and only retain the key bytes as an object field (thanks Thomas 
for catching this)


* Added additional protections against key/nonce reuse. This is a 
behavioral change to ChaCha20 and ChaCha20-Poly1305.  Instances 
of these ciphers will no longer allow you to do subsequent 
doUpdate/doFinal calls after the first doFinal without 
re-initializing the cipher with either a new key or nonce. 
Attempting to reuse the cipher without a new initialization will 
throw an IllegalStateException.  This is similar to the behavior 
of AES-GCM in encrypt mode, but for ChaCha20 it needs to be done 
for both encrypt and decrypt.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.03/

Thanks,
--Jamil

On 04/10/2018 03:34 PM, Jamil Nimeh wrote:

Hello everyone,

This is a quick update to the previous webrev:

* When using the form of engineInit that does only takes op, key 
and random,