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@openssl.org>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from https://github.com/openssl/openssl/pull/6293)

1 files changed, 27 insertions, 7 deletions

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index c38fc58a5d..1dd355d0da 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 */