[openssl-commits] [openssl] master update

[Matt Caswell]

The branch master has been updated
       via  ee94ec2ef88e0ec25dedf2829d8e48dff0aa1c50 (commit)
      from  511190b691183a1fb160e7e05e2974dc73cab0c6 (commit)


- Log -----------------------------------------------------------------
commit ee94ec2ef88e0ec25dedf2829d8e48dff0aa1c50
Author: Matt Caswell <matt at openssl.org>
Date:   Fri May 18 11:31:31 2018 +0100

    Don't cache stateless tickets in TLSv1.3
    
    In TLSv1.2 and below we always cache new sessions by default on the server
    side in the internal cache (even when we're using session tickets). This is
    in order to support resumption from a session id.
    
    In TLSv1.3 there is no session id. It is only possible to resume using the
    ticket. Therefore, in the default case,  there is no point in caching the
    session in the internal store.
    
    There is still a reason to call the external cache new session callback
    because applications may be using the callbacks just to know about when
    sessions are created (and not necessarily implementing a full cache). If
    the application also implements the remove session callback then we are
    forced to also store it in the internal cache so that we can create
    timeout events. Otherwise the external cache could just fill up
    indefinitely.
    
    This mostly addresses the issue described in #5628. That issue also proposes
    having an option to not create full stateless tickets when using the
    internal cache. That aspect hasn't been addressed yet.
    
    Reviewed-by: Rich Salz <rsalz at openssl.org>
    Reviewed-by: Ben Kaduk <kaduk at mit.edu>
    (Merged from https://github.com/openssl/openssl/pull/6293)

-----------------------------------------------------------------------

Summary of changes:
 ssl/ssl_lib.c     | 34 +++++++++++++++++++++++++++-------
 test/sslapitest.c | 21 +++++++++++++++++----
 2 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index c38fc58..1dd355d 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -3365,13 +3365,33 @@ void ssl_update_cache(SSL *s, int mode)
 
     i = s->session_ctx->session_cache_mode;
     if ((i & mode) != 0
-        && (!s->hit || SSL_IS_TLS13(s))
-        && ((i & SSL_SESS_CACHE_NO_INTERNAL_STORE) != 0
-            || SSL_CTX_add_session(s->session_ctx, s->session))
-        && s->session_ctx->new_session_cb != NULL) {
-        SSL_SESSION_up_ref(s->session);
-        if (!s->session_ctx->new_session_cb(s, s->session))
-            SSL_SESSION_free(s->session);
+        && (!s->hit || SSL_IS_TLS13(s))) {
+        /*
+         * Add the session to the internal cache. In server side TLSv1.3 we
+         * normally don't do this because its a full stateless ticket with only
+         * a dummy session id so there is no reason to cache it, unless:
+         * - we are doing early_data, in which case we cache so that we can
+         *   detect replays
+         * - the application has set a remove_session_cb so needs to know about
+         *   session timeout events
+         */
+        if ((i & SSL_SESS_CACHE_NO_INTERNAL_STORE) == 0
+                && (!SSL_IS_TLS13(s)
+                    || !s->server
+                    || s->max_early_data > 0
+                    || s->session_ctx->remove_session_cb != NULL))
+            SSL_CTX_add_session(s->session_ctx, s->session);
+
+        /*
+         * Add the session to the external cache. We do this even in server side
+         * TLSv1.3 without early data because some applications just want to
+         * know about the creation of a session and aren't doing a full cache.
+         */
+        if (s->session_ctx->new_session_cb != NULL) {
+            SSL_SESSION_up_ref(s->session);
+            if (!s->session_ctx->new_session_cb(s, s->session))
+                SSL_SESSION_free(s->session);
+        }
     }
 
     /* auto flush every 255 connections */
diff --git a/test/sslapitest.c b/test/sslapitest.c
index fe355f2..fe1c1e6 100644
--- a/test/sslapitest.c
+++ b/test/sslapitest.c
@@ -1072,9 +1072,22 @@ static int execute_test_session(int maxprot, int use_int_cache,
             || !TEST_ptr(sess2 = SSL_get1_session(serverssl1)))
         goto end;
 
-    /* Should fail because it should already be in the cache */
-    if (use_int_cache && !TEST_false(SSL_CTX_add_session(sctx, sess2)))
-        goto end;
+    if (use_int_cache) {
+        if (maxprot == TLS1_3_VERSION && !use_ext_cache) {
+            /*
+             * In TLSv1.3 it should not have been added to the internal cache,
+             * except in the case where we also have an external cache (in that
+             * case it gets added to the cache in order to generate remove
+             * events after timeout).
+             */
+            if (!TEST_false(SSL_CTX_remove_session(sctx, sess2)))
+                goto end;
+        } else {
+            /* Should fail because it should already be in the cache */
+            if (!TEST_false(SSL_CTX_add_session(sctx, sess2)))
+                goto end;
+        }
+    }
 
     if (use_ext_cache) {
         SSL_SESSION *tmp = sess2;
@@ -1088,7 +1101,7 @@ static int execute_test_session(int maxprot, int use_int_cache,
          * the external cache. We take a copy first because
          * SSL_CTX_remove_session() also marks the session as non-resumable.
          */
-        if (use_int_cache) {
+        if (use_int_cache && maxprot != TLS1_3_VERSION) {
             if (!TEST_ptr(tmp = SSL_SESSION_dup(sess2))
                     || !TEST_true(SSL_CTX_remove_session(sctx, sess2)))
                 goto end;