|
| 1 | +From 5f8d25770ae6437db119dfc951e207271a326640 Mon Sep 17 00:00:00 2001 |
| 2 | +From: Matt Caswell <matt@openssl.org> |
| 3 | +Date: Tue, 5 Mar 2024 15:43:53 +0000 |
| 4 | +Subject: [PATCH] Fix unconstrained session cache growth in TLSv1.3 |
| 5 | + |
| 6 | +In TLSv1.3 we create a new session object for each ticket that we send. |
| 7 | +We do this by duplicating the original session. If SSL_OP_NO_TICKET is in |
| 8 | +use then the new session will be added to the session cache. However, if |
| 9 | +early data is not in use (and therefore anti-replay protection is being |
| 10 | +used), then multiple threads could be resuming from the same session |
| 11 | +simultaneously. If this happens and a problem occurs on one of the threads, |
| 12 | +then the original session object could be marked as not_resumable. When we |
| 13 | +duplicate the session object this not_resumable status gets copied into the |
| 14 | +new session object. The new session object is then added to the session |
| 15 | +cache even though it is not_resumable. |
| 16 | + |
| 17 | +Subsequently, another bug means that the session_id_length is set to 0 for |
| 18 | +sessions that are marked as not_resumable - even though that session is |
| 19 | +still in the cache. Once this happens the session can never be removed from |
| 20 | +the cache. When that object gets to be the session cache tail object the |
| 21 | +cache never shrinks again and grows indefinitely. |
| 22 | + |
| 23 | +CVE-2024-2511 |
| 24 | + |
| 25 | +Reviewed-by: Neil Horman <nhorman@openssl.org> |
| 26 | +Reviewed-by: Tomas Mraz <tomas@openssl.org> |
| 27 | +--- |
| 28 | + crypto/err/openssl.txt | 3 ++- |
| 29 | + include/openssl/sslerr.h | 7 +++---- |
| 30 | + ssl/ssl_err.c | 6 +++++- |
| 31 | + ssl/ssl_lib.c | 5 +++-- |
| 32 | + ssl/ssl_sess.c | 30 +++++++++++++++++++++++------- |
| 33 | + ssl/statem/statem_srvr.c | 5 ++--- |
| 34 | + 6 files changed, 38 insertions(+), 18 deletions(-) |
| 35 | + |
| 36 | +diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt |
| 37 | +index 900b11ee9..ac8bfe50e 100644 |
| 38 | +--- a/crypto/err/openssl.txt |
| 39 | ++++ b/crypto/err/openssl.txt |
| 40 | +@@ -1187,7 +1187,7 @@ SSL_F_OSSL_STATEM_SERVER_CONSTRUCT_MESSAGE:431:* |
| 41 | + SSL_F_OSSL_STATEM_SERVER_POST_PROCESS_MESSAGE:601:\ |
| 42 | + ossl_statem_server_post_process_message |
| 43 | + SSL_F_OSSL_STATEM_SERVER_POST_WORK:602:ossl_statem_server_post_work |
| 44 | +-SSL_F_OSSL_STATEM_SERVER_PRE_WORK:640: |
| 45 | ++SSL_F_OSSL_STATEM_SERVER_PRE_WORK:640:ossl_statem_server_pre_work |
| 46 | + SSL_F_OSSL_STATEM_SERVER_PROCESS_MESSAGE:603:ossl_statem_server_process_message |
| 47 | + SSL_F_OSSL_STATEM_SERVER_READ_TRANSITION:418:ossl_statem_server_read_transition |
| 48 | + SSL_F_OSSL_STATEM_SERVER_WRITE_TRANSITION:604:\ |
| 49 | +@@ -1323,6 +1323,7 @@ SSL_F_SSL_RENEGOTIATE_ABBREVIATED:546:SSL_renegotiate_abbreviated |
| 50 | + SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT:320:* |
| 51 | + SSL_F_SSL_SCAN_SERVERHELLO_TLSEXT:321:* |
| 52 | + SSL_F_SSL_SESSION_DUP:348:ssl_session_dup |
| 53 | ++SSL_F_SSL_SESSION_DUP_INTERN:641:ssl_session_dup_intern |
| 54 | + SSL_F_SSL_SESSION_NEW:189:SSL_SESSION_new |
| 55 | + SSL_F_SSL_SESSION_PRINT_FP:190:SSL_SESSION_print_fp |
| 56 | + SSL_F_SSL_SESSION_SET1_ID:423:SSL_SESSION_set1_id |
| 57 | +diff --git a/include/openssl/sslerr.h b/include/openssl/sslerr.h |
| 58 | +index 701d61c6e..a87bc4260 100644 |
| 59 | +--- a/include/openssl/sslerr.h |
| 60 | ++++ b/include/openssl/sslerr.h |
| 61 | +@@ -1,6 +1,6 @@ |
| 62 | + /* |
| 63 | + * Generated by util/mkerr.pl DO NOT EDIT |
| 64 | +- * Copyright 1995-2020 The OpenSSL Project Authors. All Rights Reserved. |
| 65 | ++ * Copyright 1995-2024 The OpenSSL Project Authors. All Rights Reserved. |
| 66 | + * |
| 67 | + * Licensed under the OpenSSL license (the "License"). You may not use |
| 68 | + * this file except in compliance with the License. You can obtain a copy |
| 69 | +@@ -11,9 +11,7 @@ |
| 70 | + #ifndef HEADER_SSLERR_H |
| 71 | + # define HEADER_SSLERR_H |
| 72 | + |
| 73 | +-# ifndef HEADER_SYMHACKS_H |
| 74 | +-# include <openssl/symhacks.h> |
| 75 | +-# endif |
| 76 | ++# include <openssl/symhacks.h> |
| 77 | + |
| 78 | + # ifdef __cplusplus |
| 79 | + extern "C" |
| 80 | +@@ -221,6 +219,7 @@ int ERR_load_SSL_strings(void); |
| 81 | + # define SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT 320 |
| 82 | + # define SSL_F_SSL_SCAN_SERVERHELLO_TLSEXT 321 |
| 83 | + # define SSL_F_SSL_SESSION_DUP 348 |
| 84 | ++# define SSL_F_SSL_SESSION_DUP_INTERN 641 |
| 85 | + # define SSL_F_SSL_SESSION_NEW 189 |
| 86 | + # define SSL_F_SSL_SESSION_PRINT_FP 190 |
| 87 | + # define SSL_F_SSL_SESSION_SET1_ID 423 |
| 88 | +diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c |
| 89 | +index 324f2ccbb..4ad99a531 100644 |
| 90 | +--- a/ssl/ssl_err.c |
| 91 | ++++ b/ssl/ssl_err.c |
| 92 | +@@ -1,6 +1,6 @@ |
| 93 | + /* |
| 94 | + * Generated by util/mkerr.pl DO NOT EDIT |
| 95 | +- * Copyright 1995-2019 The OpenSSL Project Authors. All Rights Reserved. |
| 96 | ++ * Copyright 1995-2024 The OpenSSL Project Authors. All Rights Reserved. |
| 97 | + * |
| 98 | + * Licensed under the OpenSSL license (the "License"). You may not use |
| 99 | + * this file except in compliance with the License. You can obtain a copy |
| 100 | +@@ -113,6 +113,8 @@ static const ERR_STRING_DATA SSL_str_functs[] = { |
| 101 | + "ossl_statem_server_post_process_message"}, |
| 102 | + {ERR_PACK(ERR_LIB_SSL, SSL_F_OSSL_STATEM_SERVER_POST_WORK, 0), |
| 103 | + "ossl_statem_server_post_work"}, |
| 104 | ++ {ERR_PACK(ERR_LIB_SSL, SSL_F_OSSL_STATEM_SERVER_PRE_WORK, 0), |
| 105 | ++ "ossl_statem_server_pre_work"}, |
| 106 | + {ERR_PACK(ERR_LIB_SSL, SSL_F_OSSL_STATEM_SERVER_PROCESS_MESSAGE, 0), |
| 107 | + "ossl_statem_server_process_message"}, |
| 108 | + {ERR_PACK(ERR_LIB_SSL, SSL_F_OSSL_STATEM_SERVER_READ_TRANSITION, 0), |
| 109 | +@@ -314,6 +316,8 @@ static const ERR_STRING_DATA SSL_str_functs[] = { |
| 110 | + {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT, 0), ""}, |
| 111 | + {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_SCAN_SERVERHELLO_TLSEXT, 0), ""}, |
| 112 | + {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_SESSION_DUP, 0), "ssl_session_dup"}, |
| 113 | ++ {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_SESSION_DUP_INTERN, 0), |
| 114 | ++ "ssl_session_dup_intern"}, |
| 115 | + {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_SESSION_NEW, 0), "SSL_SESSION_new"}, |
| 116 | + {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_SESSION_PRINT_FP, 0), |
| 117 | + "SSL_SESSION_print_fp"}, |
| 118 | +diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c |
| 119 | +index 47adc3211..c01ad8291 100644 |
| 120 | +--- a/ssl/ssl_lib.c |
| 121 | ++++ b/ssl/ssl_lib.c |
| 122 | +@@ -3515,9 +3515,10 @@ void ssl_update_cache(SSL *s, int mode) |
| 123 | + |
| 124 | + /* |
| 125 | + * If the session_id_length is 0, we are not supposed to cache it, and it |
| 126 | +- * would be rather hard to do anyway :-) |
| 127 | ++ * would be rather hard to do anyway :-). Also if the session has already |
| 128 | ++ * been marked as not_resumable we should not cache it for later reuse. |
| 129 | + */ |
| 130 | +- if (s->session->session_id_length == 0) |
| 131 | ++ if (s->session->session_id_length == 0 || s->session->not_resumable) |
| 132 | + return; |
| 133 | + |
| 134 | + /* |
| 135 | +diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c |
| 136 | +index 68d1737ac..d56e77517 100644 |
| 137 | +--- a/ssl/ssl_sess.c |
| 138 | ++++ b/ssl/ssl_sess.c |
| 139 | +@@ -94,16 +94,11 @@ SSL_SESSION *SSL_SESSION_new(void) |
| 140 | + return ss; |
| 141 | + } |
| 142 | + |
| 143 | +-SSL_SESSION *SSL_SESSION_dup(SSL_SESSION *src) |
| 144 | +-{ |
| 145 | +- return ssl_session_dup(src, 1); |
| 146 | +-} |
| 147 | +- |
| 148 | + /* |
| 149 | + * Create a new SSL_SESSION and duplicate the contents of |src| into it. If |
| 150 | + * ticket == 0 then no ticket information is duplicated, otherwise it is. |
| 151 | + */ |
| 152 | +-SSL_SESSION *ssl_session_dup(SSL_SESSION *src, int ticket) |
| 153 | ++static SSL_SESSION *ssl_session_dup_intern(const SSL_SESSION *src, int ticket) |
| 154 | + { |
| 155 | + SSL_SESSION *dest; |
| 156 | + |
| 157 | +@@ -221,11 +216,32 @@ SSL_SESSION *ssl_session_dup(SSL_SESSION *src, int ticket) |
| 158 | + |
| 159 | + return dest; |
| 160 | + err: |
| 161 | +- SSLerr(SSL_F_SSL_SESSION_DUP, ERR_R_MALLOC_FAILURE); |
| 162 | ++ SSLerr(SSL_F_SSL_SESSION_DUP_INTERN, ERR_R_MALLOC_FAILURE); |
| 163 | + SSL_SESSION_free(dest); |
| 164 | + return NULL; |
| 165 | + } |
| 166 | + |
| 167 | ++SSL_SESSION *SSL_SESSION_dup(SSL_SESSION *src) |
| 168 | ++{ |
| 169 | ++ return ssl_session_dup_intern(src, 1); |
| 170 | ++} |
| 171 | ++ |
| 172 | ++/* |
| 173 | ++ * Used internally when duplicating a session which might be already shared. |
| 174 | ++ * We will have resumed the original session. Subsequently we might have marked |
| 175 | ++ * it as non-resumable (e.g. in another thread) - but this copy should be ok to |
| 176 | ++ * resume from. |
| 177 | ++ */ |
| 178 | ++SSL_SESSION *ssl_session_dup(SSL_SESSION *src, int ticket) |
| 179 | ++{ |
| 180 | ++ SSL_SESSION *sess = ssl_session_dup_intern(src, ticket); |
| 181 | ++ |
| 182 | ++ if (sess != NULL) |
| 183 | ++ sess->not_resumable = 0; |
| 184 | ++ |
| 185 | ++ return sess; |
| 186 | ++} |
| 187 | ++ |
| 188 | + const unsigned char *SSL_SESSION_get_id(const SSL_SESSION *s, unsigned int *len) |
| 189 | + { |
| 190 | + if (len) |
| 191 | +diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c |
| 192 | +index 43f77a589..f55e11bde 100644 |
| 193 | +--- a/ssl/statem/statem_srvr.c |
| 194 | ++++ b/ssl/statem/statem_srvr.c |
| 195 | +@@ -2403,9 +2403,8 @@ int tls_construct_server_hello(SSL *s, WPACKET *pkt) |
| 196 | + * so the following won't overwrite an ID that we're supposed |
| 197 | + * to send back. |
| 198 | + */ |
| 199 | +- if (s->session->not_resumable || |
| 200 | +- (!(s->ctx->session_cache_mode & SSL_SESS_CACHE_SERVER) |
| 201 | +- && !s->hit)) |
| 202 | ++ if (!(s->ctx->session_cache_mode & SSL_SESS_CACHE_SERVER) |
| 203 | ++ && !s->hit) |
| 204 | + s->session->session_id_length = 0; |
| 205 | + |
| 206 | + if (usetls13) { |
| 207 | +-- |
| 208 | +2.33.8 |
| 209 | + |
0 commit comments