From c1a0a9ad8f3fd946070557fa70f2d8e6369def60 Mon Sep 17 00:00:00 2001 From: Thibault Charbonnier Date: Tue, 4 Feb 2020 12:39:29 -0800 Subject: [PATCH] bugfix: fixed a memory leak in the OpenSSL 1.1.1 sess_set_get_cb_yield patch. This memory leak was found by running the Valgrind testing mode against lua-resty-core's `ssl-session-fetch.t` test suite: TEST 5: yield during doing handshake with client which uses low version OpenSSL ==16956== 64 (32 direct, 32 indirect) bytes in 1 blocks are definitely lost in loss record 5 of 15 ==16956== at 0x4C2B002: malloc (vg_replace_malloc.c:298) ==16956== by 0x5FFC868: CRYPTO_malloc (mem.c:222) ==16956== by 0x5FFC96F: CRYPTO_zalloc (mem.c:230) ==16956== by 0x603C54A: OPENSSL_sk_new_reserve (stack.c:209) ==16956== by 0x603C597: OPENSSL_sk_new_null (stack.c:118) ==16956== by 0x5C94A86: sk_SSL_CIPHER_new_null (ssl.h:960) ==16956== by 0x5C94A86: bytes_to_cipher_list (ssl_lib.c:5361) ==16956== by 0x5CB52E9: tls_early_post_process_client_hello (statem_srvr.c:1713) ==16956== by 0x5CB52E9: tls_post_process_client_hello (statem_srvr.c:2231) ==16956== by 0x5CB6F39: ossl_statem_server_post_process_message (statem_srvr.c:1218) ==16956== by 0x5CA4C11: read_state_machine (statem.c:664) ==16956== by 0x5CA4C11: state_machine (statem.c:434) ==16956== by 0x5CA538A: ossl_statem_accept (statem.c:255) ==16956== by 0x5C91759: SSL_do_handshake (ssl_lib.c:3609) ==16956== by 0x45456B: ngx_ssl_handshake (ngx_event_openssl.c:1606) ==16956== by 0x4698D3: ngx_http_ssl_handshake (ngx_http_request.c:751) ==16956== by 0x44ECA8: ngx_epoll_process_events (ngx_epoll_module.c:901) ==16956== by 0x443E94: ngx_process_events_and_timers (ngx_event.c:257) ==16956== by 0x44DC25: ngx_single_process_cycle (ngx_process_cycle.c:333) ==16956== by 0x4236AB: main (nginx.c:382) ==16956== { Memcheck:Leak match-leak-kinds: definite fun:malloc fun:CRYPTO_malloc fun:CRYPTO_zalloc fun:OPENSSL_sk_new_reserve fun:OPENSSL_sk_new_null fun:sk_SSL_CIPHER_new_null fun:bytes_to_cipher_list fun:tls_early_post_process_client_hello fun:tls_post_process_client_hello fun:ossl_statem_server_post_process_message fun:read_state_machine fun:state_machine fun:ossl_statem_accept fun:SSL_do_handshake fun:ngx_ssl_handshake fun:ngx_http_ssl_handshake fun:ngx_epoll_process_events fun:ngx_process_events_and_timers fun:ngx_single_process_cycle fun:main } ==16956== 368 (32 direct, 336 indirect) bytes in 1 blocks are definitely lost in loss record 8 of 15 ==16956== at 0x4C2B002: malloc (vg_replace_malloc.c:298) ==16956== by 0x5FFC868: CRYPTO_malloc (mem.c:222) ==16956== by 0x5FFC96F: CRYPTO_zalloc (mem.c:230) ==16956== by 0x603C54A: OPENSSL_sk_new_reserve (stack.c:209) ==16956== by 0x603C597: OPENSSL_sk_new_null (stack.c:118) ==16956== by 0x5C94A79: sk_SSL_CIPHER_new_null (ssl.h:960) ==16956== by 0x5C94A79: bytes_to_cipher_list (ssl_lib.c:5360) ==16956== by 0x5CB52E9: tls_early_post_process_client_hello (statem_srvr.c:1713) ==16956== by 0x5CB52E9: tls_post_process_client_hello (statem_srvr.c:2231) ==16956== by 0x5CB6F39: ossl_statem_server_post_process_message (statem_srvr.c:1218) ==16956== by 0x5CA4C11: read_state_machine (statem.c:664) ==16956== by 0x5CA4C11: state_machine (statem.c:434) ==16956== by 0x5CA538A: ossl_statem_accept (statem.c:255) ==16956== by 0x5C91759: SSL_do_handshake (ssl_lib.c:3609) ==16956== by 0x45456B: ngx_ssl_handshake (ngx_event_openssl.c:1606) ==16956== by 0x4698D3: ngx_http_ssl_handshake (ngx_http_request.c:751) ==16956== by 0x44ECA8: ngx_epoll_process_events (ngx_epoll_module.c:901) ==16956== by 0x443E94: ngx_process_events_and_timers (ngx_event.c:257) ==16956== by 0x44DC25: ngx_single_process_cycle (ngx_process_cycle.c:333) ==16956== by 0x4236AB: main (nginx.c:382) ==16956== { Memcheck:Leak match-leak-kinds: definite fun:malloc fun:CRYPTO_malloc fun:CRYPTO_zalloc fun:OPENSSL_sk_new_reserve fun:OPENSSL_sk_new_null fun:sk_SSL_CIPHER_new_null fun:bytes_to_cipher_list fun:tls_early_post_process_client_hello fun:tls_post_process_client_hello fun:ossl_statem_server_post_process_message fun:read_state_machine fun:state_machine fun:ossl_statem_accept fun:SSL_do_handshake fun:ngx_ssl_handshake fun:ngx_http_ssl_handshake fun:ngx_epoll_process_events fun:ngx_process_events_and_timers fun:ngx_single_process_cycle fun:main } --- ...openssl-1.1.1c-sess_set_get_cb_yield.patch | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/patches/openssl-1.1.1c-sess_set_get_cb_yield.patch b/patches/openssl-1.1.1c-sess_set_get_cb_yield.patch index 16eb893..a8bbea0 100644 --- a/patches/openssl-1.1.1c-sess_set_get_cb_yield.patch +++ b/patches/openssl-1.1.1c-sess_set_get_cb_yield.patch @@ -142,7 +142,7 @@ diff --git a/ssl/ssl_sess.c b/ssl/ssl_sess.c diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c --- a/ssl/statem/statem_srvr.c +++ b/ssl/statem/statem_srvr.c -@@ -1600,6 +1600,7 @@ static int tls_early_post_process_client_hello(SSL *s) +@@ -1604,6 +1604,7 @@ static int tls_early_post_process_client_hello(SSL *s) STACK_OF(SSL_CIPHER) *scsvs = NULL; CLIENTHELLO_MSG *clienthello = s->clienthello; DOWNGRADE dgrd = DOWNGRADE_NONE; @@ -150,7 +150,7 @@ diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c /* Finished parsing the ClientHello, now we can start processing it */ /* Give the ClientHello callback a crack at things */ -@@ -1707,6 +1708,7 @@ static int tls_early_post_process_client_hello(SSL *s) +@@ -1711,6 +1712,7 @@ static int tls_early_post_process_client_hello(SSL *s) } s->hit = 0; @@ -158,18 +158,18 @@ diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c if (!ssl_cache_cipherlist(s, &clienthello->ciphersuites, clienthello->isv2) || -@@ -1812,6 +1814,10 @@ static int tls_early_post_process_client_hello(SSL *s) +@@ -1816,6 +1818,10 @@ static int tls_early_post_process_client_hello(SSL *s) } else if (i == -1) { /* SSLfatal() already called */ goto err; + } else if (i == -2) { + clienthello->ciphersuites = saved_ciphers; + s->rwstate = SSL_SESS_LOOKUP; -+ return -1; ++ goto retry; } else { /* i == 0 */ if (!ssl_get_new_session(s, 1)) { -@@ -1819,6 +1825,7 @@ static int tls_early_post_process_client_hello(SSL *s) +@@ -1823,6 +1829,7 @@ static int tls_early_post_process_client_hello(SSL *s) goto err; } } @@ -177,6 +177,17 @@ diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c } if (SSL_IS_TLS13(s)) { +@@ -2084,6 +2091,10 @@ static int tls_early_post_process_client_hello(SSL *s) + s->clienthello = NULL; + + return 0; ++retry: ++ sk_SSL_CIPHER_free(ciphers); ++ sk_SSL_CIPHER_free(scsvs); ++ return -1; + } + + /* diff --git a/util/libssl.num b/util/libssl.num --- a/util/libssl.num +++ b/util/libssl.num