commit 95d31825760c6d35e597234900e33ef48d9ab809
author David Benjamin <davidben@chromium.org> Mon Jun 15 19:53:32 2015 -0400
committer Adam Langley <agl@google.com> Tue Jun 16 18:15:12 2015 +0000
tree c305b017b4c86d7ae2cf1d780313b4e4a735b127
parent fd67aa8c954d3adca722fc7487189bf17a82e6f2

Duplicate SSL_SESSIONs when renewing them.

See also upstream's 27c76b9b8010b536687318739c6f631ce4194688, CVE-2015-1791.
Rather than write a dup function, serializing and deserializing the object is
simpler. It also fixes a bug in the original fix where it never calls
new_session_cb to store the new session (for clients which use that callback;
how clients should handle the session cache is much less clear).

The old session isn't pruned as we haven't processed the Finished message yet.
RFC 5077 says:

   The server MUST NOT assume that the client actually received the updated
   ticket until it successfully verifies the client's Finished message.

Moreover, because network messages are asynchronous, a new SSL connection may
have began just before the client received the new ticket, so any such servers
are broken regardless.

Change-Id: I13b3dc986dc58ea2ce66659dbb29e14cd02a641b
Reviewed-on: https://boringssl-review.googlesource.com/5122
Reviewed-by: Adam Langley <agl@google.com>

ssl/s3_clnt.c

diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c
index c9e7fcd..90b59c5 100644
--- a/ssl/s3_clnt.c
+++ b/ssl/s3_clnt.c
@@ -1501,6 +1501,27 @@
     return n;
   }
 
+  if (s->hit) {
+    /* The server is sending a new ticket for an existing session. Sessions are
+     * immutable once established, so duplicate all but the ticket of the
+     * existing session. */
+    uint8_t *bytes;
+    size_t bytes_len;
+    if (!SSL_SESSION_to_bytes_for_ticket(s->session, &bytes, &bytes_len)) {
+      goto err;
+    }
+    SSL_SESSION *new_session = SSL_SESSION_from_bytes(bytes, bytes_len);
+    OPENSSL_free(bytes);
+    if (new_session == NULL) {
+      /* This should never happen. */
+      OPENSSL_PUT_ERROR(SSL, ssl3_get_new_session_ticket, ERR_R_INTERNAL_ERROR);
+      goto err;
+    }
+
+    SSL_SESSION_free(s->session);
+    s->session = new_session;
+  }
+
   CBS_init(&new_session_ticket, s->init_msg, n);
 
   if (!CBS_get_u32(&new_session_ticket,

ssl/ssl_asn1.c

diff --git a/ssl/ssl_asn1.c b/ssl/ssl_asn1.c
index 709e15b..76052df 100644
--- a/ssl/ssl_asn1.c
+++ b/ssl/ssl_asn1.c
@@ -261,7 +261,7 @@
     }
   }
 
-  if (in->tlsext_tick) {
+  if (in->tlsext_tick && !for_ticket) {
     if (!CBB_add_asn1(&session, &child, kTicketTag) ||
         !CBB_add_asn1(&child, &child2, CBS_ASN1_OCTETSTRING) ||
         !CBB_add_bytes(&child2, in->tlsext_tick, in->tlsext_ticklen)) {

ssl/ssl_lib.c

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index e82447c..1328cd5 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -1948,8 +1948,16 @@
     return;
   }
 
+  int has_new_session = !s->hit;
+  if (!s->server && s->tlsext_ticket_expected) {
+    /* A client may see new sessions on abbreviated handshakes if the server
+     * decides to renew the ticket. Once the handshake is completed, it should
+     * be inserted into the cache. */
+    has_new_session = 1;
+  }
+
   SSL_CTX *ctx = s->initial_ctx;
-  if ((ctx->session_cache_mode & mode) == mode && !s->hit &&
+  if ((ctx->session_cache_mode & mode) == mode && has_new_session &&
       ((ctx->session_cache_mode & SSL_SESS_CACHE_NO_INTERNAL_STORE) ||
        SSL_CTX_add_session(ctx, s->session)) &&
       ctx->new_session_cb != NULL) {