FTP fails when tunneled through HTTP proxy #701

Closed
daboul opened this Issue Mar 4, 2016 · 13 comments

Projects

None yet

2 participants

@daboul
daboul commented Mar 4, 2016

Hi.
I think I've found an easy way to replicate known bug 65

"65. When doing FTP over a socks proxy or CONNECT through HTTP proxy and the
multi interface is used, libcurl will fail if the (passive) TCP connection
for the data transfer isn't more or less instant as the code does not
properly wait for the connect to be confirmed. See test case 564 for a first
shot at a test case."

Sharing it with you in case it could help. I'm not very familiar with GitHub and participating on projects, so my apologies if I should have done something else instead of posting here.

Using win32, VS compiled version of the "LIB Release - LIB OpenSSL - LIB LibSSH2" version of the 7.47.1.

FileZilla Server for the FTP server, and I've used CCproxy and FreeProxy as HTTP proxies.

A simple: .\curl.exe -T "{bla.txt,bla2.txt}" ftp://X.X.X.X -u ftpuser:xxx -v -x X.X.X.X:808 -p fails on the upload of the second file (code 425), while the same command works fine without the proxy specified.

Connected on port 21, sending welcome message...
220-FileZilla Server 0.9.56 beta
220-written by Tim Kosse (tim.kosse@filezilla-project.org)
220 Please visit https://filezilla-project.org/
USER ftpuser
331 Password required for ftpuser
PASS ******
230 Logged on
PWD
257 "/" is current directory.
EPSV
229 Entering Extended Passive Mode (|||57046|)
TYPE I
200 Type set to I
STOR bla.txt
150 Opening data channel for file upload to server of "/bla.txt"
226 Successfully transferred "/bla.txt"
EPSV
229 Entering Extended Passive Mode (|||51815|)
STOR bla2.txt
425 Can't open data connection for transfer of "/bla2.txt"
QUIT
221 Goodbye
disconnected.

Thanks,
daboul

@bagder
Member
bagder commented Mar 4, 2016

Although your command line doesn't use a socks proxy at all...

@daboul
daboul commented Mar 5, 2016

I've read the "or CONNECT through HTTP proxy" but you're right, I don't have a CONNECT here. Then it's something else, but fairly easy to replicate.

@bagder bagder changed the title from known bug 65 replicated to FTP fails when tunneled through HTTP proxy Mar 6, 2016
@bagder bagder added the FTP label Mar 6, 2016
@daboul
daboul commented Mar 7, 2016

Hi. I'm making some progress here, after the second EPSV, when we are about to transfer the second file, libcurl doesn't do a CONNECT on the new port indicated by the FTP server (only if there is a proxy), that's why we end up getting a "425 Can't open data connection for transfer of "/bla2.txt" I'm trying to debug, but I'm new to libcurl code. I'll keep updating the thread with my findings, hopefully I'll get a fix at some point. Thanks.

@daboul
daboul commented Mar 7, 2016

So, I've followed the code, what happens here is that we're about to upload the second file via the proxy, we enter

Curl_proxyCONNECT

trying to open the new passive port but

conn->tunnel_state[sockindex] == TUNNEL_COMPLETE

is true for sockindex==SECONDARYSOCKET cause it has already been opened for the first upload, but it was for a data channel on another port. So we don't do a CONNECT on the new port and the upload can't happen. Not sure I'll be able to come up with a fix though, I don't know libcurl well enough.

I guess the second socket data should have been closed after the first STOR, but I'm not sure how to do that, would anyone have any idea how to do that properly?

Thanks,
daboul

@bagder
Member
bagder commented Mar 7, 2016

something like this perhaps?

diff --git a/lib/ftp.c b/lib/ftp.c
index b5a7f55..78810bd 100644
--- a/lib/ftp.c
+++ b/lib/ftp.c
@@ -4445,10 +4445,11 @@ static CURLcode ftp_dophase_done(struct connectdata *conn,
     if(result) {
       if(conn->sock[SECONDARYSOCKET] != CURL_SOCKET_BAD) {
         /* close the second socket if it was created already */
         Curl_closesocket(conn, conn->sock[SECONDARYSOCKET]);
         conn->sock[SECONDARYSOCKET] = CURL_SOCKET_BAD;
+        conn->tunnel_state[SECONDARYSOCKET] = TUNNEL_INIT;
       }
       return result;
     }
   }

@daboul
daboul commented Mar 7, 2016

Thanks! I'll give that a try right now.

@daboul
daboul commented Mar 7, 2016

It's fixed. Thank you very much.

@daboul
daboul commented Mar 7, 2016

Doing further tests, I still have issues related to that. Let me come back to you.

@daboul
daboul commented Mar 7, 2016

static CURLcode ftp_dophase_done(struct connectdata *conn,
bool connected)
{
struct FTP *ftp = conn->data->req.protop;
struct ftp_conn *ftpc = &conn->proto.ftpc;

if(connected) {

connected is FALSE in my case, coming from ftp_doing:

result = ftp_dophase_done(conn, FALSE /* not connected */);

So the fix isn't called. Not sure I understand the meaning of connected here.

Sorry for the confusion, I had a tweak inserted in the code and got confused.

@bagder
Member
bagder commented Mar 7, 2016

Right, it wasn't the correct spot. Or perhaps it should be there but not for your case. The ftp_done function is actually what gets called after a completed transfer so I think perhaps this is a better idea:

diff --git a/lib/ftp.c b/lib/ftp.c
index b5a7f55..cd4f45e 100644
--- a/lib/ftp.c
+++ b/lib/ftp.c
@@ -3362,10 +3362,11 @@ static CURLcode ftp_done(struct connectdata *conn, CURLcode status,
     }
     if(CURL_SOCKET_BAD != conn->sock[SECONDARYSOCKET]) {
       Curl_closesocket(conn, conn->sock[SECONDARYSOCKET]);
       conn->sock[SECONDARYSOCKET] = CURL_SOCKET_BAD;
       conn->bits.tcpconnect[SECONDARYSOCKET] = FALSE;
+      conn->tunnel_state[SECONDARYSOCKET] = TUNNEL_INIT;
     }
   }

   if(!result && (ftp->transfer == FTPTRANSFER_BODY) && ftpc->ctl_valid &&
      pp->pending_resp && !premature) {
@daboul
daboul commented Mar 8, 2016

It worked fine this time. You saved my day.

First time I work on such a project, I should be able to learn more and more about libcurl in the coming months, and I'd like to contribute to the project. I'll be using extensively SFTP/HTTPS over proxies in secure mode (TLS, certificates) and should be able to test quite a few features.

Thanks again for your tremendous work and help on this.

@bagder
Member
bagder commented Mar 8, 2016

Lovely. I intend to make the real fix slightly larger though, so that we make sure to clear that tunnel state variable at every place we close the second connection so that this problem won't come back too easily.

@bagder bagder added a commit that closed this issue Mar 8, 2016
@bagder bagder ftp_done: clear tunnel_state when secondary socket closes
Introducing a function for closing the secondary connection to make this
bug less likely to happen again.

Reported-by: daboul
Closes #701
cb222bc
@bagder bagder closed this in cb222bc Mar 8, 2016
@daboul
daboul commented Mar 8, 2016

I'll test that code as well, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment