From f1fa7b8ba469d9b8681e30f107b44004695b32e9 Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Sun, 29 Jul 2007 12:54:05 +0000 Subject: [PATCH] Bug report #1759542 (http://curl.haxx.se/bug/view.cgi?id=1759542). A bad use of a socket after it has been closed, when the FTP-SSL data connection is taken down. --- CHANGES | 11 +++++++++++ RELEASE-NOTES | 4 +++- lib/ftp.c | 8 ++++++++ lib/gtls.c | 14 +++++++------- lib/nss.c | 15 +++++---------- lib/qssl.c | 14 ++++---------- lib/sslgen.c | 35 ++++++++++++++++------------------- lib/sslgen.h | 2 +- lib/ssluse.c | 50 ++++++++++++++++++++++---------------------------- lib/ssluse.h | 6 +++++- lib/url.c | 5 +++-- lib/urldata.h | 4 +++- 12 files changed, 88 insertions(+), 80 deletions(-) diff --git a/CHANGES b/CHANGES index 9bd83b985..87a335aab 100644 --- a/CHANGES +++ b/CHANGES @@ -6,6 +6,17 @@ Changelog +Daniel S (29 July 2007) +- Jayesh A Shah filed bug report #1759542 + (http://curl.haxx.se/bug/view.cgi?id=1759542) identifying a rather serious + problem with FTPS: libcurl closed the data connection socket and then later + in the flow it would call the SSL layer to do SSL shutdown which then would + use a socket that had already been closed - so if the application had opened + a new one in the mean time, libcurl could send gibberish that way! I worked + with "Greg" to properly diagnose and fix this. The fix affects code for all + SSL libraries we support, but it has only been truly verified to work fine + for the OpenSSL version. The others have only been code reviewed. + Daniel S (23 July 2007) - Implemented the parts of Patrick Monnerat's OS/400 patch that introduces support for the OS/400 Secure Sockets Layer library. diff --git a/RELEASE-NOTES b/RELEASE-NOTES index 66ea41306..010b3084d 100644 --- a/RELEASE-NOTES +++ b/RELEASE-NOTES @@ -28,6 +28,7 @@ This release includes the following bugfixes: o bad free of memory from libssh2 o the SFTP PWD command works o HTTP Digest auth on a re-used connection + o FTPS data connection close This release includes the following known bugs: @@ -48,6 +49,7 @@ advice from friends like these: Dan Fandrich, Song Ma, Daniel Black, Giancarlo Formicuccia, Shmulik Regev, Daniel Cater, Colin Hogben, Jofell Gallardo, Daniel Johnson, - Ralf S. Engelschall, James Housley, Chris Flerackers, Patrick Monnerat + Ralf S. Engelschall, James Housley, Chris Flerackers, Patrick Monnerat, + Jayesh A Shah Thanks! (and sorry if I forgot to mention someone) diff --git a/lib/ftp.c b/lib/ftp.c index 21d29b446..4df17decb 100644 --- a/lib/ftp.c +++ b/lib/ftp.c @@ -3115,6 +3115,14 @@ CURLcode Curl_ftp_done(struct connectdata *conn, CURLcode status, bool premature shutdown(conn->sock[SECONDARYSOCKET],2); /* SD_BOTH */ #endif + if(conn->ssl[SECONDARYSOCKET].use) { + /* The secondary socket is using SSL so we must close down that part first + before we close the socket for real */ + Curl_ssl_close(conn, SECONDARYSOCKET); + + /* Note that we keep "use" set to TRUE since that (next) connection is + still requested to use SSL */ + } sclose(conn->sock[SECONDARYSOCKET]); conn->sock[SECONDARYSOCKET] = CURL_SOCKET_BAD; diff --git a/lib/gtls.c b/lib/gtls.c index 23c2a28b4..03572d88e 100644 --- a/lib/gtls.c +++ b/lib/gtls.c @@ -556,17 +556,17 @@ static void close_one(struct connectdata *conn, if(conn->ssl[index].session) { gnutls_bye(conn->ssl[index].session, GNUTLS_SHUT_RDWR); gnutls_deinit(conn->ssl[index].session); + conn->ssl[index].session = NULL; } - if(conn->ssl[index].cred) + if(conn->ssl[index].cred) { gnutls_certificate_free_credentials(conn->ssl[index].cred); + conn->ssl[index].cred = NULL; + } } -void Curl_gtls_close(struct connectdata *conn) +void Curl_gtls_close(struct connectdata *conn, int sockindex) { - if(conn->ssl[0].use) - close_one(conn, 0); - if(conn->ssl[1].use) - close_one(conn, 1); + close_one(conn, sockindex); } /* @@ -631,8 +631,8 @@ int Curl_gtls_shutdown(struct connectdata *conn, int sockindex) } gnutls_certificate_free_credentials(conn->ssl[sockindex].cred); + conn->ssl[sockindex].cred = NULL; conn->ssl[sockindex].session = NULL; - conn->ssl[sockindex].use = FALSE; return retval; } diff --git a/lib/nss.c b/lib/nss.c index 189a19a0c..c99258969 100644 --- a/lib/nss.c +++ b/lib/nss.c @@ -384,18 +384,13 @@ Curl_nss_check_cxn(struct connectdata *conn) /* * This function is called when an SSL connection is closed. */ -void Curl_nss_close(struct connectdata *conn) +void Curl_nss_close(struct connectdata *conn, int sockindex) { - int i; + struct ssl_connect_data *connssl = &conn->ssl[sockindex]; - for(i=0; i<2; i++) { - struct ssl_connect_data *connssl = &conn->ssl[i]; - - if(connssl->handle) { - PR_Close(connssl->handle); - connssl->handle = NULL; - } - connssl->use = FALSE; /* get back to ordinary socket usage */ + if(connssl->handle) { + PR_Close(connssl->handle); + connssl->handle = NULL; } } diff --git a/lib/qssl.c b/lib/qssl.c index 0e5ccba29..79de7a48e 100644 --- a/lib/qssl.c +++ b/lib/qssl.c @@ -275,7 +275,7 @@ static int Curl_qsossl_close_one(struct ssl_connect_data * conn, { int rc; - if (!conn->handle) + if(!conn->handle) return 0; rc = SSL_Destroy(conn->handle); @@ -291,22 +291,16 @@ static int Curl_qsossl_close_one(struct ssl_connect_data * conn, return -1; } - conn->use = FALSE; /* get back to ordinary socket usage */ conn->handle = NULL; return 0; } -void Curl_qsossl_close(struct connectdata * conn) +void Curl_qsossl_close(struct connectdata *conn, int sockindex) { - struct SessionHandle * data = conn->data; - struct ssl_connect_data * connssl = conn->ssl; - - if(connssl->use) - (void) Curl_qsossl_close_one(connssl, data); - - connssl++; + struct SessionHandle *data = conn->data; + struct ssl_connect_data *connssl = &conn->ssl[sockindex]; if(connssl->use) (void) Curl_qsossl_close_one(connssl, data); diff --git a/lib/sslgen.c b/lib/sslgen.c index 56f626ac2..b452d504f 100644 --- a/lib/sslgen.c +++ b/lib/sslgen.c @@ -435,46 +435,43 @@ void Curl_ssl_close_all(struct SessionHandle *data) #endif /* USE_SSL */ } -void Curl_ssl_close(struct connectdata *conn) +void Curl_ssl_close(struct connectdata *conn, int sockindex) { - if(conn->ssl[FIRSTSOCKET].use) { + DEBUGASSERT((sockindex <= 1) && (sockindex >= -1)); + #ifdef USE_SSLEAY - Curl_ossl_close(conn); + Curl_ossl_close(conn, sockindex); #endif /* USE_SSLEAY */ #ifdef USE_GNUTLS - Curl_gtls_close(conn); + Curl_gtls_close(conn, sockindex); #endif /* USE_GNUTLS */ #ifdef USE_NSS - Curl_nss_close(conn); + Curl_nss_close(conn, sockindex); #endif /* USE_NSS */ #ifdef USE_QSOSSL - Curl_qsossl_close(conn); + Curl_qsossl_close(conn, sockindex); #endif /* USE_QSOSSL */ - conn->ssl[FIRSTSOCKET].use = FALSE; - } } CURLcode Curl_ssl_shutdown(struct connectdata *conn, int sockindex) { - if(conn->ssl[sockindex].use) { #ifdef USE_SSLEAY - if(Curl_ossl_shutdown(conn, sockindex)) - return CURLE_SSL_SHUTDOWN_FAILED; + if(Curl_ossl_shutdown(conn, sockindex)) + return CURLE_SSL_SHUTDOWN_FAILED; #else #ifdef USE_GNUTLS - if(Curl_gtls_shutdown(conn, sockindex)) - return CURLE_SSL_SHUTDOWN_FAILED; + if(Curl_gtls_shutdown(conn, sockindex)) + return CURLE_SSL_SHUTDOWN_FAILED; #else #ifdef USE_QSOSSL - if(Curl_qsossl_shutdown(conn, sockindex)) - return CURLE_SSL_SHUTDOWN_FAILED; -#else - (void)conn; - (void)sockindex; + if(Curl_qsossl_shutdown(conn, sockindex)) + return CURLE_SSL_SHUTDOWN_FAILED; #endif /* USE_QSOSSL */ #endif /* USE_GNUTLS */ #endif /* USE_SSLEAY */ - } + + conn->ssl[sockindex].use = FALSE; /* get back to ordinary socket usage */ + return CURLE_OK; } diff --git a/lib/sslgen.h b/lib/sslgen.h index c24d46bf3..70bd7c562 100644 --- a/lib/sslgen.h +++ b/lib/sslgen.h @@ -35,7 +35,7 @@ CURLcode Curl_ssl_connect(struct connectdata *conn, int sockindex); CURLcode Curl_ssl_connect_nonblocking(struct connectdata *conn, int sockindex, bool *done); -void Curl_ssl_close(struct connectdata *conn); +void Curl_ssl_close(struct connectdata *conn, int sockindex); /* tell the SSL stuff to close down all open information regarding connections (and thus session ID caching etc) */ void Curl_ssl_close_all(struct SessionHandle *data); diff --git a/lib/ssluse.c b/lib/ssluse.c index 97e244896..b5fc08d18 100644 --- a/lib/ssluse.c +++ b/lib/ssluse.c @@ -703,35 +703,20 @@ struct curl_slist *Curl_ossl_engines_list(struct SessionHandle *data) /* * This function is called when an SSL connection is closed. */ -void Curl_ossl_close(struct connectdata *conn) +void Curl_ossl_close(struct connectdata *conn, int sockindex) { - int i; - /* - ERR_remove_state() frees the error queue associated with - thread pid. If pid == 0, the current thread will have its - error queue removed. + struct ssl_connect_data *connssl = &conn->ssl[sockindex]; - Since error queue data structures are allocated - automatically for new threads, they must be freed when - threads are terminated in oder to avoid memory leaks. - */ - ERR_remove_state(0); + if(connssl->handle) { + (void)SSL_shutdown(connssl->handle); + SSL_set_connect_state(connssl->handle); - for(i=0; i<2; i++) { - struct ssl_connect_data *connssl = &conn->ssl[i]; - - if(connssl->handle) { - (void)SSL_shutdown(connssl->handle); - SSL_set_connect_state(connssl->handle); - - SSL_free (connssl->handle); - connssl->handle = NULL; - } - if(connssl->ctx) { - SSL_CTX_free (connssl->ctx); - connssl->ctx = NULL; - } - connssl->use = FALSE; /* get back to ordinary socket usage */ + SSL_free (connssl->handle); + connssl->handle = NULL; + } + if(connssl->ctx) { + SSL_CTX_free (connssl->ctx); + connssl->ctx = NULL; } } @@ -827,8 +812,6 @@ int Curl_ossl_shutdown(struct connectdata *conn, int sockindex) #endif } - connssl->use = FALSE; /* get back to ordinary socket usage */ - SSL_free (connssl->handle); connssl->handle = NULL; } @@ -847,6 +830,17 @@ void Curl_ossl_session_free(void *ptr) */ int Curl_ossl_close_all(struct SessionHandle *data) { + /* + ERR_remove_state() frees the error queue associated with + thread pid. If pid == 0, the current thread will have its + error queue removed. + + Since error queue data structures are allocated + automatically for new threads, they must be freed when + threads are terminated in oder to avoid memory leaks. + */ + ERR_remove_state(0); + #ifdef HAVE_OPENSSL_ENGINE_H if(data->state.engine) { ENGINE_finish(data->state.engine); diff --git a/lib/ssluse.h b/lib/ssluse.h index 5bb7090c5..d5424d5a6 100644 --- a/lib/ssluse.h +++ b/lib/ssluse.h @@ -32,10 +32,14 @@ CURLcode Curl_ossl_connect(struct connectdata *conn, int sockindex); CURLcode Curl_ossl_connect_nonblocking(struct connectdata *conn, int sockindex, bool *done); -void Curl_ossl_close(struct connectdata *conn); /* close a SSL connection */ + +/* close a SSL connection */ +void Curl_ossl_close(struct connectdata *conn, int sockindex); + /* tell OpenSSL to close down all open information regarding connections (and thus session ID caching etc) */ int Curl_ossl_close_all(struct SessionHandle *data); + /* Sets an OpenSSL engine */ CURLcode Curl_ossl_set_engine(struct SessionHandle *data, const char *engine); diff --git a/lib/url.c b/lib/url.c index 3f93311f6..7a8effb1e 100644 --- a/lib/url.c +++ b/lib/url.c @@ -1822,7 +1822,8 @@ static void conn_free(struct connectdata *conn) Curl_destroy_thread_data(&conn->async); #endif - Curl_ssl_close(conn); + Curl_ssl_close(conn, FIRSTSOCKET); + Curl_ssl_close(conn, SECONDARYSOCKET); Curl_free_ssl_config(&conn->ssl_config); @@ -1892,7 +1893,7 @@ CURLcode Curl_disconnect(struct connectdata *conn) allocated by libidn */ #endif - Curl_ssl_close(conn); + Curl_ssl_close(conn, FIRSTSOCKET); /* Indicate to all handles on the pipe that we're dead */ if (IsPipeliningEnabled(data)) { diff --git a/lib/urldata.h b/lib/urldata.h index 6332f093d..d1da3331c 100644 --- a/lib/urldata.h +++ b/lib/urldata.h @@ -166,7 +166,9 @@ typedef enum { /* struct for data related to each SSL connection */ struct ssl_connect_data { - bool use; /* use ssl encrypted communications TRUE/FALSE */ + bool use; /* use ssl encrypted communications TRUE/FALSE, not + necessarily using it atm but at least asked to or + meaning to use it */ #ifdef USE_SSLEAY /* these ones requires specific SSL-types */ SSL_CTX* ctx;