Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#14271 closed enhancement (fixed)

Make Torbutton work with Unix Domain Socket option

Reported by: gk Owned by: brade
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-torbutton, tbb-security, TorBrowserTeam201609
Cc: arthuredelstein, mcs, proper, whonix-devel@… Actual Points:
Parent ID: #14270 Points:
Reviewer: Sponsor: SponsorU

Description (last modified by gk)

Even if it is only for *NIX systems first, we should modify Torbutton to work with the Unix Domain Socket option.

Child Tickets

Change History (23)

comment:1 Changed 4 years ago by gk

Description: modified (diff)
Summary: Make Torbutton work with SocksSocket optionMake Torbutton work with Unix Domain Socket option

comment:2 Changed 4 years ago by proper

As far I understand, this is up to Make Tor Launcher work with Unix Domain Socket option (#14272). What is Tor Button's role in this?

comment:3 Changed 4 years ago by gk

Owner: changed from gk to brade
Status: newassigned

comment:4 in reply to:  2 Changed 4 years ago by brade

Cc: arthuredelstein mcs proper added

Replying to proper:

As far I understand, this is up to Make Tor Launcher work with Unix Domain Socket option (#14272). What is Tor Button's role in this?

Torbutton includes code that interacts with tor via the control port (e.g., newnym, circuit display). If we disable all TCP in Tor Browser, control port communication will need to be via a Unix domain socket as well (so Torbutton will need to be modified).

comment:5 Changed 4 years ago by proper

Cc: whonix-devel@… added

comment:6 Changed 3 years ago by gk

Priority: MediumHigh
Sponsor: SponsorU

comment:7 Changed 3 years ago by gk

Keywords: TorBrowserTeam201608 added

Getting important SponsorU things on our August radar.

comment:8 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201608R added; TorBrowserTeam201608 removed
Severity: Normal
Status: assignedneeds_review

Here is a patch for review:
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug14271-01&id=115d96e5ca5a861573a6efe08fbb3ba7a3ce149d
For this to work correctly, you will need a Tor Launcher that includes the changes from #14272.

comment:9 Changed 3 years ago by adrelanos

Do you have a branch and/or downloadable version that I could use to check Whonix compatibility?

comment:10 Changed 3 years ago by adrelanos

I mean, one that includes both changes, #14271 and #14272.

comment:11 in reply to:  9 Changed 3 years ago by mcs

Replying to adrelanos:

Do you have a branch and/or downloadable version that I could use to check Whonix compatibility?

We do not have a packaged Tor Browser that includes these changes. You would need to build Torbutton from the bug14271-01 branch within ​https://git.torproject.org/user/brade/torbutton.git and build Tor Launcher from the bug14272-01 branch within https://git.torproject.org/user/brade/tor-launcher.git. But you may also want the patch from #19733 (it is not needed if the local Tor check is disabled within Torbutton).

But it probably makes sense to wait until after these changes are merged into master so that you can use a nightly Tor Browser build.

comment:12 Changed 3 years ago by gk

Keywords: TorBrowserTeam201609 added; TorBrowserTeam201608R removed
Status: needs_reviewneeds_revision

Here are some comments:

1)

+// given sockFile or host and port.

s/sockFile/socketFile/

2)

let tlps = Cc["@torproject.org/torlauncher-protocol-service;1"]
             .getService(Ci.nsISupports).wrappedJSObject;

We should do that once and not twice in torbutton_init().

3) Checking whether we should call torbutton_local_tor_check() should check for
m_tb_control_socket_file as well (not only for m_tb_control_port) I guess?

4) I am not so sure about

+            } catch(e) {
+              m_tb_control_port = 9151;
+            }

What was your rationale for adding that now (given you omitted it earlier)? For one, the logs might be misleading showing a probably wrong port (I mean the setup is seriously troubling if we need to assign 9151 in the catch clause at all) in an error with respect to the control connection. On the other hand, we might want to show that something is wrong with the help of torbutton_do_tor_check() which would update the toolbar button in this case (if we get that far at all with a broken setup like the one in question).

comment:13 in reply to:  12 ; Changed 3 years ago by mcs

Status: needs_revisionneeds_information

Replying to gk:

Here are some comments:

1)

+// given sockFile or host and port.

s/sockFile/socketFile/

Good catch.


2)

let tlps = Cc["@torproject.org/torlauncher-protocol-service;1"]
             .getService(Ci.nsISupports).wrappedJSObject;

We should do that once and not twice in torbutton_init().

Agreed.


3) Checking whether we should call torbutton_local_tor_check() should check for
m_tb_control_socket_file as well (not only for m_tb_control_port) I guess?

We did add that check, although if you look at the patch with git's default of three lines of context it is not so obvious.


4) I am not so sure about

+            } catch(e) {
+              m_tb_control_port = 9151;
+            }

What was your rationale for adding that now (given you omitted it earlier)? For one, the logs might be misleading showing a probably wrong port (I mean the setup is seriously troubling if we need to assign 9151 in the catch clause at all) in an error with respect to the control connection. On the other hand, we might want to show that something is wrong with the help of torbutton_do_tor_check() which would update the toolbar button in this case (if we get that far at all with a broken setup like the one in question).

Kathy and I added the 9151 default to be consistent with how m_tb_control_host is handled (it was already defaulting to 127.0.0.1). Thinking about this some more and looking at the existing Torbutton code, it seems like there is some effort to disable features (e.g., New Identity, the local Tor check) when the port is missing. So maybe we should put things back how there were and make sure we consistently check for port, password, etc. before trying to do things in Torbutton that require authenticated control port access?

The circuit display code also includes code that defaults the port to 9151, so if we decide to continue with the concept that a lack of port can be used to disable code, we should remove the || 9151 from this line in tor-circuit-display.js:
myController = controller(socketFile, host, port || 9151, password,
We can also add a check to skip the call to createTorCircuitDisplay() if port, password, etc. are missing (the existing code will log an error if initialization fails).

What do you think?

comment:14 in reply to:  13 ; Changed 3 years ago by gk

Status: needs_informationassigned

Replying to mcs:

Replying to gk:

3) Checking whether we should call torbutton_local_tor_check() should check for
m_tb_control_socket_file as well (not only for m_tb_control_port) I guess?

We did add that check, although if you look at the patch with git's default of three lines of context it is not so obvious.

Hm. No, it is even obvious then. Must have been a different kind of code-blindness, my bad.


4) I am not so sure about

+            } catch(e) {
+              m_tb_control_port = 9151;
+            }

What was your rationale for adding that now (given you omitted it earlier)? For one, the logs might be misleading showing a probably wrong port (I mean the setup is seriously troubling if we need to assign 9151 in the catch clause at all) in an error with respect to the control connection. On the other hand, we might want to show that something is wrong with the help of torbutton_do_tor_check() which would update the toolbar button in this case (if we get that far at all with a broken setup like the one in question).

Kathy and I added the 9151 default to be consistent with how m_tb_control_host is handled (it was already defaulting to 127.0.0.1). Thinking about this some more and looking at the existing Torbutton code, it seems like there is some effort to disable features (e.g., New Identity, the local Tor check) when the port is missing. So maybe we should put things back how there were and make sure we consistently check for port, password, etc. before trying to do things in Torbutton that require authenticated control port access?

The circuit display code also includes code that defaults the port to 9151, so if we decide to continue with the concept that a lack of port can be used to disable code, we should remove the || 9151 from this line in tor-circuit-display.js:
myController = controller(socketFile, host, port || 9151, password,
We can also add a check to skip the call to createTorCircuitDisplay() if port, password, etc. are missing (the existing code will log an error if initialization fails).

What do you think?

I think we should not set m_tb_control_port to 9151 now in the catch clause and open a new ticket for implementing a saner solution across all Torbutton code. We can discuss there what we want that solution to be.

comment:15 in reply to:  14 ; Changed 3 years ago by mcs

Keywords: TorBrowserTeam201609R added; TorBrowserTeam201609 removed
Status: assignedneeds_review

Replying to gk:

I think we should not set m_tb_control_port to 9151 now in the catch clause and open a new ticket for implementing a saner solution across all Torbutton code. We can discuss there what we want that solution to be.

That sounds like a good plan. I opened #20057.
Here is a revised patch for this ticket:
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug14271-02&id=abb01118a8445c541bb359d85fe9af941536b43a

comment:16 in reply to:  15 ; Changed 3 years ago by arthuredelstein

Replying to mcs:

Replying to gk:

I think we should not set m_tb_control_port to 9151 now in the catch clause and open a new ticket for implementing a saner solution across all Torbutton code. We can discuss there what we want that solution to be.

That sounds like a good plan. I opened #20057.
Here is a revised patch for this ticket:
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug14271-02&id=abb01118a8445c541bb359d85fe9af941536b43a

Sorry for the late review. This patch looks good to me -- nice work! I have a couple of minor stylistic suggestions that you may or may not want to adopt:

+// given socketFile or host and port.
+io.asyncSocketStreams = function (socketFile, host, port) {
+  let socketTransport;
+  let sts = Cc["@mozilla.org/network/socket-transport-service;1"]
+              .getService(Components.interfaces.nsISocketTransportService),
+	  UNBUFFERED = Ci.nsITransport.OPEN_UNBUFFERED;
+
+  // Create an instance of a socket transport.
+  if (socketFile) {
+    socketTransport = sts.createUnixDomainTransport(socketFile);
+  } else {
+    socketTransport = sts.createTransport(null, 0, host, port, null);
+  }
+

Maybe move let socketTransport down to after the comment // Create an instance of a socket transport? Also, in that file, I had used blank lines to separate functions, though that's probably a personal eccentricity...

Also, a somewhat bigger suggestion that could be part of this patch or left for another time. In torbutton.js, we will now have

 var m_tb_control_socket_file = null; // Set if using a UNIX domain socket.
 var m_tb_control_port = null;        // Set if not using a socket.
 var m_tb_control_host = null;        // Set if not using a socket.
 var m_tb_control_pass = null;
 var m_tb_control_desc = null;  

I imagine it might be cleaner to collect these into a single data object like

var m_tb_control = { socket_file, host, port, password, descriptor }

Then we could factor out a single factory function from torbutton_init() that generates this object. And we could use this object as a single argument for functions in tor-control-port.js and tor-circuit-display.js as well.

comment:17 in reply to:  16 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201609 added; TorBrowserTeam201609R removed
Status: needs_reviewneeds_information

Replying to arthuredelstein:

Sorry for the late review. This patch looks good to me -- nice work! I have a couple of minor stylistic suggestions that you may or may not want to adopt:

+// given socketFile or host and port.
+io.asyncSocketStreams = function (socketFile, host, port) {
+  let socketTransport;
+  let sts = Cc["@mozilla.org/network/socket-transport-service;1"]
+              .getService(Components.interfaces.nsISocketTransportService),
+	  UNBUFFERED = Ci.nsITransport.OPEN_UNBUFFERED;
+
+  // Create an instance of a socket transport.
+  if (socketFile) {
+    socketTransport = sts.createUnixDomainTransport(socketFile);
+  } else {
+    socketTransport = sts.createTransport(null, 0, host, port, null);
+  }
+

Maybe move let socketTransport down to after the comment // Create an instance of a socket transport?

Good idea.

Also, in that file, I had used blank lines to separate functions, though that's probably a personal eccentricity...

I am OK with accommodating eccentricities, but can you explain what we need to fix? I don't think we removed any blank lines, but maybe I am just not seeing it.

Also, a somewhat bigger suggestion that could be part of this patch or left for another time. In torbutton.js, we will now have

 var m_tb_control_socket_file = null; // Set if using a UNIX domain socket.
 var m_tb_control_port = null;        // Set if not using a socket.
 var m_tb_control_host = null;        // Set if not using a socket.
 var m_tb_control_pass = null;
 var m_tb_control_desc = null;  

I imagine it might be cleaner to collect these into a single data object like

var m_tb_control = { socket_file, host, port, password, descriptor }

Then we could factor out a single factory function from torbutton_init() that generates this object. And we could use this object as a single argument for functions in tor-control-port.js and tor-circuit-display.js as well.

Kathy and I like that suggestion, but let's do it as a follow up. I created #20102 for it.

comment:18 Changed 3 years ago by gk

mcs/brade could you create an updated patch? I want to have this in the alpha as well. We can accommodate Arthur's remaining concerns (if there are any) in a fixup commit.

comment:20 Changed 3 years ago by gk

Okay, pushed to master commit 28a55f5bf324f43373006328c5b03793096f71c4. Arthur, let us know if we should make additional changes or just close this bug.

comment:21 Changed 3 years ago by arthuredelstein

Looks good to me. I had noticed a couple of blank lines added to io.asyncSocketStreams, but it's really not important.

comment:22 Changed 3 years ago by gk

Resolution: fixed
Status: needs_informationclosed

comment:23 Changed 3 years ago by mcs

#3967 was resolved as a duplicate.

Note: See TracTickets for help on using tickets.