Code Review Request: JDK-8148421 (Extended Master Secret TLS extension)

[Xuelei Fan]

On 10/17/2017 11:45 AM, Martin Balao wrote:
>
> http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8148421/webrev.03/ 

TlsMasterSecretParameterSpec.java
---------------------------------
This spec update impacts the PKCS11 implementation too.  Please update 
jdk/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11TlsMasterSecretGenerator.java 
as well.


ClientHandshaker.java:
----------------------
Per RFC 7627:
1. For full handshaking, a client MUST send the "extended_master_secret" 
extension.
2. A client SHOULD NOT offer an abbreviated handshake to resume a 
session that that does not use an extended master secret.  Instead, it 
SHOULD offer a full handshake.
3. When offering an abbreviated handshake, the client MUST send the 
"extended_master_secret" extension in its ClientHello.
4. For abbreviated handshake, if the original session did not use the 
"extended_master_secret" extension but the new ServerHello contains the 
extension, the client MUST abort the handshake.
5. For abbreviated handshake, if the original session used the extension 
but the new ServerHello does not contain the extension, the client MUST 
abort the handshake.

If I'm reading correct, the update in ClientHandshaker.java implements 
#1, but missing #2, #4 and #5, and a conditional support of #3.

For the missing of #4 and #5, it might be mainly caused by that the 
800-828 lines are put in a place where full handshaking happens. 
Abbreviated handshake return at line 756 and cannot reach line 800.

For #2, I may suggest combine the extension together with System 
property "jdk.tls.allowUnsafeServerCertChange".  If not using extended 
master secret, and not allowUnsafeServerCertChange, and 
useExtendedMasterSecretExtension, do not offer abbreviated handshake. 
Using full handshake instead for TLS 1.0+.  Besides, if using the 
extension, don't use the server certificate change checking any more. 
See allowUnsafeServerCertChange comments in ClientHandshaker.java.

For #3, I may always send the "extended_master_secret" extension, the 
server side can handle it property, no matter the original session use 
the extension or not.


SSLSessionImpl.java
-------------------
   94  private boolean useExtendedMasterSecret;
  200  void setUseExtendedMasterSecret() {
  211  boolean getUseExtendedMasterSecret() {

I may suggest use "final" useExtendedMasterSecret (set during 
construction), so that the set/get methods do not compete against each 
other.  Using "final" may need to adjust some source code.  Looks like 
it is doable.


ServerHandshaker.java
---------------------
For safer, as there is no compatibility impact as if the client request 
for the extension, I think we may want to always enable the extension in 
server side.  It means the system property 
"jsse.useExtendedMasterSecret" disables the extension in client side 
only.  And the property cannot be used to disable server acceptance of 
the extension.

Per RFC 7627:
A. For full handshaking, if a server implementing this document receives 
the "extended_master_secret" extension, it MUST include the extension in 
its ServerHello message.
B. For abbreviated handshake request, If the original session did not 
use the "extended_master_secret" extension but the new ClientHello 
contains the extension, then the server MUST NOT perform the abbreviated 
handshake.  Instead, it SHOULD continue with a full handshake.
C. For abbreviated handshake request, if the original session used the 
"extended_master_secret" extension but the new ClientHello does not 
contain it, the server MUST abort the abbreviated handshake.
D. For abbreviated handshake request, if neither the original session 
nor the new ClientHello uses the extension, the server SHOULD abort the 
handshake.
E. For abbreviated handshake request, if the new ClientHello contains 
the extension and the server chooses to continue the handshake, then the 
server MUST include the "extended_master_secret" extension in its 
ServerHello message.

If I'm reading correct, the update in ServerHandshaker.java implements 
#A, #C and #E, but missing #B and #C.

For #B, I think it should be fine to follow the spec: continue with a 
full handshake.

For #C, I was wondering we may need a new system property 
(jsse.allowLegacyResumption?) to turn on/off this behavior.  If 
application want a strict mode, the server abort the abbreviated 
handshake for case #C.  Otherwise, the server can continue with an 
abbreviated handshake in order to support legacy resumption.

Hope it helps!

Regards,
Xuelei