The question about 8211018: Session Resumption without Server-Side State

[Sergey Bylokhov]

Hello, Security team.

Right now I am working on some issues related to the sslContext and would like to clarify a few 
questions about how the feature implemented by the JDK-8211018 is intended to work.
That feature is mostly implemented by this class:
https://github.com/openjdk/jdk/blob/4cec141a90bc5d3b8ec17c024291d9c74a112cd4/src/java.base/share/classes/sun/security/ssl/SessionTicketExtension.java

Here is some questions about thread safeness of this code:

  - The SessionTicketExtension class uses the currentKeyID to track the currently used keyID.
https://github.com/openjdk/jdk/blob/4cec141a90bc5d3b8ec17c024291d9c74a112cd4/src/java.base/share/classes/sun/security/ssl/SessionTicketExtension.java#L77
    * Note that the field is static and could be shared across the threads. There is even a 
synchronization block in place where we increment the key:
 
https://github.com/openjdk/jdk/blob/4cec141a90bc5d3b8ec17c024291d9c74a112cd4/src/java.base/share/classes/sun/security/ssl/SessionTicketExtension.java#L175
    * It seems wrong that it is synchronized on the "hc.sslContext.keyHashMap" since the static key 
can be shared across the different sslcontext's?
    * In another place the synchronization is not used at all:
https://github.com/openjdk/jdk/blob/4cec141a90bc5d3b8ec17c024291d9c74a112cd4/src/java.base/share/classes/sun/security/ssl/SessionTicketExtension.java#L158
    * Probably the intention was to use one currentKeyID per sslcontext?


  - The hashmap to store the keys are located in the SSLContextImpl class:
https://github.com/openjdk/jdk/blob/4cec141a90bc5d3b8ec17c024291d9c74a112cd4/src/java.base/share/classes/sun/security/ssl/SSLContextImpl.java#L74
  * It seems that the usage of this map is not synchronized at all, does it mean that it cannot be 
used across the threads? -> And it is expected that one SSLContextImpl is created per thread?


  - If the hashmap above can be used across the threads, then the code for "clean" the expired keys 
is unclear: 
https://github.com/openjdk/jdk/commit/94e1d7530ff03978be305211f6a1e6d14acb6d1c#diff-8eeef23d966ed2ebe6ecf02d16e549b8cef94f54bb17f6c9ac8d8dc151eed8fcR197
============
     // Clean up any old keys, then return the current key
     cleanup(hc);
     return ssk;
============
   Note that it is executed outside of the synchronized block, is that right?

  - Another question is about using "SessionTicketExtension#getCurrentKey+cleanup" on a different 
threads. I think it is possible to get the next situation:
    1. Thread1 call getCurrentKey() and return the key which is mostly ready to expire, but valid 
for now, This key started to be used  by the SessionTicketSpec#encrypt
    2. Thread2 calls getCurrentKey() and creates a new key, then call "cleanup" and destroys the key 
used by Thread1, since it is expired now.
    3. Thread1 is in trouble.


Note that the specification does not have any hints about how the code in question should work 
concurrently.

-- 
Best regards, Sergey.