From 71a3b826636eee85c20f064ef318f0935416a479 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 10 May 2021 15:57:45 -0400 Subject: [PATCH] Check for resumption identifiers in SSL_SESSION_is_resumable. This aligns with OpenSSL. In particular, we clear not_resumable as soon as the SSL_SESSION is complete, but it may not have an ID or ticket. (Due to APIs like SSL_get_session, SSL_SESSION needs to act both as a resumption handle and a bundle of connection properties.) Along the way, use the modified function in a few internal checks which, with the ssl_update_cache change, removes the last dependency within the library on the placeholder SHA256 IDs. Change-Id: Ic225109ff31ec63ec08625e9f61a20cf0d9dd648 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/47447 Commit-Queue: David Benjamin Reviewed-by: Adam Langley --- include/openssl/ssl.h | 6 ++++-- ssl/handshake_client.cc | 8 +++----- ssl/ssl_lib.cc | 4 +--- ssl/ssl_session.cc | 3 ++- ssl/tls13_client.cc | 4 ++-- 5 files changed, 12 insertions(+), 13 deletions(-) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 12db981e8..7a356f7a1 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -1793,8 +1793,10 @@ OPENSSL_EXPORT int SSL_SESSION_set1_id_context(SSL_SESSION *session, // used without leaking a correlator. OPENSSL_EXPORT int SSL_SESSION_should_be_single_use(const SSL_SESSION *session); -// SSL_SESSION_is_resumable returns one if |session| is resumable and zero -// otherwise. +// SSL_SESSION_is_resumable returns one if |session| is complete and contains a +// session ID or ticket. It returns zero otherwise. Note this function does not +// ensure |session| will be resumed. It may be expired, dropped by the server, +// or associated with incompatible parameters. OPENSSL_EXPORT int SSL_SESSION_is_resumable(const SSL_SESSION *session); // SSL_SESSION_has_ticket returns one if |session| has a ticket and zero diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc index 8f23d959e..1a14c110b 100644 --- a/ssl/handshake_client.cc +++ b/ssl/handshake_client.cc @@ -402,9 +402,7 @@ static enum ssl_hs_wait_t do_start_connect(SSL_HANDSHAKE *hs) { if (ssl->session != nullptr) { if (ssl->session->is_server || !ssl_supports_version(hs, ssl->session->ssl_version) || - (ssl->session->session_id_length == 0 && - ssl->session->ticket.empty()) || - ssl->session->not_resumable || + !SSL_SESSION_is_resumable(ssl->session.get()) || !ssl_session_is_time_valid(ssl, ssl->session.get()) || (ssl->quic_method != nullptr) != ssl->session->is_quic || ssl->s3->initial_handshake_complete) { @@ -1666,8 +1664,8 @@ static enum ssl_hs_wait_t do_read_session_ticket(SSL_HANDSHAKE *hs) { } session->ticket_lifetime_hint = ticket_lifetime_hint; - // Generate a session ID for this session. Some callers expect all sessions to - // have a session ID. + // Historically, OpenSSL filled in fake session IDs for ticket-based sessions. + // TODO(davidben): Are external callers relying on this? Try removing this. SHA256(CBS_data(&ticket), CBS_len(&ticket), session->session_id); session->session_id_length = SHA256_DIGEST_LENGTH; diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index e89929609..5f3614a30 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc @@ -275,9 +275,7 @@ ssl_open_record_t ssl_open_app_data(SSL *ssl, Span *out, void ssl_update_cache(SSL_HANDSHAKE *hs, int mode) { SSL *const ssl = hs->ssl; SSL_CTX *ctx = ssl->session_ctx.get(); - // Never cache sessions with empty session IDs. - if (ssl->s3->established_session->session_id_length == 0 || - ssl->s3->established_session->not_resumable || + if (!SSL_SESSION_is_resumable(ssl->s3->established_session.get()) || (ctx->session_cache_mode & mode) != mode) { return; } diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc index 41df63f93..a52ec3dd7 100644 --- a/ssl/ssl_session.cc +++ b/ssl/ssl_session.cc @@ -1004,7 +1004,8 @@ int SSL_SESSION_should_be_single_use(const SSL_SESSION *session) { } int SSL_SESSION_is_resumable(const SSL_SESSION *session) { - return !session->not_resumable; + return !session->not_resumable && + (session->session_id_length != 0 || !session->ticket.empty()); } int SSL_SESSION_has_ticket(const SSL_SESSION *session) { diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc index dae4bef10..0fd656705 100644 --- a/ssl/tls13_client.cc +++ b/ssl/tls13_client.cc @@ -1053,8 +1053,8 @@ UniquePtr tls13_create_session_with_ticket(SSL *ssl, CBS *body) { } } - // Generate a session ID for this session. Some callers expect all sessions to - // have a session ID. + // Historically, OpenSSL filled in fake session IDs for ticket-based sessions. + // TODO(davidben): Are external callers relying on this? Try removing this. SHA256(CBS_data(&ticket), CBS_len(&ticket), session->session_id); session->session_id_length = SHA256_DIGEST_LENGTH;