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

[Xuelei Fan]

Hi Jaikiran,

Thanks a lot for the update.  Your code looks fine to me.

As we are working on the re-org of the implementation[1] now, I may 
integrate your contribution shortly after the re-org changes.

Thanks,
Xuelei

[1]: 
http://mail.openjdk.java.net/pipermail/security-dev/2018-February/016815.html

On 2/22/2018 9:58 PM, Jaikiran Pai wrote:
> Given some recent changes to the class involved in this patch, in the 
> jdk repo (http://hg.openjdk.java.net/jdk/jdk), I noticed some merge 
> conflicts to this patch today. So I've now attached an updated patch 
> which resolves those merge issues. This has been tested with latest 
> jtreg (tip).
> 
> -Jaikiran
> 
> 
> On 06/12/17 9:35 PM, Jaikiran Pai wrote:
>> Thank you Xuelei. Please take your time.
>>
>> -Jaikiran
>>
>>
>>
>> On Wednesday, December 6, 2017, Xuelei Fan <xuelei.fan at oracle.com 
>> <mailto:xuelei.fan at oracle.com>> wrote:
>>
>>     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
>>         <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
>>         <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
>>         <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
>>         <https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/samples/sslengine/SSLEngineSimpleDemo.java>
>>
>>
>>         -Jaikiran
>>
>>
>>
>