Merge pull request #620 from dongbeiouba/fix83/CVE-2024-2511

Fix CVE-2024-2511 for branch 8.3
Tongsuo-Project · Jun 24, 2024 · 0f30351 · 0f30351
2 parents e3fed34 + 7c3cc5d
commit 0f30351
Show file tree

Hide file tree

Showing 12 changed files with 380 additions and 32 deletions.
diff --git a/CHANGES b/CHANGES
@@ -7,6 +7,8 @@
 
  Changes between 8.3.3 and 8.3.4 [xxxx年xx月xx日]
 
+  *) 修复CVE-2024-2511
+
   *) 修复CVE-2024-0727
 
   *) 修复CVE-2023-4807

diff --git a/CHANGES.en b/CHANGES.en
@@ -7,6 +7,8 @@
 
  Changes between 8.3.3 and 8.3.4 [xx XXX xxxx]
 
+  *) Fix CVE-2024-2511
+
   *) Fix CVE-2024-0727
 
   *) Fix CVE-2023-4807

diff --git a/crypto/err/openssl.txt b/crypto/err/openssl.txt
@@ -1466,7 +1466,7 @@ SSL_F_SSL_RENEGOTIATE:516:SSL_renegotiate
 SSL_F_SSL_RENEGOTIATE_ABBREVIATED:546:SSL_renegotiate_abbreviated
 SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT:320:*
 SSL_F_SSL_SCAN_SERVERHELLO_TLSEXT:321:*
-SSL_F_SSL_SESSION_DUP:348:ssl_session_dup
+SSL_F_SSL_SESSION_DUP_INTERN:348:ssl_session_dup_intern
 SSL_F_SSL_SESSION_NEW:189:SSL_SESSION_new
 SSL_F_SSL_SESSION_PRINT_FP:190:SSL_SESSION_print_fp
 SSL_F_SSL_SESSION_SET1_ID:423:SSL_SESSION_set1_id

diff --git a/doc/man3/SSL_SESSION_free.pod b/doc/man3/SSL_SESSION_free.pod
@@ -12,7 +12,7 @@ SSL_SESSION_free - create, free and manage SSL_SESSION structures
  #include <openssl/ssl.h>
 
  SSL_SESSION *SSL_SESSION_new(void);
- SSL_SESSION *SSL_SESSION_dup(SSL_SESSION *src);
+ SSL_SESSION *SSL_SESSION_dup(const SSL_SESSION *src);
  int SSL_SESSION_up_ref(SSL_SESSION *ses);
  void SSL_SESSION_free(SSL_SESSION *session);
 

diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
@@ -1763,7 +1763,7 @@ __owur int SSL_SESSION_set1_id(SSL_SESSION *s, const unsigned char *sid,
 __owur int SSL_SESSION_is_resumable(const SSL_SESSION *s);
 
 __owur SSL_SESSION *SSL_SESSION_new(void);
-__owur SSL_SESSION *SSL_SESSION_dup(SSL_SESSION *src);
+__owur SSL_SESSION *SSL_SESSION_dup(const SSL_SESSION *src);
 const unsigned char *SSL_SESSION_get_id(const SSL_SESSION *s,
                                         unsigned int *len);
 const unsigned char *SSL_SESSION_get0_id_context(const SSL_SESSION *s,

diff --git a/include/openssl/sslerr.h b/include/openssl/sslerr.h
@@ -302,7 +302,7 @@ int ERR_load_SSL_strings(void);
 # define SSL_F_SSL_RENEGOTIATE_ABBREVIATED                546
 # define SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT                320
 # define SSL_F_SSL_SCAN_SERVERHELLO_TLSEXT                321
-# define SSL_F_SSL_SESSION_DUP                            348
+# define SSL_F_SSL_SESSION_DUP_INTERN                     348
 # define SSL_F_SSL_SESSION_NEW                            189
 # define SSL_F_SSL_SESSION_PRINT_FP                       190
 # define SSL_F_SSL_SESSION_SET1_ID                        423

diff --git a/ssl/ssl_err.c b/ssl/ssl_err.c
@@ -477,7 +477,8 @@ static const ERR_STRING_DATA SSL_str_functs[] = {
      "SSL_renegotiate_abbreviated"},
     {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_SCAN_CLIENTHELLO_TLSEXT, 0), ""},
     {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_SCAN_SERVERHELLO_TLSEXT, 0), ""},
-    {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_SESSION_DUP, 0), "ssl_session_dup"},
+    {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_SESSION_DUP_INTERN, 0),
+    "ssl_session_dup_intern"},
     {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_SESSION_NEW, 0), "SSL_SESSION_new"},
     {ERR_PACK(ERR_LIB_SSL, SSL_F_SSL_SESSION_PRINT_FP, 0),
      "SSL_SESSION_print_fp"},

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
@@ -3616,9 +3616,10 @@ void ssl_update_cache(SSL *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_local.h b/ssl/ssl_local.h
@@ -2498,7 +2498,7 @@ __owur int ssl_get_new_session(SSL *s, int session);
 __owur SSL_SESSION *lookup_sess_in_cache(SSL *s, const unsigned char *sess_id,
                                          size_t sess_id_len);
 __owur int ssl_get_prev_session(SSL *s, CLIENTHELLO_MSG *hello);
-__owur SSL_SESSION *ssl_session_dup(SSL_SESSION *src, int ticket);
+__owur SSL_SESSION *ssl_session_dup(const SSL_SESSION *src, int ticket);
 __owur int ssl_cipher_id_cmp(const SSL_CIPHER *a, const SSL_CIPHER *b);
 DECLARE_OBJ_BSEARCH_GLOBAL_CMP_FN(SSL_CIPHER, SSL_CIPHER, ssl_cipher_id);
 __owur int ssl_cipher_ptr_id_cmp(const SSL_CIPHER *const *ap,

diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c
@@ -98,16 +98,11 @@ SSL_SESSION *SSL_SESSION_new(void)
     return ss;
 }
 
-SSL_SESSION *SSL_SESSION_dup(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(SSL_SESSION *src, int ticket)
+static SSL_SESSION *ssl_session_dup_intern(const SSL_SESSION *src, int ticket)
 {
     SSL_SESSION *dest;
 
@@ -246,11 +241,32 @@ SSL_SESSION *ssl_session_dup(SSL_SESSION *src, int ticket)
 
     return dest;
  err:
-    SSLerr(SSL_F_SSL_SESSION_DUP, ERR_R_MALLOC_FAILURE);
+    SSLerr(SSL_F_SSL_SESSION_DUP_INTERN, ERR_R_MALLOC_FAILURE);
     SSL_SESSION_free(dest);
     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)
@@ -501,6 +517,12 @@ SSL_SESSION *lookup_sess_in_cache(SSL *s, const unsigned char *sess_id,
 # endif
 
         if (ret != NULL) {
+            if (ret->not_resumable) {
+                /* If its not resumable then ignore this session */
+                if (!copy)
+                    SSL_SESSION_free(ret);
+                return NULL;
+            }
             tsan_counter(&s->session_ctx->stats.sess_cb_hit);
 
             /*
@@ -774,34 +796,35 @@ int SSL_CTX_add_session(SSL_CTX *ctx, SSL_SESSION *c)
         s = c;
     }
 
-    /* Put at the head of the queue unless it is already in the cache */
-    if (s == NULL)
-        SSL_SESSION_list_add(ctx, c);
-
-    if (s != NULL) {
-        /*
-         * existing cache entry -- decrement previously incremented reference
-         * count because it already takes into account the cache
-         */
-
-        SSL_SESSION_free(s);    /* s == c */
-        ret = 0;
-    } else {
+    if (s == NULL) {
         /*
          * new cache entry -- remove old ones if cache has become too large
+         * delete cache entry *before* add, so we don't remove the one we're adding!
          */
 
         ret = 1;
 
         if (SSL_CTX_sess_get_cache_size(ctx) > 0) {
-            while (SSL_CTX_sess_number(ctx) > SSL_CTX_sess_get_cache_size(ctx)) {
+            while (SSL_CTX_sess_number(ctx) >= SSL_CTX_sess_get_cache_size(ctx)) {
                 if (!remove_session_lock(ctx, ctx->session_cache_tail, 0))
                     break;
                 else
                     tsan_counter(&ctx->stats.sess_cache_full);
             }
         }
     }
+
+    SSL_SESSION_list_add(ctx, c);
+
+    if (s != NULL) {
+        /*
+         * existing cache entry -- decrement previously incremented reference
+         * count because it already takes into account the cache
+         */
+
+        SSL_SESSION_free(s);    /* s == c */
+        ret = 0;
+    }
     CRYPTO_THREAD_unlock(ctx->lock);
     return ret;
 }

diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
@@ -2627,9 +2627,8 @@ int tls_construct_server_hello(SSL *s, WPACKET *pkt)
      * so the following won't overwrite an ID that we're supposed
      * to send back.
      */
-    if (s->session->not_resumable ||
-        (!(s->ctx->session_cache_mode & SSL_SESS_CACHE_SERVER)
-         && !s->hit))
+    if (!(s->ctx->session_cache_mode & SSL_SESS_CACHE_SERVER)
+            && !s->hit)
         s->session->session_id_length = 0;
 
     if (usetls13) {