Opened 5 years ago

Closed 5 years ago

#13079 closed enhancement (fixed)

environment variable to skip TorButton control port verification

Reported by: proper Owned by:
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: tbb-torbutton, GeorgKoppen201501, tbb-4.5-alpha-3
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Tor Button now does its own Tor ControlPort verification.

Source #6546 comment:33 mikeperry:

TBB now performs its own control port verification. We simply check that the socks port Tor says its on is the one Firefox is configured to use.

If that test fails, the default homepage about:tor will show:

Something Went Wrong!
Tor is not working in this browser.

This is problematic in transparent / isolating proxy environments, that do not wish to allow ControlPort access for security reasons (getinfo address and other dangerous ControlPort commands).

The test is implemented in torbrowser.js, in function torbutton_local_tor_check().

[...]
  const kCmdArg = "net/listeners/socks";
[...]
  let resp = torbutton_send_ctrl_cmd("GETINFO " + kCmdArg + "\r\n");
[...]

(The environment variable TOR_SKIP_LAUNCH=1 does not prevent this. [No such variable in Tor Button's source code.])

It would be nice if there was an environment variable such as TOR_SKIP_CONTROLPORTTEST (or so), that would skip this test.

Child Tickets

Attachments (2)

Change History (16)

comment:1 Changed 5 years ago by proper

Mistake, the title is supposed to be "environment variable to skip TorButton control port verification".

comment:2 Changed 5 years ago by proper

Status: newneeds_review

Attached a patch:
https://trac.torproject.org/projects/tor/attachment/ticket/13079/0001-Bug-13078-environment-variable-to-skip-TorButton-con.patch

I am not a javascript coder. I hope implementing it this way is legitimate. Wouldn't mind anyone modifiying my patch or comming up with a better way.

Please review.

comment:3 Changed 5 years ago by proper

Forget about the older patch.

The new patch is much saner.

It makes the feature from #11722 accessible by the TOR_SKIP_CONTROLPORTTEST variable. Tested by me. Both TOR_SKIP_CONTROLPORTTEST and the pref extensions.torbutton.local_tor_check still working.

Please review.

comment:4 Changed 5 years ago by arma

Component: TorbuttonTor Browser
Summary: environment variable to TorButton control port verificationenvironment variable to skip TorButton control port verification

Moved to Tor Browser component, since last I heard Mike et al ignore the Torbutton component.

comment:5 Changed 5 years ago by gk

Keywords: tbb-torbutton MikePerry201411R added

comment:6 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201411R added; MikePerry201411R removed

comment:7 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201412R added; TorBrowserTeam201411R removed

comment:8 in reply to:  3 ; Changed 5 years ago by mcs

Replying to proper:

Forget about the older patch.

The new patch is much saner.

It makes the feature from #11722 accessible by the TOR_SKIP_CONTROLPORTTEST variable. Tested by me. Both TOR_SKIP_CONTROLPORTTEST and the pref extensions.torbutton.local_tor_check still working.

Please review.

The patch looks OK. The only question I have is whether we should check for a value of "1" in the TOR_SKIP_CONTROLPORTTEST env variable (rather than just checking for existence). In Tor Launcher, we use "1" != environ.get(...) tests, although I see one place in Torbutton where we just check for existence (search for TOR_TRANSPROXY within src/components/startup-observer.js to find it).

comment:9 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201501R added; TorBrowserTeam201412R removed

comment:10 in reply to:  8 ; Changed 5 years ago by gk

Replying to mcs:

The patch looks OK. The only question I have is whether we should check for a value of "1" in the TOR_SKIP_CONTROLPORTTEST env variable (rather than just checking for existence). In Tor Launcher, we use "1" != environ.get(...) tests, although I see one place in Torbutton where we just check for existence (search for TOR_TRANSPROXY within src/components/startup-observer.js to find it).

Personally, I think having just the existence of the environment variable is enough. It seems a bit awkward to have an environment variable set with a value that indicates that you just want to have the default behavior. Why having the variable set in the first place at all then? If there are use-cases where we need to check the value because there are more than two possible values (or where neither of the two values is the default value you get by unsetting the environment variable) then this might be a different story, of course.

That said I don't mean to imply that we should change the way Tor Launcher does this. But I'd like to keep the related Torbutton code as-is and to avoid mixing both styles.

comment:11 in reply to:  10 Changed 5 years ago by proper

Replying to mcs:

The patch looks OK.

Great!

The only question I have is whether we should check for a value of "1" in the TOR_SKIP_CONTROLPORTTEST env variable (rather than just checking for existence).

Replying to gk:

Personally, I think having just the existence of the environment variable is enough.

Yes, indeed. Why use more code than required.

That said I don't mean to imply that we should change the way Tor Launcher does this. But I'd like to keep the related Torbutton code as-is and to avoid mixing both styles.

For env var TOR_SKIP_LAUNCH (in file tl-util.jsm), tor-launcher does check the value of the env var.

(Line: return ("1" != env.get(kEnvSkipLaunch));)

We we agree it's unnecessary?

What about this...

  • This patch using the simpler method (only env.exists check) is merge able.
  • And create a follow up ticket to simplify the other env var checks?

comment:12 Changed 5 years ago by gk

Keywords: tbb-4.5-alpha-3 added

comment:13 Changed 5 years ago by gk

Keywords: GeorgKoppen201501R added; TorBrowserTeam201501R removed

comment:14 Changed 5 years ago by gk

Keywords: GeorgKoppen201501 added; GeorgKoppen201501R removed
Resolution: fixed
Status: needs_reviewclosed

Fixed by commit 38a59e819604de5018db5db54fb08fd9e1581d1f. Please open a new ticket for the Tor Launcher issue if you wish.

Note: See TracTickets for help on using tickets.