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.
