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>

1 files changed, 3 insertions, 1 deletions

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 @@ static enum ssl_hs_wait_t do_finish_server_handshake(SSL_HANDSHAKE *hs) {
     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;
   }