Opened 11 months ago

Closed 7 months ago

Last modified 7 months ago

#19646 closed defect (fixed)

Mac OS: wrong location for meek browser profile

Reported by: mcs Owned by: dcf
Priority: Medium Milestone:
Component: Obfuscation/meek Version:
Severity: Normal Keywords: tbb-regression, tbb-6.0-issues, meek, TorBrowserTeam201611R
Cc: dcf, brade, gk, arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

With Tor Browser 6.0 and newer, Meek is not working at all for some Mac OS users. Anonymous said on https://blog.torproject.org/blog/tor-browser-60-released:

Note to all OSX users encountering similar error: make sure before upgrade/clean install for 6.0+ that user installing has sudo privileges. TorBrowser needs write access to /Applications/TorBrowser-Data/ which will fail unless the user is an administrator. sudo privileges can be removed after installation and first run without problems.

Kathy and I were able to reproduce this problem on Mac OS 10.11.5 using a clean install of Tor Browser 6.0 (placed in /Applications) when logged in as a non-admin user.

The problem is that the meek-client-torbrowser code that creates the Meek browser profile does not account for the fact that the user data should be stored under ~/Library/Application Support/TorBrowser-Data when the browser is in /Applications. In fact, Meek works correctly when running as a privileged (admin) user, but the Meek helper profile is incorrectly placed under /Applications/TorBrowser-Data/ (the main browser profile is correctly placed under ~/Library/Application Support/TorBrowser-Data).

Child Tickets

Attachments (1)

0001-Bug-19646-Mac-OS-wrong-location-for-meek-browser-pro.patch (4.4 KB) - added by brade 7 months ago.
meek patch

Download all attachments as: .zip

Change History (19)

comment:1 Changed 11 months ago by mcs

A workaround for this problem is to create a folder named /Applications/TorBrowser-Data and ensure that all users who plan to use Tor Browser with Meek have write access to that folder.

comment:2 follow-up: Changed 11 months ago by mcs

  • Status changed from new to needs_information

A possible solution would be to modify Tor Launcher to set an environment variable that contains the path for the TorBrowser-Data directory, e.g.,

TOR_BROWSER_DATA_DIR='/Users/someone/Library/Application Support/TorBrowser-Data'

dcf: Would it be OK to make meek-client-torbrowser depend on such an env var?
meek-client-torbrowser is only used by Tor Browser, correct?

comment:3 in reply to: ↑ 2 Changed 10 months ago by dcf

Replying to mcs:

A possible solution would be to modify Tor Launcher to set an environment variable that contains the path for the TorBrowser-Data directory, e.g.,

TOR_BROWSER_DATA_DIR='/Users/someone/Library/Application Support/TorBrowser-Data'

dcf: Would it be OK to make meek-client-torbrowser depend on such an env var?
meek-client-torbrowser is only used by Tor Browser, correct?

Yes, this would be find.

meek-client-torbrowser is only used by Tor Browser. (Maybe it should move into a Tor Browser repo rather than a meek repo.)

comment:4 Changed 10 months ago by dcf

  • Keywords meek added
  • Status changed from needs_information to new

comment:5 Changed 10 months ago by mcs

  • Keywords TorBrowserTeam201608 added

comment:6 Changed 9 months ago by gk

  • Keywords TorBrowserTeam201609 added; TorBrowserTeam201608 removed

Tickets for September.

comment:7 Changed 8 months ago by gk

  • Keywords TorBrowserTeam201610 added; TorBrowserTeam201609 removed

Moving tickets to October.

comment:8 follow-up: Changed 7 months ago by brade

  • Cc arthuredelstein added
  • Keywords TorBrowserTeam201611R added; TorBrowserTeam201610 removed
  • Status changed from new to needs_review

Two patches are needed, one for meek (which I just attached to this ticket) and one for Tor Launcher, which is here:
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug19646-01&id=5ac452d86f93ee15ba3722afc606be862e8f3686

Please review both patches.

comment:9 Changed 7 months ago by arthuredelstein

Both patches look good to me.

comment:10 follow-ups: Changed 7 months ago by gk

Applied the tor-launcher changes to master, maint-0.2.10 and maint-0.2.9 (commit 5ac452d86f93ee15ba3722afc606be862e8f3686, 4eee281bf774f8cf4f2d8a2549573b7a89f72407 and 0082643cd0b88f34cb56501b6631307616b8cc1d).

dcf: Could you prepare a new tag for meek with the commit up for review here (in case you think the changes are okay)? I am fine putting that into 6.0.6 as well. For this we need a new tag by next Tuesday, thanks. FWIW the changes look good to me, too, although I have a hard time parsing the second sentence of the commit message. Seems to be missing something there. But might be fixable just with a git commit --amend.

comment:11 in reply to: ↑ 10 Changed 7 months ago by mcs

Replying to gk:

dcf: Could you prepare a new tag for meek with the commit up for review here (in case you think the changes are okay)? I am fine putting that into 6.0.6 as well. For this we need a new tag by next Tuesday, thanks. FWIW the changes look good to me, too, although I have a hard time parsing the second sentence of the commit message. Seems to be missing something there. But might be fixable just with a git commit --amend.

Oops. The commit message should read "This fixes a problem where meek-client-torbrowser...."

comment:12 Changed 7 months ago by mcs

The meek patch, with an amended commit message as discussed in comment:11, is now available here:
https://gitweb.torproject.org/user/brade/meek.git/commit/?h=bug19646-01&id=73e416fe9cb97d1bde9404b71b5dfe1957affc76
(availability in a git repo may make reviewing / testing / merging more convenient).

comment:13 Changed 7 months ago by dcf

Please stand by; I think I'll be able to do this tomorrow.

comment:14 in reply to: ↑ 10 Changed 7 months ago by dcf

Replying to gk:

Applied the tor-launcher changes to master, maint-0.2.10 and maint-0.2.9 (commit 5ac452d86f93ee15ba3722afc606be862e8f3686, 4eee281bf774f8cf4f2d8a2549573b7a89f72407 and 0082643cd0b88f34cb56501b6631307616b8cc1d).

dcf: Could you prepare a new tag for meek with the commit up for review here (in case you think the changes are okay)? I am fine putting that into 6.0.6 as well. For this we need a new tag by next Tuesday, thanks. FWIW the changes look good to me, too, although I have a hard time parsing the second sentence of the commit message. Seems to be missing something there. But might be fixable just with a git commit --amend.

I pushed you tag 0.25.

comment:15 in reply to: ↑ 8 ; follow-up: Changed 7 months ago by dcf

Replying to brade:

Two patches are needed, one for meek (which I just attached to this ticket) and one for Tor Launcher, which is here:
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug19646-01&id=5ac452d86f93ee15ba3722afc606be862e8f3686

Please review both patches.

As a potential improvement, let me know what you think about this. torDataDirFirefoxProfilePath is an empty string on linux and windows. It's only non-empty on mac. Therefore, the first branch of the if will only ever be taken on mac:

	var profilePath string
	var torDataDir = os.Getenv("TOR_BROWSER_TOR_DATA_DIR")
	if torDataDir != "" && torDataDirFirefoxProfilePath != "" {
		profilePath = filepath.Join(torDataDir, torDataDirFirefoxProfilePath)
	} else {
		profilePath, err = filepath.Abs(firefoxProfilePath)
		if err != nil {
			return
		}
	}

Does it make sense to remove the check for torDataDirFirefoxProfilePath != "", and just make TOR_BROWSER_TOR_DATA_DIR and torDataDirFirefoxProfilePath the primary way of choosing the path on all platforms? firefoxProfilePath can remain as a fallback, only used if TOR_BROWSER_TOR_DATA_DIR is not set. I say this because the tor-launcher patch unconditionally sets TOR_BROWSER_TOR_DATA_DIR, and it's surprising that the environment variable will be ignored on platforms other than mac.

comment:16 in reply to: ↑ 15 Changed 7 months ago by mcs

Replying to dcf:

Does it make sense to remove the check for torDataDirFirefoxProfilePath != "", and just make TOR_BROWSER_TOR_DATA_DIR and torDataDirFirefoxProfilePath the primary way of choosing the path on all platforms? firefoxProfilePath can remain as a fallback, only used if TOR_BROWSER_TOR_DATA_DIR is not set. I say this because the tor-launcher patch unconditionally sets TOR_BROWSER_TOR_DATA_DIR, and it's surprising that the environment variable will be ignored on platforms other than mac.

This is a good question to raise. The reason Kathy and I wrote the code to only use TOR_BROWSER_TOR_DATA_DIR on OSX is because on the other platforms the meek browser profile is not currently located under the Tor data directory. On OSX, we moved the location when we changed to the TorBrowser-Data structure (#13252) because we had to do migration anyway and because we thought it made more sense to put the profile under <tordatadir>/PluggableTransports/ (it is more hidden there and less likely to be confused with the main Tor Browser profile).

Someday when we move Linux and Windows to a similar scheme (see #18367 and #18369) we should definitely modify meek-client-torbrowser to only have one codepath. I created a new ticket for this (#20600) as well as a parent ticket to track all of the related work (#20599).

comment:17 Changed 7 months ago by gk

  • Resolution set to fixed
  • Status changed from needs_review to closed

Seems we are done here, thanks all!

comment:18 Changed 7 months ago by gk

FWIW: the commits for using 0.25 as the new meek tag on master, maint-6.0 and hardened-builds as 3308ef8028c10c900ee5c914cf9bb653ec498167, bd1da40b8ac70aabc1c1793d649430a1dcbbc1f8 and 0502b7aa79e336d826c80ed78800388ff573a60a.

Note: See TracTickets for help on using tickets.