479016 - ftp fails opening PASV data connection over socks with remote dns resolution enabled

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Koen Deforche]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.5) Gecko/2008121622 Ubuntu/8.04 (hardy) Firefox/3.0.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.7pre) Gecko/2009021807 Minefield/3.0.7pre

When configuring a single SOCKS 5 proxy server (such as squid), with network.proxy.socks_remote_dns = true, firefox will make an invalid SOCKS connect request to IP address 0.0.0.0 for opening the passive data connection.

This is caused by using the transport getPeerAddr() method on the control connection's transport, which is not defined when remote dns resolution is used.

Reproducible: Always

Steps to Reproduce:
1. Configure socks v5 server as sole proxy, and network.proxy.socks_remote_dns = true
2. Go to ftp://ftp.funet.fi/pub/
Actual Results:  
The connection stalls infinitely with: Connected to ftp.funet.fi...

Expected Results:  
You should see the listing of files as when proxy is not enabled or remote_dns is false.

Comment 1

[Koen Deforche]

Attachment: Proposed patch that solves the problem.

Comment 2

[Michal Novotny [:michal]]

Comment on attachment 362861 [details] [diff] [review]
Proposed patch that solves the problem.

(In reply to comment #1)

+  if (strcmp(mServerAddress, "0.0.0.0") == 0) {

Use PR_IsNetAddrType(&addr, PR_IpAddrAny)

+  strncpy(mServerAddress, mControlConnection->Host().get(),
+          sizeof(mServerAddress));

Take host name from channel's URI just like in EstablishControlConnection() :
mChannel->URI()->GetAsciiHost(host);

-    char                    mServerAddress[64];
+    char                    mServerAddress[512];

Instead of storing host name as member I would prefer to store PRNetAddr and do the conversion just before CreateTransport() in S_pasv().


Btw. this will fix IPv4 only. mNetAddr is always initialized as PR_AF_INET in nsSocketTransport::ResolveHost() when host should be resolved by proxy. So command PASV instead of EPSV will be issued in S_pasv() and this command will be refused by the server.

Comment 3

[Koen Deforche]

Thanks for the quick feedback.

Following your suggestions, I have updated my patch.

As far as I understand, but I am not very familiar with the FTP protocol, there is not much we can do about the IPv6 case, since we have no way of finding out whether we are connected to an IPv6 host if DNS resolution is remote ?

Comment 4

[Koen Deforche]

Attachment: Updated patch based on reviewer suggestions.

Comment 5

[Michal Novotny [:michal]]

(In reply to comment #4)
> Created an attachment (id=363280) [details]
> Updated patch based on reviewer suggestions.

Please add following initialization of mServerAddress after mAddressChecked = PR_TRUE;

   PR_InitializeNetAddr(PR_IpAddrAny, 0, &mServerAddress);

mServerAddress is undefined when either do_QueryInterface(controlSocket) or GetPeerAddr(&mServerAddress) fails.

> As far as I understand, but I am not very familiar with the FTP protocol, there
> is not much we can do about the IPv6 case, since we have no way of finding out
> whether we are connected to an IPv6 host if DNS resolution is remote ?

Actually this information can be obtained from our own address, get it via GetSelfAddr().

Also please make diff against hg repository if you can.

Comment 6

[Koen Deforche]

Attachment: Updated CVS patch (v3) based on reviewer suggesetions.

Comment 7

[Koen Deforche]

Attachment: hg patch (v3), same as CVS patch (v3) applied to hg tree [Backout: Comment 16]

Comment 8

[Koen Deforche]

(In reply to comment #5)
> Please add following initialization of mServerAddress after mAddressChecked =
> PR_TRUE;
> 
>    PR_InitializeNetAddr(PR_IpAddrAny, 0, &mServerAddress);

Okay.
 
> Actually this information can be obtained from our own address, get it via
> GetSelfAddr().

Okay.
 
> Also please make diff against hg repository if you can.

Okay.

Following these suggestions, I have upload both a CVS and a hg patch.

Comment 9

[Michal Novotny [:michal]]

Comment on attachment 363858 [details] [diff] [review]
hg patch (v3), same as CVS patch (v3) applied to hg tree
[Backout: Comment 16]

Cool, this looks good to me. Although I can do review, I'm not sure I'm allowed to give "+" since I'm not peer of netwerk module. Boris, can you do it?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 363858 [details] [diff] [review]
hg patch (v3), same as CVS patch (v3) applied to hg tree
[Backout: Comment 16]

Yeah, looks ok.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 363858 [details] [diff] [review]
hg patch (v3), same as CVS patch (v3) applied to hg tree
[Backout: Comment 16]

 Still, if it is
+                     * an IPV6 host, then the external address of the
+                     * socks server should also be IPv6, and this is the
+                     * self address of the transport.


why is this comment correct?

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Oh, because our SOCKS code is smarter than I thought.

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 363858 [details] [diff] [review]
hg patch (v3), same as CVS patch (v3) applied to hg tree
[Backout: Comment 16]

passing on the original hostname will not work if there are multiple FTP servers behind a hostname, but we can't really do any better than that here :/

Comment 14

[Serge Gautherie (:sgautherie)]

koen, may I interest you in filing your real name or at least an alias in Bugzilla ?

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 363858 [details] [diff] [review]
hg patch (v3), same as CVS patch (v3) applied to hg tree
[Backout: Comment 16]


http://hg.mozilla.org/mozilla-central/rev/08b98b9b0868

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 363858 [details] [diff] [review]
hg patch (v3), same as CVS patch (v3) applied to hg tree
[Backout: Comment 16]


Backed out:
http://hg.mozilla.org/mozilla-central/rev/90165b347b0e
http://hg.mozilla.org/mozilla-central/rev/bf8152c5b1ba

See for example:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1235575760.1235576226.9797.gz
Linux mozilla-central unit test on 2009/02/25 07:29:20
/builds/moz2_slave/mozilla-central-linux-unittest/build/netwerk/protocol/ftp/src/nsFtpConnectionThread.h:289: error: field ‘mServerAddress’ has incomplete type
}

Comment 17

[Koen Deforche]

Attachment: Updated patch to solve test-build failures [Checkin: Comment 20]

Comment 18

[Koen Deforche]

Truly sorry for the silly mistake, it seems that my build configuration had tests disabled.

The new patch moves the include for "prnetdb.h" from the implementation to the header file, and now passes the tests.

Comment 19

[Matthias Versen [:Matti]]

Comment on attachment 364873 [details] [diff] [review]
Updated patch to solve test-build failures
[Checkin: Comment 20]

changing reviewer based on comment #9.

Comment 20

[Serge Gautherie (:sgautherie)]

Comment on attachment 364873 [details] [diff] [review]
Updated patch to solve test-build failures
[Checkin: Comment 20]


http://hg.mozilla.org/mozilla-central/rev/7cfb5c2285f8