Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 65 additions & 6 deletions ext/ftp/ftp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@

#ifdef HAVE_FTP_SSL
#include <openssl/ssl.h>
#include <openssl/err.h>
#endif

#include "ftp.h"
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@manuelm Isn't this line supposed to be inside the body of the if statement? See also https://bugs.php.net/bug.php?id=77151.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ternary either uses sslerror or strerror(errno), so I think the current position is correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Thanks!

done = 1;
break;
}
}
}
(void)SSL_free(ssl_handle);
}
#endif
/* }}} */

/* {{{ data_close
*/
databuf_t*
Expand All @@ -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
Expand All @@ -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
Expand Down