commit 71a3b826636eee85c20f064ef318f0935416a479
author David Benjamin <davidben@google.com> Mon May 10 15:57:45 2021 -0400
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Thu May 13 16:37:52 2021 +0000
tree 551c4ff13d7948bcb8abfde155b20e351397896a
parent 6ff942985389b9b64b944b98ab3e8a66e1eb436d

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 <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>

include/openssl/ssl.h

diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 12db981..7a356f7 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -1793,8 +1793,10 @@
 // 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

ssl/handshake_client.cc

diff --git a/ssl/handshake_client.cc b/ssl/handshake_client.cc
index 8f23d95..1a14c11 100644
--- a/ssl/handshake_client.cc
+++ b/ssl/handshake_client.cc
@@ -402,9 +402,7 @@
   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 @@
   }
   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;
 

ssl/ssl_lib.cc

diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc
index e899296..5f3614a 100644
--- a/ssl/ssl_lib.cc
+++ b/ssl/ssl_lib.cc
@@ -275,9 +275,7 @@
 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;
   }

ssl/ssl_session.cc

diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc
index 41df63f..a52ec3d 100644
--- a/ssl/ssl_session.cc
+++ b/ssl/ssl_session.cc
@@ -1004,7 +1004,8 @@
 }
 
 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) {

ssl/tls13_client.cc

diff --git a/ssl/tls13_client.cc b/ssl/tls13_client.cc
index dae4bef..0fd6567 100644
--- a/ssl/tls13_client.cc
+++ b/ssl/tls13_client.cc
@@ -1053,8 +1053,8 @@
     }
   }
 
-  // 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;