Align TLS 1.2 and 1.3 server session validity checks. Having that logic in two different places is a nuisance when we go to add new checks like resumption stuff. Along the way, this adds missing tests for the ClientHello cipher/session consistency check. (We'll eventually get it for free once the cipher/resumption change is unblocked, but get this working in the meantime.) This also fixes a bug where the session validity checks happened in the wrong order relative to whether tickets_supported or renew_ticket was looked at. Fix that by lifting that logic closer to the handshake. Change-Id: I3f4b59cfe01064f9125277dc5834e62a36e64aae Reviewed-on: https://boringssl-review.googlesource.com/12230 Reviewed-by: Adam Langley <agl@google.com>
diff --git a/ssl/handshake_server.c b/ssl/handshake_server.c index c8208ad..4c12d09 100644 --- a/ssl/handshake_server.c +++ b/ssl/handshake_server.c
@@ -730,9 +730,9 @@ client_hello.random_len); /* Determine whether we are doing session resumption. */ - int send_new_ticket = 0; - switch ( - ssl_get_prev_session(ssl, &session, &send_new_ticket, &client_hello)) { + int tickets_supported = 0, renew_ticket = 0; + switch (ssl_get_prev_session(ssl, &session, &tickets_supported, + &renew_ticket, &client_hello)) { case ssl_session_success: break; case ssl_session_error: @@ -741,7 +741,6 @@ ssl->rwstate = SSL_PENDING_SESSION; goto err; } - ssl->s3->hs->ticket_expected = send_new_ticket; /* The EMS state is needed when making the resumption decision, but * extensions are not normally parsed until later. This detects the EMS @@ -754,7 +753,6 @@ TLSEXT_TYPE_extended_master_secret) && CBS_len(&ems) == 0; - int has_session = 0; if (session != NULL) { if (session->extended_master_secret && !have_extended_master_secret) { @@ -765,24 +763,23 @@ goto f_err; } - has_session = - /* Only resume if the session's version matches the negotiated - * version: most clients do not accept a mismatch. */ - ssl->version == session->ssl_version && + if (!ssl_session_is_resumable(ssl, session) || /* If the client offers the EMS extension, but the previous session * didn't use it, then negotiate a new session. */ - have_extended_master_secret == session->extended_master_secret && - /* Only resume if the session's cipher is still valid under the - * current configuration. */ - ssl_is_valid_cipher(ssl, session->cipher); + have_extended_master_secret != session->extended_master_secret) { + SSL_SESSION_free(session); + session = NULL; + } } - if (has_session) { + if (session != NULL) { /* Use the old session. */ + ssl->s3->hs->ticket_expected = renew_ticket; ssl->session = session; session = NULL; ssl->s3->session_reused = 1; } else { + ssl->s3->hs->ticket_expected = tickets_supported; ssl_set_session(ssl, NULL); if (!ssl_get_new_session(ssl, 1 /* server */)) { goto err;
diff --git a/ssl/internal.h b/ssl/internal.h index c964a83..8f4300e 100644 --- a/ssl/internal.h +++ b/ssl/internal.h
@@ -1651,6 +1651,10 @@ * it has expired. */ int ssl_session_is_time_valid(const SSL *ssl, const SSL_SESSION *session); +/* ssl_session_is_resumable returns one if |session| is resumable for |ssl| and + * zero otherwise. */ +int ssl_session_is_resumable(const SSL *ssl, const SSL_SESSION *session); + void ssl_set_session(SSL *ssl, SSL_SESSION *session); enum ssl_session_result_t { @@ -1660,14 +1664,13 @@ }; /* ssl_get_prev_session looks up the previous session based on |ctx|. On - * success, it sets |*out_session| to the session or NULL if none was found. It - * sets |*out_send_ticket| to whether a ticket should be sent at the end of the - * handshake. If the session could not be looked up synchronously, it returns + * success, it sets |*out_session| to the session or NULL if none was found. If + * the session could not be looked up synchronously, it returns * |ssl_session_retry| and should be called again. Otherwise, it returns * |ssl_session_error|. */ enum ssl_session_result_t ssl_get_prev_session( - SSL *ssl, SSL_SESSION **out_session, int *out_send_ticket, - const struct ssl_early_callback_ctx *ctx); + SSL *ssl, SSL_SESSION **out_session, int *out_tickets_supported, + int *out_renew_ticket, const struct ssl_early_callback_ctx *ctx); /* The following flags determine which parts of the session are duplicated. */ #define SSL_SESSION_DUP_AUTH_ONLY 0x0
diff --git a/ssl/ssl_session.c b/ssl/ssl_session.c index 9f61a5d..faf155c 100644 --- a/ssl/ssl_session.c +++ b/ssl/ssl_session.c
@@ -633,6 +633,18 @@ return session->timeout > (long)now.tv_sec - session->time; } +int ssl_session_is_resumable(const SSL *ssl, const SSL_SESSION *session) { + return ssl_session_is_context_valid(ssl, session) && + /* The session must not be expired. */ + ssl_session_is_time_valid(ssl, session) && + /* Only resume if the session's version matches the negotiated + * version. */ + ssl->version == session->ssl_version && + /* Only resume if the session's cipher is still valid under the + * current configuration. */ + ssl_is_valid_cipher(ssl, session->cipher); +} + /* ssl_lookup_session looks up |session_id| in the session cache and sets * |*out_session| to an |SSL_SESSION| object if found. The caller takes * ownership of the result. */ @@ -693,15 +705,8 @@ } } - if (session == NULL) { - return ssl_session_success; - } - - if (!ssl_session_is_context_valid(ssl, session)) { - /* The client did not offer a suitable ticket or session ID. */ - SSL_SESSION_free(session); - session = NULL; - } else if (!ssl_session_is_time_valid(ssl, session)) { + if (session != NULL && + !ssl_session_is_time_valid(ssl, session)) { /* The session was from the cache, so remove it. */ SSL_CTX_remove_session(ssl->initial_ctx, session); SSL_SESSION_free(session); @@ -713,8 +718,8 @@ } enum ssl_session_result_t ssl_get_prev_session( - SSL *ssl, SSL_SESSION **out_session, int *out_send_ticket, - const struct ssl_early_callback_ctx *ctx) { + SSL *ssl, SSL_SESSION **out_session, int *out_tickets_supported, + int *out_renew_ticket, const struct ssl_early_callback_ctx *ctx) { /* This is used only by servers. */ assert(ssl->server); SSL_SESSION *session = NULL; @@ -743,11 +748,8 @@ } *out_session = session; - if (session != NULL) { - *out_send_ticket = renew_ticket; - } else { - *out_send_ticket = tickets_supported; - } + *out_tickets_supported = tickets_supported; + *out_renew_ticket = renew_ticket; return ssl_session_success; }
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 24e4548..c318a9b 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c
@@ -3133,12 +3133,6 @@ memcpy(session->session_id, session_id, session_id_len); session->session_id_length = session_id_len; - if (!ssl_session_is_context_valid(ssl, session) || - !ssl_session_is_time_valid(ssl, session)) { - SSL_SESSION_free(session); - session = NULL; - } - *out_session = session; done:
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index 446c8dc..c3b6b04 100644 --- a/ssl/test/runner/common.go +++ b/ssl/test/runner/common.go
@@ -769,6 +769,11 @@ // the server believes it has actually negotiated. SendCipherSuite uint16 + // SendCipherSuites, if not nil, is the cipher suite list that the + // client will send in the ClientHello. This does not affect the cipher + // the client believes it has actually offered. + SendCipherSuites []uint16 + // AppDataBeforeHandshake, if not nil, causes application data to be // sent immediately before the first handshake message. AppDataBeforeHandshake []byte
diff --git a/ssl/test/runner/handshake_client.go b/ssl/test/runner/handshake_client.go index 3f377f3..36dc1e0 100644 --- a/ssl/test/runner/handshake_client.go +++ b/ssl/test/runner/handshake_client.go
@@ -309,6 +309,10 @@ hello.vers = c.config.Bugs.SendClientVersion } + if c.config.Bugs.SendCipherSuites != nil { + hello.cipherSuites = c.config.Bugs.SendCipherSuites + } + var helloBytes []byte if c.config.Bugs.SendV2ClientHello { // Test that the peer left-pads random.
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index 8f43e5c..717c420 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go
@@ -5633,6 +5633,7 @@ config: Config{ MaxVersion: VersionTLS12, Bugs: ProtocolBugs{ + ExpectNewTicket: true, FilterTicket: func(in []byte) ([]byte, error) { return SetShimTicketVersion(in, VersionTLS13) }, @@ -5672,6 +5673,7 @@ config: Config{ MaxVersion: VersionTLS12, Bugs: ProtocolBugs{ + ExpectNewTicket: true, FilterTicket: func(in []byte) ([]byte, error) { return SetShimTicketCipherSuite(in, TLS_AES_128_GCM_SHA256) }, @@ -5691,6 +5693,7 @@ config: Config{ MaxVersion: VersionTLS12, Bugs: ProtocolBugs{ + ExpectNewTicket: true, FilterTicket: func(in []byte) ([]byte, error) { return SetShimTicketCipherSuite(in, TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) }, @@ -5723,6 +5726,46 @@ expectResumeRejected: true, }) + // Clients must not advertise a session without also advertising the + // cipher. + testCases = append(testCases, testCase{ + testType: serverTest, + name: "Resume-Server-UnofferedCipher", + resumeSession: true, + config: Config{ + MaxVersion: VersionTLS12, + CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256}, + }, + resumeConfig: &Config{ + MaxVersion: VersionTLS12, + CipherSuites: []uint16{TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256}, + Bugs: ProtocolBugs{ + SendCipherSuites: []uint16{TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256}, + }, + }, + shouldFail: true, + expectedError: ":REQUIRED_CIPHER_MISSING:", + }) + + testCases = append(testCases, testCase{ + testType: serverTest, + name: "Resume-Server-UnofferedCipher-TLS13", + resumeSession: true, + config: Config{ + MaxVersion: VersionTLS13, + CipherSuites: []uint16{TLS_CHACHA20_POLY1305_SHA256}, + }, + resumeConfig: &Config{ + MaxVersion: VersionTLS13, + CipherSuites: []uint16{TLS_CHACHA20_POLY1305_SHA256}, + Bugs: ProtocolBugs{ + SendCipherSuites: []uint16{TLS_AES_128_GCM_SHA256}, + }, + }, + shouldFail: true, + expectedError: ":REQUIRED_CIPHER_MISSING:", + }) + // Sessions may not be resumed at a different cipher. testCases = append(testCases, testCase{ name: "Resume-Client-CipherMismatch",
diff --git a/ssl/tls13_server.c b/ssl/tls13_server.c index 0db16d9..c2f8bc5 100644 --- a/ssl/tls13_server.c +++ b/ssl/tls13_server.c
@@ -143,11 +143,7 @@ } if (session != NULL && - /* Only resume if the session's version matches. */ - (session->ssl_version != ssl->version || - !ssl_client_cipher_list_contains_cipher( - &client_hello, (uint16_t)SSL_CIPHER_get_id(session->cipher)) || - !ssl_is_valid_cipher(ssl, session->cipher))) { + !ssl_session_is_resumable(ssl, session)) { SSL_SESSION_free(session); session = NULL; } @@ -268,7 +264,16 @@ return ssl_hs_error; } - if (!ssl->s3->session_reused) { + if (ssl->s3->session_reused) { + /* Clients may not offer sessions containing unsupported ciphers. */ + if (!ssl_client_cipher_list_contains_cipher( + &client_hello, + (uint16_t)SSL_CIPHER_get_id(ssl->s3->new_session->cipher))) { + OPENSSL_PUT_ERROR(SSL, SSL_R_REQUIRED_CIPHER_MISSING); + ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); + return ssl_hs_error; + } + } else { const SSL_CIPHER *cipher = choose_tls13_cipher(ssl, &client_hello); if (cipher == NULL) { OPENSSL_PUT_ERROR(SSL, SSL_R_NO_SHARED_CIPHER);