-
Notifications
You must be signed in to change notification settings - Fork 7.7k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fix #76972: FTP data truncation due to forceful ssl socket shutdown
Do a correct bidirectional shutdown instead
- Loading branch information
Showing
2 changed files
with
71 additions
and
7 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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_accept(databuf_t *data, ftpbuf_t *ftp) | |
} | ||
/* }}} */ | ||
|
||
/* {{{ 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); | ||
This comment has been minimized.
Sorry, something went wrong. |
||
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; | ||
This comment has been minimized.
Sorry, something went wrong.
remicollet
Contributor
|
||
} | ||
} | ||
} | ||
(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 | ||
|
From man_page,
SSL_get_error
only makes sense whennread <= 0