| From e9d7083e241670332e0443da0f0d4ffb52829f08 Mon Sep 17 00:00:00 2001 |
| From: Matt Caswell <matt@openssl.org> |
| Date: Tue, 5 Mar 2024 15:43:53 +0000 |
| Subject: [PATCH] Fix unconstrained session cache growth in TLSv1.3 |
| |
| In TLSv1.3 we create a new session object for each ticket that we send. |
| We do this by duplicating the original session. If SSL_OP_NO_TICKET is in |
| use then the new session will be added to the session cache. However, if |
| early data is not in use (and therefore anti-replay protection is being |
| used), then multiple threads could be resuming from the same session |
| simultaneously. If this happens and a problem occurs on one of the threads, |
| then the original session object could be marked as not_resumable. When we |
| duplicate the session object this not_resumable status gets copied into the |
| new session object. The new session object is then added to the session |
| cache even though it is not_resumable. |
| |
| Subsequently, another bug means that the session_id_length is set to 0 for |
| sessions that are marked as not_resumable - even though that session is |
| still in the cache. Once this happens the session can never be removed from |
| the cache. When that object gets to be the session cache tail object the |
| cache never shrinks again and grows indefinitely. |
| |
| CVE-2024-2511 |
| |
| Reviewed-by: Neil Horman <nhorman@openssl.org> |
| Reviewed-by: Tomas Mraz <tomas@openssl.org> |
| (Merged from https://github.com/openssl/openssl/pull/24043) |
| |
| CVE: CVE-2024-2511 |
| Upstream-Status: Backport [https://github.com/openssl/openssl/commit/e9d7083e241670332e0443da0f0d4ffb52829f08] |
| Signed-off-by: Peter Marko <peter.marko@siemens.com> |
| --- |
| ssl/ssl_lib.c | 5 +++-- |
| ssl/ssl_sess.c | 28 ++++++++++++++++++++++------ |
| ssl/statem/statem_srvr.c | 5 ++--- |
| 3 files changed, 27 insertions(+), 11 deletions(-) |
| |
| diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c |
| index 4afb43bc86e54..c51529ddab5bb 100644 |
| --- a/ssl/ssl_lib.c |
| +++ b/ssl/ssl_lib.c |
| @@ -4457,9 +4457,10 @@ void ssl_update_cache(SSL_CONNECTION *s, int mode) |
| |
| /* |
| * If the session_id_length is 0, we are not supposed to cache it, and it |
| - * would be rather hard to do anyway :-) |
| + * would be rather hard to do anyway :-). Also if the session has already |
| + * been marked as not_resumable we should not cache it for later reuse. |
| */ |
| - if (s->session->session_id_length == 0) |
| + if (s->session->session_id_length == 0 || s->session->not_resumable) |
| return; |
| |
| /* |
| diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c |
| index 3dcc4d81e5bc6..1fa6d17c46863 100644 |
| --- a/ssl/ssl_sess.c |
| +++ b/ssl/ssl_sess.c |
| @@ -127,16 +127,11 @@ SSL_SESSION *SSL_SESSION_new(void) |
| return ss; |
| } |
| |
| -SSL_SESSION *SSL_SESSION_dup(const SSL_SESSION *src) |
| -{ |
| - return ssl_session_dup(src, 1); |
| -} |
| - |
| /* |
| * Create a new SSL_SESSION and duplicate the contents of |src| into it. If |
| * ticket == 0 then no ticket information is duplicated, otherwise it is. |
| */ |
| -SSL_SESSION *ssl_session_dup(const SSL_SESSION *src, int ticket) |
| +static SSL_SESSION *ssl_session_dup_intern(const SSL_SESSION *src, int ticket) |
| { |
| SSL_SESSION *dest; |
| |
| @@ -265,6 +260,27 @@ SSL_SESSION *ssl_session_dup(const SSL_SESSION *src, int ticket) |
| return NULL; |
| } |
| |
| +SSL_SESSION *SSL_SESSION_dup(const SSL_SESSION *src) |
| +{ |
| + return ssl_session_dup_intern(src, 1); |
| +} |
| + |
| +/* |
| + * Used internally when duplicating a session which might be already shared. |
| + * We will have resumed the original session. Subsequently we might have marked |
| + * it as non-resumable (e.g. in another thread) - but this copy should be ok to |
| + * resume from. |
| + */ |
| +SSL_SESSION *ssl_session_dup(const SSL_SESSION *src, int ticket) |
| +{ |
| + SSL_SESSION *sess = ssl_session_dup_intern(src, ticket); |
| + |
| + if (sess != NULL) |
| + sess->not_resumable = 0; |
| + |
| + return sess; |
| +} |
| + |
| const unsigned char *SSL_SESSION_get_id(const SSL_SESSION *s, unsigned int *len) |
| { |
| if (len) |
| diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c |
| index 853af8c0aa9f9..d5f0ab091dacc 100644 |
| --- a/ssl/statem/statem_srvr.c |
| +++ b/ssl/statem/statem_srvr.c |
| @@ -2445,9 +2445,8 @@ CON_FUNC_RETURN tls_construct_server_hello(SSL_CONNECTION *s, WPACKET *pkt) |
| * so the following won't overwrite an ID that we're supposed |
| * to send back. |
| */ |
| - if (s->session->not_resumable || |
| - (!(SSL_CONNECTION_GET_CTX(s)->session_cache_mode & SSL_SESS_CACHE_SERVER) |
| - && !s->hit)) |
| + if (!(SSL_CONNECTION_GET_CTX(s)->session_cache_mode & SSL_SESS_CACHE_SERVER) |
| + && !s->hit) |
| s->session->session_id_length = 0; |
| |
| if (usetls13) { |