Track SSL session types a bit better on the client

A session could be offered in one of three fields:

- The TLS 1.2 session ID
- The TLS 1.2 session ticket extension
- The TLS 1.3 PSK extension

We didn't quite keep track of which kind we had. In particular:

- We are not willing to send TLS 1.2 session tickets if SSL_OP_NO_TICKET
  is set. However, if we were configured with a ticket session AND
  enabled TLS 1.3, we'd send a non-empty session ID. If the server
  echo'd the session ID anyway, we'd get confused and think the session
  was being resumed. There's no real practical consequence to this, but
  we should reject this.

- If we somehow constructed a TLS 1.3 session with ID but no ticket, we
  would think it was an ID session and offer the session ID after the
  cleanup in
  https://boringssl-review.googlesource.com/c/boringssl/+/69947. We'd
  also send a PSK extension with an empty PSK field, and then even allow
  the server to resume it. This isn't completely absurd (except that PSK
  identities cannot be empty), but offering the session ID would trip
  QUIC up.

  This case should be impossible... but before the bug fixed in
  I1651e7887f9611ebc44ac54af89c85bf86a9feff, this was actually
  reachable. There's no practical consequence, but we should reject this
  at a better place.

- The code to decide whether the server could send pre_shared_key in
  ServerHello just checked for any session at all, even a TLS 1.2
  session. This has no practical consequence because we'll just catch it
  later, but may as well fix this.

Fix this by adding a function to classify the SSL_SESSION and then catch
on that throughout.

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

ssl/extensions.cc

@@ -943,17 +943,14 @@ static bool ext_ticket_add_clienthello(const SSL_HANDSHAKE *hs, CBB *out,
     return true;
   }
 
-  Span<const uint8_t> ticket;
-
   // Renegotiation does not participate in session resumption. However, still
   // advertise the extension to avoid potentially breaking servers which carry
   // over the state from the previous handshake, such as OpenSSL servers
   // without upstream's 3c3f0259238594d77264a78944d409f2127642c4.
-  if (!ssl->s3->initial_handshake_complete &&
+  Span<const uint8_t> ticket;
+  if (!ssl->s3->initial_handshake_complete &&  //
       ssl->session != nullptr &&
-      !ssl->session->ticket.empty() &&
-      // Don't send TLS 1.3 session tickets in the ticket extension.
-      ssl_session_protocol_version(ssl->session.get()) < TLS1_3_VERSION) {
+      ssl_session_get_type(ssl->session.get()) == SSLSessionType::kTicket) {
     ticket = ssl->session->ticket;
   }
 
@@ -1892,7 +1889,8 @@ static bool should_offer_psk(const SSL_HANDSHAKE *hs,
                              ssl_client_hello_type_t type) {
   const SSL *const ssl = hs->ssl;
   if (hs->max_version < TLS1_3_VERSION || ssl->session == nullptr ||
-      ssl_session_protocol_version(ssl->session.get()) < TLS1_3_VERSION ||
+      ssl_session_get_type(ssl->session.get()) !=
+          SSLSessionType::kPreSharedKey ||
       // TODO(https://crbug.com/boringssl/275): Should we synthesize a
       // placeholder PSK, at least when we offer early data? Otherwise
       // ClientHelloOuter will contain an early_data extension without a

ssl/handshake_client.cc

@@ -503,7 +503,9 @@ static enum ssl_hs_wait_t do_start_connect(SSL_HANDSHAKE *hs) {
 
   // If the configured session has expired or is not usable, drop it. We also do
   // not offer sessions on renegotiation.
+  SSLSessionType session_type = SSLSessionType::kNotResumable;
   if (ssl->session != nullptr) {
+    session_type = ssl_session_get_type(ssl->session.get());
     if (ssl->session->is_server ||
         !ssl_supports_version(hs, ssl->session->ssl_version) ||
         // Do not offer TLS 1.2 sessions with ECH. ClientHelloInner does not
@@ -511,11 +513,15 @@ static enum ssl_hs_wait_t do_start_connect(SSL_HANDSHAKE *hs) {
         // identity.
         (hs->selected_ech_config &&
          ssl_session_protocol_version(ssl->session.get()) < TLS1_3_VERSION) ||
-        !SSL_SESSION_is_resumable(ssl->session.get()) ||
+        session_type == SSLSessionType::kNotResumable ||
+        // Don't offer TLS 1.2 tickets if disabled.
+        (session_type == SSLSessionType::kTicket &&
+         (SSL_get_options(ssl) & SSL_OP_NO_TICKET)) ||
         !ssl_session_is_time_valid(ssl, ssl->session.get()) ||
         (ssl->quic_method != nullptr) != ssl->session->is_quic ||
         ssl->s3->initial_handshake_complete) {
       ssl_set_session(ssl, nullptr);
+      session_type = SSLSessionType::kNotResumable;
     }
   }
 
@@ -527,23 +533,16 @@ static enum ssl_hs_wait_t do_start_connect(SSL_HANDSHAKE *hs) {
     return ssl_hs_error;
   }
 
-  const bool has_id_session = ssl->session != nullptr &&
-                              !ssl->session->session_id.empty() &&
-                              ssl->session->ticket.empty();
-  const bool has_ticket_session =
-      ssl->session != nullptr && !ssl->session->ticket.empty();
-  // TLS 1.2 session tickets require a placeholder value to signal resumption.
-  const bool ticket_session_requires_random_id =
-      has_ticket_session &&
-      ssl_session_protocol_version(ssl->session.get()) < TLS1_3_VERSION;
   // Compatibility mode sends a random session ID. Compatibility mode is
   // enabled for TLS 1.3, but not when it's run over QUIC or DTLS.
   const bool enable_compatibility_mode = hs->max_version >= TLS1_3_VERSION &&
                                          ssl->quic_method == nullptr &&
                                          !SSL_is_dtls(hs->ssl);
-  if (has_id_session) {
+  if (session_type == SSLSessionType::kID) {
     hs->session_id = ssl->session->session_id;
-  } else if (ticket_session_requires_random_id || enable_compatibility_mode) {
+  } else if (session_type == SSLSessionType::kTicket ||
+             enable_compatibility_mode) {
+    // TLS 1.2 session tickets require a placeholder value to signal resumption.
     hs->session_id.ResizeForOverwrite(SSL_MAX_SSL_SESSION_ID_LENGTH);
     if (!RAND_bytes(hs->session_id.data(), hs->session_id.size())) {
       return ssl_hs_error;

ssl/internal.h

@@ -3777,6 +3777,20 @@ OPENSSL_EXPORT UniquePtr<SSL_SESSION> SSL_SESSION_parse(
 // error.
 OPENSSL_EXPORT bool ssl_session_serialize(const SSL_SESSION *in, CBB *cbb);
 
+enum class SSLSessionType {
+  // The session is not resumable.
+  kNotResumable,
+  // The session uses a TLS 1.2 session ID.
+  kID,
+  // The session uses a TLS 1.2 ticket.
+  kTicket,
+  // The session uses a TLS 1.3 pre-shared key.
+  kPreSharedKey,
+};
+
+// ssl_session_get_type returns the type of |session|.
+SSLSessionType ssl_session_get_type(const SSL_SESSION *session);
+
 // ssl_session_is_context_valid returns whether |session|'s session ID context
 // matches the one set on |hs|.
 bool ssl_session_is_context_valid(const SSL_HANDSHAKE *hs,

ssl/ssl_session.cc

@@ -565,6 +565,23 @@ bool ssl_encrypt_ticket(SSL_HANDSHAKE *hs, CBB *out,
   }
 }
 
+SSLSessionType ssl_session_get_type(const SSL_SESSION *session) {
+  if (session->not_resumable) {
+    return SSLSessionType::kNotResumable;
+  }
+  if (ssl_session_protocol_version(session) >= TLS1_3_VERSION) {
+    return session->ticket.empty() ? SSLSessionType::kNotResumable
+                                   : SSLSessionType::kPreSharedKey;
+  }
+  if (!session->ticket.empty()) {
+    return SSLSessionType::kTicket;
+  }
+  if (!session->session_id.empty()) {
+    return SSLSessionType::kID;
+  }
+  return SSLSessionType::kNotResumable;
+}
+
 bool ssl_session_is_context_valid(const SSL_HANDSHAKE *hs,
                                   const SSL_SESSION *session) {
   return session != nullptr &&
@@ -1064,8 +1081,7 @@ int SSL_SESSION_should_be_single_use(const SSL_SESSION *session) {
 }
 
 int SSL_SESSION_is_resumable(const SSL_SESSION *session) {
-  return !session->not_resumable &&
-         (!session->session_id.empty() || !session->ticket.empty());
+  return ssl_session_get_type(session) != SSLSessionType::kNotResumable;
 }
 
 int SSL_SESSION_has_ticket(const SSL_SESSION *session) {

ssl/ssl_test.cc

@@ -9778,5 +9778,23 @@ TEST(SSLTest, EarlyDataDisabledInDTLS13) {
   EXPECT_FALSE(SSL_SESSION_early_data_capable(session.get()));
 }
 
+// ID-only TLS 1.3 sessions are impossible and should not be resumable.
+TEST(SSLTest, IDOnlyTLS13Session) {
+  bssl::UniquePtr<SSL_CTX> ctx = CreateContextWithTestCertificate(TLS_method());
+  ASSERT_TRUE(ctx);
+  SSL_CTX_set_session_cache_mode(ctx.get(),
+                                 SSL_SESS_CACHE_CLIENT | SSL_SESS_CACHE_SERVER);
+
+  ASSERT_TRUE(SSL_CTX_set_max_proto_version(ctx.get(), TLS1_3_VERSION));
+  bssl::UniquePtr<SSL_SESSION> session =
+      CreateClientSession(ctx.get(), ctx.get());
+  ASSERT_TRUE(session);
+  EXPECT_TRUE(SSL_SESSION_is_resumable(session.get()));
+
+  session->ticket.Reset();
+  session->session_id.Resize(32);
+  EXPECT_FALSE(SSL_SESSION_is_resumable(session.get()));
+}
+
 }  // namespace
 BSSL_NAMESPACE_END

ssl/test/runner/common.go

@@ -1149,11 +1149,16 @@ type ProtocolBugs struct {
 	ExpectNoSessionID bool
 
 	// ExpectNoTLS12Session, if true, causes the server to fail the
-	// connection if the server offered a TLS 1.2 session. TLS 1.3 clients
+	// connection if the client offered a TLS 1.2 session. TLS 1.3 clients
 	// always offer session IDs for compatibility, so the session ID check
 	// checks for sessions the server issued.
 	ExpectNoTLS12Session bool
 
+	// ExpectNoTLS12TicketSupport, if true, causes the server to fail the
+	// connection if the client signaled TLS 1.2 session ticket support.
+	// (This implicitly enforces that the client does not send a ticket.)
+	ExpectNoTLS12TicketSupport bool
+
 	// ExpectNoTLS13PSK, if true, causes the server to fail the connection
 	// if a TLS 1.3 PSK is offered.
 	ExpectNoTLS13PSK bool

ssl/test/runner/handshake_server.go

@@ -401,6 +401,9 @@ func (hs *serverHandshakeState) readClientHello() error {
 			return fmt.Errorf("tls: client offered an unexpected session ticket")
 		}
 	}
+	if config.Bugs.ExpectNoTLS12TicketSupport && hs.clientHello.ticketSupported {
+		return fmt.Errorf("tls: client sent unexpected session ticket extension")
+	}
 
 	if config.Bugs.ExpectNoTLS13PSK && len(hs.clientHello.pskIdentities) > 0 {
 		return fmt.Errorf("tls: client offered unexpected PSK identities")

ssl/test/runner/runner.go

@@ -9253,6 +9253,13 @@ func addResumptionVersionTests() {
 						},
 					})
 				} else if !isBadDTLSResumption {
+					expectedError := ":OLD_SESSION_VERSION_NOT_RETURNED:"
+					if sessionVers.version < VersionTLS13 && resumeVers.version >= VersionTLS13 {
+						// The server will "resume" the session by sending pre_shared_key,
+						// but the shim will not have sent pre_shared_key at all. The shim
+						// should reject this because the extension was not allowed at all.
+						expectedError = ":UNEXPECTED_EXTENSION:"
+					}
 					testCases = append(testCases, testCase{
 						protocol:      protocol,
 						name:          "Resume-Client-Mismatch" + suffix,
@@ -9273,7 +9280,7 @@ func addResumptionVersionTests() {
 							version: resumeVers.version,
 						},
 						shouldFail:    true,
-						expectedError: ":OLD_SESSION_VERSION_NOT_RETURNED:",
+						expectedError: expectedError,
 					})
 				}
 
@@ -13667,6 +13674,43 @@ func addSessionTicketTests() {
 			// has established tickets.
 			flags: []string{"-on-resume-no-ticket"},
 		})
+
+		// SSL_OP_NO_TICKET implies the client must not offer ticket-based
+		// sessions. The client not only should not send the session ticket
+		// extension, but if the server echos the session ID, the client should
+		// reject this.
+		if ver.version < VersionTLS13 {
+			testCases = append(testCases, testCase{
+				name: ver.name + "-NoTicket-NoOffer",
+				config: Config{
+					MinVersion: ver.version,
+					MaxVersion: ver.version,
+				},
+				resumeConfig: &Config{
+					MinVersion: ver.version,
+					MaxVersion: ver.version,
+					Bugs: ProtocolBugs{
+						ExpectNoTLS12TicketSupport: true,
+						// Pretend to accept the session, even though the client
+						// did not offer it. The client should reject this as
+						// invalid. A buggy client will still fail because it
+						// expects resumption, but with a different error.
+						// Ideally, we would test this by actually resuming the
+						// previous session, even though the client did not
+						// provide a ticket.
+						EchoSessionIDInFullHandshake: true,
+					},
+				},
+				resumeSession:        true,
+				expectResumeRejected: true,
+				// Set SSL_OP_NO_TICKET on the second connection, after the first
+				// has established tickets.
+				flags:              []string{"-on-resume-no-ticket"},
+				shouldFail:         true,
+				expectedError:      ":SERVER_ECHOED_INVALID_SESSION_ID:",
+				expectedLocalError: "remote error: illegal parameter",
+			})
+		}
 	}
 }
 

ssl/tls13_client.cc

@@ -418,7 +418,10 @@ static enum ssl_hs_wait_t do_read_server_hello(SSL_HANDSHAKE *hs) {
 
   // When offering ECH, |ssl->session| is only offered in ClientHelloInner.
   const bool pre_shared_key_allowed =
-      ssl->session != nullptr && ssl->s3->ech_status != ssl_ech_rejected;
+      ssl->session != nullptr &&
+      ssl_session_get_type(ssl->session.get()) ==
+          SSLSessionType::kPreSharedKey &&
+      ssl->s3->ech_status != ssl_ech_rejected;
   SSLExtension key_share(TLSEXT_TYPE_key_share),
       pre_shared_key(TLSEXT_TYPE_pre_shared_key, pre_shared_key_allowed),
       supported_versions(TLSEXT_TYPE_supported_versions);