RFR: 8301686: TLS 1.3 handshake fails if server_name doesn't match resuming session [v3]

Bradford Wetmore wetmore at openjdk.org
Thu Sep 28 17:59:35 UTC 2023


On Thu, 28 Sep 2023 01:38:15 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

>> Can I please get a review of this change which proposes to fix the issue reported in https://bugs.openjdk.org/browse/JDK-8301686?
>> 
>> The internal implementation of SSLContext caches SSLSession(s). These sessions are for a particular combination or peer host and port. When a TLS handshake completes successfully, the session is then stored in this cache. If/when subsequent handshake attempts against the same host/port combination happens, using this same SSLContext instance, then the internal implementation triggers a session resumption, which is allowed by the TLS RFC. During session resumption, the client then uses the pre-shared key from the previous successful handshake and sends it as part of the `ClientHello` message. 
>> 
>> One other part of the TLS handshake is the `server_name` extension. The client sends a `SNI` in the handshake which the server side can either reject or accept. To facilitate this matching on the server side, the `javax.net.ssl.SNIMatcher` can be configured on the (server side) `SSLParameters`. Setting of `SNIMatcher` is optional. 
>> 
>> If a successful handshake session (that was cached by the client) used a SNI name which the server accepted, then this SNI name is sent as part of the session resumption `ClientHello` along with the pre-shared key.  The current issue is that, during session resumption, on the server side, if the `SNIMatcher` is no longer present, then the server rightly aborts the session resumption, but still ends up sending the pre-shared key extension as part of the `ServerHello` message. The client, upon seeing this pre-shared key extension in the `ServerHello` considers that the session resumption succeeded and ends up using that pre-shared key to derive the early secret. On the server side though, the server has already rejected this resumption request and thus when the client next sends data, the server will no longer be able to decode the data and will fail with `javax.crypto.AEADBadTagException: Tag mismatch` as noted in the JBS issue.
>> 
>> The change in this PR, removes the pre-shared key extension data from the `ServerHello` message, when the server side notices that the session resumption is being aborted.
>> 
>> A new jtreg test has been added which reproduces the issue and verifies the fix. Existing tests in tier1, tier2 and tier3 continue to pass with this change.
>
> Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
> 
>  - limit line length in test case to 80 chars
>  - Brad's suggestion - replace "the" with "this"
>  - merge latest from master branch
>  - review comment - use SSLContextTemplate for SSLContext creation in test
>  - 8301686: TLS 1.3 handshake fails if server_name doesn't match resuming session

Marked as reviewed by wetmore (Reviewer).

src/java.base/share/classes/sun/security/ssl/ServerNameExtension.java line 347:

> 345:                     shc.resumingSession = null;
> 346:                     // this server is disallowing this session resumption,
> 347:                     // so don't include the pre-shared key in the ServerHello handshake message

If you wouldn't mind <= 80 here also.  The other changes look great.  

No need for another full test cycle.

Thanks for considering.

-------------

PR Review: https://git.openjdk.org/jdk/pull/13669#pullrequestreview-1649425699
PR Review Comment: https://git.openjdk.org/jdk/pull/13669#discussion_r1340495893



More information about the security-dev mailing list