diff --git a/ssl/extensions.cc b/ssl/extensions.cc index ad4bf8151..1845bcaf7 100644 --- a/ssl/extensions.cc +++ b/ssl/extensions.cc @@ -943,17 +943,14 @@ static bool ext_ticket_add_clienthello(const SSL_HANDSHAKE *hs, CBB *out, return true; } - Span 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 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 diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index 9bd84627e..2655a6a82 100644 --- a/ssl/handshake_client.cc +++ b/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; diff --git a/ssl/internal.h b/ssl/internal.h index fcca3c73e..3e68a8bf1 100644 --- a/ssl/internal.h +++ b/ssl/internal.h @@ -3777,6 +3777,20 @@ OPENSSL_EXPORT UniquePtr 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, diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc index f455b1b62..9ceefbb0a 100644 --- a/ssl/ssl_session.cc +++ b/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) { diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index a4fedd890..a6e62d442 100644 --- a/ssl/ssl_test.cc +++ b/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 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 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 diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index 16a1df2c6..2a3b648b1 100644 --- a/ssl/test/runner/common.go +++ b/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 diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go index 8c10c8205..08165ae15 100644 --- a/ssl/test/runner/handshake_server.go +++ b/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") diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 4e90c3174..4f7d74aa7 100644 --- a/ssl/test/runner/runner.go +++ b/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", + }) + } } } diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc index a783a0a15..00215c3cd 100644 --- a/ssl/tls13_client.cc +++ b/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);