From 975610e875ca8bc7011ba0e4f57a0df8ee22378c Mon Sep 17 00:00:00 2001
From: beck <beck@openbsd.org>
Date: Sat, 4 Sep 2021 15:21:45 +0000
Subject: [PATCH] Refactor ssl_update_cache. This now matches the logic used
 for TLS 1.3 in Openssl 1.1.1 for when to call the session callbacks. I
 believe it to also generates a lot less eye bleed, confirmed by tb@

ok jsing@ tb@
---
 lib/libssl/ssl_lib.c | 128 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 106 insertions(+), 22 deletions(-)

diff --git a/lib/libssl/ssl_lib.c b/lib/libssl/ssl_lib.c
index c5cc6d05faf..142771c423f 100644
--- a/lib/libssl/ssl_lib.c
+++ b/lib/libssl/ssl_lib.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_lib.c,v 1.263 2021/08/30 19:25:43 jsing Exp $ */
+/* $OpenBSD: ssl_lib.c,v 1.264 2021/09/04 15:21:45 beck Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -2242,35 +2242,119 @@ ssl_get_auto_dh(SSL *s)
 	return (dhp);
 }
 
-void
-ssl_update_cache(SSL *s, int mode)
+static int
+ssl_should_update_external_cache(SSL *s, int mode)
 {
-	int	i;
+	int cache_mode;
+
+	cache_mode = s->session_ctx->internal->session_cache_mode;
+
+	/* Don't cache if mode says not to */
+	if ((cache_mode & mode) == 0)
+		return 0;
+
+	/* if it is not already cached, cache it */
+	if (!s->internal->hit)
+		return 1;
+
+	/* If it's TLS 1.3, do it to match OpenSSL */
+	if (S3I(s)->hs.negotiated_tls_version >= TLS1_3_VERSION)
+		return 1;
+
+	return 0;
+}
+
+static int
+ssl_should_update_internal_cache(SSL *s, int mode)
+{
+	int cache_mode;
+
+	cache_mode = s->session_ctx->internal->session_cache_mode;
+
+	/* Don't cache if mode says not to */
+	if ((cache_mode & mode) == 0)
+		return 0;
+
+	/* If it is already cached, don't cache it again */
+	if (s->internal->hit)
+		return 0;
+
+	if ((cache_mode & SSL_SESS_CACHE_NO_INTERNAL_STORE) != 0)
+		return 0;
+
+	/* If we are lesser than TLS 1.3, Cache it. */
+	if (S3I(s)->hs.negotiated_tls_version < TLS1_3_VERSION)
+		return 1;
+
+	/* Below this we consider TLS 1.3 or later */
+
+	/* If it's not a server, add it? OpenSSL does this. */
+	if (!s->server)
+		return 1;
+
+	/* XXX if we support early data / PSK need to add */
 
 	/*
-	 * If the session_id_length is 0, we are not supposed to cache it,
-	 * and it would be rather hard to do anyway :-)
+	 * If we have the remove session callback, we will want
+	 * to know about this even if it's a stateless ticket
+	 * from 1.3 so we can know when it is removed.
 	 */
+	if (s->session_ctx->internal->remove_session_cb != NULL)
+		return 1;
+
+	/* If we have set OP_NO_TICKET, cache it. */
+	if ((s->internal->options & SSL_OP_NO_TICKET) != 0)
+		return 1;
+
+	/* Otherwise do not cache */
+	return 0;
+}
+
+void
+ssl_update_cache(SSL *s, int mode)
+{
+	int cache_mode, do_callback;
+
 	if (s->session->session_id_length == 0)
 		return;
 
-	i = s->session_ctx->internal->session_cache_mode;
-	if ((i & mode) && (!s->internal->hit) && ((i & SSL_SESS_CACHE_NO_INTERNAL_STORE)
-	    || SSL_CTX_add_session(s->session_ctx, s->session))
-	    && (s->session_ctx->internal->new_session_cb != NULL)) {
-		CRYPTO_add(&s->session->references, 1, CRYPTO_LOCK_SSL_SESSION);
-		if (!s->session_ctx->internal->new_session_cb(s, s->session))
-			SSL_SESSION_free(s->session);
-	}
-
-	/* auto flush every 255 connections */
-	if ((!(i & SSL_SESS_CACHE_NO_AUTO_CLEAR)) &&
-	    ((i & mode) == mode)) {
-		if ((((mode & SSL_SESS_CACHE_CLIENT) ?
-		    s->session_ctx->internal->stats.sess_connect_good :
-		    s->session_ctx->internal->stats.sess_accept_good) & 0xff) == 0xff) {
+	cache_mode = s->session_ctx->internal->session_cache_mode;
+	do_callback = ssl_should_update_external_cache(s, mode);
+
+	if (ssl_should_update_internal_cache(s, mode)) {
+		/*
+		 * XXX should we fail if the add to the internal cache
+		 * fails? OpenSSL doesn't care..
+		 */
+		(void) SSL_CTX_add_session(s->session_ctx, s->session);
+	}
+
+	/*
+	 * Update the "external cache" by calling the new session
+	 * callback if present, even with TLS 1.3 without early data
+	 * "because some application just want to know about the
+	 * creation of a session and aren't doing a full cache".
+	 * Apparently, if they are doing a full cache, they'll have
+	 * some fun, but we endeavour to give application writers the
+	 * same glorious experience they expect from OpenSSL which
+	 * does it this way.
+	 */
+	if (do_callback && s->session_ctx->internal->new_session_cb != NULL) {
+		    CRYPTO_add(&s->session->references, 1, CRYPTO_LOCK_SSL_SESSION);
+		    if (!s->session_ctx->internal->new_session_cb(s, s->session))
+			    SSL_SESSION_free(s->session);
+	}
+
+	/* Auto flush every 255 connections. */
+	if (!(cache_mode & SSL_SESS_CACHE_NO_AUTO_CLEAR) &&
+	    (cache_mode & mode) != 0) {
+		int connections;
+		if (mode & SSL_SESS_CACHE_CLIENT)
+			connections = s->session_ctx->internal->stats.sess_connect_good;
+		else
+			connections = s->session_ctx->internal->stats.sess_accept_good;
+		if ((connections & 0xff) == 0xff)
 			SSL_CTX_flush_sessions(s->session_ctx, time(NULL));
-		}
 	}
 }
 
-- 
2.20.1