Hi Sergey, Thanks for starting this discussion. I've opened "8298381: Improve handling of session tickets for multiple SSLContexts" [1] to track this issue and submitted a pull request with a potential fix [2]. Let's continue the discussion there if you don't mind :)
Best regards, Volker [1] https://bugs.openjdk.org/browse/JDK-8298381 [2] https://github.com/openjdk/jdk/pull/11590 On Wed, Dec 7, 2022 at 10:01 PM Sergey Bylokhov <bylok...@amazon.com> wrote: > > 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.
