From c59bf8bf189dcbde868e04efcd53b705ed155231 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 12 Nov 2024 12:02:09 -0500 Subject: [PATCH] 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 Reviewed-by: Adam Langley --- include/openssl/base.h | 2 +- include/openssl/ssl.h | 6 ++++-- ssl/handshake_server.cc | 3 +++ ssl/internal.h | 5 +++++ ssl/ssl_session.cc | 16 +++++++++------- ssl/test/runner/common.go | 9 ++++++++- ssl/test/runner/conn.go | 2 +- ssl/test/runner/handshake_client.go | 4 ++++ ssl/test/runner/runner.go | 12 ++++++++++++ ssl/test/test_config.cc | 9 +++++++++ ssl/test/test_config.h | 1 + ssl/tls13_server.cc | 18 ++++++++++++++---- 12 files changed, 71 insertions(+), 16 deletions(-) diff --git a/include/openssl/base.h b/include/openssl/base.h index 29b598e15..e95efbaac 100644 --- a/include/openssl/base.h +++ b/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) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index b198e0c7c..b0abe116d 100644 --- a/include/openssl/ssl.h +++ b/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); diff --git a/ssl/handshake_server.cc b/ssl/handshake_server.cc index 1b292a728..40329c65e 100644 --- a/ssl/handshake_server.cc +++ b/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; } diff --git a/ssl/internal.h b/ssl/internal.h index 3e68a8bf1..213704d49 100644 --- a/ssl/internal.h +++ b/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 diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc index 9ceefbb0a..16fd4539f 100644 --- a/ssl/ssl_session.cc +++ b/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)) { diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index 2a3b648b1..462172af9 100644 --- a/ssl/test/runner/common.go +++ b/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 diff --git a/ssl/test/runner/conn.go b/ssl/test/runner/conn.go index 7f6ea6496..c025548ba 100644 --- a/ssl/test/runner/conn.go +++ b/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") } diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go index 70f8c287d..35ba616c7 100644 --- a/ssl/test/runner/handshake_client.go +++ b/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 diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 532fa5f81..fad3ac805 100644 --- a/ssl/test/runner/runner.go +++ b/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 diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index cf47abc05..0485f9854 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc @@ -382,6 +382,7 @@ const Flag *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}; diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index 36f970149..093d2410a 100644 --- a/ssl/test/test_config.h +++ b/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; diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc index 7fda54bdb..709dfd3db 100644 --- a/ssl/tls13_server.cc +++ b/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 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; }