751465 - Websockets leak DNS requests (Tor 5741)

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Georg Koppen]

Websockets are leaking DNS requests. In order to reproduce this choose a proxy, start your favorite network traffic analyzer and go to http://bitcoincharts.com. DNS leaks will show up in the traffic log. It does not matter whether a SOCKS(5) or an HTTP proxy is used. The problem is "solved" if websockets are disabled per preference.
For the original bugreport see: https://trac.torproject.org/projects/tor/ticket/5741#comment:1

Comment 1

[Georg Koppen]

Copy & Paste fail, the original bugreport is found here: https://trac.torproject.org/projects/tor/ticket/5741 (without pointing to the first comment)

Comment 2

[Jason Duell]

I need to fix some websockets + proxy issues soon anyway, so I'll look into this while I'm at it.

Thanks for the bug report.

Comment 3

[Jason Duell]

Note from the patch Tor used to resolve this (does the diagnosis sound right to you Patrick?):

   https://gitweb.torproject.org/torbrowser.git/blob/maint-2.2:/src/current-patches/firefox/0018-Prevent-WebSocket-DNS-leak.patch

This is due to an improper implementation of the WebSocket spec by Mozilla.

"There MUST be no more than one connection in a CONNECTING state.  If multiple connections to the same IP address are attempted simultaneously, the client MUST serialize them so that there is no more than one connection at a time running through the following steps.

If the client cannot determine the IP address of the remote host (for example, because all communication is being done through a proxy server that performs DNS queries itself), then the client MUST assume for the purposes of this step that each host name refers to a distinct remote host,"

https://tools.ietf.org/html/rfc6455#page-15

They implmented the first paragraph, but not the second...

Comment 4

[Patrick McManus [:mcmanus]]

(In reply to Jason Duell (:jduell) from comment #3)
> Note from the patch Tor used to resolve this (does the diagnosis sound right
> to you Patrick?):
> 
>  

that's the source of the confusion, but our implementation does what the spec asks.

(In reply to Jason Duell (:jduell) from comment #3)

> "There MUST be no more than one connection in a CONNECTING state.  If
> multiple connections to the same IP address are attempted simultaneously,
> the client MUST serialize them so that there is no more than one connection
> at a time running through the following steps.
> 
> If the client cannot determine the IP address of the remote host (for
> example, because all communication is being done through a proxy server that
> performs DNS queries itself), then the client MUST assume for the purposes
> of this step that each host name refers to a distinct remote host,"
> 

the spec does not say if you are using a proxy you should not do the lookup. It says if the lookup fails (perhaps due to a proxy) assume each host is unique. Just because you have a proxy doesn't mean the lookup will necessarily fail - so we try it anyhow.

you could choose to change that behavior very easily and reasonably.. but what's there isn't an issue with being out of spec.

to change it I would probly just call onlookupcomplete() directly out of applyforadmission instead of going through the asyncresolve() stage.

Comment 5

[Alex Keybl [:akeybl]]

(In reply to Patrick McManus [:mcmanus] from comment #4)
> to change it I would probly just call onlookupcomplete() directly out of
> applyforadmission instead of going through the asyncresolve() stage.

What negative user impact would making this change have? I'd lean towards helping Tor work out of the box if there is no other negative user impact.

Comment 6

[Patrick McManus [:mcmanus]]

(In reply to Alex Keybl [:akeybl] from comment #5)
> (In reply to Patrick McManus [:mcmanus] from comment #4)
> > to change it I would probly just call onlookupcomplete() directly out of
> > applyforadmission instead of going through the asyncresolve() stage.
> 
> What negative user impact would making this change have? I'd lean towards
> helping Tor work out of the box if there is no other negative user impact.

It could result in extra simultaneous connections to servers presenting a mild server dos vector that could otherwise be prevented. That is why the spec is written the way it is. But its not a big deal.

Comment 7

[Jason Duell]

So I'm new to SOCKS and our proxy code in general, and I've got a couple of questions as to how to best fix this for TOR and proxies in general.

1) I see very similar code to what websockets is doing in our HTMLDNSPrefetch
   code:

  http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLDNSPrefetch.cpp#184

Georg:  do you also see DNS "leaks" (i.e. network.proxy.socks_remote_dns not being respected) from HTML link prefetching (when we read in HTML pages, we issue DNS requests for all the links in the page, and it looks to me like these should "leak" the same way websockets is here).

2) The only code that currently looks at network.proxy.socks_remote_dns is nsProtocolProxyService.  In the common case (Resolve_Internal) it turns on TRANSPARENT_PROXY_RESOLVES_HOST only if socks_remote_dns is set (i.e if a user has manually set it in about:config):

   http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsProtocolProxyService.cpp#643
   
But if we're using a PAC file, we also turn it on (for all users, without socks_remote_dns needing to be set) if we see a SOCKS5 proxy is being used:

   http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsProtocolProxyService.cpp#643

Is there some reason to automatically turn on remote_dns for SOCKS5 only for PAC proxying?

3) Is there some reason we don't have nsDNSService also respect socks_remote_dns and/or automatically use it if SOCKS5 will be used? It seems that one reason is addons/extensions which rely on Resolve() working.  If I understand correctly, disabling all DNS requests to nsIDNSService will break addons that count on doing DNS for any purposes besides making an HTTP/FTP/etc connection:

   https://trac.torproject.org/projects/tor/wiki/doc/TorFAQ#HowdoIcheckifmyapplicationthatusesSOCKSisleakingDNSrequests

Is that a trade-off that TOR is OK with? 

I like that the patch in comment 3 only disables DNS if socks_remote_dns is set: this seems like a conservative solution in that it will only affect users who have manually set about:config.  (I'm assuming TOR is the main/only client of the socks_remote_dns setting--true?)

4) It says here that SOCKS5 could support doing DNS requests securely from the browser:  

 http://en.wikipedia.org/wiki/SOCKS#SOCKS5

Am I correct in assuming that would require firefox to have an internal DNS resolver?  It would be nice if we could get DNS prefetching and other early forms of resolution, even when using SOCKS.  But if it requires crafting UDP DNS requests, we don't have the capablity for now.

That's a lot of questions!  :)   Any answers would be useful

Comment 8

[Jason Duell]

Patrick, are you suggesting that we skip to onlookupcomplete only when network.proxy.socks_remote_dns is set (which is what the TOR patch does), or that we use some other method to detect when that's the right thing to do (like use nsIProtocolProxyService to figure out whether a proxy is being used, and what kind, and skip DNS for certain kinds of proxies)?

I assume we want for efficiency reasons to try to keep doing DNS here for proxies that don't care about the "leak", else we'll delay resolving the DNS until the HTTP channel is actually opened.

Comment 9

[Patrick McManus [:mcmanus]]

(In reply to Jason Duell (:jduell) from comment #8)
> Patrick, are you suggesting that we skip to onlookupcomplete only when
> network.proxy.socks_remote_dns is set (which is what the TOR patch does), or
> 

That or any proxy I guess.

this is really a feature request imo. certainly not a trackable item.

Comment 10

[Jason Duell]

> any proxy

It just seems sad to delay DNS until connection time for all proxied connections when it's only the ones that want to avoid DNS for privacy reasons that actually need this.

Comment 11

[Georg Koppen]

(In reply to Jason Duell (:jduell) from comment #7)
> Georg:  do you also see DNS "leaks" (i.e. network.proxy.socks_remote_dns not
> being respected) from HTML link prefetching (when we read in HTML pages, we
> issue DNS requests for all the links in the page, and it looks to me like
> these should "leak" the same way websockets is here).

No. I just tested it using some examples I found on https://developer.mozilla.org/en/Link_prefetching_FAQ and there are no leaks. Note: network.prefetch-next is not disabled in the TorBrowser.

> (I'm assuming TOR is the
> main/only client of the socks_remote_dns setting--true?)

No, definitely not.

Comment 12

[Patrick McManus [:mcmanus]]

(In reply to Jason Duell (:jduell) from comment #10)
> > any proxy
> 
> It just seems sad to delay DNS until connection time for all proxied
> connections when it's only the ones that want to avoid DNS for privacy
> reasons that actually need this.

there is no performance issue for this feature when proxying actually.. the DNS being looked up is the ws server address, which the proxy is going to have to look up on its own anyhow (there is no way to send that along to the proxy). so this value, in the case of a proxied connection, is literally only used for DoS prevention as the spec lays out.

just remember that websockets is a little special. It lets a remote website make arbitrary connections from the browser via JS. These can be to anything (including stuff behind the firewall) - so there is a CORS-like mechanism to make sure that the host being connected to opts into this WS session. The reason this provision exists in rfc 6455 is to limit the number of connections to 1 at a time with hosts that are still doing that CORS-like handshake. Its a good policy.

Unfortunately with proxies in use a significant amount of the time the lookup is going to fail due to firewalls etc - not all the time, but a lot of the time. It depends on topology. bypassing this with proxies in play still leaves weaker DoS prevention - its juts based on hostname rather than IP address.

Comment 13

[Tom Lowenthal]

(In reply to Jason Duell (:jduell) from comment #7)

> If I understand correctly, disabling all DNS requests to nsIDNSService will
> break addons that count on doing DNS for any purposes besides making an
> HTTP/FTP/etc connection:
> 
> Is that a trade-off that TOR is OK with? 

Tor would always prefer that connections fail rather than circumvent the proxy. The Tor browser bundle also doesn't care too much about other addons working, especially if the alternative is unproxied requests. Imagine the threat model where you might get put in political prison every time a request leaks outside the proxy.

---

Maybe I'm missing something here, but if I've set a SOCKS4a or SOCKS5 proxy, why should Firefox ever be making any network request that doesn't go through that proxy?

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

will track this for esr13 optimistically, can re-evaluate later if need be.

Comment 15

[Jason Duell]

I don't think we should be tracking this for esr, or even 13/14/15.  This is a fairly TOR-specific request, and for the moment TOR has their own build of Firefox that fixes the issue, so I don't think this is hurting anything on the Internet (correct me if I'm wrong). The add-on bustage in the patch here (DNS would no longer work for add-ons when socks_remote_dns is set) is not something I'd want to slip into esr or anything but an early-cycle nightly.

This is not to say we shouldn't work a lot more to get the TOR patches integrated into our codebase--we should, and the TOR folks have had to wait far too long for a bunch of SOCKS-related work to get done/reviewed by us.  I just don't see us guaranteeing we can get to this in the tracking 13/14/15 timeframe.


> Maybe I'm missing something here, but if I've set a SOCKS4a or SOCKS5 proxy, 
> why should Firefox ever be making any network request that doesn't go through 
> that proxy?

Because we don't have our own DNS resolver.  We may create one at some point, but for now we use the OS's resolver.  We might be able to get rid of all the prefetch DNS calls when SOCKS is used if Patrick is right about them all always being for the proxy's DNS address, but we have no way right now to provide add-ons with DNS queries that pass through SOCKS unless the OS's gethostbyname is set up to use SOCKS.

Comment 16

[Tom Lowenthal]

I don't think that this request is specific to Tor (not TOR). This is a question of whether Firefox complies with the user's proxy settings. Sure, Tor has their own build and the penalties for proxy failure can be very high.

When a user sets a SOCKS5 proxy, they expect all of Firefox's traffic to go through that proxy. There may even be non-Tor users for whom this is dangerously important. SOCKS4a & SOCKS5 are designed so that the proxy deals with DNS, which is in line with the user's expectation anyway. Most people don't understand DNS and don't expect it to bypass their proxy.

Comment 17

[Alex Keybl [:akeybl]]

This doesn't appear to be something we'll resolve in time for FF13's release. Wontfixing.

Comment 18

[Jason Duell]

Georg/Tom,

Is the patch linked to in comment 3 up to date?  It seems like the basic idea of turning off DNS when network.proxy.socks_remote_dns is set is perhaps the best tradeoff for now (no DNS leak for SOCKS, but no DNS for the few add-ons that use it).  If Patrick agrees with me I'll review the patch and we can land it.

Comment 19

[Georg Koppen]

(In reply to Jason Duell (:jduell) from comment #18)
> Georg/Tom,
> 
> Is the patch linked to in comment 3 up to date? 

It is up to date in the sense that it is still deployed in the Tor Browser Bundle. As far as I know it is not tested against the latest nightly. If you think this is a precondition for getting reviewed let me know and I'll do it and get back to you.

Comment 20

[Patrick McManus [:mcmanus]]

i'm ok with that.. though the "literal only" part of the dns change should probly be merged with the pending commit of https://bugzilla.mozilla.org/show_bug.cgi?id=87717 .. that will allow static entries and such too.

Comment 21

[Jerry Baker]

As a user who relies extensively on SOCKS proxying and not a TOR user (and until today erroneously assumed Firefox was respecting my wishes to proxy DNS requests) it's not clear to me what conditions are required before Firefox decides it's time to circumvent my proxy. I'd like to do more testing, but I'm not clear what I should be looking for.

Comment 22

[grarpamp]

Hey all... it's been a year and a half with no action, meanwhile users are still being hit by this. Can we get some traction on this one? What needs to be done to wrap it up? Thanks.

Comment 23

[Mike Perry]

Attachment: Updated WebSocket + DNS patch against FF24.

Comment 24

[Mike Perry]

I attached the latest version of our patch (against FF24.1.1-esr).

Users concerned about proxy bypass should also be aware of Bug 939319, and also the pref 'media.peerconnection.enabled' (to disable WebRTC).

Comment 25

[Bob Clary [:bc] (inactive)]

Mike, you need to pick a person to do the review. Otherwise it will probably not see prompt action.

Comment 26

[Patrick McManus [:mcmanus]]

Comment on attachment 833211 [details] [diff] [review]
Updated WebSocket + DNS patch against FF24.

Review of attachment 833211 [details] [diff] [review]:
-----------------------------------------------------------------

we can address this issue without disabling all of DNS based on a socks preference... and we can do it without relying on prefs. I have an alternate patch that figures out if a proxy is needed and disables the lookups in those cases. The code to do that asynchronously is more recent than the original websocket code, which is why it didn't originally work that way.

so I'm going to r- this particular patch.

Mike, I would however merge a separate reviewed patch in a separate bug that disabled all dns resolutions based on a new preference (network.dns.disable for instance). If you'd like to do that please open a bug in the DNS category and propose a patch and r? :sworkman on it to review. You can reference this comment in the new bug.

Comment 27

[Patrick McManus [:mcmanus]]

Attachment: patch 0

Comment 28

[Patrick McManus [:mcmanus]]

some builds we be available here to try it out

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-3f72ad79f09d/

Comment 29

[Jason Duell]

Comment on attachment 8333882 [details] [diff] [review]
patch 0

Review of attachment 8333882 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for absurd review latency.

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +2241,5 @@
> +    // expect the callback in ::OnLookupComplete
> +    LOG(("WebSocketChannel::ApplyForAdmission: checking for concurrent open\n"));
> +    return DoAdmissionDNS();
> +  }    
> +  

some trailing whitespace in last two lines.

@@ +2333,3 @@
>  
>    if (mStopped) {
>      LOG(("WebSocketChannel::OnLookupComplete: Request Already Stopped\n"));

should we set mCancelable = null here, as you added for OnProxyAvailable?  (or even easier null it out in both right before the if (mStopped) check)

@@ +2340,5 @@
>  
>    // These failures are not fatal - we just use the hostname as the key
>    if (NS_FAILED(aStatus)) {
>      LOG(("WebSocketChannel::OnLookupComplete: No DNS Response\n"));
> +    mURI->GetHost(mAddress);

Took me a couple passes to see why we need the set here.  Maybe add comment:

 // set host here in case we got here w/o calling DoAdmissionsDNS

Or maybe even better, just set mAddress to the Uri Host at the start of ApplyForAdmission.

Comment 30

[Patrick McManus [:mcmanus]]

(In reply to Jason Duell (:jduell) from comment #29)

> 
> should we set mCancelable = null here, as you added for OnProxyAvailable? 

good thought

Comment 31

[Patrick McManus [:mcmanus]]

Comment on attachment 8333882 [details] [diff] [review]
patch 0

Review of attachment 8333882 [details] [diff] [review]:
-----------------------------------------------------------------

bsmedgerg - this patch adds a ISUPPORTS12 - is that ok by you?

Comment 32

[Patrick McManus [:mcmanus]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/302362be5075

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/302362be5075

Comment 34

[Bogdan Maris, Desktop QA]

Ok so I tried to reproduce the initial issue on a older Nightly but after I download Tor Browser Bundle and ran start-tor-browser, it opens the firefox that comes with the Tor package. I used that version of FF to replicate the issue, captured a log using tcpdump and opened the log file in Wireshark. I am confused on where can I see the DNS leaks. Can you assist me on this in order to verify the fix?

Comment 35

[Georg Koppen]

Not sure what you mean but you won't see a DNS proxy bypass with the Tor Browser Bundle. So, what you need is a Nightly < rev 162205 and a Nightly >= rev 162205 and a proxy. If you use a SOCKS proxy you should set "network.proxy.socks_remote_dns" to "true" to make sure you request remote DNS resolution.

Comment 36

[Bogdan Maris, Desktop QA]

I followed all the steps but I still don't know where do I see an example of DNS proxy leak in wireshark. Where do I have to look more exactly? Or could you verify that this is fixed yourself in Firefox 29beta?

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/29.0b7-candidates/build1/linux-i686/en-US/

Comment 37

[Georg Koppen]

Not sure what you are doing wrong. There should be some tutorials out there that describe how to detect a DNS leak. Tor Browser is using an ESR 24 as base, so I am of no direct help for you here and I currently don't have the time to verify the results myself. I have a patch in bug 971153 pending review that fails if there is a Websocket DNS leak and it passes in rev > 162205 but fails in rev < 162205. Not sure if that counts as a verification in your sense, though.

Comment 38

[Ioana (away)]

I tested this on the 01/03 Nightly (rev < 162205) using the same HTTPS proxy server for all protocols. I did see the DNS leak, but only every now and then (quite rarely, to be honest). I flushed the DNS cache before every try.

I also tested this on Firefox 29.0b8 and never saw the leak there, but can't really call this verified given the bug was intermittent from the start for me.