commit bfdd1a93084f55d01448648e98a3fb3e97e0e8c1
author David Benjamin <davidben@google.com> Fri Jun 29 16:26:38 2018 -0400
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Tue Jul 03 22:57:56 2018 +0000
tree cd0d3f7444c46919baf569f074f9a88186d3efd4
parent 58150ed59b69c2e6c35379d96f4f43c5f5838d03

Give SSL_SESSION a destructor.

Previously we'd partially attempted the ssl_st / bssl::SSLConnection
subclassing split, but that gets messy when we actually try to add a
destructor, because CRYPTO_EX_DATA's cleanup function needs an ssl_st*,
not a bssl::SSLConnection*. Downcasting is technically undefined at this
point and will likely offend some CFI-like check.

Moreover, it appears that even with today's subclassing split,
New<SSL>() emits symbols like:

W ssl_st*& std::forward<ssl_st*&>(std::remove_reference<ssl_st*&>::type&)

The compiler does not bother emitting them in optimized builds, but it
does suggest we can't really avoid claiming the ssl_st type name at the
symbol level, short of doing reinterpret_casts at all API boundaries.
And, of course, we've already long claimed it at the #include level.

So I've just left this defining directly on ssl_session_st. The cost is
we need to write some silly "bssl::" prefixes in the headers, but so it
goes. In the likely event we change our minds again, we can always
revise this.

Change-Id: Ieb429e8eaabe7c2961ef7f8d9234fb71f19a5e2a
Reviewed-on: https://boringssl-review.googlesource.com/29587
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>

ssl/t1_lib.cc

diff --git a/ssl/t1_lib.cc b/ssl/t1_lib.cc
index 6db7de7..49c8955 100644
--- a/ssl/t1_lib.cc
+++ b/ssl/t1_lib.cc
@@ -943,8 +943,7 @@
     return true;
   }
 
-  const uint8_t *ticket_data = NULL;
-  int ticket_len = 0;
+  Span<const uint8_t> ticket;
 
   // Renegotiation does not participate in session resumption. However, still
   // advertise the extension to avoid potentially breaking servers which carry
@@ -952,17 +951,16 @@
   // without upstream's 3c3f0259238594d77264a78944d409f2127642c4.
   if (!ssl->s3->initial_handshake_complete &&
       ssl->session != NULL &&
-      ssl->session->tlsext_tick != NULL &&
+      !ssl->session->ticket.empty() &&
       // Don't send TLS 1.3 session tickets in the ticket extension.
       ssl_session_protocol_version(ssl->session) < TLS1_3_VERSION) {
-    ticket_data = ssl->session->tlsext_tick;
-    ticket_len = ssl->session->tlsext_ticklen;
+    ticket = ssl->session->ticket;
   }
 
-  CBB ticket;
+  CBB ticket_cbb;
   if (!CBB_add_u16(out, TLSEXT_TYPE_session_ticket) ||
-      !CBB_add_u16_length_prefixed(out, &ticket) ||
-      !CBB_add_bytes(&ticket, ticket_data, ticket_len) ||
+      !CBB_add_u16_length_prefixed(out, &ticket_cbb) ||
+      !CBB_add_bytes(&ticket_cbb, ticket.data(), ticket.size()) ||
       !CBB_flush(out)) {
     return false;
   }
@@ -1343,9 +1341,8 @@
   //
   // TODO(davidben): Enforce this anyway.
   if (!ssl->s3->session_reused) {
-    CRYPTO_BUFFER_free(hs->new_session->signed_cert_timestamp_list);
-    hs->new_session->signed_cert_timestamp_list =
-        CRYPTO_BUFFER_new_from_CBS(contents, ssl->ctx->pool);
+    hs->new_session->signed_cert_timestamp_list.reset(
+        CRYPTO_BUFFER_new_from_CBS(contents, ssl->ctx->pool));
     if (hs->new_session->signed_cert_timestamp_list == nullptr) {
       *out_alert = SSL_AD_INTERNAL_ERROR;
       return false;
@@ -1875,7 +1872,7 @@
   }
 
   size_t binder_len = EVP_MD_size(ssl_session_get_digest(ssl->session));
-  return 15 + ssl->session->tlsext_ticklen + binder_len;
+  return 15 + ssl->session->ticket.size() + binder_len;
 }
 
 static bool ext_pre_shared_key_add_clienthello(SSL_HANDSHAKE *hs, CBB *out) {
@@ -1909,8 +1906,8 @@
       !CBB_add_u16_length_prefixed(out, &contents) ||
       !CBB_add_u16_length_prefixed(&contents, &identity) ||
       !CBB_add_u16_length_prefixed(&identity, &ticket) ||
-      !CBB_add_bytes(&ticket, ssl->session->tlsext_tick,
-                     ssl->session->tlsext_ticklen) ||
+      !CBB_add_bytes(&ticket, ssl->session->ticket.data(),
+                     ssl->session->ticket.size()) ||
       !CBB_add_u32(&identity, obfuscated_ticket_age) ||
       !CBB_add_u16_length_prefixed(&contents, &binders) ||
       !CBB_add_u8_length_prefixed(&binders, &binder) ||
@@ -2076,10 +2073,8 @@
       hs->received_hello_retry_request ||
       // In case ALPN preferences changed since this session was established,
       // avoid reporting a confusing value in |SSL_get0_alpn_selected|.
-      (ssl->session->early_alpn_len != 0 &&
-       !ssl_is_alpn_protocol_allowed(
-           hs, MakeConstSpan(ssl->session->early_alpn,
-                             ssl->session->early_alpn_len)))) {
+      (!ssl->session->early_alpn.empty() &&
+       !ssl_is_alpn_protocol_allowed(hs, ssl->session->early_alpn))) {
     return true;
   }