]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Invalid FTP connection handling on blocked content.
authorChristos Tsantilas <chtsanti@users.sourceforge.net>
Mon, 25 Jan 2016 17:54:50 +0000 (19:54 +0200)
committerChristos Tsantilas <chtsanti@users.sourceforge.net>
Mon, 25 Jan 2016 17:54:50 +0000 (19:54 +0200)
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.

src/FwdState.cc
src/clients/Client.cc
src/clients/Client.h
src/clients/FtpClient.cc
src/clients/FtpClient.h
src/clients/FtpGateway.cc
src/clients/FtpRelay.cc
src/http.cc
src/http.h
src/servers/FtpServer.cc
src/servers/FtpServer.h

index 3658dcce8a3be5e8e046128b65751747e03c3eb1..143594915c79886a8eaa128156c537375a072cf7 100644 (file)
@@ -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
index 777406bb06922c51d680f16a168f13bd5cf0df78..f582d95d468178bfde82a85b87fdb77077e2b768 100644 (file)
@@ -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,
index eb98eeddf57077fa34aed2c4791344a6648d71a3..246647eb858fde37a764b853757fd40a67549bda 100644 (file)
@@ -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();
index 36aa91ad2c0a60e4d9d4970ba799361b90789061..b44f999dd81ab7f4eabe7794bcc8470c3d9b2a6f 100644 (file)
@@ -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);
index 457c4934d148db5ae0e17f751dd3765c9f72d197..708fc18237472af2a26271e0e243ae46355b7af1 100644 (file)
@@ -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);
index 0d7ecc2729a3f6b256e6fdcb188dfd09e997a3c5..bdb673599ebcc257721c5d9d6e356e4b7ee7ee7d 100644 (file)
@@ -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;
index f715c95dd9d8b3485fea420a33eff7798d4f4d1b..5f1c3d744788bb407eb17b5ffdfc99d94e08b5fc 100644 (file)
@@ -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<ConnStateData> &mgr = fwd->request->clientConnectionManager;
+    if (mgr.valid()) {
+        if (Ftp::Server *srv = dynamic_cast<Ftp::Server*>(mgr.get())) {
+            typedef NullaryMemFunT<Ftp::Server> 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)
 {
index 5b39187bf4af07a08212fa1797f77b8a88dcc127..f4bb44defeb4ba899454d53b177265502685c2da 100644 (file)
@@ -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");
 }
 
index 0c5f976d9b5c118606bb37d2c23641b289b91117..5eb40745f11124f9739f7bc28fc59159c776dc43 100644 (file)
@@ -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.
index a3d4b8b2590fa4cb0eb742567047508ac58d1856..340e1eeb73ce2178cb12a6bbd8440632fa9b48a8 100644 (file)
@@ -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)
index 97192caf87a140eff93eb476c9261f7ab0f3f50c..d3ea1d5aec32c33fe03f3327712a6acb734cb179 100644 (file)
@@ -41,12 +41,16 @@ class MasterState: public RefCountable
 public:
     typedef RefCount<MasterState> 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 &params);