Added nil dereference checks for Ftp::Client::ctrl.conn, including:
- Ftp::Client::handlePasvReply() and handleEpsvReply() that dereference
ctrl.conn in DBG_IMPORTANT messages.
- Many functions inside FtpClient.cc and FtpGateway.cc files.
TODO: We need to find a better way to handle nil ctrl.conn. It is only a
matter of time when we forget to add another dereference check or
discover a place we missed during this change.
Also disabled forwarding of EPRT and PORT commands to origin servers.
Squid support for those commands is broken and their forwarding may
cause segfaults (bug #4004). Active FTP is still supported, of course.
This is a Measurement Factory project.
Segfault via Ftp::Client::readControlReply.
Added nil dereference checks for Ftp::Client::ctrl.conn, including:
- Ftp::Client::handlePasvReply() and handleEpsvReply() that dereference
ctrl.conn in DBG_IMPORTANT messages.
- Many functions inside FtpClient.cc and FtpGateway.cc files.
TODO: We need to find a better way to handle nil ctrl.conn. It is only
a matter of time when we forget to add another dereference check or
discover a place we missed during this change.
Also disabled forwarding of EPRT and PORT commands to origin servers.
Squid support for those commands is broken and their forwarding may
cause segfaults (bug #4004). Active FTP is still supported, of course.
This is a Measurement Factory project.
=== modified file 'src/clients/FtpClient.cc'
--- src/clients/FtpClient.cc 2016-07-27 08:18:03 +0000
+++ src/clients/FtpClient.cc 2016-11-10 16:17:11 +0000
@@ -425,71 +425,81 @@
if (ctrl.offset == bytes_used) {
/* used it all up */
ctrl.offset = 0;
} else {
/* Got some data past the complete reply */
assert(bytes_used < ctrl.offset);
ctrl.offset -= bytes_used;
memmove(ctrl.buf, ctrl.buf + bytes_used, ctrl.offset);
}
debugs(9, 3, "state=" << state << ", code=" << ctrl.replycode);
}
bool
Ftp::Client::handlePasvReply(Ip::Address &srvAddr)
{
int code = ctrl.replycode;
char *buf;
debugs(9, 3, status());
+ if (!Comm::IsConnOpen(ctrl.conn)) {
+ debugs(9, 5, "The control connection to the remote end is closed");
+ return false;
+ }
+
if (code != 227) {
debugs(9, 2, "PASV not supported by remote end");
return false;
}
/* 227 Entering Passive Mode (h1,h2,h3,h4,p1,p2). */
/* ANSI sez [^0-9] is undefined, it breaks on Watcom cc */
debugs(9, 5, "scanning: " << ctrl.last_reply);
buf = ctrl.last_reply + strcspn(ctrl.last_reply, "0123456789");
const char *forceIp = Config.Ftp.sanitycheck ?
fd_table[ctrl.conn->fd].ipaddr : NULL;
if (!Ftp::ParseIpPort(buf, forceIp, srvAddr)) {
debugs(9, DBG_IMPORTANT, "Unsafe PASV reply from " <<
ctrl.conn->remote << ": " << ctrl.last_reply);
return false;
}
data.addr(srvAddr);
return true;
}
bool
Ftp::Client::handleEpsvReply(Ip::Address &remoteAddr)
{
int code = ctrl.replycode;
char *buf;
debugs(9, 3, status());
+ if (!Comm::IsConnOpen(ctrl.conn)) {
+ debugs(9, 5, "The control connection to the remote end is closed");
+ return false;
+ }
+
if (code != 229 && code != 522) {
if (code == 200) {
/* handle broken servers (RFC 2428 says OK code for EPSV MUST be 229 not 200) */
/* vsftpd for one send '200 EPSV ALL ok.' without even port info.
* Its okay to re-send EPSV 1/2 but nothing else. */
debugs(9, DBG_IMPORTANT, "Broken FTP Server at " << ctrl.conn->remote << ". Wrong accept code for EPSV");
} else {
debugs(9, 2, "EPSV not supported by remote end");
}
return sendPassive();
}
if (code == 522) {
/* Peer responded with a list of supported methods:
* 522 Network protocol not supported, use (1)
* 522 Network protocol not supported, use (1,2)
* 522 Network protocol not supported, use (2)
* TODO: Handle the (1,2) case which may happen after EPSV ALL. Close
* data + control without self-destructing and re-open from scratch.
*/
@@ -718,40 +728,45 @@
}
}
break;
}
}
if (ctrl.message)
wordlistDestroy(&ctrl.message);
ctrl.message = NULL; //No message to return to client.
ctrl.offset = 0; //reset readed response, to make room read the next response
writeCommand(mb.content());
shortenReadTimeout = true;
return true;
}
void
Ftp::Client::connectDataChannel()
{
+ if (!Comm::IsConnOpen(ctrl.conn)) {
+ debugs(9, 5, "The control connection to the remote end is closed");
+ return;
+ }
+
safe_free(ctrl.last_command);
safe_free(ctrl.last_reply);
ctrl.last_command = xstrdup("Connect to server data port");
// Generate a new data channel descriptor to be opened.
Comm::ConnectionPointer conn = new Comm::Connection;
conn->setAddrs(ctrl.conn->local, data.host);
conn->local.port(0);
conn->remote.port(data.port);
conn->tos = ctrl.conn->tos;
conn->nfmark = ctrl.conn->nfmark;
debugs(9, 3, "connecting to " << conn->remote);
typedef CommCbMemFunT<Client, CommConnectCbParams> Dialer;
data.opener = JobCallback(9, 3, Dialer, this, Ftp::Client::dataChannelConnected);
Comm::ConnOpener *cs = new Comm::ConnOpener(conn, data.opener, Config.Timeout.connect);
cs->setHost(data.host);
=== modified file 'src/clients/FtpGateway.cc'
--- src/clients/FtpGateway.cc 2016-04-03 23:41:58 +0000
+++ src/clients/FtpGateway.cc 2016-11-10 16:28:28 +0000
@@ -426,40 +426,45 @@
if (colonPos != SBuf::npos) {
const SBuf pass = login.substr(colonPos+1, SBuf::npos);
SBuf::size_type upto = pass.copy(password, sizeof(password)-1);
password[upto]='\0';
debugs(9, 9, "found password=" << pass << " " <<
(upto != pass.length() ? ", truncated-to=" : ", length=") << upto <<
", escaped=" << escaped);
if (escaped) {
rfc1738_unescape(password);
password_url = 1;
}
debugs(9, 9, "found password=" << password << " (" << strlen(password) << ") unescaped.");
}
debugs(9, 9, "OUT: login=" << login << ", escaped=" << escaped << ", user=" << user << ", password=" << password);
}
void
Ftp::Gateway::listenForDataChannel(const Comm::ConnectionPointer &conn)
{
+ if (!Comm::IsConnOpen(ctrl.conn)) {
+ debugs(9, 5, "The control connection to the remote end is closed");
+ return;
+ }
+
assert(!Comm::IsConnOpen(data.conn));
typedef CommCbMemFunT<Gateway, CommAcceptCbParams> AcceptDialer;
typedef AsyncCallT<AcceptDialer> AcceptCall;
RefCount<AcceptCall> call = static_cast<AcceptCall*>(JobCallback(11, 5, AcceptDialer, this, Ftp::Gateway::ftpAcceptDataConnection));
Subscription::Pointer sub = new CallSubscription<AcceptCall>(call);
const char *note = entry->url();
/* open the conn if its not already open */
if (!Comm::IsConnOpen(conn)) {
conn->fd = comm_open_listener(SOCK_STREAM, IPPROTO_TCP, conn->local, conn->flags, note);
if (!Comm::IsConnOpen(conn)) {
debugs(5, DBG_CRITICAL, HERE << "comm_open_listener failed:" << conn->local << " error: " << errno);
return;
}
debugs(9, 3, HERE << "Unconnected data socket created on " << conn);
}
conn->tos = ctrl.conn->tos;
conn->nfmark = ctrl.conn->nfmark;
@@ -1147,41 +1152,41 @@
}
base_href.append(authority.rawContent(), authority.length());
base_href.append(request->url.path().rawContent(), request->url.path().length());
base_href.append("/");
}
void
Ftp::Gateway::start()
{
if (!checkAuth(&request->header)) {
/* create appropriate reply */
HttpReply *reply = ftpAuthRequired(request, ftpRealm());
entry->replaceHttpReply(reply);
serverComplete();
return;
}
checkUrlpath();
buildTitleUrl();
- debugs(9, 5, "FD " << ctrl.conn->fd << " : host=" << request->url.host() <<
+ debugs(9, 5, "FD " << (ctrl.conn ? ctrl.conn->fd : -1) << " : host=" << request->url.host() <<
", path=" << request->url.path() << ", user=" << user << ", passwd=" << password);
state = BEGIN;
Ftp::Client::start();
}
/* ====================================================================== */
void
Ftp::Gateway::handleControlReply()
{
Ftp::Client::handleControlReply();
if (ctrl.message == NULL)
return; // didn't get complete reply yet
/* Copy the message except for the last line to cwd_message to be
* printed in error messages.
*/
for (wordlist *w = ctrl.message; w && w->next; w = w->next) {
cwd_message.append('\n');
cwd_message.append(w->key);
@@ -1702,80 +1707,87 @@
}
#if USE_ADAPTATION
if (adaptationAccessCheckPending) {
debugs(9,3, HERE << "returning due to adaptationAccessCheckPending");
return;
}
#endif
// processReplyBody calls serverComplete() since there is no body
processReplyBody();
}
static void
ftpReadPasv(Ftp::Gateway * ftpState)
{
Ip::Address srvAddr; // unused
if (ftpState->handlePasvReply(srvAddr))
ftpState->connectDataChannel();
else {
- ftpSendEPRT(ftpState);
+ ftpFail(ftpState);
return;
+ // Currently disabled, does not work correctly
+ ftpSendEPRT(ftpState);
}
}
void
Ftp::Gateway::dataChannelConnected(const CommConnectCbParams &io)
{
debugs(9, 3, HERE);
data.opener = NULL;
if (io.flag != Comm::OK) {
debugs(9, 2, HERE << "Failed to connect. Retrying via another method.");
// ABORT on timeouts. server may be waiting on a broken TCP link.
if (io.xerrno == Comm::TIMEOUT)
writeCommand("ABOR");
// try another connection attempt with some other method
ftpSendPassive(this);
return;
}
data.opened(io.conn, dataCloser());
ftpRestOrList(this);
}
static void
ftpOpenListenSocket(Ftp::Gateway * ftpState, int fallback)
{
/// Close old data channels, if any. We may open a new one below.
if (ftpState->data.conn != NULL) {
if ((ftpState->data.conn->flags & COMM_REUSEADDR))
// NP: in fact it points to the control channel. just clear it.
ftpState->data.clear();
else
ftpState->data.close();
}
safe_free(ftpState->data.host);
+ if (!Comm::IsConnOpen(ftpState->ctrl.conn)) {
+ debugs(9, 5, "The control connection to the remote end is closed");
+ return;
+ }
+
/*
* Set up a listen socket on the same local address as the
* control connection.
*/
Comm::ConnectionPointer temp = new Comm::Connection;
temp->local = ftpState->ctrl.conn->local;
/*
* REUSEADDR is needed in fallback mode, since the same port is
* used for both control and data.
*/
if (fallback) {
int on = 1;
errno = 0;
if (setsockopt(ftpState->ctrl.conn->fd, SOL_SOCKET, SO_REUSEADDR,
(char *) &on, sizeof(on)) == -1) {
int xerrno = errno;
// SO_REUSEADDR is only an optimization, no need to be verbose about error
debugs(9, 4, "setsockopt failed: " << xstrerr(xerrno));
}
@@ -1836,40 +1848,44 @@
}
static void
ftpReadPORT(Ftp::Gateway * ftpState)
{
int code = ftpState->ctrl.replycode;
debugs(9, 3, HERE);
if (code != 200) {
/* Fall back on using the same port as the control connection */
debugs(9, 3, "PORT not supported by remote end");
ftpOpenListenSocket(ftpState, 1);
}
ftpRestOrList(ftpState);
}
static void
ftpSendEPRT(Ftp::Gateway * ftpState)
{
+ /* check the server control channel is still available */
+ if (!ftpState || !ftpState->haveControlChannel("ftpSendEPRT"))
+ return;
+
if (Config.Ftp.epsv_all && ftpState->flags.epsv_all_sent) {
debugs(9, DBG_IMPORTANT, "FTP does not allow EPRT method after 'EPSV ALL' has been sent.");
return;
}
if (!Config.Ftp.eprt) {
/* Disabled. Switch immediately to attempting old PORT command. */
debugs(9, 3, "EPRT disabled by local administrator");
ftpSendPORT(ftpState);
return;
}
debugs(9, 3, HERE);
ftpState->flags.pasv_supported = 0;
ftpOpenListenSocket(ftpState, 0);
debugs(9, 3, "Listening for FTP data connection with FD " << ftpState->data.conn);
if (!Comm::IsConnOpen(ftpState->data.conn)) {
/* XXX Need to set error message */
ftpFail(ftpState);
@@ -1897,56 +1913,62 @@
if (code != 200) {
/* Failover to attempting old PORT command. */
debugs(9, 3, "EPRT not supported by remote end");
ftpSendPORT(ftpState);
return;
}
ftpRestOrList(ftpState);
}
/** "read" handler to accept FTP data connections.
*
\param io comm accept(2) callback parameters
*/
void
Ftp::Gateway::ftpAcceptDataConnection(const CommAcceptCbParams &io)
{
debugs(9, 3, HERE);
- if (EBIT_TEST(entry->flags, ENTRY_ABORTED)) {
- abortAll("entry aborted when accepting data conn");
- data.listenConn->close();
- data.listenConn = NULL;
+ if (!Comm::IsConnOpen(ctrl.conn)) { /*Close handlers will cleanup*/
+ debugs(9, 5, "The control connection to the remote end is closed");
return;
}
if (io.flag != Comm::OK) {
data.listenConn->close();
data.listenConn = NULL;
debugs(9, DBG_IMPORTANT, "FTP AcceptDataConnection: " << io.conn << ": " << xstrerr(io.xerrno));
/** \todo Need to send error message on control channel*/
ftpFail(this);
return;
}
+ if (EBIT_TEST(entry->flags, ENTRY_ABORTED)) {
+ abortAll("entry aborted when accepting data conn");
+ data.listenConn->close();
+ data.listenConn = NULL;
+ io.conn->close();
+ return;
+ }
+
/* data listening conn is no longer even open. abort. */
if (!Comm::IsConnOpen(data.listenConn)) {
data.listenConn = NULL; // ensure that it's cleared and not just closed.
return;
}
/* data listening conn is no longer even open. abort. */
if (!Comm::IsConnOpen(data.conn)) {
data.clear(); // ensure that it's cleared and not just closed.
return;
}
/** \par
* When squid.conf ftp_sanitycheck is enabled, check the new connection is actually being
* made by the remote client which is connected to the FTP control socket.
* Or the one which we were told to listen for by control channel messages (may differ under NAT).
* This prevents third-party hacks, but also third-party load balancing handshakes.
*/
if (Config.Ftp.sanitycheck) {
// accept if either our data or ctrl connection is talking to this remote peer.
@@ -2666,41 +2688,41 @@
* which should be sent to either StoreEntry, or to ICAP...
*/
void
Ftp::Gateway::writeReplyBody(const char *dataToWrite, size_t dataLength)
{
debugs(9, 5, HERE << "writing " << dataLength << " bytes to the reply");
addVirginReplyBody(dataToWrite, dataLength);
}
/**
* A hack to ensure we do not double-complete on the forward entry.
*
\todo Ftp::Gateway logic should probably be rewritten to avoid
* double-completion or FwdState should be rewritten to allow it.
*/
void
Ftp::Gateway::completeForwarding()
{
if (fwd == NULL || flags.completed_forwarding) {
debugs(9, 3, HERE << "completeForwarding avoids " <<
- "double-complete on FD " << ctrl.conn->fd << ", Data FD " << data.conn->fd <<
+ "double-complete on FD " << (ctrl.conn ? ctrl.conn->fd : -1) << ", Data FD " << data.conn->fd <<
", this " << this << ", fwd " << fwd);
return;
}
flags.completed_forwarding = true;
Client::completeForwarding();
}
/**
* Have we lost the FTP server control channel?
*
\retval true The server control channel is available.
\retval false The server control channel is not available.
*/
bool
Ftp::Gateway::haveControlChannel(const char *caller_name) const
{
if (doneWithServer())
return false;
_______________________________________________
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
