From ca536eab9d2e27a17bba78f1c829b65f45dc011f Mon Sep 17 00:00:00 2001 From: Neil Horman Date: Thu, 9 Nov 2023 08:13:58 -0500 Subject: [PATCH 1/4] zero data in hm_fragment on alloc if we allocate a new hm_frament in dtls1_buffer_message with dtls1_hm_fragment_new, the returned fragment contains uninitalized data in the msg_header field. If an error then occurs, and we free the fragment, dtls_hm_fragment_free interrogates the msg_header field (which is garbage), and potentially references undefined values, or worse, accidentally references available memory that is not owned, leading to various corruptions. Reviewed-by: Tomas Mraz Reviewed-by: Tim Hudson Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/2261) Signed-off-by: fly2x --- ssl/statem/statem_dtls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ssl/statem/statem_dtls.c b/ssl/statem/statem_dtls.c index 788d0eff65..2e98df6235 100644 --- a/ssl/statem/statem_dtls.c +++ b/ssl/statem/statem_dtls.c @@ -59,7 +59,7 @@ static hm_fragment *dtls1_hm_fragment_new(size_t frag_len, int reassembly) unsigned char *buf = NULL; unsigned char *bitmask = NULL; - if ((frag = OPENSSL_malloc(sizeof(*frag))) == NULL) { + if ((frag = OPENSSL_zalloc(sizeof(*frag))) == NULL) { ERR_raise(ERR_LIB_SSL, ERR_R_MALLOC_FAILURE); return NULL; } -- Gitee From f68a42d7cd667d5c7017334fc33dbaea452ceee7 Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Thu, 9 Nov 2023 14:45:33 +0000 Subject: [PATCH 2/4] Move freeing of an old enc_write_ctx/write_hash to dtls1_clear_sent_buffer When we are clearing the sent messages queue we should ensure we free any old enc_write_ctx/write_hash that are no longer in use. Previously this logic was in dtls1_hm_fragment_free() - but this can end up freeing the current enc_write_ctx/write_hash under certain error conditions. Fixes #22664 Reviewed-by: Tomas Mraz Reviewed-by: Tim Hudson (Merged from https://github.com/openssl/openssl/pull/2261) Signed-off-by: fly2x --- ssl/d1_lib.c | 17 +++++++++++++++++ ssl/ssl_lib.c | 8 ++++++-- ssl/statem/statem_dtls.c | 6 +----- 3 files changed, 24 insertions(+), 7 deletions(-) diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c index 871c187a9d..6c2d4ba73e 100644 --- a/ssl/d1_lib.c +++ b/ssl/d1_lib.c @@ -130,6 +130,23 @@ void dtls1_clear_sent_buffer(SSL *s) while ((item = pqueue_pop(s->d1->sent_messages)) != NULL) { frag = (hm_fragment *)item->data; + + if (frag->msg_header.is_ccs) { + /* + * If we're freeing the CCS then we're done with the old + * enc_write_ctx/write_hash and they can be freed + */ + if (s->enc_write_ctx + != frag->msg_header.saved_retransmit_state.enc_write_ctx) + EVP_CIPHER_CTX_free(frag->msg_header.saved_retransmit_state + .enc_write_ctx); + + if (s->write_hash + != frag->msg_header.saved_retransmit_state.write_hash) + EVP_MD_CTX_free(frag->msg_header.saved_retransmit_state + .write_hash); + } + dtls1_hm_fragment_free(frag); pitem_free(item); } diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index 26cbcd2c32..a0185cce07 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -1216,8 +1216,6 @@ void SSL_free(SSL *s) SSL_SESSION_free(s->psksession); OPENSSL_free(s->psksession_id); - clear_ciphers(s); - ssl_cert_free(s->cert); OPENSSL_free(s->shared_sigalgs); /* Free up if allocated */ @@ -1253,6 +1251,12 @@ void SSL_free(SSL *s) if (s->method != NULL) s->method->ssl_free(s); + /* + * Must occur after s->method->ssl_free(). The DTLS sent_messages queue + * may reference the EVP_CIPHER_CTX/EVP_MD_CTX that are freed here. + */ + clear_ciphers(s); + SSL_CTX_free(s->ctx); ASYNC_WAIT_CTX_free(s->waitctx); diff --git a/ssl/statem/statem_dtls.c b/ssl/statem/statem_dtls.c index 2e98df6235..040c23035c 100644 --- a/ssl/statem/statem_dtls.c +++ b/ssl/statem/statem_dtls.c @@ -95,11 +95,7 @@ void dtls1_hm_fragment_free(hm_fragment *frag) { if (!frag) return; - if (frag->msg_header.is_ccs) { - EVP_CIPHER_CTX_free(frag->msg_header. - saved_retransmit_state.enc_write_ctx); - EVP_MD_CTX_free(frag->msg_header.saved_retransmit_state.write_hash); - } + OPENSSL_free(frag->fragment); OPENSSL_free(frag->reassembly); OPENSSL_free(frag); -- Gitee From 64a6612e198675136d7da78172ac17120e006236 Mon Sep 17 00:00:00 2001 From: Tomas Mraz Date: Mon, 13 Nov 2023 12:17:43 +0100 Subject: [PATCH 3/4] x86_64-xlate.pl: Fix build with icx and nvc compilers Fixes #22594 Reviewed-by: Hugo Landau Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/22714) (cherry picked from commit 1da7c09f7987a227701b6324e56003a89e9febf2) Signed-off-by: fly2x --- crypto/perlasm/x86_64-xlate.pl | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crypto/perlasm/x86_64-xlate.pl b/crypto/perlasm/x86_64-xlate.pl index 359670a2da..6b93cfb84f 100755 --- a/crypto/perlasm/x86_64-xlate.pl +++ b/crypto/perlasm/x86_64-xlate.pl @@ -111,7 +111,12 @@ elsif (`$ENV{CC} -Wa,-v -c -o /dev/null -x assembler /dev/null 2>&1` $gnuas=1; } elsif (`$ENV{CC} --version 2>/dev/null` - =~ /clang .*/) + =~ /(clang .*|Intel.*oneAPI .*)/) +{ + $gnuas=1; +} +elsif (`$ENV{CC} -V 2>/dev/null` + =~ /nvc .*/) { $gnuas=1; } -- Gitee From 3413e51ccb70269b4184a5222f0a6a73e66f5ece Mon Sep 17 00:00:00 2001 From: Bernd Edlinger Date: Fri, 17 Nov 2023 14:47:36 +0100 Subject: [PATCH 4/4] Fix a possible memory leak in ct_move_scts Instead of trying to move the doomed sct back to the src stack, which may fail as well, simply free the sct object, as the src list will be deleted anyway. Reviewed-by: Paul Yang Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/22762) (cherry picked from commit a435d786046fabc85acdb89cbf47f154a09796e1) Signed-off-by: fly2x --- ssl/ssl_lib.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index a0185cce07..e5603b9130 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -4981,6 +4981,8 @@ IMPLEMENT_OBJ_BSEARCH_GLOBAL_CMP_FN(SSL_CIPHER, SSL_CIPHER, ssl_cipher_id); * If |dst| points to a NULL pointer, a new stack will be created and owned by * the caller. * Returns the number of SCTs moved, or a negative integer if an error occurs. + * The |dst| stack is created and possibly partially populated even in case + * of error, likewise the |src| stack may be left in an intermediate state. */ static int ct_move_scts(STACK_OF(SCT) **dst, STACK_OF(SCT) *src, sct_source_t origin) @@ -5000,15 +5002,14 @@ static int ct_move_scts(STACK_OF(SCT) **dst, STACK_OF(SCT) *src, if (SCT_set_source(sct, origin) != 1) goto err; - if (sk_SCT_push(*dst, sct) <= 0) + if (!sk_SCT_push(*dst, sct)) goto err; scts_moved += 1; } return scts_moved; err: - if (sct != NULL) - sk_SCT_push(src, sct); /* Put the SCT back */ + SCT_free(sct); return -1; } -- Gitee