Fix #76972: FTP data truncation due to forceful ssl socket shutdown

Do a correct bidirectional shutdown instead

2 files changed, 71 insertions, 7 deletions

diff --git a/NEWS b/NEWS
index 5d9da2b8bf..837d306650 100644
--- a/NEWS
+++ b/NEWS
@@ -6,7 +6,12 @@ PHP                                                                        NEWS
   . Fixed bug #76946 (Cyclic reference in generator not detected). (Nikita)
 
 - FCGI:
-  . Fixed #76948 (Failed shutdown/reboot or end session in Windows). (Anatol)
+  . Fixed bug #76948 (Failed shutdown/reboot or end session in Windows).
+    (Anatol)
+
+- FTP:
+  . Fixed bug #76972 (Data truncation due to forceful ssl socket shutdown).
+    (Manuel Mausz)
 
 11 Oct 2018, PHP 7.1.23
 
diff --git a/ext/ftp/ftp.c b/ext/ftp/ftp.c
index b7905778cf..88553b969c 100644
--- a/ext/ftp/ftp.c
+++ b/ext/ftp/ftp.c
@@ -67,6 +67,7 @@
 
 #ifdef HAVE_FTP_SSL
 #include <openssl/ssl.h>
+#include <openssl/err.h>
 #endif
 
 #include "ftp.h"
@@ -111,6 +112,11 @@ static databuf_t*	data_close(ftpbuf_t *ftp, databuf_t *data);
 /* generic file lister */
 static char**		ftp_genlist(ftpbuf_t *ftp, const char *cmd, const char *path);
 
+#ifdef HAVE_FTP_SSL
+/* shuts down a TLS/SSL connection */
+static void		ftp_ssl_shutdown(ftpbuf_t *ftp, php_socket_t fd, SSL *ssl_handle);
+#endif
+
 /* IP and port conversion box */
 union ipbox {
 	struct in_addr	ia[2];
@@ -184,8 +190,7 @@ ftp_close(ftpbuf_t *ftp)
 	if (ftp->fd != -1) {
 #ifdef HAVE_FTP_SSL
 		if (ftp->ssl_active) {
-			SSL_shutdown(ftp->ssl_handle);
-			SSL_free(ftp->ssl_handle);
+			ftp_ssl_shutdown(ftp, ftp->fd, ftp->ssl_handle);
 		}
 #endif
 		closesocket(ftp->fd);
@@ -1743,6 +1748,62 @@ data_accepted:
 }
 /* }}} */
 
+/* {{{ ftp_ssl_shutdown
+ */
+#ifdef HAVE_FTP_SSL
+static void ftp_ssl_shutdown(ftpbuf_t *ftp, php_socket_t fd, SSL *ssl_handle) {
+	/* In TLS 1.3 it's common to receive session tickets after the handshake has completed. We need to train
+	   the socket (read the tickets until EOF/close_notify alert) before closing the socket. Otherwise the
+	   server might get an ECONNRESET which might lead to data truncation on server side.
+	*/
+	char buf[256]; /* We will use this for the OpenSSL error buffer, so it has
+			  to be at least 256 bytes long.*/
+	int done = 1, err, nread;
+	unsigned long sslerror;
+
+	err = SSL_shutdown(ssl_handle);
+	if (err < 0) {
+		php_error_docref(NULL, E_WARNING, "SSL_shutdown failed");
+	}
+	else if (err == 0) {
+		/* The shutdown is not yet finished. Call SSL_read() to do a bidirectional shutdown. */
+		done = 0;
+	}
+
+	while (!done) {
+		if (data_available(ftp, fd)) {
+			ERR_clear_error();
+			nread = SSL_read(ssl_handle, buf, sizeof(buf));
+			err = SSL_get_error(ssl_handle, nread);
+			switch (err) {
+				case SSL_ERROR_NONE: /* this is not an error */
+				case SSL_ERROR_ZERO_RETURN: /* no more data */
+					/* This is the expected response. There was no data but only
+					   the close notify alert */
+					done = 1;
+					break;
+				case SSL_ERROR_WANT_READ:
+					/* there's data pending, re-invoke SSL_read() */
+					break;
+				case SSL_ERROR_WANT_WRITE:
+					/* SSL wants a write. Really odd. Let's bail out. */
+					done = 1;
+					break;
+				default:
+					if ((sslerror = ERR_get_error())) {
+					    ERR_error_string_n(sslerror, buf, sizeof(buf));
+					}
+					php_error_docref(NULL, E_WARNING, "SSL_read on shutdown: %s (%d)", (sslerror ? buf : strerror(errno)), errno);
+					done = 1;
+					break;
+			}
+		}
+	}
+	(void)SSL_free(ssl_handle);
+}
+#endif
+/* }}} */
+
 /* {{{ data_close
  */
 databuf_t*
@@ -1758,8 +1819,7 @@ data_close(ftpbuf_t *ftp, databuf_t *data)
 #ifdef HAVE_FTP_SSL
 		if (data->ssl_active) {
 			/* don't free the data context, it's the same as the control */
-			SSL_shutdown(data->ssl_handle);
-			SSL_free(data->ssl_handle);
+			ftp_ssl_shutdown(ftp, data->listener, data->ssl_handle);
 			data->ssl_active = 0;
 		}
 #endif
@@ -1769,8 +1829,7 @@ data_close(ftpbuf_t *ftp, databuf_t *data)
 #ifdef HAVE_FTP_SSL
 		if (data->ssl_active) {
 			/* don't free the data context, it's the same as the control */
-			SSL_shutdown(data->ssl_handle);
-			SSL_free(data->ssl_handle);
+			ftp_ssl_shutdown(ftp, data->fd, data->ssl_handle);
 			data->ssl_active = 0;
 		}
 #endif