Add support for domain socket/fifo connection to proxy.

RESOLVED FIXED in Firefox 51

Status

()

defect
P2
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: huseby, Assigned: xeonchen)

Tracking

(Blocks 3 bugs)

Trunk
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [tor][necko-active][proxy])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Assignee: nobody → huseby
Whiteboard: [tor]
Target Milestone: --- → mozilla45
(Reporter)

Updated

4 years ago
Depends on: 892114
(Reporter)

Updated

4 years ago
Version: unspecified → Trunk
(Reporter)

Updated

4 years ago
Status: NEW → ASSIGNED
Whiteboard: [tor] → [tor][necko-backlog]
(Reporter)

Updated

3 years ago
Status: ASSIGNED → NEW
(Assignee)

Updated

3 years ago
Assignee: huseby → xeonchen
Status: NEW → ASSIGNED
Whiteboard: [tor][necko-backlog] → [tor][necko-active]
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.
(Assignee)

Comment 3

3 years ago
Posted file torrc
This creates a domain socket that redirects traffics to local Tor port.
(Assignee)

Comment 4

3 years ago
Posted patch 0001-WIP.patch (obsolete) — Splinter Review
[WIP] this enables the ability connecting to Tor SOCKS proxy via domain socket.
(Assignee)

Comment 5

3 years ago
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?
Attachment #8766673 - Attachment is obsolete: true
Attachment #8770463 - Flags: feedback?(daniel)
(Assignee)

Comment 6

3 years ago
Posted patch bypass proxy port UI checking (obsolete) — Splinter Review
(Reporter)

Updated

3 years ago
Priority: -- → P1
(Reporter)

Updated

3 years ago
Priority: P1 → P2
Depends on: 1287994
(Assignee)

Updated

3 years ago
See Also: → 1288308
(Assignee)

Updated

3 years ago
No longer depends on: 1287994
See Also: → 1287994
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?
Attachment #8770463 - Flags: feedback?(daniel) → feedback+
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
(Assignee)

Comment 11

3 years ago
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/
Attachment #8777279 - Flags: review?(MattN+bmo)
(Assignee)

Comment 12

3 years ago
(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

3 years ago
mozreview-review
Comment on attachment 8777278 [details]
Bug 1211567 - Enable domain socket support for SOCKS;

https://reviewboard.mozilla.org/r/68878/#review67058
Attachment #8777278 - Flags: review?(daniel) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Attachment #8777279 - Flags: review?(MattN+bmo)
(Assignee)

Updated

3 years ago
Blocks: 1294611
(Assignee)

Comment 19

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

Updated

3 years ago
Flags: needinfo?(huseby)
(Reporter)

Comment 20

3 years ago
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.
Flags: needinfo?(huseby) → needinfo?(xeonchen)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 years ago
Attachment #8777279 - Attachment is obsolete: true
(Assignee)

Comment 22

3 years ago
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?
Flags: needinfo?(xeonchen)
Attachment #8777278 - Flags: review?(daniel)
Attachment #8777278 - Flags: review+
Attachment #8777278 - Flags: feedback?(huseby)

Comment 23

3 years ago
mozreview-review
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?
Attachment #8777278 - Flags: review?(daniel) → review-
Comment hidden (mozreview-request)

Comment 25

3 years ago
mozreview-review
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 hidden (mozreview-request)
(Assignee)

Updated

3 years ago
Whiteboard: [tor][necko-active] → [tor][necko-active][proxy]
Comment hidden (mozreview-request)

Comment 28

3 years ago
mozreview-review
Comment on attachment 8777278 [details]
Bug 1211567 - Enable domain socket support for SOCKS;

https://reviewboard.mozilla.org/r/68878/#review71640
Attachment #8777278 - Flags: review?(daniel) → review+

Comment 29

3 years ago
Pushed by gachen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d58a180022cf
Enable domain socket support for SOCKS; r=bagder

Comment 30

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d58a180022cf
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED

Updated

3 years ago
Target Milestone: mozilla45 → mozilla51
Blocks: meta_tor
See bug 1308275 for a follow up issue.
You need to log in before you can comment on or make changes to this bug.