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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Networking: FTP
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Koen Deforche, Assigned: Koen Deforche)

Tracking

Trunk
mozilla1.9.2a1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
Created attachment 362861 [details] [diff] [review]
Proposed patch that solves the problem.
Status: UNCONFIRMED → NEW
Component: General → Networking: FTP
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → networking.ftp
Attachment #362861 - Flags: review?(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.
Attachment #362861 - Flags: review?(michal) → review-
(Assignee)

Comment 3

10 years ago
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 ?
(Assignee)

Comment 4

10 years ago
Created attachment 363280 [details] [diff] [review]
Updated patch based on reviewer suggestions.
(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.
(Assignee)

Comment 6

10 years ago
Created attachment 363857 [details] [diff] [review]
Updated CVS patch (v3) based on reviewer suggesetions.
Attachment #362861 - Attachment is obsolete: true
Attachment #363280 - Attachment is obsolete: true
(Assignee)

Comment 7

10 years ago
Created attachment 363858 [details] [diff] [review]
hg patch (v3), same as CVS patch (v3) applied to hg tree
[Backout: Comment 16]
(Assignee)

Comment 8

10 years ago
(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.
(Assignee)

Updated

10 years ago
Attachment #363857 - Flags: review?(michal)
(Assignee)

Updated

10 years ago
Attachment #363858 - Flags: review?(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?
Attachment #363858 - Flags: review?(michal) → review?(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 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?
Attachment #363857 - Flags: review?(michal)
Oh, because our SOCKS code is smarter than I thought.
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 :/
Attachment #363858 - Flags: superreview+
Keywords: checkin-needed
koen, may I interest you in filing your real name or at least an alias in Bugzilla ?
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
Attachment #363858 - Attachment description: hg patch (v3), same as CVS patch (v3) applied to hg tree → hg patch (v3), same as CVS patch (v3) applied to hg tree [Backout: Comment 16]
Attachment #363858 - Attachment is obsolete: true
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
}
Assignee: nobody → koen
Status: NEW → ASSIGNED
Keywords: checkin-needed
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
Attachment #363857 - Attachment is obsolete: true
(Assignee)

Comment 17

9 years ago
Created attachment 364873 [details] [diff] [review]
Updated patch to solve test-build failures
[Checkin: Comment 20]
Attachment #364873 - Flags: review?(michal)
(Assignee)

Comment 18

9 years ago
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.
Attachment #364873 - Flags: review?(michal) → review?(bzbarsky)
Comment on attachment 364873 [details] [diff] [review]
Updated patch to solve test-build failures
[Checkin: Comment 20]

changing reviewer based on comment #9.
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
Attachment #364873 - Attachment description: Updated patch to solve test-build failures → Updated patch to solve test-build failures [Checkin: Comment 20]
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.