Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mutability of TLS 1.3 SSL_SESSIONs and thread-safety #10210

Closed
kaduk opened this issue Oct 17, 2019 · 3 comments
Closed

Mutability of TLS 1.3 SSL_SESSIONs and thread-safety #10210

kaduk opened this issue Oct 17, 2019 · 3 comments
Labels
triaged: bug The issue/pr is/fixes a bug

Comments

@kaduk
Copy link
Contributor

kaduk commented Oct 17, 2019

This is mostly a follow up from my comment at #6563 (comment) to attempt to determine whether there's an error in my understanding/expectations or the code. (Both cases would require code changes, just different ones, so this isn't a pull request.)

During the development of our TLS 1.3 support, the only session resumption mechanism that we supported was the stateless ticket design that's described repeatedly in RFC 8446. Accordingly, our libssl behavior involved a new SSL_SESSION being created for each incoming TLS 1.3 connection, regardless of whether or not it was a resumption handshake or a full handshake. This crept into our design assumptions repeatedly, e.g. in e374335 and 1c4aa31. (The fact that these are the only instances easily detectable via commit messages and are both authored by me does not escape my notice.) When we changed our resumption behavior in #6563 to allow the use of the stateful session cache, the invariant that TLS 1.3 connections get new SSL_SESSIONS was removed, and so then TLS 1.3 processing is subject to the same thread-safety concerns that all other connections are: the SSL_SESSION object is immutable (other than refcounts) after it's in the cache. However, we don't currently adhere to the new invariant; the first example that comes to mind is in final_server_name(), where we store the received hostname in the session for non-resumptions and for TLS 1.3 connections. Until somewhat recently we also did this for supported_groups and peer_ciphers, which was fixed in #9176 by moving those fields out of the SSL_SESSION and into the SSL.

Given the above, is the right conclusion to draw that we want to stick with the new invariant of SSL_SESSION immutability and remove any lingering vestiges of the previous invariant that TLS 1.3 connections always get a new SSL_SESSION?

@kaduk kaduk added the issue: bug report The issue was opened to report a bug label Oct 17, 2019
@mattcaswell
Copy link
Member

Yes, we need SSL_SESSION immutability. I've been working on it but it is competing for time with other work.

@davidben
Copy link
Contributor

One thing I've found handy is to check in tests that exercise some multi-threaded behavior (use an RSA on multiple threads, resume an SSL_SESSION twice in parallel, etc.) and then have a CI config that runs with ThreadSanitizer enabled.

@mattcaswell mattcaswell added triaged: bug The issue/pr is/fixes a bug and removed issue: bug report The issue was opened to report a bug labels Oct 29, 2019
@kaduk
Copy link
Contributor Author

kaduk commented Mar 12, 2020

Closing, as (per #10943) the main issues were all fixed after #10441.

@kaduk kaduk closed this as completed Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants