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

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Martin Balao
Hi,

This is my proposal for JDK-8148421 (Support Transport Layer Security (TLS) Session Hash and Extended Master Secret Extension) [1]:


Notes:

 * There is no PKCS#11 support for Extended Master Secret key derivation at this moment. NSS supports it through a vendor-specific type definition (CKM_NSS_TLS_EXTENDED_MASTER_KEY_DERIVE and CKM_NSS_TLS_EXTENDED_MASTER_KEY_DERIVE_DH in pkcs11n.h file). Thus, P11TlsMasterSecretGenerator uses the legacy Master Key Derivation method only.

Thanks in advanced,
Martin.-

--
Reply | Threaded
Open this post in threaded view
|

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

Xuelei Fan-2
Hi Martin,

Sorry for the delay.

I like this no-API-change design.

There may be some interoperbility/compatibility issues because of
implementation issues of the Extended Master Secret Extension.  Maybe,
we want an approach to turn off the extension if there is a concern.  It
could be a system property (for example,
jsse.useExtendedMasterSecret="false").

Would you mind file a Compatibility & Specification Review (CSR) request
for this feature proposal?  For more information, see the CSR wiki at
OpenJDK:
    https://wiki.openjdk.java.net/display/csr/Main

I may have some comments about the implementation if the CSR request get
approved.

Thanks & Regards,
Xuelei

On 8/4/2017 6:18 AM, Martin Balao wrote:

> Hi,
>
> This is my proposal for JDK-8148421 (Support Transport Layer Security
> (TLS) Session Hash and Extended Master Secret Extension) [1]:
>
>   *
> http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8148421/webrev.01/ 
> <http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8148421/webrev.01/>(browse
> online)
>   *
> http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8148421/webrev.01/8148421.webrev.01.zip 
> (download)
>
> Notes:
>
>   * There is no PKCS#11 support for Extended Master Secret key
> derivation at this moment. NSS supports it through a vendor-specific
> type definition (CKM_NSS_TLS_EXTENDED_MASTER_KEY_DERIVE and
> CKM_NSS_TLS_EXTENDED_MASTER_KEY_DERIVE_DH in pkcs11n.h file). Thus,
> P11TlsMasterSecretGenerator uses the legacy Master Key Derivation method
> only.
>
> Thanks in advanced,
> Martin.-
>
> --
> [1] - https://bugs.openjdk.java.net/browse/JDK-8148421
Reply | Threaded
Open this post in threaded view
|

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

Martin Balao
Hi Xuelei,

Webrev 03
--------------------


Differences with previous webrev 02:

 * jsse.useExtendedMasterSecret system property was added to completely disable the feature on both client and server sides.

 * Bug fix: if the server does not support TLS 1.0+ version (thus, a previous TLS version was negotiated), the Extended Master Secret extension is not used even if the client sends the extension in its ClientHello message.

 * Bug fix: When TLS 1.0 or 1.1 are used, the session hash is calculated concatenating a MD5 with a SHA1 hash.

 * 2 regression testcases were fixed.

 * knownExtensions array (in ExtensionType class) has now an initial length of 16.


Feature documentation (for JSSE Reference Guides [1])
----------------------------------------------------

## Extended Master Secret Extension

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](https://www.mitls.org/pages/attacks/3SHAKE). The Extension is defined by [RFC 7627](https://tools.ietf.org/html/rfc7627) (*Transport Layer Security (TLS) Session Hash and Extended Master Secret Extension*) and applies to TLS 1.0+.

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 by setting *jsse.useExtendedMasterSecret* system property to *false* (i.e. through the *-Djsse.useExtendedMasterSecret="false"* command-line argument).

### How is the new algorithm different than the previous?

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.

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.

### Sessions resumption

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. JSSE raises an error if an incongruence is found here.

Release note: Extended Master Secret Extension support was added to JSSE for both client and server sides
---------------------------------------------------------------------------------------------------------

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](https://www.mitls.org/pages/attacks/3SHAKE) are mitigated. 

The extension, enabled by default, can be turned off by setting *jsse.useExtendedMasterSecret* system property to *false*.

See further information about the Extension in [RFC 7627](https://tools.ietf.org/html/rfc7627) (*Transport Layer Security (TLS) Session Hash and Extended Master Secret Extension*).


Testing
-----------

I've run jtreg regression tests for the following categories:

 * javax/net/ssl
 * sun/security/ssl

Results:
 
 * Found 2 bugs running regression tests (both fixed in webrev 03).

 * Found 2 tests broken because of this patch (both fixed in webrev 03).

 * The number of tests that pass is equal with the patch than without, so I assume that no regression was introduced given the current coverage.

Kind regards,
Martin.-

--

On Sat, Aug 26, 2017 at 7:49 PM, Xuelei Fan <[hidden email]> wrote:
Hi Martin,

Sorry for the delay.

I like this no-API-change design.

There may be some interoperbility/compatibility issues because of implementation issues of the Extended Master Secret Extension.  Maybe, we want an approach to turn off the extension if there is a concern.  It could be a system property (for example, jsse.useExtendedMasterSecret="false").

Would you mind file a Compatibility & Specification Review (CSR) request for this feature proposal?  For more information, see the CSR wiki at OpenJDK:
   https://wiki.openjdk.java.net/display/csr/Main

I may have some comments about the implementation if the CSR request get approved.

Thanks & Regards,
Xuelei

On 8/4/2017 6:18 AM, Martin Balao wrote:
Hi,

This is my proposal for JDK-8148421 (Support Transport Layer Security (TLS) Session Hash and Extended Master Secret Extension) [1]:

  * http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8148421/webrev.01/ <http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8148421/webrev.01/>(browse online)
  * http://cr.openjdk.java.net/~sgehwolf/webrevs/mbalaoal/JDK-8148421/webrev.01/8148421.webrev.01.zip (download)

Notes:

  * There is no PKCS#11 support for Extended Master Secret key derivation at this moment. NSS supports it through a vendor-specific type definition (CKM_NSS_TLS_EXTENDED_MASTER_KEY_DERIVE and CKM_NSS_TLS_EXTENDED_MASTER_KEY_DERIVE_DH in pkcs11n.h file). Thus, P11TlsMasterSecretGenerator uses the legacy Master Key Derivation method only.

Thanks in advanced,
Martin.-

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

Reply | Threaded
Open this post in threaded view
|

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

Xuelei Fan-2
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
Reply | Threaded
Open this post in threaded view
|

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

Martin Balao
Hi Xuelei,

Webrev 04:


TlsMasterSecretParameterSpec.java
---------------------------------

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.

ClientHandshaker.java:
----------------------

#4 and #5:
    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.

#2:
    Done.

#3:
    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.

SSLSessionImpl.java
-------------------

Done.

ServerHandshaker.java
---------------------

 * jsse.useExtendedMasterSecret does not longer disable the extension on the server side. Documentation and Release Notes updated (see below).

#B:
    Done.

#C:
    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).

Testing
---------------------

Sucessfully ran the following regression test categories:

 * javax/net/ssl
 * sun/security/ssl

Feature documentation (for JSSE Reference Guides [1])
----------------------------------------------------

## Extended Master Secret Extension

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](https://www.mitls.org/pages/attacks/3SHAKE). The Extension is defined by [RFC 7627](https://tools.ietf.org/html/rfc7627) (*Transport Layer Security (TLS) Session Hash and Extended Master Secret Extension*) and applies to TLS 1.0+.

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).

### How is the new algorithm different than the previous?

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.

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.

### Sessions resumption

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*.

Release note: Extended Master Secret Extension support was added to JSSE for both client and server sides
---------------------------------------------------------------------------------------------------------

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](https://www.mitls.org/pages/attacks/3SHAKE) are mitigated. 

The extension, enabled by default, can be turned off on the client side by setting *jsse.useExtendedMasterSecret* system property to *false*.

See further information about the Extension in [RFC 7627](https://tools.ietf.org/html/rfc7627) (*Transport Layer Security (TLS) Session Hash and Extended Master Secret Extension*).


Kind regards,
Martin.-

--

On Wed, Oct 18, 2017 at 9:54 AM, Xuelei Fan <[hidden email]> wrote:
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