From: Christos Tsantilas Date: Mon, 25 Jan 2016 17:54:50 +0000 (+0200) Subject: Invalid FTP connection handling on blocked content. X-Git-Tag: SQUID_4_0_5~26 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=92cfc72f72e0dfd7509337fdee2104d15cb4000f;p=thirdparty%2Fsquid.git Invalid FTP connection handling on blocked content. FTP client gets stuck after the following chain of events: * Client requests a file that will be blocked by ICAP. * Squid starts downloading the file from the FTP server and sends "150 Opening..." to the FTP client. * Squid aborts the data connection with the FTP server as soon as the ICAP service blocks it. * Squid sends "451 Forbidden" to the FTP client. * The FTP server sends "500 OOPS: setsockopt: linger" to Squid. * Squid terminates the control connection to the FTP server. * Squid establishes a new control connection to the FTP server but does not authenticate itself. * Further commands from the FTP client do not work any more. The above and many similar problems exist because Squid handles FTP client-to-squid and squid-to-FTP server data connections independently from each other. In many cases, one connection does not get notified about the problems with the other connection. This patch: - Add Ftp::MasterState::userDataDone to record received the FTP client final response status code to sent (or to be send) to the client. - The Ftp::MasterState::waitForOriginData flag to hold status of the squid-to-server side. If the squid-to-server side is not finishes yet this is true. - Send a control reply to the FTP client only after the data transfered on both server and client sides. - Split Client::abortTransaction to Client::abortOnData and to Client::abortAll() - Implement the Ftp::Relay::abortOnData() and Ftp::Relay::Abort() (i.e., StoreEntry abort handler) to avoid closing the control connection when the data connection is closed unexpectedly. This is a Measurement Factory project. --- diff --git a/src/FwdState.cc b/src/FwdState.cc index 3658dcce8a..143594915c 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -161,7 +161,10 @@ void FwdState::start(Pointer aSelf) // We hope that either the store entry aborts or peer is selected. // Otherwise we are going to leak our object. - entry->registerAbort(FwdState::abort, this); + // Ftp::Relay needs to preserve control connection on data aborts + // so it registers its own abort handler that calls ours when needed. + if (!request->flags.ftpNative) + entry->registerAbort(FwdState::abort, this); #if STRICT_ORIGINAL_DST // Bug 3243: CVE 2009-0801 diff --git a/src/clients/Client.cc b/src/clients/Client.cc index 777406bb06..f582d95d46 100644 --- a/src/clients/Client.cc +++ b/src/clients/Client.cc @@ -247,7 +247,7 @@ Client::abortOnBadEntry(const char *abortReason) return false; debugs(11,5, HERE << "entry is not Accepting!"); - abortTransaction(abortReason); + abortOnData(abortReason); return true; } @@ -293,6 +293,13 @@ Client::noteBodyProducerAborted(BodyPipe::Pointer bp) handleRequestBodyProducerAborted(); } +bool +Client::abortOnData(const char *reason) +{ + abortAll(reason); + return true; +} + // more origin request body data is available void Client::handleMoreRequestBodyAvailable() @@ -367,12 +374,12 @@ Client::sentRequestBody(const CommIoCbParams &io) err = new ErrorState(ERR_WRITE_ERROR, Http::scBadGateway, fwd->request); err->xerrno = io.xerrno; fwd->fail(err); - abortTransaction("I/O error while sending request body"); + abortOnData("I/O error while sending request body"); return; } if (EBIT_TEST(entry->flags, ENTRY_ABORTED)) { - abortTransaction("store entry aborted while sending request body"); + abortOnData("store entry aborted while sending request body"); return; } @@ -847,7 +854,7 @@ Client::handleAdaptationAborted(bool bypassable) // TODO: bypass if possible if (!handledEarlyAdaptationAbort()) - abortTransaction("adaptation failure with a filled entry"); + abortAll("adaptation failure with a filled entry"); } /// If the store entry is still empty, fully handles adaptation abort, returning @@ -861,7 +868,7 @@ Client::handledEarlyAdaptationAbort() err->detailError(ERR_DETAIL_ICAP_RESPMOD_EARLY); fwd->fail(err); fwd->dontRetry(true); - abortTransaction("adaptation failure with an empty entry"); + abortAll("adaptation failure with an empty entry"); return true; // handled } @@ -883,7 +890,7 @@ Client::handleAdaptationBlocked(const Adaptation::Answer &answer) if (!entry->isEmpty()) { // too late to block (should not really happen) if (request) request->detailError(ERR_ICAP_FAILURE, ERR_DETAIL_RESPMOD_BLOCK_LATE); - abortTransaction("late adaptation block"); + abortAll("late adaptation block"); return; } @@ -899,7 +906,7 @@ Client::handleAdaptationBlocked(const Adaptation::Answer &answer) fwd->fail(err); fwd->dontRetry(true); - abortTransaction("timely adaptation block"); + abortOnData("timely adaptation block"); } void @@ -936,7 +943,7 @@ Client::sendBodyIsTooLargeError() ErrorState *err = new ErrorState(ERR_TOO_BIG, Http::scForbidden, request); fwd->fail(err); fwd->dontRetry(true); - abortTransaction("Virgin body too large."); + abortOnData("Virgin body too large."); } // TODO: when HttpStateData sends all errors to ICAP, diff --git a/src/clients/Client.h b/src/clients/Client.h index eb98eeddf5..246647eb85 100644 --- a/src/clients/Client.h +++ b/src/clients/Client.h @@ -54,7 +54,12 @@ public: virtual void maybeReadVirginBody() = 0; /// abnormal transaction termination; reason is for debugging only - virtual void abortTransaction(const char *reason) = 0; + virtual void abortAll(const char *reason) = 0; + + /// abnormal data transfer termination + /// \retval true the transaction will be terminated (abortAll called) + /// \reval false the transaction will survive + virtual bool abortOnData(const char *reason); /// a hack to reach HttpStateData::orignal_request virtual HttpRequest *originalRequest(); diff --git a/src/clients/FtpClient.cc b/src/clients/FtpClient.cc index 36aa91ad2c..b44f999dd8 100644 --- a/src/clients/FtpClient.cc +++ b/src/clients/FtpClient.cc @@ -350,8 +350,8 @@ Ftp::Client::readControlReply(const CommIoCbParams &io) return; if (EBIT_TEST(entry->flags, ENTRY_ABORTED)) { - abortTransaction("entry aborted during control reply read"); - return; + if (abortOnData("entry aborted during control reply read")) + return; } assert(ctrl.offset < ctrl.size); @@ -381,7 +381,7 @@ Ftp::Client::readControlReply(const CommIoCbParams &io) } /* XXX this may end up having to be serverComplete() .. */ - abortTransaction("zero control reply read"); + abortAll("zero control reply read"); return; } @@ -915,7 +915,7 @@ Ftp::Client::dataRead(const CommIoCbParams &io) assert(io.fd == data.conn->fd); if (EBIT_TEST(entry->flags, ENTRY_ABORTED)) { - abortTransaction("entry aborted during dataRead"); + abortOnData("entry aborted during dataRead"); return; } @@ -999,7 +999,7 @@ Ftp::Client::dataComplete() * including canceling close handlers */ void -Ftp::Client::abortTransaction(const char *reason) +Ftp::Client::abortAll(const char *reason) { debugs(9, 3, "aborting transaction for " << reason << "; FD " << (ctrl.conn!=NULL?ctrl.conn->fd:-1) << ", Data FD " << (data.conn!=NULL?data.conn->fd:-1) << ", this " << this); diff --git a/src/clients/FtpClient.h b/src/clients/FtpClient.h index 457c4934d1..708fc18237 100644 --- a/src/clients/FtpClient.h +++ b/src/clients/FtpClient.h @@ -167,7 +167,7 @@ protected: virtual void closeServer(); virtual bool doneWithServer() const; virtual const Comm::ConnectionPointer & dataConnection() const; - virtual void abortTransaction(const char *reason); + virtual void abortAll(const char *reason); virtual Http::StatusCode failedHttpStatus(err_type &error); void ctrlClosed(const CommCloseCbParams &io); diff --git a/src/clients/FtpGateway.cc b/src/clients/FtpGateway.cc index 0d7ecc2729..bdb673599e 100644 --- a/src/clients/FtpGateway.cc +++ b/src/clients/FtpGateway.cc @@ -989,7 +989,7 @@ Ftp::Gateway::processReplyBody() * probably was aborted because content length exceeds one * of the maximum size limits. */ - abortTransaction("entry aborted after calling appendSuccessHeader()"); + abortAll("entry aborted after calling appendSuccessHeader()"); return; } @@ -1697,7 +1697,7 @@ Ftp::Gateway::processHeadResponse() * trying to write to the client. */ if (EBIT_TEST(entry->flags, ENTRY_ABORTED)) { - abortTransaction("entry aborted while processing HEAD"); + abortAll("entry aborted while processing HEAD"); return; } @@ -1914,7 +1914,7 @@ Ftp::Gateway::ftpAcceptDataConnection(const CommAcceptCbParams &io) debugs(9, 3, HERE); if (EBIT_TEST(entry->flags, ENTRY_ABORTED)) { - abortTransaction("entry aborted when accepting data conn"); + abortAll("entry aborted when accepting data conn"); data.listenConn->close(); data.listenConn = NULL; return; diff --git a/src/clients/FtpRelay.cc b/src/clients/FtpRelay.cc index f715c95dd9..5f1c3d7447 100644 --- a/src/clients/FtpRelay.cc +++ b/src/clients/FtpRelay.cc @@ -54,6 +54,7 @@ protected: virtual void handleRequestBodyProducerAborted(); virtual bool mayReadVirginReplyBody() const; virtual void completeForwarding(); + virtual bool abortOnData(const char *reason); /* AsyncJob API */ virtual void start(); @@ -88,6 +89,9 @@ protected: void readUserOrPassReply(); void scheduleReadControlReply(); + void finalizeDataDownload(); + + static void abort(void *d); // TODO: Capitalize this and FwdState::abort(). bool forwardingCompleted; ///< completeForwarding() has been called @@ -148,6 +152,8 @@ Ftp::Relay::Relay(FwdState *const fwdState): // Nothing we can do at request creation time can mark the response as // uncachable, unfortunately. This prevents "found KEY_PRIVATE" WARNINGs. entry->releaseRequest(); + // TODO: Convert registerAbort() to use AsyncCall + entry->registerAbort(Ftp::Relay::abort, this); } Ftp::Relay::~Relay() @@ -281,7 +287,14 @@ Ftp::Relay::processReplyBody() * probably was aborted because content length exceeds one * of the maximum size limits. */ - abortTransaction("entry aborted after calling appendSuccessHeader()"); + abortOnData("entry aborted after calling appendSuccessHeader()"); + return; + } + + if (master().userDataDone) { + // Squid-to-client data transfer done. Abort data transfer on our + // side to allow new commands from ftp client + abortOnData("Squid-to-client data connection is closed"); return; } @@ -477,7 +490,7 @@ void Ftp::Relay::sendCommand() { if (!fwd->request->header.has(Http::HdrType::FTP_COMMAND)) { - abortTransaction("Internal error: FTP relay request with no command"); + abortAll("Internal error: FTP relay request with no command"); return; } @@ -675,7 +688,7 @@ Ftp::Relay::readTransferDoneReply() " after reading response data"); } - serverComplete(); + finalizeDataDownload(); } void @@ -703,6 +716,55 @@ Ftp::Relay::scheduleReadControlReply() Ftp::Client::scheduleReadControlReply(0); } +void +Ftp::Relay::finalizeDataDownload() +{ + debugs(9, 2, "Complete data downloading/Uploading"); + + updateMaster().waitForOriginData = false; + + CbcPointer &mgr = fwd->request->clientConnectionManager; + if (mgr.valid()) { + if (Ftp::Server *srv = dynamic_cast(mgr.get())) { + typedef NullaryMemFunT CbDialer; + AsyncCall::Pointer call = JobCallback(11, 3, CbDialer, srv, + Ftp::Server::originDataCompletionCheckpoint); + ScheduleCallHere(call); + } + } + serverComplete(); +} + +bool +Ftp::Relay::abortOnData(const char *reason) +{ + debugs(9, 3, "aborting transaction for " << reason << + "; FD " << (ctrl.conn != NULL ? ctrl.conn->fd : -1) << ", Data FD " << (data.conn != NULL ? data.conn->fd : -1) << ", this " << this); + // this method is only called to handle data connection problems + // the control connection should keep going + +#if USE_ADAPTATION + if (adaptedBodySource != NULL) + stopConsumingFrom(adaptedBodySource); +#endif + + if (Comm::IsConnOpen(data.conn)) + dataComplete(); + + return !Comm::IsConnOpen(ctrl.conn); +} + +void +Ftp::Relay::abort(void *d) +{ + Ftp::Relay *ftpClient = (Ftp::Relay *)d; + debugs(9, 2, "Client Data connection closed!"); + if (!cbdataReferenceValid(ftpClient)) + return; + if (Comm::IsConnOpen(ftpClient->data.conn)) + ftpClient->dataComplete(); +} + AsyncJob::Pointer Ftp::StartRelay(FwdState *const fwdState) { diff --git a/src/http.cc b/src/http.cc index 5b39187bf4..f4bb44defe 100644 --- a/src/http.cc +++ b/src/http.cc @@ -2466,7 +2466,7 @@ HttpStateData::sentRequestBody(const CommIoCbParams &io) // TODO: destruction should be sufficient as the destructor should cleanup, // including canceling close handlers void -HttpStateData::abortTransaction(const char *reason) +HttpStateData::abortAll(const char *reason) { debugs(11,5, HERE << "aborting transaction for " << reason << "; " << serverConnection << ", this " << this); @@ -2477,6 +2477,6 @@ HttpStateData::abortTransaction(const char *reason) } fwd->handleUnregisteredServerEnd(); - mustStop("HttpStateData::abortTransaction"); + mustStop("HttpStateData::abortAll"); } diff --git a/src/http.h b/src/http.h index 0c5f976d9b..5eb40745f1 100644 --- a/src/http.h +++ b/src/http.h @@ -84,9 +84,11 @@ private: virtual bool getMoreRequestBody(MemBuf &buf); virtual void closeServer(); // end communication with the server virtual bool doneWithServer() const; // did we end communication? - virtual void abortTransaction(const char *reason); // abnormal termination + virtual void abortAll(const char *reason); // abnormal termination virtual bool mayReadVirginReplyBody() const; + void abortTransaction(const char *reason) { abortAll(reason); } // abnormal termination + /** * determine if read buffer can have space made available * for a read. diff --git a/src/servers/FtpServer.cc b/src/servers/FtpServer.cc index a3d4b8b259..340e1eeb73 100644 --- a/src/servers/FtpServer.cc +++ b/src/servers/FtpServer.cc @@ -307,11 +307,18 @@ Ftp::Server::clientPinnedConnectionClosed(const CommCloseCbParams &io) { ConnStateData::clientPinnedConnectionClosed(io); - // if the server control connection is gone, reset state to login again - resetLogin("control connection closure"); + // TODO: Keep the control connection open after fixing the reset + // problem below + if (Comm::IsConnOpen(clientConnection)) + clientConnection->close(); - // XXX: Reseting is not enough. FtpRelay::sendCommand() will not re-login - // because FtpRelay::serverState() is not going to be fssConnected. + // TODO: If the server control connection is gone, reset state to login + // again. Reseting login alone is not enough: FtpRelay::sendCommand() will + // not re-login because FtpRelay::serverState() is not going to be + // fssConnected. Calling resetLogin() alone is also harmful because + // it does not reset correctly the client-to-squid control connection (eg + // respond if required with an error code, in all cases) + // resetLogin("control connection closure"); } /// clear client and server login-related state after the old login is gone @@ -979,8 +986,7 @@ Ftp::Server::wroteReplyData(const CommIoCbParams &io) if (io.flag != Comm::OK) { debugs(33, 3, "FTP reply data writing failed: " << xstrerr(io.xerrno)); - closeDataConnection(); - writeCustomReply(426, "Data connection error; transfer aborted"); + userDataCompletionCheckpoint(426); return; } @@ -1000,21 +1006,19 @@ Ftp::Server::replyDataWritingCheckpoint() return; case STREAM_COMPLETE: debugs(33, 3, "FTP reply data transfer successfully complete"); - writeCustomReply(226, "Transfer complete"); + userDataCompletionCheckpoint(226); break; case STREAM_UNPLANNED_COMPLETE: debugs(33, 3, "FTP reply data transfer failed: STREAM_UNPLANNED_COMPLETE"); - writeCustomReply(451, "Server error; transfer aborted"); + userDataCompletionCheckpoint(451); break; case STREAM_FAILED: + userDataCompletionCheckpoint(451); debugs(33, 3, "FTP reply data transfer failed: STREAM_FAILED"); - writeCustomReply(451, "Server error; transfer aborted"); break; default: fatal("unreachable code"); } - - closeDataConnection(); } void @@ -1483,6 +1487,9 @@ Ftp::Server::handleDataRequest(String &, String &) if (!checkDataConnPre()) return false; + master->waitForOriginData = true; + master->userDataDone = 0; + changeState(fssHandleDataRequest, "handleDataRequest"); return true; @@ -1494,6 +1501,9 @@ Ftp::Server::handleUploadRequest(String &, String &) if (!checkDataConnPre()) return false; + master->waitForOriginData = true; + master->userDataDone = 0; + if (Config.accessList.forceRequestBodyContinuation) { ClientHttpRequest *http = pipeline.front()->http; HttpRequest *request = http->request; @@ -1712,6 +1722,47 @@ Ftp::Server::callException(const std::exception &e) AsyncJob::callException(e); } +void +Ftp::Server::originDataCompletionCheckpoint() +{ + if (!master->userDataDone) { + debugs(33, 5, "Transfering from/to client not finished yet"); + return; + } + + completeDataExchange(); +} + +void Ftp::Server::userDataCompletionCheckpoint(int finalStatusCode) +{ + Must(!master->userDataDone); + master->userDataDone = finalStatusCode; + + if (bodyParser) + finishDechunkingRequest(false); + + // The origin control connection is gone, nothing to wait for + if (!Comm::IsConnOpen(pinning.serverConnection)) + master->waitForOriginData = false; + + if (master->waitForOriginData) { + // The completeDataExchange() is not called here unconditionally + // because we want to signal the FTP user that we are not fully + // done processing its data stream, even though all data bytes + // have been sent or received already. + debugs(33, 5, "Transfering from/to FTP server is not complete"); + return; + } + + completeDataExchange(); +} + +void Ftp::Server::completeDataExchange() +{ + writeCustomReply(master->userDataDone, master->userDataDone == 226 ? "Transfer complete" : "Server error; transfer aborted"); + closeDataConnection(); +} + /// Whether Squid FTP Relay supports a named feature (e.g., a command). static bool Ftp::SupportedCommand(const SBuf &name) diff --git a/src/servers/FtpServer.h b/src/servers/FtpServer.h index 97192caf87..d3ea1d5aec 100644 --- a/src/servers/FtpServer.h +++ b/src/servers/FtpServer.h @@ -41,12 +41,16 @@ class MasterState: public RefCountable public: typedef RefCount Pointer; - MasterState(): serverState(fssBegin), clientReadGreeting(false) {} + MasterState(): serverState(fssBegin), clientReadGreeting(false), userDataDone(0), waitForOriginData(false) {} Ip::Address clientDataAddr; ///< address of our FTP client data connection SBuf workingDir; ///< estimated current working directory for URI formation ServerState serverState; ///< what our FTP server is doing bool clientReadGreeting; ///< whether our FTP client read their FTP server greeting + /// Squid will send or has sent this final status code to the FTP client + int userDataDone; + /// whether the transfer on the Squid-origin data connection is not over yet + bool waitForOriginData; }; /// Manages a control connection from an FTP client. @@ -62,6 +66,11 @@ public: /* AsyncJob API */ virtual void callException(const std::exception &e); + /// Called by Ftp::Client class when it is done receiving or + /// sending data. Waits for both agents to be done before + /// responding to the FTP client and closing the data connection. + void originDataCompletionCheckpoint(); + // This is a pointer in hope to minimize future changes when MasterState // becomes a part of MasterXaction. Guaranteed not to be nil. MasterState::Pointer master; ///< info shared among our FTP client and server jobs @@ -110,6 +119,14 @@ protected: bool createDataConnection(Ip::Address cltAddr); void closeDataConnection(); + /// Called after data trasfer on client-to-squid data connection is + /// finished. + void userDataCompletionCheckpoint(int finalStatusCode); + + /// Writes the data-transfer status reply to the FTP client and + /// closes the data connection. + void completeDataExchange(); + void calcUri(const SBuf *file); void changeState(const Ftp::ServerState newState, const char *reason); ClientSocketContext *handleUserRequest(const SBuf &cmd, SBuf ¶ms);