Support skipping tickets in both ticket callbacks

TLS 1.2 and TLS 1.3 both support some mechanism for the server to change
its mind at the last minute and not send a ticket. In TLS 1.2, you leave
the ticket field empty. In TLS 1.3, you just don't send NewSessionTicket
in the first place.

We have two ticket encryption callbacks, the old "ticket key callback"
and the "ticket AEAD callback".

The ticket key callback has no way to trigger this behavior, but it has
an unused zero return that could be repurposed for this, in principle.
Back in 2016, OpenSSL did so in
https://github.com/openssl/openssl/pull/1098

The ticket AEAD callback could, in principle, support this by
successfully returning the empty string. This goes against the current
documented API contract, but it accidentally worked in TLS 1.2. In TLS
1.3, it caused our servers to send a syntax error.

The QUIC folks want this behavior and used to accidentally trigger the
syntax error, and have switched to returning some garbage placeholder
string. Between that, the new OpenSSL behavior, and it accidentally
working in TLS 1.2 anyway, let's just go ahead and support this.

To aid in version skew messes, this also bumps BORINGSSL_API_VERSION.

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

include/openssl/base.h

@@ -109,7 +109,7 @@ extern "C" {
 // A consumer may use this symbol in the preprocessor to temporarily build
 // against multiple revisions of BoringSSL at the same time. It is not
 // recommended to do so for longer than is necessary.
-#define BORINGSSL_API_VERSION 32
+#define BORINGSSL_API_VERSION 33
 
 #if defined(BORINGSSL_SHARED_LIBRARY)
 

include/openssl/ssl.h

@@ -2464,7 +2464,8 @@ OPENSSL_EXPORT int SSL_CTX_set_tlsext_ticket_keys(SSL_CTX *ctx, const void *in,
 // When encrypting a new ticket, |encrypt| will be one. It writes a public
 // 16-byte key name to |key_name| and a fresh IV to |iv|. The output IV length
 // must match |EVP_CIPHER_CTX_iv_length| of the cipher selected. In this mode,
-// |callback| returns 1 on success and -1 on error.
+// |callback| returns 1 on success, 0 to decline sending a ticket, and -1 on
+// error.
 //
 // When decrypting a ticket, |encrypt| will be zero. |key_name| will point to a
 // 16-byte key name and |iv| points to an IV. The length of the IV consumed must
@@ -2508,7 +2509,8 @@ struct ssl_ticket_aead_method_st {
   // seal encrypts and authenticates |in_len| bytes from |in|, writes, at most,
   // |max_out_len| bytes to |out|, and puts the number of bytes written in
   // |*out_len|. The |in| and |out| buffers may be equal but will not otherwise
-  // alias. It returns one on success or zero on error.
+  // alias. It returns one on success or zero on error. If the function returns
+  // but |*out_len| is zero, BoringSSL will skip sending a ticket.
   int (*seal)(SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out_len,
               const uint8_t *in, size_t in_len);
 

ssl/handshake_server.cc

@@ -1835,6 +1835,9 @@ static enum ssl_hs_wait_t do_send_server_finished(SSL_HANDSHAKE *hs) {
         !CBB_add_u32(&body, session->timeout) ||
         !CBB_add_u16_length_prefixed(&body, &ticket) ||
         !ssl_encrypt_ticket(hs, &ticket, session) ||
+        // |ticket| may be empty to skip sending a ticket. In TLS 1.2, servers
+        // skip sending tickets by sending empty NewSessionTicket, so no special
+        // handling is needed.
         !ssl_add_message_cbb(ssl, cbb.get())) {
       return ssl_hs_error;
     }

ssl/internal.h

@@ -3755,8 +3755,13 @@ bool ssl_is_key_type_supported(int key_type);
 bool ssl_compare_public_and_private_key(const EVP_PKEY *pubkey,
                                        const EVP_PKEY *privkey);
 bool ssl_get_new_session(SSL_HANDSHAKE *hs);
+
+// ssl_encrypt_ticket encrypt a ticket for |session| and writes the result to
+// |out|. It returns true on success and false on error. If, on success, nothing
+// was written to |out|, the caller should skip sending a ticket.
 bool ssl_encrypt_ticket(SSL_HANDSHAKE *hs, CBB *out,
                         const SSL_SESSION *session);
+
 bool ssl_ctx_rotate_ticket_encryption_key(SSL_CTX *ctx);
 
 // ssl_session_new returns a newly-allocated blank |SSL_SESSION| or nullptr on

ssl/ssl_session.cc

@@ -443,14 +443,11 @@ static int ssl_encrypt_ticket_with_cipher_ctx(SSL_HANDSHAKE *hs, CBB *out,
   ScopedEVP_CIPHER_CTX ctx;
   ScopedHMAC_CTX hctx;
 
-  // If the session is too long, emit a dummy value rather than abort the
-  // connection.
+  // If the session is too long, decline to send a ticket.
   static const size_t kMaxTicketOverhead =
       16 + EVP_MAX_IV_LENGTH + EVP_MAX_BLOCK_LENGTH + EVP_MAX_MD_SIZE;
   if (session_len > 0xffff - kMaxTicketOverhead) {
-    static const char kTicketPlaceholder[] = "TICKET TOO LARGE";
-    return CBB_add_bytes(out, (const uint8_t *)kTicketPlaceholder,
-                         strlen(kTicketPlaceholder));
+    return 1;
   }
 
   // Initialize HMAC and cipher contexts. If callback present it does all the
@@ -459,10 +456,15 @@ static int ssl_encrypt_ticket_with_cipher_ctx(SSL_HANDSHAKE *hs, CBB *out,
   uint8_t iv[EVP_MAX_IV_LENGTH];
   uint8_t key_name[16];
   if (tctx->ticket_key_cb != NULL) {
-    if (tctx->ticket_key_cb(hs->ssl, key_name, iv, ctx.get(), hctx.get(),
-                            1 /* encrypt */) < 0) {
+    int ret = tctx->ticket_key_cb(hs->ssl, key_name, iv, ctx.get(), hctx.get(),
+                                  1 /* encrypt */);
+    if (ret < 0) {
       return 0;
     }
+    if (ret == 0) {
+      // The caller requested to send no ticket, so write nothing to |out|.
+      return 1;
+    }
   } else {
     // Rotate ticket key if necessary.
     if (!ssl_ctx_rotate_ticket_encryption_key(tctx)) {

ssl/test/runner/common.go

@@ -1480,10 +1480,17 @@ type ProtocolBugs struct {
 	// modes to send. If a non-nil empty slice, no extension will be sent.
 	SendPSKKeyExchangeModes []byte
 
-	// ExpectNoNewSessionTicket, if present, means that the client will fail upon
+	// ExpectNoNewSessionTicket, if true, means that the client will fail upon
 	// receipt of a NewSessionTicket message.
 	ExpectNoNewSessionTicket bool
 
+	// ExpectNoNonEmptyNewSessionTicket, if true, means that the client will
+	// fail upon receipt of a NewSessionTicket message that was non-empty. In
+	// TLS 1.3, this is the same as ExpectNoNewSessionTicket. In TLS 1.2, this
+	// allows the server to commit to sending NewSessionTicket, but then decline
+	// to send one.
+	ExpectNoNonEmptyNewSessionTicket bool
+
 	// DuplicateTicketEarlyData causes an extra empty extension of early_data to
 	// be sent in NewSessionTicket.
 	DuplicateTicketEarlyData bool

ssl/test/runner/conn.go

@@ -1611,7 +1611,7 @@ func (c *Conn) processTLS13NewSessionTicket(newSessionTicket *newSessionTicketMs
 		return errors.New("tls: no early_data ticket extension found")
 	}
 
-	if c.config.Bugs.ExpectNoNewSessionTicket {
+	if c.config.Bugs.ExpectNoNewSessionTicket || c.config.Bugs.ExpectNoNonEmptyNewSessionTicket {
 		return errors.New("tls: received unexpected NewSessionTicket")
 	}
 

ssl/test/runner/handshake_client.go

@@ -2237,6 +2237,10 @@ func (hs *clientHandshakeState) readSessionTicket() error {
 		return unexpectedMessageError(sessionTicketMsg, msg)
 	}
 
+	if c.config.Bugs.ExpectNoNonEmptyNewSessionTicket && len(sessionTicketMsg.ticket) != 0 {
+		return errors.New("tls: received unexpected non-empty NewSessionTicket")
+	}
+
 	session.sessionTicket = sessionTicketMsg.ticket
 	hs.session = session
 

ssl/test/runner/runner.go

@@ -8608,6 +8608,18 @@ func addExtensionTests() {
 						resumeSession: true,
 					})
 				}
+				testCases = append(testCases, testCase{
+					protocol: protocol,
+					testType: serverTest,
+					name:     "TicketCallback-Skip-" + callbackSuffix,
+					config: Config{
+						MaxVersion: ver.version,
+						Bugs: ProtocolBugs{
+							ExpectNoNonEmptyNewSessionTicket: true,
+						},
+					},
+					flags: []string{flag, "-skip-ticket"},
+				})
 
 				// Test that the ticket callback is only called once when everything before
 				// it in the ClientHello is asynchronous. This corrupts the ticket so

ssl/test/test_config.cc

@@ -382,6 +382,7 @@ const Flag<TestConfig> *FindFlag(const char *name) {
         BoolFlag("-use-ticket-aead-callback",
                  &TestConfig::use_ticket_aead_callback),
         BoolFlag("-renew-ticket", &TestConfig::renew_ticket),
+        BoolFlag("-skip-ticket", &TestConfig::skip_ticket),
         BoolFlag("-enable-early-data", &TestConfig::enable_early_data),
         Base64Flag("-expect-ocsp-response", &TestConfig::expect_ocsp_response),
         BoolFlag("-check-close-notify", &TestConfig::check_close_notify),
@@ -858,6 +859,9 @@ static int TicketKeyCallback(SSL *ssl, uint8_t *key_name, uint8_t *iv,
   static const uint8_t kZeros[16] = {0};
 
   if (encrypt) {
+    if (GetTestConfig(ssl)->skip_ticket) {
+      return 0;
+    }
     OPENSSL_memcpy(key_name, kZeros, sizeof(kZeros));
     RAND_bytes(iv, 16);
   } else if (OPENSSL_memcmp(key_name, kZeros, 16) != 0) {
@@ -1225,6 +1229,11 @@ static size_t AsyncTicketMaxOverhead(SSL *ssl) {
 static int AsyncTicketSeal(SSL *ssl, uint8_t *out, size_t *out_len,
                            size_t max_out_len, const uint8_t *in,
                            size_t in_len) {
+  if (GetTestConfig(ssl)->skip_ticket) {
+    *out_len = 0;
+    return 1;
+  }
+
   auto out_span = bssl::MakeSpan(out, max_out_len);
   // Encrypt the ticket with the all zero key and a random nonce.
   static const uint8_t kKey[16] = {0};

ssl/test/test_config.h

@@ -139,6 +139,7 @@ struct TestConfig {
   bool use_ticket_callback = false;
   bool use_ticket_aead_callback = false;
   bool renew_ticket = false;
+  bool skip_ticket = false;
   bool enable_early_data = false;
   std::string ocsp_response;
   bool check_close_notify = false;

ssl/tls13_server.cc

@@ -141,6 +141,7 @@ static bool add_new_session_tickets(SSL_HANDSHAKE *hs, bool *out_sent_tickets) {
   ssl_session_rebase_time(ssl, hs->new_session.get());
 
   assert(ssl->session_ctx->num_tickets <= kMaxTickets);
+  bool sent_tickets = false;
   for (size_t i = 0; i < ssl->session_ctx->num_tickets; i++) {
     UniquePtr<SSL_SESSION> session(
         SSL_SESSION_dup(hs->new_session.get(), SSL_SESSION_INCLUDE_NONAUTH));
@@ -177,10 +178,18 @@ static bool add_new_session_tickets(SSL_HANDSHAKE *hs, bool *out_sent_tickets) {
         !CBB_add_u32(&body, session->ticket_age_add) ||
         !CBB_add_u8_length_prefixed(&body, &nonce_cbb) ||
         !CBB_add_bytes(&nonce_cbb, nonce, sizeof(nonce)) ||
-        !CBB_add_u16_length_prefixed(&body, &ticket) ||
         !tls13_derive_session_psk(session.get(), nonce, SSL_is_dtls(ssl)) ||
-        !ssl_encrypt_ticket(hs, &ticket, session.get()) ||
-        !CBB_add_u16_length_prefixed(&body, &extensions)) {
+        !CBB_add_u16_length_prefixed(&body, &ticket) ||
+        !ssl_encrypt_ticket(hs, &ticket, session.get())) {
+      return false;
+    }
+
+    if (CBB_len(&ticket) == 0) {
+      // The caller decided not to encrypt a ticket. Skip the message.
+      continue;
+    }
+
+    if (!CBB_add_u16_length_prefixed(&body, &extensions)) {
       return false;
     }
 
@@ -204,9 +213,10 @@ static bool add_new_session_tickets(SSL_HANDSHAKE *hs, bool *out_sent_tickets) {
     if (!ssl_add_message_cbb(ssl, cbb.get())) {
       return false;
     }
+    sent_tickets = true;
   }
 
-  *out_sent_tickets = true;
+  *out_sent_tickets = sent_tickets;
   return true;
 }