commit ef1b0093444e62644f9cdec4e516705cb79bcf79
author David Benjamin <davidben@chromium.org> Sat Nov 21 14:05:44 2015 -0500
committer Adam Langley <alangley@gmail.com> Fri Jan 15 20:08:52 2016 +0000
tree 22ffec96b2b7be87931b6d9012681fe2024d1bfb
parent dd1f6f4fbae4d420062fb7d7810badc754a909e2

Consider session if the client supports tickets but offered a session ID.

This is a minor regression from
https://boringssl-review.googlesource.com/5235.

If the client, for whatever reason, had an ID-based session but also
supports tickets, it will send non-empty ID + empty ticket extension.
If the ticket extension is non-empty, then the ID is not an ID but a
dummy signaling value, so 5235 avoided looking it up. But if it is
present and empty, the ID is still an ID and should be looked up.

This shouldn't have any practical consequences, except if a server
switched from not supporting tickets and then started supporting it,
while keeping the session cache fixed.

Add a test for this case, and tighten up existing ID vs ticket tests so
they fail if we resume with the wrong type.

Change-Id: Id4d08cd809af00af30a2b67fe3a971078e404c75
Reviewed-on: https://boringssl-review.googlesource.com/6554
Reviewed-by: Adam Langley <alangley@gmail.com>

ssl/ssl_session.c

diff --git a/ssl/ssl_session.c b/ssl/ssl_session.c
index 3d59bc3..24de4ec 100644
--- a/ssl/ssl_session.c
+++ b/ssl/ssl_session.c
@@ -430,7 +430,7 @@
   /* This is used only by servers. */
   assert(ssl->server);
   SSL_SESSION *session = NULL;
-  int send_ticket = 0;
+  int renew_ticket = 0;
 
   /* If tickets are disabled, always behave as if no tickets are present. */
   const uint8_t *ticket = NULL;
@@ -440,24 +440,27 @@
       ssl->version > SSL3_VERSION &&
       SSL_early_callback_ctx_extension_get(ctx, TLSEXT_TYPE_session_ticket,
                                            &ticket, &ticket_len);
-  if (tickets_supported) {
-    if (!tls_process_ticket(ssl, &session, &send_ticket, ticket, ticket_len,
+  int from_cache = 0;
+  if (tickets_supported && ticket_len > 0) {
+    if (!tls_process_ticket(ssl, &session, &renew_ticket, ticket, ticket_len,
                             ctx->session_id, ctx->session_id_len)) {
       return ssl_session_error;
     }
   } else {
-    /* The client does not support session tickets, so the session ID should be
-     * used instead. */
+    /* The client didn't send a ticket, so the session ID is a real ID. */
     enum ssl_session_result_t lookup_ret = ssl_lookup_session(
         ssl, &session, ctx->session_id, ctx->session_id_len);
     if (lookup_ret != ssl_session_success) {
       return lookup_ret;
     }
+    from_cache = 1;
   }
 
   if (session == NULL ||
       session->sid_ctx_length != ssl->sid_ctx_length ||
       memcmp(session->sid_ctx, ssl->sid_ctx, ssl->sid_ctx_length) != 0) {
+    /* The client did not offer a suitable ticket or session ID. If supported,
+     * the new session should use a ticket. */
     goto no_session;
   }
 
@@ -471,11 +474,12 @@
      * effectively disable the session cache by accident without anyone
      * noticing). */
     OPENSSL_PUT_ERROR(SSL, SSL_R_SESSION_ID_CONTEXT_UNINITIALIZED);
-    goto fatal_error;
+    SSL_SESSION_free(session);
+    return ssl_session_error;
   }
 
   if (session->timeout < (long)(time(NULL) - session->time)) {
-    if (!tickets_supported) {
+    if (from_cache) {
       /* The session was from the cache, so remove it. */
       SSL_CTX_remove_session(ssl->initial_ctx, session);
     }
@@ -483,13 +487,9 @@
   }
 
   *out_session = session;
-  *out_send_ticket = send_ticket;
+  *out_send_ticket = renew_ticket;
   return ssl_session_success;
 
-fatal_error:
-  SSL_SESSION_free(session);
-  return ssl_session_error;
-
 no_session:
   *out_session = NULL;
   *out_send_ticket = tickets_supported;