commit 1f6c3dc7d736357d5454a73217d47ce93d2fb240
author David Benjamin <davidben@google.com> Mon May 10 15:32:02 2021 -0400
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Thu May 13 15:27:42 2021 +0000
tree 3684d3e7f6c1c827c7633d977db6560bfaff462b
parent 962b375bcb7de6422c2bc5c4ef6e527d8f74c74d

Simplify renego + resumption handling.

We do not offer sessions on renegotiation. Rather than applying this at
both the ClientHello and ServerHello, just drop ssl->session, which
takes care of both cases.

Change-Id: I5ebaedc8d9cc0fca61242ed9b85fa3449636dfec
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47445
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

ssl/handshake_client.cc

diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index c7cadec..2a8c75c 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -397,17 +397,18 @@
         hs->max_version >= TLS1_2_VERSION ? TLS1_2_VERSION : hs->max_version;
   }
 
-  // If the configured session has expired or was created at a disabled
-  // version, drop it.
-  if (ssl->session != NULL) {
+  // If the configured session has expired or is not usable, drop it. We also do
+  // not offer sessions on renegotiation.
+  if (ssl->session != nullptr) {
     if (ssl->session->is_server ||
         !ssl_supports_version(hs, ssl->session->ssl_version) ||
         (ssl->session->session_id_length == 0 &&
          ssl->session->ticket.empty()) ||
         ssl->session->not_resumable ||
         !ssl_session_is_time_valid(ssl, ssl->session.get()) ||
-        (ssl->quic_method != nullptr) != ssl->session->is_quic) {
-      ssl_set_session(ssl, NULL);
+        (ssl->quic_method != nullptr) != ssl->session->is_quic ||
+        ssl->s3->initial_handshake_complete) {
+      ssl_set_session(ssl, nullptr);
     }
   }
 
@@ -418,8 +419,7 @@
   // Never send a session ID in QUIC. QUIC uses TLS 1.3 at a minimum and
   // disables TLS 1.3 middlebox compatibility mode.
   if (ssl->quic_method == nullptr) {
-    if (ssl->session != nullptr && !ssl->s3->initial_handshake_complete &&
-        ssl->session->session_id_length > 0) {
+    if (ssl->session != nullptr && ssl->session->session_id_length > 0) {
       hs->session_id_len = ssl->session->session_id_length;
       OPENSSL_memcpy(hs->session_id, ssl->session->session_id,
                      hs->session_id_len);
@@ -642,10 +642,11 @@
     }
   }
 
-  if (!ssl->s3->initial_handshake_complete && ssl->session != nullptr &&
-      ssl->session->session_id_length != 0 &&
+  if (ssl->session != nullptr && ssl->session->session_id_length != 0 &&
       CBS_mem_equal(&session_id, ssl->session->session_id,
                     ssl->session->session_id_length)) {
+    // We never offer sessions on renegotiation.
+    assert(!ssl->s3->initial_handshake_complete);
     ssl->s3->session_reused = true;
   } else {
     // The server may also have echoed back the TLS 1.3 compatibility mode