Only clear not_resumable after the handshake.
In renegotiation handshakes and, later, ECH ClientHelloOuter handshakes,
we don't want to add sessions to the session cache. We also don't want
to release a session as resumable until the handshake completes.
Ideally we'd only construct SSL_SESSION at the end of the handshake, but
existing APIs like SSL_get_session must work mid-handshake, so
SSL_SESSION is both a handle to immutable resumption state, and a
container for in-progress connection properties. We manage this with a
not_resumable flag that's only cleared after the handshake is done and
the SSL_SESSION finalized.
However, TLS 1.2 ticket renewal currently clears the flag too early and
breaks the invariant. This won't actually affect renegotiation or
ClientHelloOuter because those handshakes never resume. Still, we can
maintain the invariant storing the copy in hs->new_session. Note this
does sacrifice a different invariant: previously, ssl->session and
hs->new_session were never set at the same time.
This change also means ssl_update_cache does not need to special-case
ticket renewal.
Change-Id: I03230cd9c63e5bee6bd60cd05c0439e16533c6d4
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/48132
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index 1f26242..0fba737 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -1739,39 +1739,30 @@
return ssl_hs_read_change_cipher_spec;
}
- SSL_SESSION *session = hs->new_session.get();
- UniquePtr<SSL_SESSION> renewed_session;
- if (ssl->session != NULL) {
+ if (ssl->session != nullptr) {
// The server is sending a new ticket for an existing session. Sessions are
// immutable once established, so duplicate all but the ticket of the
// existing session.
- renewed_session =
+ assert(!hs->new_session);
+ hs->new_session =
SSL_SESSION_dup(ssl->session.get(), SSL_SESSION_INCLUDE_NONAUTH);
- if (!renewed_session) {
- // This should never happen.
- OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+ if (!hs->new_session) {
return ssl_hs_error;
}
- session = renewed_session.get();
}
// |ticket_lifetime_hint| is measured from when the ticket was issued.
- ssl_session_rebase_time(ssl, session);
+ ssl_session_rebase_time(ssl, hs->new_session.get());
- if (!session->ticket.CopyFrom(ticket)) {
+ if (!hs->new_session->ticket.CopyFrom(ticket)) {
return ssl_hs_error;
}
- session->ticket_lifetime_hint = ticket_lifetime_hint;
+ hs->new_session->ticket_lifetime_hint = ticket_lifetime_hint;
// Historically, OpenSSL filled in fake session IDs for ticket-based sessions.
// TODO(davidben): Are external callers relying on this? Try removing this.
- SHA256(CBS_data(&ticket), CBS_len(&ticket), session->session_id);
- session->session_id_length = SHA256_DIGEST_LENGTH;
-
- if (renewed_session) {
- session->not_resumable = false;
- ssl->session = std::move(renewed_session);
- }
+ SHA256(CBS_data(&ticket), CBS_len(&ticket), hs->new_session->session_id);
+ hs->new_session->session_id_length = SHA256_DIGEST_LENGTH;
ssl->method->next_message(ssl);
hs->state = state_process_change_cipher_spec;
@@ -1808,12 +1799,17 @@
ssl->method->on_handshake_complete(ssl);
- if (ssl->session != NULL) {
+ // Note TLS 1.2 resumptions with ticket renewal have both |ssl->session| (the
+ // resumed session) and |hs->new_session| (the session with the new ticket).
+ if (hs->new_session == nullptr) {
+ assert(ssl->session != nullptr);
ssl->s3->established_session = UpRef(ssl->session);
} else {
- // We make a copy of the session in order to maintain the immutability
- // of the new established_session due to False Start. The caller may
- // have taken a reference to the temporary session.
+ // When False Start is enabled, the handshake reports completion early. The
+ // caller may then have passed the (then unresuable) |hs->new_session| to
+ // another thread via |SSL_get0_session| for resumption. To avoid potential
+ // race conditions in such callers, we duplicate the session before
+ // clearing |not_resumable|.
ssl->s3->established_session =
SSL_SESSION_dup(hs->new_session.get(), SSL_SESSION_DUP_ALL);
if (!ssl->s3->established_session) {
diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc
index f6feb87..ec5c8b9 100644
--- a/ssl/handshake_server.cc
+++ b/ssl/handshake_server.cc
@@ -1791,9 +1791,11 @@
ssl->ctx->x509_method->session_clear(hs->new_session.get());
}
- if (ssl->session != NULL) {
+ if (hs->new_session == nullptr) {
+ assert(ssl->session != nullptr);
ssl->s3->established_session = UpRef(ssl->session);
} else {
+ assert(ssl->session == nullptr);
ssl->s3->established_session = std::move(hs->new_session);
ssl->s3->established_session->not_resumable = false;
}
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index 8b67cf8..ae9883d 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -283,12 +283,7 @@
// Clients never use the internal session cache.
int use_internal_cache = ssl->server && !(ctx->session_cache_mode &
SSL_SESS_CACHE_NO_INTERNAL_STORE);
-
- // A client may see new sessions on abbreviated handshakes if the server
- // decides to renew the ticket. Once the handshake is completed, it should be
- // inserted into the cache.
- if (ssl->s3->established_session.get() != ssl->session.get() ||
- (!ssl->server && hs->ticket_expected)) {
+ if (ssl->s3->established_session.get() != ssl->session.get()) {
if (use_internal_cache) {
SSL_CTX_add_session(ctx, ssl->s3->established_session.get());
}
