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

[Jaikiran Pai]

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