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

[Xuelei Fan]

Hi Jaikiran,

As you noticed, we updated to use the ClientHello.client_version and 
session version for version negotiation during resumption.  It's not the 
best option for performance, but it is a safer option for compatibility 
before I'm able to make further evaluation.   I need more time to think 
about the impact if backporting t0 JDK 9.

Thanks,
Xuelei

On 2/23/2018 9:17 PM, Jaikiran Pai wrote:
> Sounds fine, thank you Xuelei. Would this later be backported to Java 9 
> too?
> 
> -Jaikiran
> 
> 
> On 24/02/18 12:21 AM, Xuelei Fan wrote:
>> 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
>>>>
>>>>
>>>>
>>>
>