1211567 - Add support for domain socket/fifo connection to proxy.

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Dave Huseby [:huseby]]

The Tor Project would like us to add support for Firefox making a domain socket/fifo connection to a local proxy.  The proposal is to modify the proxy settings to accept a "file://" URI in the Firefox connection settings.  

The benefit of this change is that, when set, Firefox can be denied from making any networking system calls using OS-based security settings (e.g. seccomp). This eliminates whole classes of network based de-anonymization attacks in the Tor Browser.  It would also benefit Firefox as a "hardening" enhancement.

Comment 1

[Dave Huseby [:huseby]]

Related Tor trac tickets:

https://trac.torproject.org/projects/tor/ticket/14270
https://trac.torproject.org/projects/tor/ticket/14271
https://trac.torproject.org/projects/tor/ticket/14272
https://trac.torproject.org/projects/tor/ticket/14273

Comment 2

[Christopher Soghoian]

Dear Team Mozilla,

As researcher who studies how government agencies attack and seek to identify Tor users, I am keenly following this bug.
This feature would be a total game changer, and would be a great way for a tech organization like Mozilla to help to protect user privacy in an area where law and policy are lagging so far behind.

Comment 3

[Gary Chen [:xeonchen]]

Attachment: torrc

This creates a domain socket that redirects traffics to local Tor port.

Comment 4

[Gary Chen [:xeonchen]]

Attachment: 0001-WIP.patch

[WIP] this enables the ability connecting to Tor SOCKS proxy via domain socket.

Comment 5

[Gary Chen [:xeonchen]]

Attachment: 0001-Bug-1211567-Enable-domain-socket-support-for-SOCKS-r.patch

Hi Daniel,

This patch adds support for domain socket connection to SOCKS server,
I think it basically works but needs to be polished, would you take a look at this?

Comment 6

[Gary Chen [:xeonchen]]

Attachment: bypass proxy port UI checking

Comment 7

[Daniel Stenberg [:bagder]]

I think this looks like a perfectly fine approach.

I think my biggest problem is the fact that this still store a path in the variable for host name 'mSOCKSProxyHost' which may lead to confusions or get downright misleading when reading the code. I think that can be fixed by either renaming the field (which I think I prefer) or by adding clear comments around it when used for non-hostname purposes.

I'm also not a big fan of adding #ifdefs in code, so I would prefer if the nsSOCKSIOLayer.cpp changes would make the IsHostDomainSocket function return false on non-unix so perhaps the ConnectToProxy and DoHandshake methods could be written without #ifdefs?

Comment 8

[Gary Chen [:xeonchen]]

Attachment: Bug 1211567 - Enable domain socket support for SOCKS;

Review commit: https://reviewboard.mozilla.org/r/68878/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68878/

Comment 9

[Gary Chen [:xeonchen]]

Attachment: Bug 1211567 - bypass port checking for local domain socket proxy;

Review commit: https://reviewboard.mozilla.org/r/68880/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68880/

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8777279 [details]
Bug 1211567 - bypass port checking for local domain socket proxy;

https://reviewboard.mozilla.org/r/68880/#review66096

::: browser/components/preferences/connection.js:27
(Diff revision 1)
>        // Only worry about ports which are currently active. If the share option is on, then ignore
>        // all ports except the HTTP port
> -      if (proxyPref.value != "" && proxyPortPref.value == 0 &&
> +      if (proxyPref.value != "" && proxyPref.value.charAt(0) != "/" &&

Please add a test case to browser/components/preferences/in-content/tests/browser_connection_bug388287.js

::: browser/components/preferences/connection.js:29
(Diff revision 1)
> -      if (proxyPref.value != "" && proxyPortPref.value == 0 &&
> +      if (proxyPref.value != "" && proxyPref.value.charAt(0) != "/" &&
> +            proxyPortPref.value == 0 &&

Nit: .startsWith is easier to read

Comment 11

[Gary Chen [:xeonchen]]

Comment on attachment 8777279 [details]
Bug 1211567 - bypass port checking for local domain socket proxy;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68880/diff/1-2/

Comment 12

[Gary Chen [:xeonchen]]

(In reply to Matthew N. [:MattN] from comment #10)
> Comment on attachment 8777279 [details]
> Bug 1211567 - bypass port checking for local domain socket proxy;
> 
> https://reviewboard.mozilla.org/r/68880/#review66096
> 
> ::: browser/components/preferences/connection.js:27
> (Diff revision 1)
> >        // Only worry about ports which are currently active. If the share option is on, then ignore
> >        // all ports except the HTTP port
> > -      if (proxyPref.value != "" && proxyPortPref.value == 0 &&
> > +      if (proxyPref.value != "" && proxyPref.value.charAt(0) != "/" &&
> 
> Please add a test case to
> browser/components/preferences/in-content/tests/browser_connection_bug388287.
> js
> 

Thank you Matthew, I've added the test case.

Comment 13

[Daniel Stenberg [:bagder]]

Comment on attachment 8777278 [details]
Bug 1211567 - Enable domain socket support for SOCKS;

https://reviewboard.mozilla.org/r/68878/#review67058

Comment 14

[Gary Chen [:xeonchen]]

Comment on attachment 8777278 [details]
Bug 1211567 - Enable domain socket support for SOCKS;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68878/diff/1-2/

Comment 15

[Gary Chen [:xeonchen]]

Comment on attachment 8777279 [details]
Bug 1211567 - bypass port checking for local domain socket proxy;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68880/diff/2-3/

Comment 16

[Gary Chen [:xeonchen]]

Comment on attachment 8777278 [details]
Bug 1211567 - Enable domain socket support for SOCKS;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68878/diff/2-3/

Comment 17

[Gary Chen [:xeonchen]]

Comment on attachment 8777279 [details]
Bug 1211567 - bypass port checking for local domain socket proxy;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68880/diff/3-4/

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8777279 [details]
Bug 1211567 - bypass port checking for local domain socket proxy;

https://reviewboard.mozilla.org/r/68880/#review68524

I'm hesitating the give r+ since there are some concerns which I would like addressed first.

::: browser/components/preferences/connection.js:1
(Diff revision 4)
>  /* -*- indent-tabs-mode: nil; js-indent-level: 4 -*- */

A comment for connection.xul / connection.dtd:

Unless there are plans to revamp the UI we should consider changing the "SOCKS Host:" label as we no longer only expect a "host". Can we use "SOCKS Proxy:" to match the other labels on the page?

::: browser/components/preferences/connection.js:30
(Diff revision 4)
>        let proxyPortPref = document.getElementById("network.proxy." + prefName + "_port");
>        let proxyPref = document.getElementById("network.proxy." + prefName);
>        // Only worry about ports which are currently active. If the share option is on, then ignore
>        // all ports except the HTTP port
>        if (proxyPref.value != "" && proxyPortPref.value == 0 &&
> +            !(proxyPref.value.startsWith("/") && prefName == "socks") &&

It looks like bug 1287994 will have to fix this for Windows support but this is fine for now.

It also seems like we're no longer going to use a file:// URI and instead are using a file path. I'm not sure if that's an intentional change from comment 0.

::: browser/components/preferences/connection.js:32
(Diff revision 4)
>          document.getElementById("networkProxy" + prefName.toUpperCase() + "_Port").focus();
>          return false;
>        }

I think there is a possibility for confusion when a socket is specified and the port is also filled in (non-zero). It may be worth considering slightly different UI with a toggle between using a socket or a port. That doesn't need to block this bug if you don't want.

Comment 19

[Gary Chen [:xeonchen]]

(In reply to Matthew N. [:MattN] from comment #18)
> Comment on attachment 8777279 [details]
> Bug 1211567 - bypass port checking for local domain socket proxy;
> 
> https://reviewboard.mozilla.org/r/68880/#review68524
> 
> I'm hesitating the give r+ since there are some concerns which I would like
> addressed first.
> 

Thanks for reviewing, seems it's more complicated than I thought on UI side.
Maybe it's time to move this to a follow-up bug 1294611.

> ::: browser/components/preferences/connection.js:1
> (Diff revision 4)
> >  /* -*- indent-tabs-mode: nil; js-indent-level: 4 -*- */
> 
> A comment for connection.xul / connection.dtd:
> 
> Unless there are plans to revamp the UI we should consider changing the
> "SOCKS Host:" label as we no longer only expect a "host". Can we use "SOCKS
> Proxy:" to match the other labels on the page?
> 
> ::: browser/components/preferences/connection.js:30
> (Diff revision 4)
> >        let proxyPortPref = document.getElementById("network.proxy." + prefName + "_port");
> >        let proxyPref = document.getElementById("network.proxy." + prefName);
> >        // Only worry about ports which are currently active. If the share option is on, then ignore
> >        // all ports except the HTTP port
> >        if (proxyPref.value != "" && proxyPortPref.value == 0 &&
> > +            !(proxyPref.value.startsWith("/") && prefName == "socks") &&
> 
> It looks like bug 1287994 will have to fix this for Windows support but this
> is fine for now.
> 
> It also seems like we're no longer going to use a file:// URI and instead
> are using a file path. I'm not sure if that's an intentional change from
> comment 0.
> 

Hi Dave, do you think "file://" is better and direct file path?
I didn't use protocol identifier because we don't have that for original fields.

> ::: browser/components/preferences/connection.js:32
> (Diff revision 4)
> >          document.getElementById("networkProxy" + prefName.toUpperCase() + "_Port").focus();
> >          return false;
> >        }
> 
> I think there is a possibility for confusion when a socket is specified and
> the port is also filled in (non-zero). It may be worth considering slightly
> different UI with a toggle between using a socket or a port. That doesn't
> need to block this bug if you don't want.

Comment 20

[Dave Huseby [:huseby]]

Are we not using "file://" because the box usually takes an IP dotted quad or FQDN and not a full URI?  Ideally, we'd have some UI element that allowed the user to choose between network socket and domain socket.  When they choose domain socket, we give them a button that opens the file picker dialog.

For now, having raw paths will be fine, but please make sure we're handling it correctly. Parsing paths is notoriously difficult.  I also think we need to take a closer look at the automatic proxy configuration code to decide if it will be safe to allow that mechanism to set up a domain socket proxy connection.  That seems pretty dangerous to me.

Comment 21

[Gary Chen [:xeonchen]]

Comment on attachment 8777278 [details]
Bug 1211567 - Enable domain socket support for SOCKS;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68878/diff/3-4/

Comment 22

[Gary Chen [:xeonchen]]

Comment on attachment 8777278 [details]
Bug 1211567 - Enable domain socket support for SOCKS;

Hi Daniel,

I've made it support "file:" based path instead of direct path.
And some minor changes since your last review are to fix errors on try server.

Hi Dave,
Do you think any potential risk still had to be revised?

Comment 23

[Daniel Stenberg [:bagder]]

Comment on attachment 8777278 [details]
Bug 1211567 - Enable domain socket support for SOCKS;

https://reviewboard.mozilla.org/r/68878/#review70968

::: netwerk/socket/nsSOCKSIOLayer.cpp:148
(Diff revision 4)
> +        }
> +
> +        aProxyAddr->raw.family = AF_UNIX;
> +        memcpy(aProxyAddr->local.path,
> +               path.get(),
> +               sizeof(aProxyAddr->local.path));

This is a string right? Won't this easily read beyond the buffer if the path is very short and the local.path buffer is very big? And why would it always copy the buffer size instead of just the path length?

Comment 24

[Gary Chen [:xeonchen]]

Comment on attachment 8777278 [details]
Bug 1211567 - Enable domain socket support for SOCKS;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68878/diff/4-5/

Comment 25

[Daniel Stenberg [:bagder]]

Comment on attachment 8777278 [details]
Bug 1211567 - Enable domain socket support for SOCKS;

https://reviewboard.mozilla.org/r/68878/#review71622

::: netwerk/socket/nsSOCKSIOLayer.cpp:154
(Diff revisions 4 - 5)
> +          NS_WARNING("domain socket path too long.");
> +          return NS_ERROR_FAILURE;
>          }
>  
>          aProxyAddr->raw.family = AF_UNIX;
> -        memcpy(aProxyAddr->local.path,
> +        strcpy(aProxyAddr->local.path, path.get());

This is better, but isn't this still risking a one byte overrun?

strcpy copies LENGTH + a zero byte number of bytes to the destination.

The check above that protects against a buffer overrun checks if the size of the buffer is smaller than LENGTH.

Imagine the buffer size being 5 bytes and the string length is 5 bytes. Then the first check will equal false (it isn't smaller) but the subsequent strcpy() will overwrite the last byte.

This could be fixed by making the size check use <= instead of just <.

Comment 26

[Gary Chen [:xeonchen]]

Comment on attachment 8777278 [details]
Bug 1211567 - Enable domain socket support for SOCKS;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68878/diff/5-6/

Comment 27

[Gary Chen [:xeonchen]]

Comment on attachment 8777278 [details]
Bug 1211567 - Enable domain socket support for SOCKS;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68878/diff/6-7/

Comment 28

[Daniel Stenberg [:bagder]]

Comment on attachment 8777278 [details]
Bug 1211567 - Enable domain socket support for SOCKS;

https://reviewboard.mozilla.org/r/68878/#review71640

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d58a180022cf
Enable domain socket support for SOCKS; r=bagder

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/d58a180022cf

Comment 31

[Mark Smith [:mcs]]

See bug 1308275 for a follow up issue.