From 0cbb1af41fe8a857e8aa09a6791b244666c2803e Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 17 Jul 2018 21:26:05 -0400 Subject: [PATCH] Don't mint TLS 1.3 tickets if SSL_OP_NO_TICKETS is set. Change-Id: I03e05acb024e34beaeaf2f02860da1763e08a093 Reviewed-on: https://boringssl-review.googlesource.com/29844 Reviewed-by: Adam Langley --- ssl/test/runner/common.go | 8 ++++-- ssl/test/runner/handshake_client.go | 6 +++++ ssl/test/runner/runner.go | 38 ++++++++++++++++++++++++++ ssl/test/test_config.cc | 4 +++ ssl/test/test_config.h | 1 + ssl/tls13_server.cc | 42 ++++++++++++++++------------- 6 files changed, 79 insertions(+), 20 deletions(-) diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index f77a4ccc3..aa173501e 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go @@ -137,8 +137,8 @@ const ( extensionNextProtoNeg uint16 = 13172 // not IANA assigned extensionRenegotiationInfo uint16 = 0xff01 extensionQUICTransportParams uint16 = 0xffa5 // draft-ietf-quic-tls-13 - extensionChannelID uint16 = 30032 // not IANA assigned - extensionDummyPQPadding uint16 = 54537 // not IANA assigned + extensionChannelID uint16 = 30032 // not IANA assigned + extensionDummyPQPadding uint16 = 54537 // not IANA assigned ) // TLS signaling cipher suite values @@ -1168,6 +1168,10 @@ type ProtocolBugs struct { // sessions use session tickets instead of session IDs. RequireSessionTickets bool + // RequireSessionIDs, if true, causes the client to require new sessions use + // session IDs instead of session tickets. + RequireSessionIDs bool + // NullAllCiphers, if true, causes every cipher to behave like the null // cipher. NullAllCiphers bool diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go index f367dc4ba..3e20d8716 100644 --- a/ssl/test/runner/handshake_client.go +++ b/ssl/test/runner/handshake_client.go @@ -729,6 +729,9 @@ NextCipherSuite: if c.config.Bugs.RequireSessionTickets && len(hs.session.sessionTicket) == 0 { return errors.New("tls: new session used session IDs instead of tickets") } + if c.config.Bugs.RequireSessionIDs && len(hs.session.sessionId) == 0 { + return errors.New("tls: new session used session tickets instead of IDs") + } sessionCache.Put(cacheKey, hs.session) } @@ -1672,6 +1675,9 @@ func (hs *clientHandshakeState) readSessionTicket() error { if c.vers == VersionSSL30 { return errors.New("tls: negotiated session tickets in SSL 3.0") } + if c.config.Bugs.ExpectNoNewSessionTicket { + return errors.New("tls: received unexpected NewSessionTicket") + } msg, err := c.readHandshake() if err != nil { diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index f5fac2b5a..7597764af 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go @@ -11163,6 +11163,44 @@ func addSessionTicketTests() { resumeSession: true, expectResumeRejected: true, }) + + for _, ver := range tlsVersions { + // Prior to TLS 1.3, disabling session tickets enables session IDs. + useStatefulResumption := ver.version < VersionTLS13 + + // SSL_OP_NO_TICKET implies the server must not mint any tickets. + testCases = append(testCases, testCase{ + testType: serverTest, + name: ver.name + "-NoTicket-NoMint", + config: Config{ + MinVersion: ver.version, + MaxVersion: ver.version, + Bugs: ProtocolBugs{ + ExpectNoNewSessionTicket: true, + RequireSessionIDs: useStatefulResumption, + }, + }, + resumeSession: useStatefulResumption, + tls13Variant: ver.tls13Variant, + flags: []string{"-no-ticket"}, + }) + + // SSL_OP_NO_TICKET implies the server must not accept any tickets. + testCases = append(testCases, testCase{ + testType: serverTest, + name: ver.name + "-NoTicket-NoAccept", + config: Config{ + MinVersion: ver.version, + MaxVersion: ver.version, + }, + tls13Variant: ver.tls13Variant, + 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"}, + }) + } } func addChangeCipherSpecTests() { diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 1731431ce..c973fb527 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc @@ -66,6 +66,7 @@ const Flag kBoolFlags[] = { { "-no-tls12", &TestConfig::no_tls12 }, { "-no-tls11", &TestConfig::no_tls11 }, { "-no-tls1", &TestConfig::no_tls1 }, + { "-no-ticket", &TestConfig::no_ticket }, { "-enable-channel-id", &TestConfig::enable_channel_id }, { "-shim-writes-first", &TestConfig::shim_writes_first }, { "-expect-session-miss", &TestConfig::expect_session_miss }, @@ -1498,6 +1499,9 @@ bssl::UniquePtr TestConfig::NewSSL( if (no_tls1) { SSL_set_options(ssl.get(), SSL_OP_NO_TLSv1); } + if (no_ticket) { + SSL_set_options(ssl.get(), SSL_OP_NO_TICKET); + } if (!expected_channel_id.empty() || enable_channel_id) { SSL_set_tls_channel_id_enabled(ssl.get(), 1); } diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index fef029fe2..f6c66e6da 100644 --- a/ssl/test/test_config.h +++ b/ssl/test/test_config.h @@ -50,6 +50,7 @@ struct TestConfig { bool no_tls12 = false; bool no_tls11 = false; bool no_tls1 = false; + bool no_ticket = false; std::string expected_channel_id; bool enable_channel_id = false; std::string send_channel_id; diff --git a/ssl/tls13_server.cc b/ssl/tls13_server.cc index 3c2c7741b..203e704d7 100644 --- a/ssl/tls13_server.cc +++ b/ssl/tls13_server.cc @@ -148,8 +148,18 @@ static const SSL_CIPHER *choose_tls13_cipher( return best; } -static int add_new_session_tickets(SSL_HANDSHAKE *hs) { +static bool add_new_session_tickets(SSL_HANDSHAKE *hs, bool *out_sent_tickets) { SSL *const ssl = hs->ssl; + if (// If the client doesn't accept resumption with PSK_DHE_KE, don't send a + // session ticket. + !hs->accept_psk_mode || + // We only implement stateless resumption in TLS 1.3, so skip sending + // tickets if disabled. + (SSL_get_options(ssl) & SSL_OP_NO_TICKET)) { + *out_sent_tickets = false; + return true; + } + // TLS 1.3 recommends single-use tickets, so issue multiple tickets in case // the client makes several connections before getting a renewal. static const int kNumTickets = 2; @@ -162,11 +172,11 @@ static int add_new_session_tickets(SSL_HANDSHAKE *hs) { UniquePtr session( SSL_SESSION_dup(hs->new_session.get(), SSL_SESSION_INCLUDE_NONAUTH)); if (!session) { - return 0; + return false; } if (!RAND_bytes((uint8_t *)&session->ticket_age_add, 4)) { - return 0; + return false; } session->ticket_age_add_valid = true; if (ssl->enable_early_data) { @@ -188,7 +198,7 @@ static int add_new_session_tickets(SSL_HANDSHAKE *hs) { !tls13_derive_session_psk(session.get(), nonce) || !ssl_encrypt_ticket(hs, &ticket, session.get()) || !CBB_add_u16_length_prefixed(&body, &extensions)) { - return 0; + return false; } if (ssl->enable_early_data) { @@ -197,7 +207,7 @@ static int add_new_session_tickets(SSL_HANDSHAKE *hs) { !CBB_add_u16_length_prefixed(&extensions, &early_data_info) || !CBB_add_u32(&early_data_info, session->ticket_max_early_data) || !CBB_flush(&extensions)) { - return 0; + return false; } } @@ -205,15 +215,16 @@ static int add_new_session_tickets(SSL_HANDSHAKE *hs) { if (!CBB_add_u16(&extensions, ssl_get_grease_value(hs, ssl_grease_ticket_extension)) || !CBB_add_u16(&extensions, 0 /* empty */)) { - return 0; + return false; } if (!ssl_add_message_cbb(ssl, cbb.get())) { - return 0; + return false; } } - return 1; + *out_sent_tickets = true; + return true; } static enum ssl_hs_wait_t do_select_parameters(SSL_HANDSHAKE *hs) { @@ -723,11 +734,12 @@ static enum ssl_hs_wait_t do_send_server_finished(SSL_HANDSHAKE *hs) { assert(hs->hash_len <= 0xff); uint8_t header[4] = {SSL3_MT_FINISHED, 0, 0, static_cast(hs->hash_len)}; + bool unused_sent_tickets; if (!hs->transcript.Update(header) || !hs->transcript.Update( MakeConstSpan(hs->expected_client_finished, hs->hash_len)) || !tls13_derive_resumption_secret(hs) || - !add_new_session_tickets(hs)) { + !add_new_session_tickets(hs, &unused_sent_tickets)) { return ssl_hs_error; } } @@ -904,19 +916,13 @@ static enum ssl_hs_wait_t do_read_client_finished(SSL_HANDSHAKE *hs) { } static enum ssl_hs_wait_t do_send_new_session_ticket(SSL_HANDSHAKE *hs) { - // If the client doesn't accept resumption with PSK_DHE_KE, don't send a - // session ticket. - if (!hs->accept_psk_mode) { - hs->tls13_state = state_done; - return ssl_hs_ok; - } - - if (!add_new_session_tickets(hs)) { + bool sent_tickets; + if (!add_new_session_tickets(hs, &sent_tickets)) { return ssl_hs_error; } hs->tls13_state = state_done; - return ssl_hs_flush; + return sent_tickets ? ssl_hs_flush : ssl_hs_ok; } enum ssl_hs_wait_t tls13_server_handshake(SSL_HANDSHAKE *hs) {