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>
diff --git a/include/openssl/base.h b/include/openssl/base.h index 29b598e..e95efba 100644 --- a/include/openssl/base.h +++ b/include/openssl/base.h
@@ -109,7 +109,7 @@ // 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)
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index b198e0c..b0abe11 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h
@@ -2464,7 +2464,8 @@ // 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 @@ // 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);
diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc index 1b292a7..40329c6 100644 --- a/ssl/handshake_server.cc +++ b/ssl/handshake_server.cc
@@ -1835,6 +1835,9 @@ !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; }
diff --git a/ssl/internal.h b/ssl/internal.h index 3e68a8b..213704d 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -3755,8 +3755,13 @@ 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
diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc index 9ceefbb..16fd453 100644 --- a/ssl/ssl_session.cc +++ b/ssl/ssl_session.cc
@@ -443,14 +443,11 @@ 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 @@ 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)) {
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index 2a3b648..462172a 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go
@@ -1480,10 +1480,17 @@ // 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
diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go index 7f6ea64..c025548 100644 --- a/ssl/test/runner/conn.go +++ b/ssl/test/runner/conn.go
@@ -1611,7 +1611,7 @@ 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") }
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go index 70f8c28..35ba616 100644 --- a/ssl/test/runner/handshake_client.go +++ b/ssl/test/runner/handshake_client.go
@@ -2237,6 +2237,10 @@ 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
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 532fa5f..fad3ac8 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go
@@ -8608,6 +8608,18 @@ 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
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index cf47abc..0485f98 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc
@@ -382,6 +382,7 @@ 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 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 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};
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index 36f9701..093d241 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h
@@ -139,6 +139,7 @@ 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;
diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc index 7fda54b..709dfd3 100644 --- a/ssl/tls13_server.cc +++ b/ssl/tls13_server.cc
@@ -141,6 +141,7 @@ 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 @@ !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 @@ if (!ssl_add_message_cbb(ssl, cbb.get())) { return false; } + sent_tickets = true; } - *out_sent_tickets = true; + *out_sent_tickets = sent_tickets; return true; }