<div dir="ltr"><div>Hi Xuelei,</div><div><br></div><div>Webrev 04:</div><div><br></div><div> * <a href="http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8148421/webrev.04/">http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8148421/webrev.04/</a> (browse online)</div><div> * <a href="http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8148421/webrev.04/8148421.webrev.04.zip">http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8148421/webrev.04/8148421.webrev.04.zip</a> (download)</div><div><br></div><div>TlsMasterSecretParameterSpec.java</div><div>---------------------------------</div><div><br></div><div>As far as I know, a mechanism for Extended Master Secret has not been included in PKCS#11 standard (as of v.2.40, which is the current version [1]). That's why I didn't update P11TlsMasterSecretGenerator class. PKCS#11 standard has mechanisms and data structures for the legacy master key derivation. As an example, CK_TLS12_MASTER_KEY_DERIVE_PARAMS data type has a CK_SSL3_RANDOM_DATA member which can hold a "server random" and a "client random" value. That was not intended to hold a "session hash". In regard to implementations, NSS software token uses the "master secret" legacy label as a parameter to the "TLS_PRF" function for the following mechanisms: CKM_TLS12_MASTER_KEY_DERIVE and CKM_TLS12_MASTER_KEY_DERIVE_DH. That is hardcoded: the client cannot request "extended master secret" label to be used. It's not intended for Extended Master Secret. NSS software token includes a custom mechanism for Extended Master Secret (CKM_NSS_TLS_EXTENDED_MASTER_KEY_DERIVE); that is not part of the PKCS#11 standard but a vendor specific mechanism.</div><div><br></div><div>ClientHandshaker.java:</div><div>----------------------</div><div><br></div><div>#4 and #5:</div><div>    Good point! Thanks. That's why I prefer a single return point when possible -particularly for long functions-. I remember changing this code block to the end of the function but did not realize the resumption flow.</div><div><br></div><div>#2:</div><div>    Done.</div><div><br></div><div>#3:</div><div>    I assume that you mean to always send the extension in abbreviated handshakes when "useExtendedMasterSecretExtension" is set to true. But I have some concerns. If we always send the extension, there may be cases in which a client that previously negotitatied SSLv3 protocol sends the extension -which has no meaning, as the extension applies to TLS 1.0+-. There may be cases in which Extended Master Secret was not used in the previous full-handshake but the abbreviated handshake is still offered because of allowUnsafeServerCertChange (see conditions for #2). I think conditions in #2 are the real/strong check in regard to abbreviated handshakes.</div><div><br></div><div>SSLSessionImpl.java</div><div>-------------------</div><div><br></div><div>Done.</div><div><br></div><div>ServerHandshaker.java</div><div>---------------------</div><div><br></div><div> * jsse.useExtendedMasterSecret does not longer disable the extension on the server side. Documentation and Release Notes updated (see below).</div><div><br></div><div>#B:</div><div>    Done.</div><div><br></div><div>#C:</div><div>    I'm trying to think of a real-use case for this but sounds weird. Anyways, done (for both client and server). Documentation and Release Notes updated (see below).</div><div><br></div><div>Testing</div><div>---------------------</div><div><br></div><div>Sucessfully ran the following regression test categories:</div><div><br></div><div> * javax/net/ssl</div><div> * sun/security/ssl</div><div><br></div><div>Feature documentation (for JSSE Reference Guides [1])</div><div>----------------------------------------------------</div><div><br></div><div>## Extended Master Secret Extension</div><div><br></div><div>The Extended Master Secret Extension is a feature that replaces the algorithm used to derive the *master secret* for a TLS session. The new algorithm provides a security enhancement to mitigate attacks such as the [Triple Handshake Attack](<a href="https://www.mitls.org/pages/attacks/3SHAKE">https://www.mitls.org/pages/attacks/3SHAKE</a>). The Extension is defined by [RFC 7627](<a href="https://tools.ietf.org/html/rfc7627">https://tools.ietf.org/html/rfc7627</a>) (*Transport Layer Security (TLS) Session Hash and Extended Master Secret Extension*) and applies to TLS 1.0+.</div><div><br></div><div>Clients and servers need to agree on the usage of this extension during the TLS full handshake, or the previous algorithm is used as a fallback. JSSE supports and enables the Extension by default on both client and server sides. However, for compatibility reasons, disabling is possible on the client side by setting *jsse.useExtendedMasterSecret* system property to *false* (i.e. through the *-Djsse.useExtendedMasterSecret="false"* command-line argument).</div><div><br></div><div>### How is the new algorithm different than the previous?</div><div><br></div><div>The original algorithm uses a PRF function to derive the *master secret* from the following inputs: *pre-master secret* (result of a previous key exchange); "master secret" string; and, client and server random values.</div><div><br></div><div>The new algorithm replaces the client and server random values with a hash of the previously exchanged handshake messages. As a result, the session *hash* contains information from certificates, key exchange parameters and other handshake-specific values; in addition to the client and server random numbers. Through binding *master secret* to the connection, an active man-in-the-middle attacker cannot force the generation of an identical value in a parallel connection. Thus, values that depend on the *master secret* for authentication (such as the TLS "tls-unique" binding value) can be trusted.</div><div><br></div><div>### Sessions resumption</div><div><br></div><div>The *master secret* for a TLS session is established during a full handshake. When a session is resumed, the original *master secret* is used without any further negotiations. However, for checking purposes, Extended Master Secret Extension messages are exchanged during the abreviated handshake to indicate that the Extension was used when the original *master secret* was derived. If the Extension was not used in the original handshake but is present when resuming, the server moves to a full handshake. On the other hand, if the Extension was used in the original handshake but is not present when resuming, both client and server abort the handshake unless "jsse.allowLegacyResumption" system property is set to *true*.</div><div><br></div><div>Release note: Extended Master Secret Extension support was added to JSSE for both client and server sides</div><div>---------------------------------------------------------------------------------------------------------</div><div><br></div><div>The Extended Master Secret Extension for TLS 1.0+ is now supported on JSSE for both client and server sides. By modifying the algorithm to derive the session *master secret* (during a full handshake) and binding it to connection-specific values, attacks such as the [Triple Handshake Attack](<a href="https://www.mitls.org/pages/attacks/3SHAKE">https://www.mitls.org/pages/attacks/3SHAKE</a>) are mitigated. </div><div><br></div><div>The extension, enabled by default, can be turned off on the client side by setting *jsse.useExtendedMasterSecret* system property to *false*.</div><div><br></div><div>See further information about the Extension in [RFC 7627](<a href="https://tools.ietf.org/html/rfc7627">https://tools.ietf.org/html/rfc7627</a>) (*Transport Layer Security (TLS) Session Hash and Extended Master Secret Extension*).</div><div><br></div><div><br></div><div>Kind regards,</div><div>Martin.-</div><div><br></div><div>--</div><div>[1] - <a href="http://docs.oasis-open.org/pkcs11/pkcs11-curr/v2.40/cs01/pkcs11-curr-v2.40-cs01.html">http://docs.oasis-open.org/pkcs11/pkcs11-curr/v2.40/cs01/pkcs11-curr-v2.40-cs01.html</a></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 18, 2017 at 9:54 AM, Xuelei Fan <span dir="ltr"><<a href="mailto:xuelei.fan@oracle.com" target="_blank">xuelei.fan@oracle.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 10/17/2017 11:45 AM, Martin Balao wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<a href="http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8148421/webrev.03/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~sg<wbr>ehwolf/webrevs/mbalaoal/JDK-81<wbr>48421/webrev.03/</a> <br>
</blockquote>
<br>
TlsMasterSecretParameterSpec.j<wbr>ava<br>
------------------------------<wbr>---<br>
This spec update impacts the PKCS11 implementation too.  Please update jdk/src/jdk.crypto.cryptoki/sh<wbr>are/classes/sun/security/pkcs1<wbr>1/P11TlsMasterSecretGenerator.<wbr>java as well.<br>
<br>
<br>
ClientHandshaker.java:<br>
----------------------<br>
Per RFC 7627:<br>
1. For full handshaking, a client MUST send the "extended_master_secret" extension.<br>
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.<br>
3. When offering an abbreviated handshake, the client MUST send the "extended_master_secret" extension in its ClientHello.<br>
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.<br>
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.<br>
<br>
If I'm reading correct, the update in ClientHandshaker.java implements #1, but missing #2, #4 and #5, and a conditional support of #3.<br>
<br>
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.<br>
<br>
For #2, I may suggest combine the extension together with System property "jdk.tls.allowUnsafeServerCert<wbr>Change".  If not using extended master secret, and not allowUnsafeServerCertChange, and useExtendedMasterSecretExtensi<wbr>on, 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.<br>
<br>
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.<br>
<br>
<br>
SSLSessionImpl.java<br>
-------------------<br>
  94  private boolean useExtendedMasterSecret;<br>
 200  void setUseExtendedMasterSecret() {<br>
 211  boolean getUseExtendedMasterSecret() {<br>
<br>
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.<br>
<br>
<br>
ServerHandshaker.java<br>
---------------------<br>
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.<br>
<br>
Per RFC 7627:<br>
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.<br>
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.<br>
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.<br>
D. For abbreviated handshake request, if neither the original session nor the new ClientHello uses the extension, the server SHOULD abort the handshake.<br>
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.<br>
<br>
If I'm reading correct, the update in ServerHandshaker.java implements #A, #C and #E, but missing #B and #C.<br>
<br>
For #B, I think it should be fine to follow the spec: continue with a full handshake.<br>
<br>
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.<br>
<br>
Hope it helps!<br>
<br>
Regards,<br>
Xuelei<br>
</blockquote></div><br></div>