From ee94ec2ef88e0ec25dedf2829d8e48dff0aa1c50 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Fri, 18 May 2018 11:31:31 +0100 Subject: [PATCH] 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 Reviewed-by: Ben Kaduk (Merged from https://github.com/openssl/openssl/pull/6293) --- 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 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 */ diff --git a/test/sslapitest.c b/test/sslapitest.c index fe355f2f4c..fe1c1e6ff3 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; -- 2.25.1