aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDaniel Stenberg <daniel@haxx.se>2007-07-29 12:54:05 +0000
committerDaniel Stenberg <daniel@haxx.se>2007-07-29 12:54:05 +0000
commitf1fa7b8ba469d9b8681e30f107b44004695b32e9 (patch)
tree80542fd7f33b8d77ecb6f66ae13d9f3e32ed2cc9
parent86ff3194fa902e131c7a105a329202058327dcc7 (diff)
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.
-rw-r--r--CHANGES11
-rw-r--r--RELEASE-NOTES4
-rw-r--r--lib/ftp.c8
-rw-r--r--lib/gtls.c14
-rw-r--r--lib/nss.c15
-rw-r--r--lib/qssl.c14
-rw-r--r--lib/sslgen.c35
-rw-r--r--lib/sslgen.h2
-rw-r--r--lib/ssluse.c50
-rw-r--r--lib/ssluse.h6
-rw-r--r--lib/url.c5
-rw-r--r--lib/urldata.h4
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;
-
- for(i=0; i<2; i++) {
- struct ssl_connect_data *connssl = &conn->ssl[i];
+ struct ssl_connect_data *connssl = &conn->ssl[sockindex];
- 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.
-
- 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);
-
- for(i=0; i<2; i++) {
- struct ssl_connect_data *connssl = &conn->ssl[i];
+ struct ssl_connect_data *connssl = &conn->ssl[sockindex];
- if(connssl->handle) {
- (void)SSL_shutdown(connssl->handle);
- SSL_set_connect_state(connssl->handle);
+ 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;