Merge ~ahasenack/ubuntu/+source/squid3:xenial-squid-passive-ftp-1560429 into ~usd-import-team/ubuntu/+source/squid3:ubuntu/xenial-devel

[Andreas Hasenack]

d/p/passive-ftp-segfault-1560429.patch: patch to avoid a segfault when proxying to an ftp server which does not allow passive mode (LP: #1560429)

[ChristianEhrhardt (paelzer)]

reviewed - changes LGTM in general.

I don't like that the nil check fix and the detection of a closed connection are in one change. But that is how upstream did it so I agree to stick with that.

Please do note that the change is meant for SRU but the bug has no SRU template yet.
That would block a potential usdi-merge/sponsoring - so add that now if you can.

review: Approve

[Robie Basak (racb)]

Looks good! Nice and clear changelog entry, and the dep3 headers contain all the needed information.

A couple of very minor suggestions on the quilt patch:

1. I recommend configuring quilt with "-p ab --no-timestamps --no-index". See https://wiki.debian.org/UsingQuilt. This removes diff noise in future quilt refreshes.

2. Technically the header should be "Origin: backport, ..." as you made changes to the upstream patch to make it fit, as you noted in the description. That also makes you an Author, so you can add a couple of Author lines (one for the upstream author, and one for you) if you wish.

I pushed these suggestions to https://code.launchpad.net/~racb/ubuntu/+source/squid3/+git/squid3/+ref/xenial-squid-passive-ftp-1560429

review: Approve

[Andreas Hasenack (ahasenack)]

Thanks, good suggestions. Please tag/upload your branch

diff --git a/debian/changelog b/debian/changelog
index 1d42208..b873856 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+squid3 (3.5.12-1ubuntu7.4) xenial; urgency=medium
+
+  * debian/patches/passive-ftp-segfault-1560429.patch: Fix for segfault
+    when ftp passive mode is not available.  Closes: #793473, LP:
+    #1560429.
+
+ -- Andreas Hasenack <[e-mail]>  Fri, 07 Jul 2017 09:39:40 -0300
+
 squid3 (3.5.12-1ubuntu7.3) xenial-security; urgency=medium
 
   * SECURITY UPDATE: cookie data leak via If-Not-Modified HTTP conditional
diff --git a/debian/patches/passive-ftp-segfault-1560429.patch b/debian/patches/passive-ftp-segfault-1560429.patch
new file mode 100644
index 0000000..b29ddec
--- /dev/null
+++ b/debian/patches/passive-ftp-segfault-1560429.patch
@@ -0,0 +1,185 @@
+Description: Fix for segfault when ftp passive mode is not available
+ If the remote FTP server does not support EPSV/PASV commands or for a reason
+ respond with error on these commands then squid will try to use EPRT/PORT and
+ may crash because these are not fully implemented.
+ .
+ Patch was backported from upstream's 3.5 branch, r14115, as it required a
+ small conflict resolution to a hunk in FtpGateway.cc.
+ .
+ Upstream commit message:
+ Bug 4004 partial: Fix 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.
+Origin: upstream, http://bazaar.launchpad.net/~squid/squid/3.5/revision/14115
+Bug: http://bugs.squid-cache.org/show_bug.cgi?id=4004
+Bug-Debian: https://bugs.debian.org/793473
+Bug-Ubuntu: https://launchpad.net/bugs/1560429
+Last-Update: 2017-07-04
+
+--- squid3-3.5.12.orig/src/clients/FtpClient.cc
++++ squid3-3.5.12/src/clients/FtpClient.cc
+@@ -437,6 +437,11 @@ Ftp::Client::handlePasvReply(Ip::Address
+     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;
+@@ -468,6 +473,11 @@ Ftp::Client::handleEpsvReply(Ip::Address
+     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) */
+@@ -728,6 +738,11 @@ Ftp::Client::sendPassive()
+ 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);
+--- squid3-3.5.12.orig/src/clients/FtpGateway.cc
++++ squid3-3.5.12/src/clients/FtpGateway.cc
+@@ -212,7 +212,9 @@ static FTPSM ftpSendMdtm;
+ static FTPSM ftpReadMdtm;
+ static FTPSM ftpSendSize;
+ static FTPSM ftpReadSize;
++#if 0
+ static FTPSM ftpSendEPRT;
++#endif
+ static FTPSM ftpReadEPRT;
+ static FTPSM ftpSendPORT;
+ static FTPSM ftpReadPORT;
+@@ -450,6 +452,11 @@ Ftp::Gateway::loginParser(const char *lo
+ 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;
+@@ -1183,7 +1190,7 @@ Ftp::Gateway::start()
+ 
+     checkUrlpath();
+     buildTitleUrl();
+-    debugs(9, 5, HERE << "FD " << ctrl.conn->fd << " : host=" << request->GetHost() <<
++    debugs(9, 5, "FD " << (ctrl.conn != NULL ? ctrl.conn->fd : -1) << " : host=" << request->GetHost() <<
+            ", path=" << request->urlpath << ", user=" << user << ", passwd=" << password);
+     state = BEGIN;
+     Ftp::Client::start();
+@@ -1750,7 +1757,9 @@ ftpReadPasv(Ftp::Gateway * ftpState)
+     if (ftpState->handlePasvReply(srvAddr))
+         ftpState->connectDataChannel();
+     else {
+-        ftpSendEPRT(ftpState);
++        ftpFail(ftpState);
++        // Currently disabled, does not work correctly:
++        // ftpSendEPRT(ftpState);
+         return;
+     }
+ }
+@@ -1790,6 +1799,11 @@ ftpOpenListenSocket(Ftp::Gateway * ftpSt
+     }
+     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.
+@@ -1875,9 +1889,14 @@ ftpReadPORT(Ftp::Gateway * ftpState)
+     ftpRestOrList(ftpState);
+ }
+ 
++#if 0
+ 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;
+@@ -1913,6 +1932,7 @@ ftpSendEPRT(Ftp::Gateway * ftpState)
+     ftpState->writeCommand(cbuf);
+     ftpState->state = Ftp::Client::SENT_EPRT;
+ }
++#endif
+ 
+ static void
+ ftpReadEPRT(Ftp::Gateway * ftpState)
+@@ -1939,10 +1959,8 @@ Ftp::Gateway::ftpAcceptDataConnection(co
+ {
+     debugs(9, 3, HERE);
+ 
+-    if (EBIT_TEST(entry->flags, ENTRY_ABORTED)) {
+-        abortTransaction("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;
+     }
+ 
+@@ -1955,6 +1973,14 @@ Ftp::Gateway::ftpAcceptDataConnection(co
+         return;
+     }
+ 
++    if (EBIT_TEST(entry->flags, ENTRY_ABORTED)) {
++        abortTransaction("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.
+@@ -2705,8 +2731,8 @@ 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 <<
++        debugs(9, 3, "avoid double-complete on FD " <<
++               (ctrl.conn != NULL ? ctrl.conn->fd : -1) << ", Data FD " << data.conn->fd <<
+                ", this " << this << ", fwd " << fwd);
+         return;
+     }
diff --git a/debian/patches/series b/debian/patches/series
index 36c0a12..cb76767 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -10,3 +10,4 @@ CVE-2016-4554.patch
 CVE-2016-4555.patch
 CVE-2016-10002.patch
 CVE-2016-10003.patch
+passive-ftp-segfault-1560429.patch