1424820 - "Use system proxy settings" uses SOCKSv4

Status: RESOLVED DUPLICATE of bug 1700857

(Core :: Networking, defect, P2)

Description

[Ryan McCue]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171130160223

Steps to reproduce:

1. Run a SOCKSv5-only proxy (I'm using https://github.com/mscdex/socksv5) on port 1080
2. In macOS System Preferences, open Network Preferences > Proxies > SOCKS Proxy
3. Set the host to 127.0.0.1 and the port to 1080
3. Enable SOCKS Proxy and apply settings
4. In Firefox, set proxy settings to "Use system proxy settings"
5. Load a URL in Firefox


Actual results:

The URL should load, after passing through the proxy.


Expected results:

Firefox displays an "unable to connect" error.

Comment 1

[Ryan McCue]

I believe this is due to this line in nsOSXSystemProxySettings.mm: https://github.com/mozilla/gecko-dev/blob/86897859913403b68829dbf9a154f5a87c4b0638/toolkit/system/osxproxy/nsOSXSystemProxySettings.mm#L293

This appears to be PAC-formatted, which means it should be `SOCKS5 ...` instead of `SOCKS ...`. Using a PAC file with `SOCKS` also causes this issue.

This may also be related to https://bugzilla.mozilla.org/show_bug.cgi?id=624837#c22

Comment 2

[Ryan McCue]

I suspect changing this line to the following should fix it, although I don't have a FF development setup to compile and test.

  aResult.Assign(NS_LITERAL_CSTRING("SOCKS5 ") + proxyHost + nsPrintfCString(":%d", proxyPort) + NS_LITERAL_CSTRING("; SOCKS ") + proxyHost + nsPrintfCString(":%d", proxyPort));

Comment 3

[Amy Chung [:Amy]]

Hi Gary,
Would you take a look at this bug?
Thanks!

Comment 4

[Gary Chen [:xeonchen]]

I think it's very likely the problem is what Ryan mentioned. 
Probably Will can take this?

Comment 5

[Gary Chen [:xeonchen]]

(In reply to Ryan McCue from comment #1)
> I believe this is due to this line in nsOSXSystemProxySettings.mm:
> https://github.com/mozilla/gecko-dev/blob/
> 86897859913403b68829dbf9a154f5a87c4b0638/toolkit/system/osxproxy/
> nsOSXSystemProxySettings.mm#L293
> 
> This appears to be PAC-formatted, which means it should be `SOCKS5 ...`
> instead of `SOCKS ...`. Using a PAC file with `SOCKS` also causes this issue.
> 
> This may also be related to
> https://bugzilla.mozilla.org/show_bug.cgi?id=624837#c22

In the macOS preferences, it doesn't mention anything about SOCKS version, so for better compatibility I think it's better to keep SOCKSv4 instead of SOCKSv5, and that's what current code stands for.

Comment 6

[Ryan McCue]

(In reply to Gary Chen [:xeonchen] (needinfo plz) from comment #5)
> (In reply to Ryan McCue from comment #1)
> > This appears to be PAC-formatted, which means it should be `SOCKS5 ...`
> > instead of `SOCKS ...`. Using a PAC file with `SOCKS` also causes this issue.
> > 
> > This may also be related to
> > https://bugzilla.mozilla.org/show_bug.cgi?id=624837#c22
> 
> In the macOS preferences, it doesn't mention anything about SOCKS version,
> so for better compatibility I think it's better to keep SOCKSv4 instead of
> SOCKSv5, and that's what current code stands for.

I was imprecise in my language there; I meant "for this specific connection it would need `SOCKS5 ...`".

IMO, returning `SOCKS5 ...; SOCKS ...` is the best solution, although potentially flipping the order would be better for backwards compatibility with users expecting that v4 will be used.

Also FWIW, using a SOCKSv5-only proxy works in both apps using the system's networking stack (Safari, other WebKit) and Chrome (which I assume is not).

Comment 7

[Kershaw Chang [:kershaw]]

Amy is going to take care of this.

Comment 11

[Michal Novotny [:michal]]

(In reply to Ryan McCue from comment #6)

IMO, returning SOCKS5 ...; SOCKS ... is the best solution, although
potentially flipping the order would be better for backwards compatibility
with users expecting that v4 will be used.

I did a quick test and when both SOCKS versions are returned at https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/toolkit/system/unixproxy/nsUnixSystemProxySettings.cpp#369 the second proxyInfo is available as mProxyInfo->mNext at https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/netwerk/base/nsSocketTransport2.cpp#1180, but AFAICS there is no way how to get the info using nsIProxyInfo interface.

Dragana, is the right solution to this bug to expose the whole list of proxyInfos via nsIProxyInfo and try to use next proxyInfo in nsSocketTransport::RecoverFromError() when proxy returns an error?

Comment 12

[Dragana Damjanovic [:dragana]]

(In reply to Michal Novotny [:michal] from comment #11)

(In reply to Ryan McCue from comment #6)

IMO, returning SOCKS5 ...; SOCKS ... is the best solution, although
potentially flipping the order would be better for backwards compatibility
with users expecting that v4 will be used.

I did a quick test and when both SOCKS versions are returned at https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/toolkit/system/unixproxy/nsUnixSystemProxySettings.cpp#369 the second proxyInfo is available as mProxyInfo->mNext at https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/netwerk/base/nsSocketTransport2.cpp#1180, but AFAICS there is no way how to get the info using nsIProxyInfo interface.

Dragana, is the right solution to this bug to expose the whole list of proxyInfos via nsIProxyInfo and try to use next proxyInfo in nsSocketTransport::RecoverFromError() when proxy returns an error?

This is specific to SOCKS proxy. We can expose mProxyInfo->mNext in nsIProxyInfo, but do a fallback only for SOCKS proxies (thinking of it we probably do not have a fallback for other proxies).
We will need to remember that a socks proxy version does not work so that we do not need to do fallbacks all the time, maybe remember that in nsSocketTransportService. But we need to clear the info if other version fails as well. I think it is fine to keep the info for a session.

Comment 13

[josh9051]

Any update?

Comment 14

[Kershaw Chang [:kershaw]]

(In reply to josh9051 from comment #13)

Any update?

I'm trying to fix this issue in bug 1700857.