134105 - SOCKS5: DNS lookups (host resolving) should occur on proxy, not client side.

Status: RESOLVED INCOMPLETE

(Core :: Networking, enhancement)

Description

[f_x]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.9+)
Gecko/20020328
BuildID:    20020328

i have entries for http,ssl,ftp and socks5 proxy. If i open a webpage(same for
ssl,ftp) the http proxy resolves the hostname on the proxy machine. If i use
Chatzilla the hostname was resolved on the client machine. In a enviroment where
the client has no access to an external dns server, the hostname must be
resolved on the proxy machine. The socks5 protocol supports this topology.
Socks4 needs a dns resolver on the client side. I have choosen socks5 in the
proxy preferences but he works like a socks4 proxy(the dns part).

Reproducible: Always
Steps to Reproduce:
1. choose a socks5 proxy under proxy preferences
2. use chatzilla with an internet irc server
3. chatzilla/mozilla tries to resolve the hostnames on the client side

Actual Results:  i cannot connect to the irc servers, because of the client
machine has no access to an external dns server.

Expected Results:  mozilla should pass the hostname to the socks5 proxy server
and then the socks5 proxy should resolve the hostname.

from the socks5 faq from the socks5-v1.0r11.tar.gz (www.socks.nec.com) package.

What is SOCKS5 different from SOCKS4?
 3. SOCKS4 clients require full support of DNS
    while SOCKS5 clients can rely on SOCKS5 server
    to perform the DNS lookup.

How does SOCKS interact with DNS?
 In a SOCKS4 environment, SOCKS clients are required
 to be able to resolve IP address of remote hosts no
 matter whether they are local hosts or internet
 hosts. Therefore DNS must be configured in such a
 way that SOCKS clients' resolver is able to do so.
 Special arrangement needs to be made when more than
 one DNS servers are being used (such as dual DNS
 environment).
 In a SOCKS5 environment, the above requirement is
 no longer necessary. SOCKS clients can passing the
 un-resolvable host names to SOCKS servers and the
 servers will try to resolve those names. As a
 result, so long as one of the resolvers used by
 either SOCKS clients or SOCKS servers is able to
 resolve a given host, SOCKS will work OK.

Comment 1

[benc]

NEW

I sort of backwards confirmed this by setting my browser to use SOCKS 5, ran a
packet trace on SOCKS 5 server, and entered a non-existent hostname on my browser.

No connection occurred.

(Did this backwards because while trying to do a straight up trace of a SOCKS 5
connection, found that SOCKS library might go deaf if you activate/deactivate
SOCKS in a single browser session). 

Comment 2

[benc]

Temporarily "futuring" all PAC&SOCKS bugs to clear new-networking queue.

I will review later. (I promise)

If you object, and can make a case for a mozilla 1.0 fix, please reset milestone
to  "--" or email me.

Comment 3

[Bradley Baetz (:bbaetz)]

*** Bug 161154 has been marked as a duplicate of this bug. ***

Comment 4

[Justin Bradford]

I think this can be done pretty easily, but I need to discuss my approach with
nspr and netwerk people first. Also, how do we want to handle configuration? An
app-wide switch (resolve locally or resolve remotely) or on a per-SOCKS server
basis? I believe the later will be quite annoying, but I haven't looked at it
much, yet.

Also, there is an extension to SOCKS 4 (called SOCKS 4a) which allows one to do
the same thing. We could open another bug for it, but since the vast majority of
the work for the feature is common to both 4 and 5, I think we should make this
bug about both.

Comment 5

[Justin Bradford]

-> [e-mail]

Comment 6

[benc]

Things have a tendency to not always go forward logically, so anything 
that is distinct should have it's own bug. Please file a bug on SOCKS4a, 
and put any links or spec information you might have in the bug.

Comment 7

[Justin Bradford]

Attachment: full implementation for v5 and v4a

This patch implements the hostname connection method for SOCKS 4 and 5 and is
configurable via a new checkbox in the proxy preferences. I've tested
everything on a couple different SOCKS servers, and it's all working fine.

Comment 8

[Justin Bradford]

This patch should be ready to go, so can anyone review it?

Comment 9

[Darin Fisher]

justin:

i haven't fully reviewed the patch yet, but one nit: please declare a const
unsigned long value for the flag value, (possibly) like so:

interface nsIProxyInfo : nsISupports
{
  ...

  const unsigned long TRANSPARENT_PROXY_RESOLVES_HOST = 1 << 0;
};

Comment 10

[Sami Lehtinen]

I tried this patch, and it works for me. (had to apply a couple of chunks by
hand, though, because the tree has changed some after this patch)

I tried with a "socks server" on localhost (ssh2 client) and now I could connect
an internal network (from the Internet, behind the firewall).

Thanks!

Now if I only could configure mozilla to use a SOCKS server based on an address
match... (inclusion, not exclusion like now)

Comment 11

[Justin Bradford]

Attachment: updated patch

This new patch should apply cleanly now, and it uses a named constant for the
flag  (as requested in comment #9).

Comment 12

[Justin Bradford]

updating target

Comment 13

[Darin Fisher]

Comment on attachment 106288 [details] [diff] [review]
updated patch

patch looks good overall; just some nits...


>Index: netwerk/base/src/nsSocketTransportService.cpp

>+    // ...but only if the address we were sent is good...
>+    if (PR_IsNetAddrType(addr, PR_IpAddrAny)) return;

nit: insert newline before |return| statement.


>Index: netwerk/socket/base/nsSOCKSIOLayer.cpp

>+nsSOCKSSocketInfo::GetDestinationHost(char * *aDestinationHost)
>+{
>+    if (!aDestinationHost) return NS_ERROR_NULL_POINTER;

use NS_ENSURE_ARG_POINTER(aDestinationHost) instead.

...
>+        return (*aDestinationHost == nsnull) ? NS_ERROR_OUT_OF_MEMORY : NS_OK;
>+    }
>+    else

else after return is meaningless.

>+nsSOCKSSocketInfo::SetDestinationHost(const char * aDestinationHost)
...
>+        return (mDestinationHost == nsnull) ? NS_ERROR_OUT_OF_MEMORY : NS_OK;
>+    }
>+    else

again, else after return is meaningless.


>+nsSOCKSSocketInfo::GetDestinationPort(PRInt32 *aDestinationPort)
>+{

how about adding NS_ENSURE_ARG_POINTER(aDestinationPort) here as well?	or
maybe remove it from above.

>+    // get destination port
>+    PRInt32 destPort = PR_ntohs(PR_NetAddrInetPort(addr));
>+
>+    if (PR_IsNetAddrType(addr, PR_IpAddrAny)) {

>+        char * destHost;
>+        info->GetDestinationHost(&destHost);

use nsXPIDLCString instead of raw char pointer.

	  nsXPIDLCString destHost;
	  info->GetDestinationHost(getter_Copies(destHost));

that way you don't have to worry about freeing the string later.

>+        unsigned int host_len = strlen(destHost);

if you use nsXPIDLCString, then you don't have to compute the length of the
string again.  you can just use destHost.Length().

>+        char * destHost;
>+        info->GetDestinationHost(&destHost);

same nsXPIDLCString business here.

>+        unsigned int host_len = strlen(destHost);

destHost.Length()

>+        if (PR_IsNetAddrType(addr, PR_IpAddrV4Mapped)) {
>+            // store the ip
>+            memcpy(request+4, (char*)(&addr->ipv6.ip.pr_s6_addr[12]), 4);

i wish NSPR exposed a function or macro for extracting the IPv4 address
out of a IPv4-mapped IPv6 address.

>Index: netwerk/util/nsNetUtil.h

thanks for fixing up these files, but they aren't part of the build.  they'll
most likely be cvs removed someday soon ;)

can you attach a screenshot of the pref UI changes?  we'll need to get someone
from the UI team to approve the changes.

Comment 14

[Justin Bradford]

Attachment: patch v3

A new patch addressing Darin's review comments.
I did not do anything regarding the IPv6 to IPv4 conversion, though.

Comment 15

[Justin Bradford]

Attachment: screenshot of the new proxy pref panel

Comment 16

[Darin Fisher]

Comment on attachment 107268 [details] [diff] [review]
patch v3

it seems like it would be nice if there were an "advanced" SOCKS prefs dialog.
can you find someone on the UE team to approve these XUL changes?

r=darin

Comment 17

[Justin Bradford]

Darin, by UE do you mean someone from "Browser/Composer Front End QA", such as
Sarah Liberman?

Also, what do you think about proceeding with super-review and then just
committing the back-end parts until the interface bit is settled?

Comment 18

[Darin Fisher]

justin: i sent mail to [e-mail] for UI review.  let's see what he
has to say.  it sounds like a great idea to separate this patch into two parts.
 the backend is ready, and we could get it in quickly.

Comment 19

[jag (Peter Annema)]

I'm okay with this UI change.

Comment 20

[Justin Bradford]

Comment on attachment 107268 [details] [diff] [review]
patch v3

Since the UI bit was ok'ed, I'll just get superreview on the current patch,
rather than breaking it into backend and UI pieces. Ok?

Comment 21

[Darin Fisher]

justin: ok

Comment 22

[[e-mail] (gone - use [e-mail] instead)]

Justin, might it be more efficient to target this at a specific superreviewer? 
It seems nobody besides Darin has looked at the patch yet ;-(

Comment 23

[Brian Ryner (not reading)]

Comment on attachment 107268 [details] [diff] [review]
patch v3

Looks ok.  I do have one concern though.  Why is a separate checkbox needed in
prefs?	Can't this always be enabled for SOCKS5 and disabled for SOCKS4, or is
that a bad idea for some reason?

Comment 24

[Sami Lehtinen]

Ping.

Is this fix going forward, and if not, why?

I would imagine there is more than 2 people in the world needing this.

The latest patch no longer applies against the current tree (whole files have
been renamed/removed).

Is there anything that for example I could do to help speed this up?

Comment 25

[Darin Fisher]

Sami: it shouldn't take that much work to merge this with the trunk, but someone
has to own this.  i definitely don't have the time myself.  justin?

Comment 26

[Justin Bradford]

brian: on for SOCKS5, off for SOCKS4 is probably an ok default (but would change
the default behavior). However, there are almost certainly exceptions. People
will want it to be configurable. Its ok with me if the feature is not accessible
from the GUI, but since I don't use SOCKS, I'm not really the person to ask.

As for the patch in general, I'll get a new version posted soon. I've been busy
with other things lately. Once I post a new patch, will it need to be
re-reviewed? Or can I just land the updated version?

Comment 27

[Howard Chang]

a) to the best of my knowledge, all SOCKS servers that came out within the last 
2 years support both SOCKS4 and SOCKS5.  Not certain if SOCKS4 support is even 
necessary in Mozilla.  
b) According to SOCKS5 spec, DNS lookup can be done in local or remote.  a 
select box that chooses either local DNS resolution or remote DNS resolution, 
in addition to a menu that one can use to select which domain to perform 
local/remote lookup would be extremely helpful.  (given that most large 
corporation have seperate internal/external DNS)
  
  hope this makes it into the main branch soon.  

Comment 28

[Darin Fisher]

justin: depends on how much the patch changes ;-)

Comment 29

[benc]

I don't know if this needs to be a pref. The hackish mapping of just socket
connectivity in SOCKS4 w/o supporting DNS as part of the application->network
connection process was considered a huge weakness of SOCKS4.

If you are using SOCKS 5, you want DNS delegated to the proxy server. I think I
saw one person asking for proxy client to do DNS resolution and send only IP
requests, but that person did not make a good technical case (+ there might be
ways this behavior would really confuse the HTTP proxy).

My suggestion is to make DNS delegation work for SOCKS5 with a hidden pref, but
not make a UI change. If people start complaining that they don't like editing
user.js, then get a new bug and start considering it. We have several other more
pressing proxy pref UI improvements that should be done first (like bug 72444
and bug 50380).

The whole UI is pretty overloaded, I don't know how much new functionality we
can bolt on before it becomes completely senseless.

Comment 30

[Hao Huang]

Has this bug been fixed?  It is still assigned and no words since 9 months ago.

Comment 31

[Hao Huang]

This is a joke.  It has remained assigned for probably over a year with no update

Comment 32

[Matthias Versen [:Matti]]

assign it to you and fiy it yourself !

Comment 33

[wr0ngway]

Was anything ever resolved for this?
I find using the socks server built into ssh makes for a nice VPN alternative,
but its painfull to use without remote dns resolution

Comment 34

[f_x]

The bug was resolved with Mozilla 1.1 and reoccured with 1.5 :(

Comment 35

[Malcolm Smith]

(In reply to comment #32)
> assign it to you and fiy it yourself !

I'm not allowed to assign it to myself.

I was working on a fix for this last night, which was only half successful. 
I'll integrate my changes with Justin's to create a more current update.

Out of interest, why was Justin's patch not accepted into trunk?

Comment 36

[Malcolm Smith]

Attachment: Patch v4

This patch updates Justin's patch to work with Mozilla 1.4.1, the version which
I'm currently using.  I'll try to update it to work with 1.7 when that's
released, but don't have the bandwidth to use CVS :\

At the moment, it doesn't seem to automatically recognise changes made to the
setting without a restart or some form of timeout.  If anybody knows what
causes this, let me know.

Comment 37

[Malcolm Smith]

Attachment: Patch v5 (moz 1.8a1)

This is the patch updated for 1.8 alpha1.

I'd really like to get this reviewed and into trunk soon.

There is one thing in it that I'm not happy about; in nsSocketTransport2.cpp,
mNetAddr is initialised as an AF_INET; this is to overcome problems when it's
INET6.	When initialised to inet6, NSPR adds the layer to send IPv4 over v6,
which fails if the DNS resolution of the host in nsSOCKSIOLayer.cpp is for
IPv4.  The type of the connection in nsSOCKSIOLayer.cpp is dependent upon how
it was initialised in nsSocketTransport2.cpp, but that doesn't know about what
it will actually be connecting to.  In 1.4.1, this was not a problem.

(ie: the current implementation would likely fail connecting to an IPv6 SOCKS
server.)

If anybody knows this code better than me and wants to look at how it should be
done, please do.

Comment 38

[timeless]

Comment on attachment 149807 [details] [diff] [review]
Patch v5 (moz 1.8a1)

i'm not really reading this code, but it looks like you're caching a pref w/o
setting up a way to be informed when the pref is changed. this is bad form, and
since i'm the one likely to have to clean it up later, i'd rather flag it now.

>              LOGERROR(("IPv6 not supported in SOCK 4."));

someone should fix that to 'is not' and 'SOCKS' ...

Comment 39

[Bradley Baetz (:bbaetz)]

What timeless said.

Also, what if someone does try to go to http://0.0.0.0/ ?

Comment 40

[Malcolm Smith]

Attachment: Patch v6 [politically correct bloat]

ok,

This patch is in response to the feedback posted here.

Major Changes:
 - Does handle 0.0.0.0 correctly.  This involved updating the nsISocketProvider
interface, and all of its dependencies, including code in security and
directory.
 - Fixes 'SOCK 4'
 - Updates help to include documentation about the updated UI
 - Removed stale SOCKS4 code from netwerk/socket/base that is no longer in the
build

Major issues:
 - gtk2 build is broken, not sure if this patch is responsible for it.	I'll
have to test an identical build without patch and see.	I can't see how this
patch could break it, though.
 - That AF_INET kludge.  Help appreciated.
 - Caching of preferences.  This new setting appears to be handled the same as
all other SOCKS settings.  If the pref is changed, and you browse to a
*different* webserver, it picks up the new configuration; browsing to the
*same* webserver uses the old configuration.  I can't see a good way around
this; having nsSocketTransport depend on the contents of the proxyInfo variable
seems like a bad idea - it might get freed or destroyed, and if
nsSocketTransport references it, might need to free or destroy it when it's
destroyed...

Comment 41

[Malcolm Smith]

(In reply to comment #40)
>  - gtk2 build is broken, not sure if this patch is responsible for it.
	I'll
> have to test an identical build without patch and see.	I can't see how 
this
> patch could break it, though.

Confirmed that this patch didn't barf gtk2.  It barfs all by itself on my setup.

Comment 42

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

timeless: this is already observing the preference. an observer exists for
network.proxy.*, and the newly added code is in the observer.

some comments on the patch...

firstly, thanks for updating the help

+        nsXPIDLCString destHost;
+        destHost = info->DestinationHost();

this should be something like:
   const nsCString& destHost = info->DestinationHost();
to avoid the copying. in addition, nsXPIDLCString is really only meant to be
used together with |out string| parameters in XPCOM interfaces.

hm... shouldn't you, around nsSocketTransport2.cpp line 958, also reset proxyFlags?

>If the pref is changed, and you browse to a
>*different* webserver, it picks up the new configuration; browsing to the
>*same* webserver uses the old configuration. 

really? why is that? doesn't a new proxyinfo get created each time a new channel
is created? http-keepalive might be an issue here, but not really for this pref,
since you don't need to re-lookup the http name if you already have a connection



can you, in the future, use the -p flag when creating patches, so it's easy to
see which function the changes are in?

Comment 43

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

(In reply to comment #40)
>  - Caching of preferences.  This new setting appears to be handled the same as
> all other SOCKS settings.  If the pref is changed, and you browse to a
> *different* webserver, it picks up the new configuration; browsing to the
> *same* webserver uses the old configuration.

see bug 233811, esp. bug 233811 comment 7

Comment 44

[timeless]

Comment on attachment 149980 [details] [diff] [review]
Patch v6 [politically correct bloat]

i don't suppose i could get you to consider using cvs diff (in the future). it
helps when patches get old or files change and you need to do merges.

Comment 45

[Malcolm Smith]

Attachment: the guilt-free panacea

> firstly, thanks for updating the help

Updated again for your aesthetic convenience :)

> +	   nsXPIDLCString destHost;
> +	   destHost = info->DestinationHost();

> this should be something like:
>    const nsCString& destHost = info->DestinationHost();

Done.

> hm... shouldn't you, around nsSocketTransport2.cpp line 958, also reset
> proxyFlags?

Done, although this means that the proxyFlags must be set individually for each
layer in the stack, so it also has to be initialised only for a socks/socks4
layer.

>> If the pref is changed, and you browse to a
>> *different* webserver, it picks up the new configuration; browsing to the
>> *same* webserver uses the old configuration. 

> really? why is that?

I was hoping you'd know :)

> can you, in the future, use the -p flag when creating patches, so it's
> easy to see which function the changes are in?

Done.

> i don't suppose i could get you to consider using cvs diff (in the
> future). it helps when patches get old or files change and you need to
> do merges.

I know; the problem is I don't have a huge amount of bandwidth to play with. 
I've done a cvs diff this time, while I'm in CA soaking up the sun and network
capacity, but I won't be able to do things like this forever...

Comment 46

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

Comment on attachment 152073 [details] [diff] [review]
the guilt-free panacea

If enabled, you no longer need DNS
+	    access to your browser.

how about: "Your browser no longer needs DNS access"

hmm, 3 versions of this help file? *sigh*

ah, this patch does already not apply anymore. unfortunate timing.

I'll review this tomorrow...

Comment 47

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

Comment on attachment 152073 [details] [diff] [review]
the guilt-free panacea

hrm, it looks like this patch does not address PAC-configured SOCKS proxies. it
probably should. see the new function BuildProxyList.

+    , mProxyTransparentResolvesHost(PR_FALSE)

hmm, this is a bit of a bad name in my opinion. how about mProxyResolvesHost?
or maybe mTransparentProxyResolvesHost

I prefer mProxyResolvesHost, and the flag may be better called
PROXY_RESOLVES_HOST, too...

more tomorrow. note to self:
Index: netwerk/socket/base/nsSOCKSIOLayer.cpp

Comment 48

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

Attachment: updated to trunk

this is attachment 152073 [details] [diff] [review] updated to trunk so that it compiles... I'm not sure
how:
+	 strncpy((char*) request+5, destHost, host_len);
ever compiled; I added a .get() to destHost.

Comment 49

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

Comment on attachment 152073 [details] [diff] [review]
the guilt-free panacea

+    if (info->Flags() && nsISocketProvider::PROXY_RESOLVES_HOST) {

surely &, not &&?

Comment 50

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

/home/chb/clean-mozilla/mozilla/directory/xpcom/base/src/nsLDAPSecurityGlue.cpp:215:
error: no
   matching function for call to `nsDerivedSafe<nsISSLSocketProvider>::
   AddToSocket(int, char*&, int&, int, int, PRFileDesc*&,
   nsGetterAddRefs<nsISupports>)'

Comment 51

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

(In reply to comment #47)
> I prefer mProxyResolvesHost, and the flag may be better called
> PROXY_RESOLVES_HOST, too...

Hmmm. PROXY_RESOLVES_HOST would match HTTP Proxies as well. So the current name
probably is better.

Comment 52

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

Comment on attachment 152073 [details] [diff] [review]
the guilt-free panacea

+    void Init(PRInt32 version, const char *proxyHost, PRInt32 proxyPort,
+	 const char *destinationHost, PRInt32 destinationPort, PRInt32 flags);

hm... why do you need tge destinationPort in here? Doesn't the one suffice that
you get in the PRNetAddr, later on?

..which you do seem to be using:
+    PRInt32 destPort = PR_ntohs(PR_NetAddrInetPort(addr));

+	 destPort = info->DestinationPort();
is there ever a case where info->DestinationPort() is not equal to
PR_ntohs(PR_NetAddrInetPort(addr))?

+	     return NS_ERROR_FAILURE;	// Hostname was too big for buffer.
well, reasonable error codes are not really a strength of this file...

+	 strncpy((char*) request+5, destHost, host_len);
why cast to char*? in fact, why not just use memcpy, since you don't need any
fancy string handling?

+    // add the destination port to the request    

no need for trailing whitespace :)

+    if (info->Flags() && nsISocketProvider::PROXY_RESOLVES_HOST) {

hmm, second time where an && should be &

+	 strncpy((char*) request+request_len, destHost, host_len);

since you want a terminating zero here, and already checked the string length,
why not just use "strcpy"? or strncpy with host_len+1. or memcpy with host_len
+ 1 :)
(then you don't need the explicit set of the terminating null)

+	 // store the ip
+	 memcpy(request+4, (char*)(&addr->inet.ip), 4);

why are you using memcpy here, and explicit assignments in the SOCKS 5 case?

+    // store the port
+    request[2] = (unsigned char)(destPort >> 8);
+    request[3] = (unsigned char)destPort;

I'd prefer if this was moved back after the set of [0] and [1]...

+	   <row>
+	     <spacer/>
+	     <checkbox id="networkProxySOCKSRemoteDNS" 
+		       label="&socksRemoteDNS.label;"
+		       accesskey="&socksRemoteDNS.accesskey;"
+		       prefstring="network.proxy.socks_remote_dns"/>
+	   </row>

hmm. with this change, this pref panel does not fit into the pref window :( (at
least on my system)

that's not good. unfortunately I can't think of a way to rearrange things in
this panel... hm...

so, r=me with the above changes; but please attach a new patch. darin should sr
the patch.


(socks4 protocol descriptions seem to be at:
http://www.smartftp.com/Products/SmartFTP/RFC/socks4.protocol
and 4a:
http://www.smartftp.com/Products/SmartFTP/RFC/socks4a.protocol)

Comment 53

[Malcolm Smith]

Attachment: glossy polish

> how about: "Your browser no longer needs DNS access"

Done.

> hmm, 3 versions of this help file? *sigh*

Hey, there was four...

> hrm, it looks like this patch does not address PAC-configured
> SOCKS proxies. it probably should. see the new function BuildProxyList.

I've looked through that code before and still can't figure it out.
I'll have another look and look at what's in .pac files...but this
patch would involve extending that format - isn't it some kind of
standard?

> I'm not sure how:
> +	 strncpy((char*) request+5, destHost, host_len);
> ever compiled; I added a .get() to destHost.

It didn't - I must have submitted a slightly older than current patch
*blushes.*  There was one LOG line that also needed a .get(), that's
in this patch.

> +    if (info->Flags() && nsISocketProvider::PROXY_RESOLVES_HOST) {
> surely &, not &&?

Done - that was stupid *blushes again*.

>
/home/chb/clean-mozilla/mozilla/directory/xpcom/base/src/nsLDAPSecurityGlue.cpp:215:

> error: no
>    matching function for call to `nsDerivedSafe<nsISSLSocketProvider>::
>    AddToSocket(int, char*&, int&, int, int, PRFileDesc*&,
>    nsGetterAddRefs<nsISupports>)'

Done - this was in earlier versions of this patch for a few revs,
have no idea how it got dropped.

> hm... why do you need tge destinationPort in here? Doesn't the one suffice
that
> you get in the PRNetAddr, later on?

Done.  Although, in previous versions of this patch, the PRNetAddr port would
always be zero if this option was enabled, this behaviour has been changed.

> +	     return NS_ERROR_FAILURE;	// Hostname was too big for buffer.
> well, reasonable error codes are not really a strength of this file...

Done - I changed this to NS_ERROR_INVALID_ARG, which is closer, but not
really perfect.  Out of memory?  There doesn't seem to be a good error for
this.  And there's still a *lot* of other NS_ERROR_FAILURE conditions in
this function.

> +	 strncpy((char*) request+5, destHost, host_len);
> why cast to char*? in fact, why not just use memcpy, since you don't need any

> fancy string handling?

Done - I think the (char *) was there to avoid a warning from strncpy.

> +    // add the destination port to the request    
> no need for trailing whitespace :)

Done.

> +    if (info->Flags() && nsISocketProvider::PROXY_RESOLVES_HOST) {
> hmm, second time where an && should be &

Done.

> +	 strncpy((char*) request+request_len, destHost, host_len);
> since you want a terminating zero here, and already checked the string
length,
> why not just use "strcpy"? or strncpy with host_len+1. or memcpy with
host_len
> + 1 :) (then you don't need the explicit set of the terminating null)

Done - using strncpy with host_len + 1.  I'm just too nervy to completely trust

things like this, especially strcpy.  memcpy might be ok.

> +	 memcpy(request+4, (char*)(&addr->inet.ip), 4);
> why are you using memcpy here, and explicit assignments in the SOCKS 5 case?

Done - changed SOCKS4 to explicit assignments.	In answer, that's just the way
the code has always been, I didn't change anything, just adopted what was
there.
I'm in two minds here though, because memcpy could give better performance than

byte-by-byte assignments, especially on IPv6.  Maybe memcpy should be used
instead...

> +    // store the port
> +    request[2] = (unsigned char)(destPort >> 8);
> +    request[3] = (unsigned char)destPort;
> I'd prefer if this was moved back after the set of [0] and [1]...

Done - it was like this because destPort may come from different places
depending on the if, but now ports are handled the same in either case.

> hmm. with this change, this pref panel does not fit into the pref window :(
(at
> least on my system)

Hrm - I haven't changed this code since Justin was working on it, and it fits
on both my dev machines (Linux+Mac.)  What would make it fit on some and not
others...?

Comment 54

[Malcolm Smith]

Attachment: negative astrology

>> hmm, 3 versions of this help file? *sigh*

> Hey, there was four...

And now there are two.	So in the last two days, my patch broke due to its
removal and reformatting of another file.  So here is an update to fix help.

Note: check out the spaces/tabs formatting in nav_help.xhtml.  It's a classic.

Comment 55

[Malcolm Smith]

(In reply to comment #53)

> > hrm, it looks like this patch does not address PAC-configured
> > SOCKS proxies. it probably should. see the new function BuildProxyList.
> 
> I've looked through that code before and still can't figure it out.
> I'll have another look and look at what's in .pac files...but this
> patch would involve extending that format - isn't it some kind of
> standard?

Confirmed.

I found this spec file:
http://wp.netscape.com/eng/mozilla/2.0/relnotes/demo/proxy-live.html

I don't see how this patch can be represented at all within this format, and
there are several parts that make me wonder whether this patch is compatible
with PAC-as-we-know-it.

With this patch, there should *never* be an isInNet() in a PAC file, because
that requires DNS resolution, and this patch is to eliminate exactly that. 
isResolvable() could be used on good PAC scripts to remove the requirement to
resolve.  But having done that, there needs to be a format to represent a SOCKS
(with server-side nameresolution) and SOCKS (with client-side nameresolution.)

Currently, the only return values I can find are either "DIRECT" or "PROXY
host:port" (or lists thereof.)  This patch would require an extension to this
return value.

I do not believe that it is worth waiting to land this patch based on a
significant modification to the PAC file format.  This should be handled in a
seperate bug, and see if there is a demand for this.  On sites that are
currently using PAC, I imagine most will have DNS access (given the nature of
the scripts and how they are used.)

This is the *only* way our local proxy.pac is used from where I'm writing this:
http://www.cs.rmit.edu.au/proxy.pac .

So could somebody please mark the previous patch for review please?

Comment 56

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

Comment on attachment 154330 [details] [diff] [review]
negative astrology

hmm, PAC in mozilla already supports SOCKS and SOCKS5 (or was it SOCKS4?)
instead of PROXY in pac files. isInNet / isResolvable are a problem of course,
but we can ignore that, I think... I was just thinking that these SOCKS entries
from PAC should respect this pref as well.


can you not request reviews yourself?

Comment 57

[Malcolm Smith]

(In reply to comment #56)
> (From update of attachment 154330 [details] [diff] [review])
> hmm, PAC in mozilla already supports SOCKS and SOCKS5 (or was it SOCKS4?)
> instead of PROXY in pac files. isInNet / isResolvable are a problem of course,
> but we can ignore that, I think... I was just thinking that these SOCKS entries
> from PAC should respect this pref as well.

If we do ignore it, we'll still need to extend this format somehow, ie:
return "SOCKS(servernameres) sockshost.com:1080"

(I don't mean this is how it *should* be done, but that if PACs can do it, it
has to be specified somehow...suggestions welcome.)

> can you not request reviews yourself?

Wasn't aware that I could - I just found how though, in the 'Edit' link (this is
my first Moz patch, I'm a bugzilla n00b.)

Comment 58

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

Comment on attachment 154330 [details] [diff] [review]
negative astrology

>(I don't mean this is how it *should* be done, but that if PACs can do it, it
>has to be specified somehow...suggestions welcome.)

well, could PAC not just respect this preference?

modules/libpref/src/init/all.js
+pref("network.proxy.socks_remote_dns",      false);
would true be a better default value?

netwerk/base/public/nsNetUtil.h
+		 PRUint16	flags,
wrong indentation

netwerk/base/src/nsSocketTransport2.cpp
+	     if ((mProxyTransparentResolvesHost == PR_TRUE) &&

make that if (mProxyTransparentResolvesHost &&

r=biesi

Comment 59

[Malcolm Smith]

Attachment: embrace and extend

> well, could PAC not just respect this preference?

Technically, of course.  But again, I don't think this is a good
approach - having a "Proxy Auto-Config" that requires manual
configuration to work correctly seems a little suboptimal.

The point I was trying to make before is that if this pref is
to be included in PAC, then we need to think about the "right"
format for that that is compatible with other environments.

What I have done in this latest patch is created "flags" in
PAC responses, one of which is defined for this patch, so
instead of
 return "socks5 192.168.0.1:1080";
you can now have
 return "socks5 192.168.0.1:1080[clientdns]";

These flags are comma-delimited, and should handle whitespace
around them without problems.

Again, extending the PAC format is not something to be done
lightly, but the idea of PAC is to eliminate manual config,
so if we want a configurable proxy setting having a way to
express it in PAC is probably better than not.

> modules/libpref/src/init/all.js
> +pref("network.proxy.socks_remote_dns",      false);
> would true be a better default value?

Yes, it would.	See comment #29.  I can't see why any SOCKS5
environment should ever have this disabled, and I'd be surprised
if there are SOCKS4 environments old enough in production to
want it disabled, either.  The false default was historic to
avoid a behaviour change.

But if you'll let me have true as a default, that's great ;)
- I've updated this patch accordingly.

> netwerk/base/public/nsNetUtil.h
> +		 PRUint16	flags,
> wrong indentation
Fixed.

> netwerk/base/src/nsSocketTransport2.cpp
> +	     if ((mProxyTransparentResolvesHost == PR_TRUE) &&
> make that if (mProxyTransparentResolvesHost &&
Fixed.

Comment 60

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

Comment on attachment 154949 [details] [diff] [review]
embrace and extend

 return "socks5 192.168.0.1:1080[clientdns]";

hrm, up to now PAC was working cross-browser in MSIE, NS4, Mozilla...
I want benc and darin to agree to this change. I have not looked at the code
for this yet.

+	 // If it's a SOCKS proxy, default to doing name resolution on the
+	 // server side. 

this, though, sounds good.

+	 if ((type == kProxyType_SOCKS || type == kProxyType_SOCKS4))

might as well remove one pair of parentheses.

>But if you'll let me have true as a default, that's great ;)

sure :)

Comment 61

[Darin Fisher]

many companies deploy a single PAC file for all browsers.  that means that any
extensions to PAC need to be done in a backwards compatible way.  adding a flags
parameter as you have done breaks the parsing done by older builds.  i suppose
servers could do UA sniffing to serve the right PAC file :-/

i would recommend separating the issue of PAC specifying client-dns into a
separate bug since the issue of how to specify the flag from PAC is going to be
more contentious then everything else in this patch.

Comment 62

[Malcolm Smith]

(In reply to comment #61)
> many companies deploy a single PAC file for all browsers.  that means that any
> extensions to PAC need to be done in a backwards compatible way.  adding a flags
> parameter as you have done breaks the parsing done by older builds.

Hrm, I tried to make sure the flags stuff wouldn't break the code as it existed,
so I was hopeful it wouldn't break older Mozilla builds (can't speak for other
browsers.)
> 
> i would recommend separating the issue of PAC specifying client-dns into a
> separate bug since the issue of how to specify the flag from PAC is going to be
> more contentious then everything else in this patch.

Agreed.

So what should the 'interim' method for handling PAC be?

Should a way (let's say, server-side name resolution) *always* be used when a
PAC is specified?  This has the advantage of consistency when using PACs,
although it's not configurable.

Or should the pref apply to PACs?  This may mean inconsistent client behavior
when using PACs.  Also, how could the UI be arranged so that the checkbox is
applicable to manual and auto proxy config but not direct?

Comment 63

[Darin Fisher]

> Hrm, I tried to make sure the flags stuff wouldn't break the code as it existed,
> so I was hopeful it wouldn't break older Mozilla builds (can't speak for other
> browsers.)

I don't doubt that you made it work with older versions of Mozilla... my main
concern is not with recent versions of Mozilla, but rather with other browsers.
I should have said "older browsers" instead of "older builds" ;-)


> Should a way (let's say, server-side name resolution) *always* be used when a
> PAC is specified?  This has the advantage of consistency when using PACs,
> although it's not configurable.

what does IE do?


> Or should the pref apply to PACs?  This may mean inconsistent client behavior
> when using PACs.  Also, how could the UI be arranged so that the checkbox is
> applicable to manual and auto proxy config but not direct?

ugh.. i'm not a UI expert and the proxy pref panel is already getting very
cluttered.  might be good to enlist Ben Goodger's help (or somebody else's who
knows about UI design).

Comment 64

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

hm... does this pref require UI? if we change the default to "true", things
should "just work" for SOCKS users...

note that there is another bug which wants to add something to this panel - WPAD.

this panel needs a redesign...

Comment 65

[Malcolm Smith]

(In reply to comment #63)

> > Should a way (let's say, server-side name resolution) *always* be
> > used when a PAC is specified?  This has the advantage of
> > consistency when using PACs, although it's not configurable.
> 
> what does IE do?

IE for Windows only supports SOCKS4 and requires client-side DNS lookups.  IE
for Mac supports SOCKS5 and does server-side DNS lookups.  I don't think either
support configuration for this option.  So I'll take that as a vote for "pick
the best and go with it" and in this case, server side name resolution is the best.

> hm... does this pref require UI? if we change the default to
> "true", things should "just work" for SOCKS users...

ok, that's the $1m question.  When I wrote my solution for this bug, there was
no pref, it was SOCKS5 only and it changed everything to use server-side name
resolution, no questions asked.  Justin's, on the other hand, had a pref,
supported SOCKS4a and was generally a better patch, so I worked on his instead.

My answer: SOCKS5 should "just work" with server-side name resolution, unless
the server is horribly broken.  I doubt there are any SOCKS5 servers *that*
broken in use.

SOCKS4 does not support server-side name resolution except as an extension,
SOCKS4a.  Any plain, SOCKS4 server requires client-side name resolution.  Many
SOCKS4 servers do support 4a (mine does, MS does) but there are many "very
minimal" SOCKS4 implementations out there because the protocol was so simple,
and these don't.

So it doesn't *need* the UI unless we plan on having some support for small,
SOCKS4 only servers.  If we want to support these things, then there needs to be
a way to change the pref, and surely a UI would be the better way...

IMO, the Proxy panel needs to be less cluttered - maybe have the 3 "direct,
manual, auto" options, and a button for "manual" that pops up the rest.  This
should work well with WPAD and this patch.

Comment 66

[Malcolm Smith]

Attachment: strategic retreat

Full PAC handling has been removed, as has the double-brackets.

PACs now all use server-side name resolution.

Comment 67

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

Comment on attachment 155243 [details] [diff] [review]
strategic retreat

looks good

Comment 68

[Darin Fisher]

Comment on attachment 155243 [details] [diff] [review]
strategic retreat

>Index: modules/libpref/src/init/all.js

>+pref("network.proxy.socks_remote_dns",      true);

so, this changes our default behavior right?  is that really 
a good idea?  will we confuse users by enabling this pref by
default?  what do IE and Opera do?


>Index: netwerk/base/public/nsIProtocolProxyService.idl

>-    nsIProxyInfo newProxyInfo(in ACString aType, in AUTF8String aHost, in long aPort);
>+    nsIProxyInfo newProxyInfo(in ACString aType, in AUTF8String aHost,
>+                              in long aPort, in unsigned short aFlags);

when changing the signature of an interface method, it is imperative
that you change the UUID of the interface.


>Index: netwerk/base/public/nsIProxyInfo.idl

>+    [noscript, notxpcom] PRUint16     Flags();

the same applies to this interface.  i think flags should be a PRUint32
since that is anyways the size of a WORD on most platforms so making it
be a 16-bit integer isn't that useful.


>Index: netwerk/base/src/nsProtocolProxyService.cpp

>+        // If it's a SOCKS proxy, do name resolution on the server side.
>+        if (type == kProxyType_SOCKS || type == kProxyType_SOCKS4)
>+            flags |= nsIProxyInfo::TRANSPARENT_PROXY_RESOLVES_HOST;

shouldn't this be conditional on the pref?  or are you saying that
PAC will always do host resolution on the server?  why enable for
SOCKS4?  shouldn't this be for SOCKS5 only?


>         if (mSOCKSProxyVersion == 4) 
>             type = kProxyType_SOCKS4;
>         else
>             type = kProxyType_SOCKS;
>         port = mSOCKSProxyPort;
>+        if (mSOCKSProxyRemoteDNS)
>+            proxyFlags |= nsIProxyInfo::TRANSPARENT_PROXY_RESOLVES_HOST;

same question?	shouldn't this be for SOCKS5 only?


>Index: netwerk/base/src/nsSocketTransport2.cpp

>+    if (!mProxyHost.IsEmpty() && mProxyTransparentResolvesHost) {
>+        // Name resolution is done on the server side.  Just pretend
>+        // client resolution is complete, this will get picked up later.
>+        // since we don't need to do DNS now, we bypass the resolving
>+        // step by initializing mNetAddr to an empty address, but we
>+        // must keep the port. The SOCKS IO layer will use the hostname
>+        // we send it when it's created, rather than the empty address
>+        // we send with the connect call.
>+        mState = STATE_RESOLVING;
>+        PR_SetNetAddr(PR_IpAddrAny, PR_AF_INET, SocketPort(), &mNetAddr);
>+        rv = PostEvent(MSG_DNS_LOOKUP_COMPLETE, NS_OK, nsnull);
>+        return rv;

nit: "return PostEvent(...);"

also, i seem to recall that our FTP code needs to know the IP address
of the host it is talking to in order to know if the host is IPv4 or
IPv6.  if it is IPv6 then it needs to issue an EPSV command instead of
a PASV command to initiate a passive data connection.  how will this
affect FTP to an IPv6 node over SOCKS?


>Index: netwerk/base/src/nsSocketTransport2.h

>     PRBool      mProxyTransparent;
>+    PRBool      mProxyTransparentResolvesHost;

nit: these should probably use PRPackedBool instead of PRBool to
reduce the size of the class a bitty bit ;-)


>Index: netwerk/socket/base/nsISocketProvider.idl

>     void newSocket(in long            aFamily,
>                    in string          aHost, 
>                    in long            aPort,
>                    in string          aProxyHost,
>                    in long            aProxyPort,
>+                   in long            aFlags,

this interface also needs a new UUID.


>Index: netwerk/socket/base/nsSOCKSIOLayer.cpp

>+        // make sure the hostname will fit in the buffer we're using,
>+        // taking into account the 7 other bytes we use.
>+        if (host_len > sizeof(request) - 7) {
>+            // Hostname was too big for buffer.
>+            LOGERROR(("Hostname was too long to fit in the request buffer."));
>+            return NS_ERROR_INVALID_ARG;
>+        }

ouch.. so, is there nothing we can do to work around this problem?  can the
size of the SOCKS message be variable length?  should we just malloc a buffer?


>+        // make sure the hostname will fit in the buffer we're using,
>+        // taking into account the other bytes we use.
>+        if (host_len > sizeof(request) - request_len - 1) {
>+            // Hostname was too big for buffer.
>+            LOGERROR(("Hostname was too long to fit in the request buffer."));
>+            return NS_ERROR_INVALID_ARG;
>+        }

same issue here.


>+        // the hostname is then added at the end of the request with NULL.
>+        strncpy((char*) request+request_len, destHost.get(), host_len + 1);

use PL_strncpyz instead to ensure null termination?  strncpy does not promise
to null terminate; however, since you have checked host_len ahead of time, this
is not really an issue.  so, feel free to ignore.

Comment 69

[Malcolm Smith]

(In reply to comment #68)

I will prepare a new patch to address the issues you've raised, but first, some
quick responses to the 'policy' issues.

> (From update of attachment 155243 [details] [diff] [review])
> >Index: modules/libpref/src/init/all.js
> 
> >+pref("network.proxy.socks_remote_dns",      true);
> 
> so, this changes our default behavior right?  is that really 
> a good idea?  will we confuse users by enabling this pref by
> default?  what do IE and Opera do?

This was discussed in comments #29, and #58-#60.

Yes, it would be a behaviour change, but I don't think it would really be
confusing.  This is the default on some browsers, such as IE for Mac.  It's a
moot point on IE for Windows, which doesn't support SOCKS5.

I cannot picture a sane scenario where changing this behavior would cause any
issues on SOCKS5, or on a SOCKS4 server supporting 4a.  The only issues arise
with SOCKS4-only servers.

Again, if you want the default to be disabled again, that's ok with me, but I'd
prefer it to be enabled.

> shouldn't this be conditional on the pref?  or are you saying that
> PAC will always do host resolution on the server?  why enable for
> SOCKS4?  shouldn't this be for SOCKS5 only?

See comment #62.  If you want me to change this behaviour I'm happy to do that,
but I'm really scared about the idea of a single .pac file generating different
results depending on manual configuration.  This seems like an administrative
nightmare - and then there's the issue of UI design.

> same question?	shouldn't this be for SOCKS5 only?

No - SOCKS4a supports this feature, and this patch supports SOCKS4a.  Most
SOCKS4 servers today do too.  Things like MSProxy2.0 support 4a (loudly.)  Most
SOCKS4 servers should have this option enabled, it should only be disabled where
the server doesn't support 4a.

> also, i seem to recall that our FTP code needs to know the IP address
> of the host it is talking to in order to know if the host is IPv4 or
> IPv6.  if it is IPv6 then it needs to issue an EPSV command instead of
> a PASV command to initiate a passive data connection.  how will this
> affect FTP to an IPv6 node over SOCKS?

I'll look into it, but my simple answer is I'd be really suprised if this patch
modified the current behaviour here in any way.  Out of interest, can't EPSV be
used on IPv4...?

> >+        // make sure the hostname will fit in the buffer we're using,
> >+        // taking into account the 7 other bytes we use.
> >+        if (host_len > sizeof(request) - 7) {
> >+            // Hostname was too big for buffer.
> >+            LOGERROR(("Hostname was too long to fit in the request buffer."));
> >+            return NS_ERROR_INVALID_ARG;
> >+        }
> 
> ouch.. so, is there nothing we can do to work around this problem?  can the
> size of the SOCKS message be variable length?  should we just malloc a buffer?

If you think it's important, of course we can.  The main issue here was
minimising the number of network sends.  This is TCP, and there's no reason why
it needs to be copied at all.

Comment 70

[Malcolm Smith]

Comment on attachment 155243 [details] [diff] [review]
strategic retreat

>>Index: modules/libpref/src/init/all.js
>>+pref("network.proxy.socks_remote_dns",      true);

>so, this changes our default behavior right?  is that
>really a good idea?  will we confuse users by enabling
>this pref by default?	what do IE and Opera do?

This patch retains the default of true.  I will change
this if it's absolutely required.  I do consider it
a good idea - it will fix DNS problems with SOCKS,
subject only to problems of incomplete SOCKS
implementations.  It will only confuse if the SOCKS
server is broken.

And for the record: looking to things like IE for
leadership on points like this is futile.  They're
not leading anymore, we are.

>when changing the signature of an interface method, it
>is imperative that you change the UUID of the interface.
Fixed.

>>Index: netwerk/base/public/nsIProxyInfo.idl
>>+    [noscript, notxpcom] PRUint16	 Flags();

>the same applies to this interface.  i think flags
>should be a PRUint32 since that is anyways the size
>of a WORD on most platforms so making it be a 16-bit
>integer isn't that useful.
Fixed.

>>Index: netwerk/base/src/nsProtocolProxyService.cpp
>>+	   // If it's a SOCKS proxy, do name resolution on the server side.
>>+	   if (type == kProxyType_SOCKS || type == kProxyType_SOCKS4)
>>+	       flags |= nsIProxyInfo::TRANSPARENT_PROXY_RESOLVES_HOST;

>shouldn't this be conditional on the pref?  or are you
>saying that PAC will always do host resolution on the
>server?  why enable for SOCKS4?  shouldn't this be for
>SOCKS5 only?
As mentioned before, I don't think subjecting PACs to
the pref is wise, but will do so if required.  This
code is PAC-only.  And on reflection, you're right,
enabling it for SOCKS4 is probably a mistake, so now
it's always on for PAC with SOCKS5 and always off for
PAC with SOCKS4.

>>	   if (mSOCKSProxyVersion == 4) 
>>	       type = kProxyType_SOCKS4;
>>	   else
>>	       type = kProxyType_SOCKS;
>>	   port = mSOCKSProxyPort;
>>+	   if (mSOCKSProxyRemoteDNS)
>>+	       proxyFlags |= nsIProxyInfo::TRANSPARENT_PROXY_RESOLVES_HOST;
>same question? shouldn't this be for SOCKS5 only?

No - if the user wants to use SOCKS4a, they chose it, we support it.
(aside: look at PuTTY for how this can work really well [Thanks
Justin!])

>>Index: netwerk/base/src/nsSocketTransport2.cpp
>>+    if (!mProxyHost.IsEmpty() && mProxyTransparentResolvesHost) {
>>+	   // Name resolution is done on the server side.  Just pretend
>>+	   // client resolution is complete, this will get picked up later.
>>+	   // since we don't need to do DNS now, we bypass the resolving
>>+	   // step by initializing mNetAddr to an empty address, but we
>>+	   // must keep the port. The SOCKS IO layer will use the hostname
>>+	   // we send it when it's created, rather than the empty address
>>+	   // we send with the connect call.
>>+	   mState = STATE_RESOLVING;
>>+	   PR_SetNetAddr(PR_IpAddrAny, PR_AF_INET, SocketPort(), &mNetAddr);
>>+	   rv = PostEvent(MSG_DNS_LOOKUP_COMPLETE, NS_OK, nsnull);
>>+	   return rv;
>nit: "return PostEvent(...);"
Fixed.

>>Index: netwerk/base/src/nsSocketTransport2.h
>>     PRBool	   mProxyTransparent;
>>+    PRBool	   mProxyTransparentResolvesHost;
>nit: these should probably use PRPackedBool instead of
>PRBool to reduce the size of the class a bitty bit ;-)
Fixed

>>Index: netwerk/socket/base/nsISocketProvider.idl
>>     void newSocket(in long		 aFamily,
>>		      in string 	 aHost, 
>>		      in long		 aPort,
>>		      in string 	 aProxyHost,
>>		      in long		 aProxyPort,
>>+		      in long		 aFlags,
>this interface also needs a new UUID.
Fixed

>>Index: netwerk/socket/base/nsSOCKSIOLayer.cpp
>>+	   // make sure the hostname will fit in the buffer we're using,
>>+	   // taking into account the 7 other bytes we use.
>>+	   if (host_len > sizeof(request) - 7) {
>>+	       // Hostname was too big for buffer.
>>+	       LOGERROR(("Hostname was too long to fit in the request
buffer."));
>>+	       return NS_ERROR_INVALID_ARG;
>>+	   }
>ouch.. so, is there nothing we can do to work around
>this problem?	can the size of the SOCKS message be
>variable length?  should we just malloc a buffer?

My last statement here was premature.  SOCKS5 uses a
single byte to record the hostname length.  It will
not support a hostname of greater than 255 chars, we
have to live with that.

I used multiple sends to take it up to 255 chars
without needing to copy or buffer, though.

>>+	   // make sure the hostname will fit in the buffer we're using,
>>+	   // taking into account the other bytes we use.
>>+	   if (host_len > sizeof(request) - request_len - 1) {
>>+	       // Hostname was too big for buffer.
>>+	       LOGERROR(("Hostname was too long to fit in the request
buffer."));
>>+	       return NS_ERROR_INVALID_ARG;
>>+	   }
SOCKS4a uses NULL-terminated strings, so now with
multiple sends, the sky's the limit.

>>+	   // the hostname is then added at the end of the request with NULL.
>>+	   strncpy((char*) request+request_len, destHost.get(), host_len + 1);
>use PL_strncpyz instead to ensure null termination?
>strncpy does not promise to null terminate; however,
>since you have checked host_len ahead of time, this
>is not really an issue.  so, feel free to ignore.

This code has been axed as part of the above.

Comment 71

[Darin Fisher]

> This patch retains the default of true.  I will change
...
> implementations.  It will only confuse if the SOCKS
> server is broken.

OK, makes sense.  Thanks for clarifying that.


> And for the record: looking to things like IE for
> leadership on points like this is futile.  They're
> not leading anymore, we are.

Uhm... last I checked, our market share is minuscule
next to that of IE.  They have a SOCKS implementation,
so it is prudent to consider their implementation when
deciding on our default behavior.  NOTE: It doesn't 
matter that IE is old... it is the market leader!!

Comment 72

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

Comment on attachment 155465 [details] [diff] [review]
spot the deliberate mistake

>it is imperative
>that you change the UUID of the interface.

I should not have missed that :(

>> also, i seem to recall that our FTP code needs to know the IP address
> I'd be really suprised if this patch modified the current behaviour here

well, this patch certainly means that the FTP code does not know the IP Address
of the host it is talking to...
http://www.cse.ohio-state.edu/cgi-bin/rfc/rfc2428.html#sec-3 sounds like it can
be used both for IPv6 and IPv4.

-    PRUint16 destPort = PR_htons(PR_NetAddrInetPort(addr));
+    PRInt32 destPort = PR_ntohs(PR_NetAddrInetPort(addr));

hm? why this change?

Comment 73

[Malcolm Smith]

(In reply to comment #72)
<snip>
> -    PRUint16 destPort = PR_htons(PR_NetAddrInetPort(addr));
> +    PRInt32 destPort = PR_ntohs(PR_NetAddrInetPort(addr));
> hm? why this change?

Short answer: no idea.  This code has been around since before I've been 
working on it, and I hadn't picked it up.

I agree, it looks (at least superficially) as a bad idea, so I'll look into 
changing it.

> NOTE: It doesn't matter that IE is old... it is the
> market leader!!

lol :) It may be prudent to consider what IE does when deciding defaults, but 
it can quickly come to a point where this holds us back in 2001 where they 
are.  Imagine if SOCKS6 were released tomorrow: would we not support it?  Or 
not support any of the new features because IE doesn't?  Would we default to 
using SOCKS4?

SOCKS5 provides us with new stuff like a well designed and implemented method 
for name resolution, as well as authentication (will look at this after this 
patch.)  IMO, it's silly not to take advantage of these things because IE is 
still SOCKS4-only.

In terms of PAC, I agree with you, because it's important that one PAC file 
works with all browsers.  In terms of manual defaults, going with sane/sensible 
defaults seems preferable to going with IE's defaults.  In most cases, IE's 
will be sane and sensible, but not always: server-side name resolution would be 
a bad idea if you're implementing SOCKS4, when it might not be there.  It is 
sane and sensible on SOCKS5, where server support is a core requirement.  IE 
recognised this when they implemented SOCKS5 on Mac.

Comment 74

[benc]

Is it possible to move the PAC implementation discussion to a new bug, or is it
inseperable? If no, my answer to the protocol extension is "no. please, no."
Darin already extended the SOCKS directive once by adding "SOCKS4 and SOCKS5".
We never tested other browsers to understand what they did w/ the SOCKS
directive, and we never tested to see if they handled PAC files w/ SOCKS4 and
SOCKS5 correctly.

As for the manual proxy behavior, the short version is: SOCKS4 did not support
Proxy-side DNS, SOCKS5 does, and should. Network administrators deploy SOCKS4 or
SOCKS5 proxies, they understand this limitation, so we just need to implement
this  and document our protocol support clearly.

Comment 75

[Malcolm Smith]

(In reply to comment #74)
> Is it possible to move the PAC implementation discussion to a new bug, or is it
> inseperable? If no, my answer to the protocol extension is "no. please, no."

This has been seperated in the latest version of the patch for the reasons you
describe.

I haven't opened a new bug for it, because at the moment, it's a hypothetical
bug - it only exists once this is committed.

Comment 76

[Mike Shaver (:shaver emeritus)]

So, if I were want to risk my life by asking about taking this for Fx 1.0RC1,
what _technical_ work would need to be done?

SOCKS5 is growing in use, and I'm starting to hear grumbling from a lot of
deployment-corners about this.

Comment 77

[Darin Fisher]

Comment on attachment 155465 [details] [diff] [review]
spot the deliberate mistake

>Index: directory/xpcom/base/src/nsLDAPSecurityGlue.cpp

>     rv = tlsSocketProvider->AddToSocket(PR_AF_INET,
> 					sessionClosure->hostname, defport,
>+					nsnull, 0, 0, socketInfo.soinfo_prfd,
>                                         getter_AddRefs(securityInfo));

do me a favor and fix the indentation here so the params line up
vertically.  thx!


>Index: extensions/help/resources/locale/en-US/cs_nav_prefs_advanced.xhtml
>Index: extensions/help/resources/locale/en-US/nav_help.xhtml

Do these need some sort of review or approval from the help system
module owner?  (I don't know who that is or even if there is such
a person.)


>Index: netwerk/base/public/nsIProxyInfo.idl

>+    const unsigned short TRANSPARENT_PROXY_RESOLVES_HOST = 1 << 0;

I think this flag deserves a comment explaining what it does.
Though the name of the flag is pretty self-explanatory, a comment
is always a good idea ;-)


>Index: netwerk/base/src/nsProtocolProxyService.cpp

>+    , mSOCKSProxyRemoteDNS(PR_FALSE)

In prefs, this defaults to true.  I think we should make
this setting consistent with the value in all.js.


>         if (mSOCKSProxyVersion == 4) 
>             type = kProxyType_SOCKS4;
>         else
>             type = kProxyType_SOCKS;
>         port = mSOCKSProxyPort;
>+        if (mSOCKSProxyRemoteDNS)
>+            proxyFlags |= nsIProxyInfo::TRANSPARENT_PROXY_RESOLVES_HOST;

So, I'm confused.  It seems to me that we want SOCKS v5 to default to 
remote DNS, but we want SOCKS v4 to default to local DNS.  Is that not
true?  If so, then doesn't this code cause problems for SOCKS v4?  The
default for remote DNS is true, so wouldn't that cause existing SOCKS v4
users to have to flip the pref to false in order to continue browsing the
web?  Perhaps we need to default remote DNS to false unless and until the
user enables SOCKS v5 in the pref panel.  In other words, we'd suggest
remote DNS in the pref panel when SOCKS v5 is selected, but otherwise we'd
default to false.  Existing installations would then be unaffected by this
patch.



>Index: netwerk/base/src/nsSocketTransport2.cpp

>+            if (mProxyTransparentResolvesHost &&
>+                ((strcmp(mTypes[i], "socks") == 0) ||
>+                (strcmp(mTypes[i], "socks4") == 0)))
>+                proxyFlags |= nsISocketProvider::PROXY_RESOLVES_HOST;

If mProxyTransparentResolvesHost is true, then perhaps there is no
reason to check for "socks" or "socks4".  You already did that check
earlier when you assigned mProxyTransparentResolveHost, right?

Also, I would call the parameter |sockFlags| or just |flags| instead
of |proxyFlags| since nsISocketProvider does not refer to them as proxy
specific flags.


>Index: netwerk/socket/base/nsISocketProvider.idl

>+    const long PROXY_RESOLVES_HOST = 1 << 0;

please document this flag.


>Index: netwerk/socket/base/nsSOCKSIOLayer.cpp

>+    void Init(PRInt32 version, const char *proxyHost, PRInt32 proxyPort,
>+        const char *destinationHost, PRUint32 flags);

nit: make parameter list line up under the opening paranthesis.  thx!


>+        LOGDEBUG(("we are sending the server the hostname rather than IP\n"));

technically, we might be sending an IP, right?	perhaps this should say
"rather than resolving it first" or something like that.


>Index: xpfe/components/prefwindow/resources/content/pref-proxies.xul

>+          <row>
>+            <spacer/>
>+            <checkbox id="networkProxySOCKSRemoteDNS" 
>+                      label="&socksRemoteDNS.label;"
>+                      accesskey="&socksRemoteDNS.accesskey;"
>+                      prefstring="network.proxy.socks_remote_dns"/>
>+          </row>

This needs to be reviewed / approved by the Mozilla Seamonkey UI owner,
which is probably Neil Rashbrook (last I heard).

Also, what about Firefox pref UI?  This patch doesn't include changes to
the pref UI for Firefox.  If this is to be considered for Firefox 1.0,
then it seems that UI for it would need to be created.	See:

http://lxr.mozilla.org/mozilla/find?string=pref-connection.xul

Comment 78

[Darin Fisher]

cc'ing RJK and Neil for review of help system changes (Neil cc'd also for
Seamonkey pref UI changes).  thanks!

Comment 79

[[e-mail]]

Regarding both help and pref UI changes, Stefan Borggraefe is trying to simplify
the proxy panel so that he can add UI for WPAD support, which is bug 257758. It
would probably be a good idea if we rolled this UI change into that patch.

Comment 80

[Darin Fisher]

Comment on attachment 155465 [details] [diff] [review]
spot the deliberate mistake

I had meant to minus this yesterday.

Comment 81

[Mike Shaver (:shaver emeritus)]

>+    const unsigned short TRANSPARENT_PROXY_RESOLVES_HOST = 1 << 0;

Ignoring for a moment that Darin will tell you, if you pause for breath, that
there is no such thing as a transparent proxy, SOCKS5 isn't transparent at all,
is it?

Comment 82

[Darin Fisher]

> >+    const unsigned short TRANSPARENT_PROXY_RESOLVES_HOST = 1 << 0;
> 
> Ignoring for a moment that Darin will tell you, if you pause for breath, that
> there is no such thing as a transparent proxy, SOCKS5 isn't transparent at all,
> is it?

It is not a transparent proxy per the definition in RFC 2616 (section 1.3), but
it is a "transparent proxy" as far as Necko is concerned.  From Necko's point of
view, a proxy is transparent if it does not require modifications to higher
level protocols.  SOCKS is such a layer in the network stack.  E.g., the HTTP
protocol need not be aware of the SOCKS protocol.

The reality is that "transparent proxy" is a horribly overloaded term :-/  Our
APIs speak of proxies, and proxy transparency in the sense that I have
described, so this patch is consistent at least with other Necko APIs.

Comment 83

[Stefan Borggraefe]

(In reply to comment #79)
> Regarding both help and pref UI changes, Stefan Borggraefe is trying to simplify
> the proxy panel so that he can add UI for WPAD support, which is bug 257758. It
> would probably be a good idea if we rolled this UI change into that patch.

I agree this would be a good idea. I could adopt the UI changes for Seamonkey
from attachment 155465 [details] [diff] [review] and the Help file changes and integrate them into my
patch for bug 257758 if you want. This would probably avoid some CVS conflicts.
Also I wanted to ask Neil for review anyway, so this part of your patch will end
up in the review queue of the Seamonkey front-end module owner (and Neil is a
Help peer, too).

Note however that bug 257758 does not deal with Firefox. So this is a different
story.

Comment 84

[Malcolm Smith]

Attachment: feedback response

ok, I've fixed most of the simple coding issues that Darin points out, but I'm
a little confused as to where to proceed to from here.

If the UI stuff should be removed from here and integrated into another patch,
that's fine by me, but there are a few things to think about.  This patch
updates the Firefox UI (untested - somebody who builds this please have a look
for me).  What should happen to that?  Also, unless the default issues are
sorted, having this patch committed without a UI would be likely fatal.

It would be nice to have a different default for SOCKS4 and SOCKS5.  But doing
so involves more issues: should changing from v4 to v5 (or vice-versa) modify
the value automatically?  Won't this be confusing for the user also?  Or should
there be two stored prefs, one for server-side DNS lookups in SOCKS4 and one
for SOCKS5?

Should Thunderbird have its UI updated too?  SOCKS is probably more intensively
used in the mail world than the web world.  But as above, where should this
happen?

I'm really concerned that this patch will get bogged down trying to make it
consensus-perfect; different people have different ideas, see different issues,
and waiting for complete agreement may delay this patch for years.  The
function of this code had been reviewed and superreviewed previously, and the
changes being made reflect different design ideas since then.  Although
perfection is a laudable aim, this is still a fix for a bug, and we do need to
think which option is preferable: have something committed that can be improved
(and the bug fixed in 1.8), or leave the bug around for one or two more
releases aiming for a "better" fix.

Comment 85

[Darin Fisher]

Malcolm,

I hear your concerns.  I think we should try to separate the UI work from the
backend work as much as possible precisely because each app may wish to do
things slightly differently.

How about this:

  If the pref is unassigned, assume remote dns for SOCKS v5 but not v4.
  If the pref is assigned, then use the value for both SOCKS v5 and v4.

If you make the backend behave like this, then the backend is flexible enough,
and we'd get a suitable default behavior.

In parallel, or once the backend changes are done, we can go and engage the UI
owners for the various products: Neil Rashbrook for SeaMonkey, Ben Goodger for
Firefox, Scott Macgregor for Thunderbird, Daniel Glazman for Nvu, etc.

Does this sound reasonable to you?

Comment 86

[Darin Fisher]

My pref suggestions requires of course that the pref is not given a default
value in all.js.

Comment 87

[[e-mail]]

(In reply to comment #86)
>My pref suggestions requires of course that the pref is not given a default
>value in all.js.
The pref window won't leave the pref undefined, so we'd need to check on panel
load to see if the pref was undefined and if so set it to whatever the default
for the given SOCKS type is.

Comment 88

[Malcolm Smith]

(In reply to comment #87)

> The pref window won't leave the pref undefined, so we'd need to check on panel
> load to see if the pref was undefined and if so set it to whatever the default
> for the given SOCKS type is.

Would that really work in practice?

If the user has never been into prefs before, it will always default to the same
value (if SOCKS5 is default, for instance, this pref will always default to
true.)  If the user changes from SOCKS5 to SOCKS4, this pref would remain true?

And having been into prefs, a value would be saved, so the default would never
occur again?

Or are you suggesting one of those silly 3-state checkboxes (in effect)...so it
also remembers a value of 'undefined'...and won't we need some way to remember
this value after the user accepts the prefs dialog?

Comment 89

[Darin Fisher]

here's how i see it: once someone loads up the pref panel, it is fine to fix the
value to true or false.  why?  well, if they then change the SOCKS proxy to v4,
they can likewise change the remote DNS option at that time.  i'm not sure there
is much reason from a UI point of view to ever need to restore the unset mode. 
we'd only need to support that mode for compatibility / migration purposes really.

does this make sense?

Comment 90

[[e-mail]]

(In reply to comment #89)
>here's how i see it: once someone loads up the pref panel, it is fine to fix
>the value to true or false.
OK, as long as we check the panel when it loads loads to see if it was
undefined, so that it will get fixed with the correct setting.

Comment 91

[benc]

It might be late in the game, but I'd still like to see separate bugs that
explain what we are doing for PAC and the UI. I'm really confused as to the
current state of the implementation. 

Comment 92

[chris hofmann]

doesn't look like this is going to ba baked enough for 1.0.  renominate if a
well tested patch appears.

Comment 93

[Mikel Manitius]

Is anyone still working on fixing this?

I tried setting SOCKS5_FAKEALLHOSTS=1 in the environment
(per
http://www.socks.permeo.com/TechnicalResources/DevelopDocuments/SOCKSifyingClientAp0CE5/RunsockManPage.asp)
But that does not appear to be implemented either.

This Mozilla "patch kit":

http://yallara.cs.rmit.edu.au/~malsmith/products/mozkit/

Offers two interesting features:

1. Allow setting of an option in the PAC to indicate client or server side DNS
resolution with SOCKS5

     i.e. socks5 192.168.0.1:1080[serverdns]

2. A preference file setting:

     pref("network.proxy.socks_remote_dns",      true);


It does not deal with the GUI issue. However, simply implementing these back-end
features would be a big help.

Comment 94

[Malcolm Smith]

(In reply to comment #93)
> Is anyone still working on fixing this?

I've recently accepted a job with Microsoft and won't be able to work on this patch.

> 
> I tried setting SOCKS5_FAKEALLHOSTS=1 in the environment
> (per
>
http://www.socks.permeo.com/TechnicalResources/DevelopDocuments/SOCKSifyingClientAp0CE5/RunsockManPage.asp)
> But that does not appear to be implemented either.

That is only relevant if you're running Mozilla under their SOCKS library, using
'runsocks' on *nix, and (possibly) SOCKSCap on Win32.

> 
> This Mozilla "patch kit":
> 
> http://yallara.cs.rmit.edu.au/~malsmith/products/mozkit/
> 
> Offers two interesting features:
> 
> 1. Allow setting of an option in the PAC to indicate client or server side DNS
> resolution with SOCKS5
> 
>      i.e. socks5 192.168.0.1:1080[serverdns]
> 
> 2. A preference file setting:
> 
>      pref("network.proxy.socks_remote_dns",      true);
> 
> 
> It does not deal with the GUI issue. However, simply implementing these back-end
> features would be a big help.

That patch is an old version of what's attached to this bug; it's the same patch
:) - although the UI should be implemented on all versions of this patch back to
time immemorial (check out the Proxy section in Prefs.)

- M

Comment 95

[Darin Fisher]

Unless someone else volunteers, I'll work on completing the backend changes for
this patch.  Please volunteer, I'm swamped! ;-)

Comment 96

[James Ross]

*** Bug 215386 has been marked as a duplicate of this bug. ***

Comment 97

[Darin Fisher]

Attachment: backend only changes, remote dns disabled by default

I went ahead and landed the backend changes from the latest patch (attachment
158939 [details] [diff] [review]) with the remote dns pref disabled by default.  I think it is better to
get the backend changes in place, so that people can test them out instead of
holding up that part of this patch for the UI changes.	I settled on disabling
the remote dns pref by default because 1) that would change the behavior of
existing setups, and 2) I found a pretty serious bug where failed DNS lookups
result in no error being reported to the user.	Both of those problems need to
be resolved in a clean way before I'll agree to enabled the remote dns pref by
default.

This bug can stay open I suppose for the remaining work.  Again, I don't expect
to have any time to work on what's left.  I just didn't want the backend work
to bit-rot.

Comment 98

[Darin Fisher]

pushing out... i don't have cycles to work on pref ui for this.  if anyone is
interested in working on this bug, please let me know.

Comment 99

[Malcolm Smith]

Attachment: SOCKS error conditions fixes

This patch fixes a number of SOCKS error issues.  One nasty non-error condition
is fixed by that '!mProxyTransparent' thing, which is a bug regardless of the
backend patch that was committed.  The rest are hacks to ensure that errors
properly relate to either the proxy or the remote host.  I don't like the
messiness here.  It'd be nice to just send PR_SetError an NS_ERROR_*, and
collect this in nsSocketTransport2.cpp, but that doesn't appear to be how it's
designed to work.  On the other hand, this 'layered' model isn't designed to
care about where a connection failure occurred, and with SOCKS errors, this
matters.

Comment 100

[Malcolm Smith]

Attachment: Slightly updated UI stuff

Updated to be a bit more current, and remove the accesskey which now clashes
with the "Set all" button.  No character in this UI had an available accesskey,
so I left it off.

As I understand it, this pref panel is in for a big redesign anyway, so this
patch should only be considered temporary for those wishing to use this
feature.

Comment 101

[Malcolm Smith]

Attachment: Help for the UI patch

This is seperated because it's non-critical, and the irredemably obscure help
indentation seems to change from one madness to another every five minutes, so
the UI patch should apply cleanly for a while even if this won't.

Comment 102

[Matthias Versen [:Matti]]

*** Bug 275807 has been marked as a duplicate of this bug. ***

Comment 103

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

the current patch for bug 257758 adds UI for this pref to seamonkey, adding
dependency

Comment 104

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

*** Bug 279520 has been marked as a duplicate of this bug. ***

Comment 105

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

bug 257758 added the UI for this. anything left to do here?

Comment 106

[Daniel Guido]

Did that UI bug also add UI for Firefox?  I'm using the latest nightly
(01/23/2004) and I don't see an option to specify remote DNS lookups except in
about:config.

Comment 107

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

ah, yeah. it didn't.

Comment 108

[Daniel Guido]

(In reply to comment #107)
> ah, yeah. it didn't.

Can we make this blocking-aviary1.1?

As long as the functionality is in the Core and Mozilla Suite, it ought to be in
firefox too.  It's important to corporates who have firewalls without internal
DNS servers and a few other special purposes that would set Firefox apart.  I
don't want this to get passed over for 1.1 when it's just a matter of adding a
checkbox in the prefs.

Comment 109

[Sergey Sokoloff]

Asking for blocking-aviary1.1, as Daniel Guido could have asked.

Comment 110

[Dan Kaminsky]

Darin,

   Does Malcolm's new error condition patch resolve the fault you detected?

Comment 111

[Darin Fisher]

>    Does Malcolm's new error condition patch resolve the fault you detected?

I don't know.  I haven't had any free cycles to get back to this bug.  Help wanted!

Comment 112

[Matthias Versen [:Matti]]

*** Bug 284371 has been marked as a duplicate of this bug. ***

Comment 113

[Fridtjof Busse]

Since I really need this feature (only way to make DNS-lookup on my net is via
SOCKS5 or 4a): Is there currently a build/nightly that actually adds DNS-support
for Socks?
Or will I have to build my own firefox with the patches attached to this bug?

Comment 114

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

oh yes, nightly trunk builds of firefox support it. there is no pref UI though,
afaik.

Comment 115

[Fridtjof Busse]

OK, thanks.
Right now it doesn't work for me, but I'm not really sure if that's actually
firefox' mistake. Did anybody get it working?

Comment 116

[Fridtjof Busse]

Nevermind, it was my mistake. DNS over Socks 5 works just fine. Thanks. :)

Comment 117

[Matthias Versen [:Matti]]

*** Bug 295148 has been marked as a duplicate of this bug. ***

Comment 118

[n.outpost]

It appears socks_remote_dns in about:config has been removed entirely as of
1.0.5. It would be nice if this actually worked again...

Comment 119

[Matthias Versen [:Matti]]

This is AFAIK only fixed on the trunk and not in the old Firefox 1.0.X branch

Comment 120

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

(In reply to comment #99)
> Created an attachment (id=169178) [edit]
> SOCKS error conditions fixes

FWIW, this patch sounds like it fixes bug 241479

Comment 121

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

Comment on attachment 169178 [details] [diff] [review]
SOCKS error conditions fixes

@@ -1354,17 +1355,17 @@ nsSocketTransport::OnSocketEvent(PRUint3
-	     if ((status == NS_ERROR_UNKNOWN_HOST) && !mProxyHost.IsEmpty())
+	     if ((status == NS_ERROR_UNKNOWN_HOST) && !mProxyHost.IsEmpty() &&
!mProxyTransparent)

Why this change? When is this event posted with an error status, when SOCKS is
used?


Why not store the OS error code in an nsresult variable and only cast once?

some of the error codes in SOCKSIOLayer are really kinda wrong... imo you
shouldn't map all kinds of stuff to connection refused.

Comment 122

[Paul Rubin]

Anything up with this?  See also related bug #306819.  This is a serious securit
y bug (privacy leak), since it means the client leaks DNS requests when the "adv
ertised" functionality is to route all traffic through the proxy.

Example: you are a Chinese user behind the Great Firewall.  You set up a SOCKS5 
proxy through an ssh tunnel (not a big problem all by itself) to someplace outsi
de the firewall, and start browsing Wikipedia through it.  Your web traffic is a
ll encrypted but the hostnames that you view all get leaked to the Chinese gover
nment.  Pretty soon, troops beat down your door and haul you to prison.

Another example: you are at home browsing some evil organization's site through a proxy.  When you visit www.evilorganization.com, the HTTP hits come from the proxy but the DNS hits come from your home.  They use the DNS log to track you down and beat you up.

I don't see why there needs to be a user preference.  The browser (when using SO
CKS5) should always let the proxy handle DNS, and never do it on the client.  Wi
th SOCKS4, the proxy configuration screen should clearly warn the user about thi
s privacy leak.

Comment 123

[David Multer]

I've got a PAC file that works fine for sending traffic to a SOCKS 
proxy by comparing URL patterns. For example: 
else if (shExpMatch(url, "http://site1.domain.com*")) 
    return "SOCKS localhost:6001"; 

On Mac OS X, I configure an automatic proxy configuration file and 
everything sent to site1.domain.com is sent to the proxy, while 
everything else isn't. Safari uses this setting perfectly. 

This doesn't work under Firefox. I've seen in Bugzilla that there may 
be work on getting Firefox to use the System Preferences setting, but 
I was able to get it to work in Firefox with a few changes just the 
way it is. Here are the changes I needed: 

1. Configure PAC file in Firefox preferences. 
2. Open about:config and set network.proxy.socks_remote_dns to true. I 
need this because the server side of my SSH tunnel uses a 
permitopen=site1.domain.com key to restrict access for each user. 
Firefox needs to send the domain before it's resolved. 
3. Change the PAC file to specify SOCKS5 instead of just SOCKS. 
Otherwise it appears that the domain is still resolved locally as 
sending domains is only part of SOCKS v5 protocol and I guess the 
default is NOT to use SOCKS v5 if "SOCKS" is used. 

I've also seen in Bugzilla for Firefox that there's been a large 
discussion on this topic, but it's hard to tell where it was left. The 
problem is that Mac OS X and Safari don't work if I specify "SOCKS5" 
for the return value in the PAC file. This leaves me with the need to 
have 2 different PAC files based on the browser used. This is of 
course far from ideal.  Maybe there is some approach that preserves 
browser compatibility, but I haven't found it. I would like to see 
Firefox come up with a solution that preserves the functionality I 
need without breaking compatibility. 

Comment 124

[Mike Shaver (:shaver emeritus)]

Could you not test the user agent in the PAC file, and use SOCKS or SOCKS5 depending on what you found?

Though if Safari can get away with that behaviour too, maybe we should do the same in FF3...

Comment 125

[David Multer]

From what I read, I am very limited in the functions I can call from a PAC file. The listing of functions in the PAC spec does not include anything I can use to detect the user agent (browser). They seemed pretty explicit that you can't do everything JavaScript can do. If you know otherwise, I would be more than happy to alter my PAC file so I can have one version.

Again, it's very hard to tell from this bug thread what things have been decided (at least to me), but it sure would be nice if the PAC file defaulted to SOCKS v5 when using just "SOCKS" as the return value. I know you have many compatiblity problems to tackle with anything like this.

Comment 126

[Gert Doering]

Hi,

actually this functionality (servers-side DNS resolving for SOCKS5) is something that would also help a lot if you connect to servers that have IPv6 addresses in DNS.

Right now, when Firefox is set to use a SOCKS proxy, and the target server has IPv4 and IPv6, it will do *nothing* - it seems to decide internally "I want to do IPv6, I cannot do that over SOCKS, so I don't do anything".  It doesn't fall back to IPv4 either.  This can be worked around by turning off IPv6 in the about:config - but turning off IPv6 is a workaround I really dislike.

As my laptop travels quite a lot between IPv6-enabled networks and networks where I need to use SOCKS (and connect to servers that have IPv6), turning on and off IPv6 all the time is a real nuisance.

Example server: http://www.space.net/

Comment 127

[benc]

Since Comment 122, this bug has started to drift. From what I can tell, this stuff is working in core right? (I'll worry about UI later, it looks like the UI never took the flag).

If so, can we resolve/fixed?

Comment 128

[BarkerJr]

Yes, but I'd suggest providing the bug # for the UI first, so everyone can move their votes.

Comment 129

[benc]

I would file a new bug, but I don't even know what the expected behavior is, so I don't know if we need a UI bug. 

If it is:

Not used for SOCKS4, settable for only SOCKS5, and not used in PAC for SOCKS, then I don't think we need a UI pref.

Comment 130

[jag (Peter Annema)]

With network.proxy.socks_remote_dns set to true (about:config in Firefox, UI in SeaMonkey) names are resolved on the remote (SOCKS5) host. Haven't tried with SOCKS4.

Mind you, due to DNS prefetch (bug 453403) you'll still see some DNS traffic locally (see bug 488162), but in principle this works (internal names that aren't externally visible resolve just fine for me).

I think we can mark this bug fixed. File a new bug on adding pref UI to Firefox or enabling remote DNS by default when SOCKS5 is used.

If the mixed IPv4/IPv6 from comment 126 is still an issue, please file a follow-up bug on that as well.

Comment 131

[Chen Ze]

Mac OSX 10.5.8 & Firefox 3.5.8

when with network.proxy.socks_remote_dns set to true:

only "Manual proxy configuration" works . It not works with "Automatic proxy configuration URL" and "Use system's proxy configuration".

I use "tcpdump -X -s0 -i ppp0 port domain" to test. when with "Automatic proxy configuration URL", the dns lookup is via nornal network, not via socks5.

Here you can find my pac file: http://nehcfrus.googlecode.com/svn/trunk/network/surfchen.pac 

FYI: The system's proxy configuration is pac-based too.

Comment 132

[Eric Jung [:ericjung]]

(In reply to comment #131)
> when with network.proxy.socks_remote_dns set to true:
> 
> only "Manual proxy configuration" works . It not works with "Automatic proxy
> configuration URL" and "Use system's proxy configuration".

There is no guarantee the PAC file returns a SOCKS proxy server.

Comment 133

[Chen Ze]

(In reply to comment #132)
> (In reply to comment #131)
> > when with network.proxy.socks_remote_dns set to true:
> > 
> > only "Manual proxy configuration" works . It not works with "Automatic proxy
> > configuration URL" and "Use system's proxy configuration".
> 
> There is no guarantee the PAC file returns a SOCKS proxy server.

Yes. But the PAC file is written by myself and the URL I used to test is the URL that will let PAC return SOCKS server. 

I event tested with this PAC configuration:
function FindProxyForURL(url, host) {
        return "SOCKS 127.0.0.1:9060";
}

Comment 134

[windsor.locks]

I confirm this. I realized this just yesterday that DNS quries are not being sent over proxy connection in case of proxy.pac file. Here is my proxy.pac

 function FindProxyForURL(url, host) {
      if(isInNet(host, "192.168.*.*", "255.255.0.0"))  { 
      	return "DIRECT";
      } else if (host == "localhost") {
		return "DIRECT";
      } else {
		 return "PROXY localhost:9000";
      }
   }

Comment 135

[Eric Jung [:ericjung]]

(In reply to comment #134)
>          return "PROXY localhost:9000";
>       }
>    }

HTTP(S) proxies don't support remote DNS lookups; only SOCKS 4a and 5.

Comment 136

[windsor.locks]

(In reply to comment #135)
> (In reply to comment #134)
> >          return "PROXY localhost:9000";
> >       }
> >    }
> 
> HTTP(S) proxies don't support remote DNS lookups; only SOCKS 4a and 5.

Well I expected it to work because DNS queries over HTTP proxy works if I do it manually.

Comment 138

[Mike Kaply [:mkaply]]

So why is this bug still open?

Comment 139

[Malcolm Smith]

(In reply to Michael Kaply (mkaply) from comment #138)
> So why is this bug still open?

The last official word was Darin in comment #97.  I think this boils down to:
1. The UI changes for Firefox never happened (although SeaMonkey has UI.)
2. The error handling for SOCKS is (was?) busted.  If a connection attempt is made via SOCKS, there was no way to disambiguate connection errors to the proxy vs. connection errors to the final host.  I don't remember where the code settled on this - Darin was concerned about empty pages being returned to the user without meaningful errors, but I wanted this to go further and provide meaningful errors.  It's possible that this has all changed in the last 7 years.
3. PAC handling is just odd.  I don't know what happened here either, but a PAC can filter by name or IP, which doesn't work when the client has no idea what the IP is going to be.  (How does this work for HTTP proxies?)
4. The default issue.  It makes sense (to me at least) that this be enabled by default for SOCKS5 since the protocol requires support for it.  Even the most common remaining SOCKS4 servers like MS Proxy server/ISA server/TMG server support 4a.  There is obviously a compatibility risk with changing any default though, and the default before this change happened was client-side name resolution, since that was all that existed.

That said, it would be logical to me to close this bug and open any new, more specific bugs for completeness of this feature.

Comment 140

[Malcolm Smith]

5. There's also no way to control this setting via a PAC file, which would be strongly desirable.  Without this, a PAC file will do different things depending on a client-specific setting, and if a site requires a particular setting they need to push down custom config, which defeats the purpose of having a PAC file.  I investigated how to make this work, but it would need some changes to make this happen in a backwards compatible (and cross browser compatible) way.

Comment 141

[Mike Kaply [:mkaply]]

Darin has long since been off the project.

Hopefully some networking person will pick this up.

Comment 142

[356345635656]

Has this been resolved for FF 11 yet?

Comment 143

[Jesus Cea]

More than ten years and counting... amazing :-)

Please, vote for this bug to be fixed.

Comment 144

[Jesus Cea]

Comment #131 reproductable here too :-(.

Comment 145

[Oliver.agar]

(In reply to Gert Doering from comment #126) - I can confirm that the bug for proxy servers with both IPv4 and IPv6 still exists in Firefox 15.0.1

No dns queries are made.

Comment 146

[troy]

Still exists in 18.0 and 19.0a2.

Comment 147

[Jesus Cea]

Ping...

Comment 148

[Soo-Hyun Choi]

still reproducible with Firefox 24.0 on Ubuntu 13.04 (x86_64).

Comment 149

[nedR]

I see a "Remote DNS" option for Socks 5 in firefox preferences (Firefox 30.0, Ubuntu)
Does this mean this issue is fixed?

Comment 150

[David Carlin]

Works when I setup a Manual configuration, but does not work via PAC file.  Firefox 35.0.1 on MacOS X 10.9.5

Comment 151

[Mast]

I can confirm that on FF 46.0.1 thi sis still a bug. I checked the "Remote DNS" checkbox and set SOCKS5 and the DNS requests are still resolve locally, rather than through the proxy (inspected with Wireshark).

This is now 14 years old. There is another bug open for SOCKS5 to allow password auth which is also 10+ years old.

Rather than adding minor changes, invisible to most users, why not fix such problems? Currently FF is not SOCKS5 compliant ...

Comment 152

[Masatoshi Kimura [:emk]]

We already added a pref UI (bug 665319).

Comment 153

[Gary Chen [:xeonchen]]

I've tested on:
- Firefox 47.0.1, 48.0.2 on Windows 10 x64
- Firefox 51.0a1 on OS X El Capitan

Both works well (inspected w/ Wireshark), the DNS records are not resolved locally if the "Remote DNS" option is checked.

The patch has landed ([1], comment 152).
I think this works and should be closed.

[1] https://github.com/mozilla/gecko-dev/commit/d19eff04b7231e6d4c7f0a95d03862bf1f708c40

Comment 154

[Jason Duell]

OK folks: at this point we're unable to reproduce this bug locally.  If you are still experiencing this issue please let us know.  And if you can, also send us an HTTP log (it looks to me like our DNS code may need some logging annotations added to it to be fully useful for tracking this down, but even the existing logging would be a lot better than nothing):

   https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/HTTP_logging#Turning_off_DNS_query_logging

Thanks for looking into this Gary! Let's keep this bug open for a while to see if anyone replies.

Comment 155

[Jason Duell]

Ignore the #Turning_off_DNS_query_logging part of the URI in the last comment (don't disable DNS logging!).

Comment 156

[Zero King]

Will Thunderbird get a pref UI too?

Comment 157

[Gary Chen [:xeonchen]]

It's almost a year after comment 154, I think it's time to close this.