From 60738f398cdd66312bce6ce92a87f19e71feacf4 Mon Sep 17 00:00:00 2001 From: Michael Kaufmann Date: Wed, 22 Sep 2021 12:04:25 +0200 Subject: [PATCH] vtls: Fix a memory leak if an SSL session cannot be added to the cache On connection shutdown, a new TLS session ticket may arrive after the SSL session cache has already been destructed. In this case, the new SSL session cannot be added to the SSL session cache. The callers of Curl_ssl_addsessionid() need to know whether the SSL session has been added to the cache. If it has not been added, the reference counter of the SSL session must not be incremented, or memory used by the SSL session must be freed. This is now possible with the new output parameter "added" of Curl_ssl_addsessionid(). Fixes #7683 Closes #7752 --- lib/vtls/bearssl.c | 6 ++++-- lib/vtls/gtls.c | 6 ++++-- lib/vtls/mbedtls.c | 7 +++++-- lib/vtls/mesalink.c | 2 +- lib/vtls/openssl.c | 9 ++++++--- lib/vtls/schannel.c | 5 +++-- lib/vtls/sectransp.c | 2 +- lib/vtls/vtls.c | 10 +++++++++- lib/vtls/vtls.h | 3 ++- lib/vtls/wolfssl.c | 2 +- 10 files changed, 36 insertions(+), 16 deletions(-) diff --git a/lib/vtls/bearssl.c b/lib/vtls/bearssl.c index e87649e2a7..9b772d064d 100644 --- a/lib/vtls/bearssl.c +++ b/lib/vtls/bearssl.c @@ -608,6 +608,7 @@ static CURLcode bearssl_connect_step3(struct Curl_easy *data, if(SSL_SET_OPTION(primary.sessionid)) { bool incache; + bool added = FALSE; void *oldsession; br_ssl_session_parameters *session; @@ -623,10 +624,11 @@ static CURLcode bearssl_connect_step3(struct Curl_easy *data, Curl_ssl_delsessionid(data, oldsession); ret = Curl_ssl_addsessionid(data, conn, SSL_IS_PROXY() ? TRUE : FALSE, - session, 0, sockindex); + session, 0, sockindex, &added); Curl_ssl_sessionid_unlock(data); - if(ret) { + if(!added) free(session); + if(ret) { return CURLE_OUT_OF_MEMORY; } } diff --git a/lib/vtls/gtls.c b/lib/vtls/gtls.c index 1b145d8ebb..f3a308fbcd 100644 --- a/lib/vtls/gtls.c +++ b/lib/vtls/gtls.c @@ -1287,6 +1287,7 @@ gtls_connect_step3(struct Curl_easy *data, if(connect_sessionid) { bool incache; + bool added = FALSE; void *ssl_sessionid; /* extract session ID to the allocated buffer */ @@ -1306,10 +1307,11 @@ gtls_connect_step3(struct Curl_easy *data, result = Curl_ssl_addsessionid(data, conn, SSL_IS_PROXY() ? TRUE : FALSE, connect_sessionid, connect_idsize, - sockindex); + sockindex, &added); Curl_ssl_sessionid_unlock(data); - if(result) { + if(!added) free(connect_sessionid); + if(result) { result = CURLE_OUT_OF_MEMORY; } } diff --git a/lib/vtls/mbedtls.c b/lib/vtls/mbedtls.c index 780d13e188..08c79e1624 100644 --- a/lib/vtls/mbedtls.c +++ b/lib/vtls/mbedtls.c @@ -784,6 +784,7 @@ mbed_connect_step3(struct Curl_easy *data, struct connectdata *conn, mbedtls_ssl_session *our_ssl_sessionid; void *old_ssl_sessionid = NULL; bool isproxy = SSL_IS_PROXY() ? TRUE : FALSE; + bool added = FALSE; our_ssl_sessionid = malloc(sizeof(mbedtls_ssl_session)); if(!our_ssl_sessionid) @@ -807,11 +808,13 @@ mbed_connect_step3(struct Curl_easy *data, struct connectdata *conn, Curl_ssl_delsessionid(data, old_ssl_sessionid); retcode = Curl_ssl_addsessionid(data, conn, isproxy, our_ssl_sessionid, - 0, sockindex); + 0, sockindex, &added); Curl_ssl_sessionid_unlock(data); - if(retcode) { + if(!added) { mbedtls_ssl_session_free(our_ssl_sessionid); free(our_ssl_sessionid); + } + if(retcode) { failf(data, "failed to store ssl session"); return retcode; } diff --git a/lib/vtls/mesalink.c b/lib/vtls/mesalink.c index 3db9184f79..0a1dea3ac0 100644 --- a/lib/vtls/mesalink.c +++ b/lib/vtls/mesalink.c @@ -365,7 +365,7 @@ mesalink_connect_step3(struct connectdata *conn, int sockindex) if(!incache) { result = Curl_ssl_addsessionid(data, conn, isproxy, our_ssl_sessionid, 0, - sockindex); + sockindex, NULL); if(result) { Curl_ssl_sessionid_unlock(data); failf(data, "failed to store ssl session"); diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c index 87f4b02b71..aafc2ae09f 100644 --- a/lib/vtls/openssl.c +++ b/lib/vtls/openssl.c @@ -2493,6 +2493,7 @@ static int ossl_new_session_cb(SSL *ssl, SSL_SESSION *ssl_sessionid) if(SSL_SET_OPTION(primary.sessionid)) { bool incache; + bool added = FALSE; void *old_ssl_sessionid = NULL; Curl_ssl_sessionid_lock(data); @@ -2511,9 +2512,11 @@ static int ossl_new_session_cb(SSL *ssl, SSL_SESSION *ssl_sessionid) if(!incache) { if(!Curl_ssl_addsessionid(data, conn, isproxy, ssl_sessionid, - 0 /* unknown size */, sockindex)) { - /* the session has been put into the session cache */ - res = 1; + 0 /* unknown size */, sockindex, &added)) { + if(added) { + /* the session has been put into the session cache */ + res = 1; + } } else failf(data, "failed to store ssl session"); diff --git a/lib/vtls/schannel.c b/lib/vtls/schannel.c index 20c4785339..ef3c919bb3 100644 --- a/lib/vtls/schannel.c +++ b/lib/vtls/schannel.c @@ -1436,6 +1436,7 @@ schannel_connect_step3(struct Curl_easy *data, struct connectdata *conn, /* save the current session data for possible re-use */ if(SSL_SET_OPTION(primary.sessionid)) { bool incache; + bool added = FALSE; struct Curl_schannel_cred *old_cred = NULL; Curl_ssl_sessionid_lock(data); @@ -1453,13 +1454,13 @@ schannel_connect_step3(struct Curl_easy *data, struct connectdata *conn, if(!incache) { result = Curl_ssl_addsessionid(data, conn, isproxy, BACKEND->cred, sizeof(struct Curl_schannel_cred), - sockindex); + sockindex, &added); if(result) { Curl_ssl_sessionid_unlock(data); failf(data, "schannel: failed to store credential handle"); return result; } - else { + else if(added) { /* this cred session is now also referenced by sessionid cache */ BACKEND->cred->refcount++; DEBUGF(infof(data, diff --git a/lib/vtls/sectransp.c b/lib/vtls/sectransp.c index 1e6ed5f06d..0bf515460d 100644 --- a/lib/vtls/sectransp.c +++ b/lib/vtls/sectransp.c @@ -2109,7 +2109,7 @@ static CURLcode sectransp_connect_step1(struct Curl_easy *data, } result = Curl_ssl_addsessionid(data, conn, isproxy, ssl_sessionid, - ssl_sessionid_len, sockindex); + ssl_sessionid_len, sockindex, NULL); Curl_ssl_sessionid_unlock(data); if(result) { failf(data, "failed to store ssl session"); diff --git a/lib/vtls/vtls.c b/lib/vtls/vtls.c index e5bbe1f5f0..6007bbba0f 100644 --- a/lib/vtls/vtls.c +++ b/lib/vtls/vtls.c @@ -516,7 +516,8 @@ CURLcode Curl_ssl_addsessionid(struct Curl_easy *data, const bool isProxy, void *ssl_sessionid, size_t idsize, - int sockindex) + int sockindex, + bool *added) { size_t i; struct Curl_ssl_session *store; @@ -536,6 +537,10 @@ CURLcode Curl_ssl_addsessionid(struct Curl_easy *data, const char *hostname = conn->host.name; #endif (void)sockindex; + + if(added) + *added = FALSE; + if(!data->state.session) return CURLE_OK; @@ -609,6 +614,9 @@ CURLcode Curl_ssl_addsessionid(struct Curl_easy *data, return CURLE_OUT_OF_MEMORY; } + if(added) + *added = TRUE; + DEBUGF(infof(data, "Added Session ID to cache for %s://%s:%d [%s]", store->scheme, store->name, store->remote_port, isProxy ? "PROXY" : "server")); diff --git a/lib/vtls/vtls.h b/lib/vtls/vtls.h index beaa83d9e3..c7bbba082d 100644 --- a/lib/vtls/vtls.h +++ b/lib/vtls/vtls.h @@ -261,7 +261,8 @@ CURLcode Curl_ssl_addsessionid(struct Curl_easy *data, const bool isProxy, void *ssl_sessionid, size_t idsize, - int sockindex); + int sockindex, + bool *added); /* Kill a single session ID entry in the cache * Sessionid mutex must be locked (see Curl_ssl_sessionid_lock). * This will call engine-specific curlssl_session_free function, which must diff --git a/lib/vtls/wolfssl.c b/lib/vtls/wolfssl.c index 16fbb89288..4b501b1c8c 100644 --- a/lib/vtls/wolfssl.c +++ b/lib/vtls/wolfssl.c @@ -749,7 +749,7 @@ wolfssl_connect_step3(struct Curl_easy *data, struct connectdata *conn, if(!incache) { result = Curl_ssl_addsessionid(data, conn, isproxy, our_ssl_sessionid, - 0, sockindex); + 0, sockindex, NULL); if(result) { Curl_ssl_sessionid_unlock(data); failf(data, "failed to store ssl session");