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.

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;