[PATCH] JDK-8190917 : SSL session resumption, through handshake, in SSLEngine is broken for any protocols lesser than TLSv1.2

[Xuelei Fan]

Hi Jaikiran,

I will sponsor this contribution.  I need more time for the review and 
testing.

Thanks,
Xuelei

On 11/23/2017 9:22 PM, Jaikiran Pai wrote:
> As noted in [1], there's a regression in Java 9, where SSL session 
> resumption no longer works for SSL protocols other than TLSv1.2. The 
> code which is responsible for session resumption resides in the 
> ServerHandshaker[2], in the clientHello method. This method, in its 
> logic to decide whether or not to resume a (previously) cached session, 
> checks if the client hello message has a session id associated. If it 
> does, it then fetches a session from the cache. If it does find a 
> previously cached session for that id, it then goes ahead to check if 
> the SSL protocol associated with the cached session matches with the 
> protocol version that's "applicable/selected" of the incoming client 
> hello message. However, a relatively recent change[3] has, IMO, 
> unintentionally caused the protocol version check logic to fail for 
> protocols other than TLSv1.2. The protocol version check looks like:
> 
> 
> // cannot resume session with different
> 
> if (oldVersion != protocolVersion) {
> resumingSession = false;
> }
> 
> The `protocolVersion` variable in this case happens to be a _default 
> initialized value_ (TLSv1.2) instead of the one that's "selected" based 
> on the incoming client hello message. As a result the `protocolVersion` 
> value is always TLSv1.2 and thus for any other protocols, this 
> comparison fails, even when the client hello message uses the right 
> session id and the right protocol that matches the previous session.
> 
> The attached patch, includes a potential fix to this issue. The change 
> makes sure that the protocol selection, based on the client hello 
> message, is done before this session resumption check and also makes 
> sure it uses that "selected" protocol in the version comparison check 
> instead of the class member `protocolVersion` (which gets set when the 
> server hello message is finally being sent).
> 
> The attached patch also contains a (jtreg) test case which reproduces 
> this bug and verifies this fix. The test case tests the session 
> resumption against TLSv1, TLSv1.1 and TLSv1.2. The test code itself is a 
> minor modification of the SSL demo that's available here [4].
> 
> This is my first OpenJDK patch and my OCA got approved a few days back. 
> Could someone please help with the review of this patch?
> 
> P.S: I noticed that the JIRA got assigned a few days back. I am not sure 
> if that means the fix is already being worked upon by someone else from 
> the team. If that's the case, then let me know and I'll let it be 
> handled by them.
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8190917
> 
> [2] 
> http://hg.openjdk.java.net/jdk/jdk/file/b7ae1437111b/src/java.base/share/classes/sun/security/ssl/ServerHandshaker.java 
> 
> 
> [3] http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/42268eb6e04e
> 
> [4] 
> https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/samples/sslengine/SSLEngineSimpleDemo.java 
> 
> 
> -Jaikiran
> 
> 
>