Opened 4 years ago

Closed 2 years ago

#13252 closed defect (fixed)

Tor Browser on OS X should not store data into the application bundle

Reported by: torosx Owned by: mcs
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201604R
Cc: mcs, brade, gk, mikeperry, teor, mrphs, arlolra, arthuredelstein, dcf Actual Points:
Parent ID: #6540 Points:
Reviewer: Sponsor: None

Description

The Tor application on OS X stores user data into its bundle (TorBrowser.app/Data/). This is bad. This causes various issues:

  • the Tor application can't be code sign which decreases the security. See Ticket #13251: CodeSign Tor for OS X
  • when installing a new version of Tor, all previous user data (bookmarks) are deleted.

Child Tickets

TicketStatusOwnerSummaryComponent
#18371closedmcsTorBrowser.app.meek-http-helper symlinks incompatible with Gatekeeper signingApplications/Tor Browser

Attachments (1)

signit.sh (884 bytes) - added by mcs 2 years ago.
Mark and Kathy's codesign test script

Download all attachments as: .zip

Change History (64)

comment:1 Changed 4 years ago by torosx

User data should probably be stored into the ~/Library/Application Support/ folder

comment:2 Changed 4 years ago by gk

Parent ID: #6540
Summary: Tor on OS X should not stored data into the application bundleTor Browser on OS X should not store data into the application bundle

comment:3 Changed 4 years ago by mikeperry

Mozilla hacked around this by placing stuff they needed to modify in MozResources: https://bugzilla.mozilla.org/show_bug.cgi?id=1047584.

comment:4 Changed 3 years ago by mikeperry

Cc: mcs brade added
Severity: Normal

OK, I attempted to sign our app dir via the command line using codesign as per:
https://stackoverflow.com/questions/9245149/jenkins-on-os-x-xcodebuild-gives-code-sign-error and then
https://lists.torproject.org/pipermail/tor-dev/2015-October/009780.html

I had to add a --keychain to codesign to specify a non-default keychain, but otherwise it was pretty much that verbatim.

Unfortunately, I did indeed hit issues. codesign bailed with this error:
./TBBDir/TorBrowser.app/: unsealed contents present in the bundle root

Some searching revealed that this likely means there are non-executable resources mixed in with directories that contain binaries. I think we may indeed have to reorganize the Mac bundle dir again for this.

mcs+brade: This seems like something you might be able to help with, since you reorganized our Mac bundles before? I can get you a Mac "developer" certificate if you give me a CSR using those jenkins instructions, if you need that. (Though tbh, I don't get what the difference is between the developer certs and the app store certs.. if there is no difference from our users' point of view, maybe we should find another way to test this.. Do we even need to involve Apple's official cert process at all to get past these errors?)

Last edited 3 years ago by mikeperry (previous) (diff)

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

Replying to mikeperry:

mcs+brade: This seems like something you might be able to help with, since you reorganized our Mac bundles before? I can get you a Mac "developer" certificate if you give me a CSR using those jenkins instructions, if you need that. (Though tbh, I don't get what the difference is between the developer certs and the app store certs.. if there is no difference from our users' point of view, maybe we should find another way to test this.. Do we even need to involve Apple's official cert process at all to get past these errors?)

Kathy and I will take a look. I think what we ultimately want for signing is a Production "Developer ID" certificate, not an App Store one. There are also "Mac Development" certificates that should work for testing the build process but presumably are not suitable for distribution of the .app bundle to end-users. Kathy and I should be able to generate certs for testing because we have access to an Apple Developer account (we use it for some iOS work and Apple merged all of their programs together, so in theory we can create certs for Mac apps also).

But the showstopper issue with the Tor Browser bundle structure is going to be the Tor and browser profile data that we store inside the bundle. I am almost certain that we will need to move all files that may change out of the .app bundle. Maybe we will have to ship a folder that contains a TorBrowser folder as well as TorBrowser.app (instead of having everything inside TorBrowser.app as we do today). Assuming that works with Gatekeeper, we then have the problem that users can break things by moving one folder without the other :(

comment:6 Changed 3 years ago by mcs

Owner: changed from tbb-team to mcs
Status: newassigned

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

Cc: gk mikeperry teor added
Status: assignedneeds_information

Replying to mcs:

But the showstopper issue with the Tor Browser bundle structure is going to be the Tor and browser profile data that we store inside the bundle. I am almost certain that we will need to move all files that may change out of the .app bundle.

Digging a little deeper and experimenting on a Mac OS 10.10.5 system, Kathy and I learned some interesting things:

  1. It is possible to add a valid signature to Tor Browser if we remove the TorBrowser directory.
  2. It appears that the signature is only checked when the app bundle has a quarantine attribute in the file system. See http://www.cocoabuilder.com/archive/xcode/319946-revoking-gatekeeper-exceptions.html#320000
  3. After an application is opened once, the quarantine attribute is modified by the system so that the app bundle is no longer subjected to signature checks.
  4. It is possible to add a valid signature to Tor Browser if we move the TorBrowser directory under TorBrowser.app/Contents/.

Because of 2-4 above, we might be able to cheat a little and just relocate the TorBrowser directory. This will mean that our app bundle's signature will be broken as soon as Tor Browser is opened for the first time (this is because we make changes under TorBrowser/Data and Apple's signature "seals" everything under Contents/ -- nothing can be modified without invalidating the signature).

It is possible Apple will be even more strict in a future release of their Gatekeeper technology, so our other option is to keep our data outside TorBrowser.app (either in a side-by-side folder like Ricochet does or in the standard location under ~/Library/Application Support/).

What do other people think?

comment:8 in reply to:  7 Changed 3 years ago by teor

Replying to mcs:

Because of 2-4 above, we might be able to cheat a little and just relocate the TorBrowser directory. This will mean that our app bundle's signature will be broken as soon as Tor Browser is opened for the first time (this is because we make changes under TorBrowser/Data and Apple's signature "seals" everything under Contents/ -- nothing can be modified without invalidating the signature).

It is possible Apple will be even more strict in a future release of their Gatekeeper technology, so our other option is to keep our data outside TorBrowser.app (either in a side-by-side folder like Ricochet does or in the standard location under ~/Library/Application Support/).

What do other people think?

If it works for now, then that's a much better user experience.
Except for users who pass around copies of Tor Browser, who will see the "gatekeeper" check on every new machine.

I think we should avoid storing data in Application Support, because it violates our "leave no disk traces" goal. Storing the data beside the app is ugly, but more obvious to the user. (And it makes it easier for them to share just Tor Browser with others, or reset their copy of Tor Browser.)

(I wish Apple had thought of apps that do not want to leave any traces when it designed Application Support and code signing etc.)

comment:9 Changed 3 years ago by mikeperry

I think the app sharing case isn't that bad to break, because people should be sharing the .dmg rather than .app dirs. However, we may also be breaking the USB drive usecase with this change, since when you take the drive to a new computer Gatekeeper will barf at the modified app.

One option might be to do the hack only if we're in /Applications, but do side-by-side directory otherwise? But if that is too complicated, I think it's not too much to tell people that if they want to do the USB drive thing, they need to keep the .dmg on their USB drive and erase the TBB app every time. It would be better than status quo, where many people can't even figure out how to get the thing to run in the first place.

It is looking like long term for MacOS we're going to want an uninstaller/wipe button, though. Probably for Windows also, esp if we can clean up the registry at that point.

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

Replying to mikeperry:

I think the app sharing case isn't that bad to break, because people should be sharing the .dmg rather than .app dirs. However, we may also be breaking the USB drive usecase with this change, since when you take the drive to a new computer Gatekeeper will barf at the modified app.

I don't think the quarantine bit gets set when app bundles are copied to or from a Flash drive (but I do not have a computer that is running Mac OS 10.11 El Capitan, so it is possible Apple made changes in that OS release).

comment:11 Changed 3 years ago by gk

19:14 < GeKo> re 13252: i think i am fine with the cheating approach to get the signing going asap on the alphas to look for other issues
19:14 < GeKo> but i am actually worried about apply restricting their policy further with an update breaking that short-cut
19:14 < GeKo> leaving users with a scary warning again
19:15 < GeKo> which is especially bad as it worked until then
19:16 < GeKo> s/apply/apple/

I am not sure whether that would mean we should work on both things in parallel or just try The Real Thing. Especially if the latter turns out to be time-consuming, resulting in not having signed alpha bundles in our next alpha release on Jan 26.

comment:12 Changed 3 years ago by gk

Keywords: TorBrowserTeam201601 added
Status: needs_informationassigned

comment:13 Changed 3 years ago by mcs

If we implement the "side-by-side" solution where we have a TorBrowser-Data folder next to TorBrowser.app, then we will probably end up solving the profile creation problem as well (#14981).

comment:14 Changed 3 years ago by gk

Keywords: TorBrowserTeam201602 added; TorBrowserTeam201601 removed

Putting stuff on the radar for February.

comment:15 Changed 3 years ago by mrphs

Cc: mrphs added

comment:16 Changed 3 years ago by arlolra

Cc: arlolra added

comment:17 Changed 3 years ago by mcs

Status: assignedneeds_information

As we are proceeding with this work, Kathy and I are wondering whether we want to just move to a side-by-side data model on all platforms in the Tor Browser 6.0 timeframe.

Con: it will add some extra risk on Linux and Windows and some people may not like the change.

Pro: better cross-platform consistency and all platforms will get the benefits that go along with moving user data out of the area where the binaries live. Examples: easier and safer sharing of Tor Browser with other users, annoyances like #18179 will be eliminated, support for multiple profiles and new profile creation, it will be easy to "reset" and start over (just delete the TorBrowser-Data directory), etc.

Opinions?

comment:18 Changed 3 years ago by gk

Status: needs_informationassigned

I think that idea sounds like a good one generally. But I fear we won't have the time for 6.0 for this (given the lack of testing time alone). I think filing two additional bugs where the works gets done and getting that ready in the 6.5 timeframe seems like the better plan to me.

comment:19 Changed 2 years ago by arthuredelstein

Cc: arthuredelstein added

comment:20 in reply to:  18 Changed 2 years ago by mcs

Replying to gk:

I think that idea sounds like a good one generally. But I fear we won't have the time for 6.0 for this (given the lack of testing time alone). I think filing two additional bugs where the works gets done and getting that ready in the 6.5 timeframe seems like the better plan to me.

I filed #18367 for Windows and #18369 for Linux.

comment:21 Changed 2 years ago by mcs

See #18371 for a child ticket related to meek.

comment:22 Changed 2 years ago by mcs

Cc: dcf added
Keywords: TorBrowserTeam201603 added; TorBrowserTeam201602 removed

We have now completed most of the work for this ticket. With our changes, all user data is stored in a directory named TorBrowser-Data which is created next to the app bundle (the TorBrowser-Data directory does not exist initially; it is created by the browser the first time Tor Browser is opened). The content of TorBrowser-Data looks like this:

  Browser/
    Caches/
    profiles.ini
    qf0enfzt.default/            (user's browser profile)
  Tor/                           (Tor data directory)
    ...
    PluggableTransports/
      profile.meek-http-helper/  (meek browser profile)
    ...
    torrc
  UpdateInfo/                    (update history and temporary space)

There are two major loose ends:
1) FTE needs to be repackaged to separate the Python scripts from the compiled libraries.
2) Kathy and I need to write code to relocate the user profile files during an upgrade from a version of Tor Browser that uses the old "embedded user data" layout to one that uses the new layout.

See #18371 for our meek changes.

Our tor-browser patches (two fixup commits plus another one) are on bug13252-01 branch, here:
https://gitweb.torproject.org/user/brade/tor-browser.git/log/?h=bug13252-01
There is a new --enable-tor-browser-data-outside-app-dir configure option which is used to enable the new disk layout.

Our Tor Launcher patch is here:
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug13252-01&id=fe41b6d6032ffde989b860dd94a7f53301ef97ed
With these changes, Tor Launcher should automatically handle both the old and new layout.

Our tor-browser-bundle (builder) patch is here:
https://gitweb.torproject.org/user/brade/tor-browser-bundle.git/commit/?h=bug13252-01&id=1c2485467efc31b5539585f39fc1a861d421fb28
The new packaging is triggered by setting DATA_OUTSIDE_APP_DIR=1 in the versions file (our patch sets it in versions.alpha).

Early reviews are welcome; just keep in mind that additional changes are needed to address the two loose ends I mentioned above.

comment:23 Changed 2 years ago by teor

What happens when the user doesn't have permission to write to /Applications (or the directory Tor Browser is in?)

comment:24 in reply to:  23 Changed 2 years ago by mcs

Replying to teor:

What happens when the user doesn't have permission to write to /Applications (or the directory Tor Browser is in?)

Bad things will happen, which is also true today if TB cannot write inside its .app bundle. I just tried running the new Tor Browser in a folder that I do not have write access to and this message was displayed:

Profile Missing

Your Tor Browser profile cannot be loaded. It may be missing
or inaccessible.

I think we may want to add a new, more accurate error message that is displayed when the browser is unable to start up because the profile directory is not writable/cannot be created.

In the long run we should probably support two modes like Ricochet does: if TorBrowser.app is installed into /Applications, store user data inside ~/Library/Application\ Support like all non-portable apps do; otherwise, use the TorBrowser-Data side-by-side/portable approach.

comment:25 Changed 2 years ago by mcs

Keywords: TorBrowserTeam201603R added; TorBrowserTeam201603 removed
Status: assignedneeds_review

Kathy and I finished the migration code and added it as an additional commit on this branch:
https://gitweb.torproject.org/user/brade/tor-browser.git/log/?h=bug13252-01
(we plan to combine the two commits that are labeled with "Bug 13252" after we receive some review feedback).

Since in that commit we added a localized string which is retrieved from Torbutton, we now have a tiny Torbutton commit as well:
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug13252-01&id=9be4fe9beb087d6a421bab705b4d1a1d60fd5f90

All of this is now ready for review. Please look at the two commits I just mentioned as well as those mentioned in comment:22. Another approach is to look at the bug13252-01 branch in each of the following user/brade repos: tor-browser, torbutton, tor-launcher, and tor-browser-bundle.

comment:26 Changed 2 years ago by mcs

See #18495 for a child ticket related to FTE.

comment:27 Changed 2 years ago by gk

Okay, let's start with the easiest one: the Torbutton patch looks good to me. :)

Comments to the tor-browser-bundle changes:

1) It seems to me we can get rid of creating Caches altogether without the need for treating that step specially in the non-signed bundles? I tested it a bit and Caches seems to get created anyway.

2) Could you add the FTE ticket number in the FTE TODO comment?

3) The second rm -rf $SKELETON_TMP seems wrong to me as we are still in the directory we want to delete. And we already delete it before we begin with our skeleton preparation. Maybe a copy/paste fail?

4) Do we have some alternative for handling the bookmarks file? Putting it into the omni.ja file seems a bit weird to me.

comment:28 in reply to:  27 ; Changed 2 years ago by mcs

Replying to gk:

Comments to the tor-browser-bundle changes:

1) It seems to me we can get rid of creating Caches altogether without the need for treating that step specially in the non-signed bundles? I tested it a bit and Caches seems to get created anyway.

Yes, you are correct that we can just let the browser create it when it is opened for the first time. Do you want us to make that change (skip creation of the Caches directory) for all platforms at this time or just for Mac OS?

2) Could you add the FTE ticket number in the FTE TODO comment?

Sure; good idea.

3) The second rm -rf $SKELETON_TMP seems wrong to me as we are still in the directory we want to delete. And we already delete it before we begin with our skeleton preparation. Maybe a copy/paste fail?

I think it should just be moved after the cd -, not removed. The idea is to clean up the directory now that its contents have been captured inside inputs/mac-skeleton.zip.

4) Do we have some alternative for handling the bookmarks file? Putting it into the omni.ja file seems a bit weird to me.

When Firefox creates a new profile, it copies the default bookmarks from defaults/profile/bookmarks.html within browser/omni.ja. In other words, we are using the standard mechanism that Firefox uses to get our bookmarks into new profiles. We could have replaced browser/locales/generic/profile/bookmarks.html.in with our own content inside the tor-browser repo (via a Firefox patch) but since (1) our bookmarks are already part of tor-browser-bundle and (2) we already touch browser/omni.ja to modify the 000-tor-browser.js, we thought it best to handle the bookmarks in a similar way.

comment:29 in reply to:  28 ; Changed 2 years ago by gk

Replying to mcs:

Replying to gk:

Comments to the tor-browser-bundle changes:

1) It seems to me we can get rid of creating Caches altogether without the need for treating that step specially in the non-signed bundles? I tested it a bit and Caches seems to get created anyway.

Yes, you are correct that we can just let the browser create it when it is opened for the first time. Do you want us to make that change (skip creation of the Caches directory) for all platforms at this time or just for Mac OS?

I think we can get rid of it for all platforms.

[snip]

3) The second rm -rf $SKELETON_TMP seems wrong to me as we are still in the directory we want to delete. And we already delete it before we begin with our skeleton preparation. Maybe a copy/paste fail?

I think it should just be moved after the cd -, not removed. The idea is to clean up the directory now that its contents have been captured inside inputs/mac-skeleton.zip.

Sounds good. But what is the purpose of the first rm -rf $SKELETON_TMP then? I thought that one was there for making sure everything is in a clean state before starting with the skeletion extraction? Do we really need a clean-up twice in that code snippet?

4) Do we have some alternative for handling the bookmarks file? Putting it into the omni.ja file seems a bit weird to me.

When Firefox creates a new profile, it copies the default bookmarks from defaults/profile/bookmarks.html within browser/omni.ja. In other words, we are using the standard mechanism that Firefox uses to get our bookmarks into new profiles. We could have replaced browser/locales/generic/profile/bookmarks.html.in with our own content inside the tor-browser repo (via a Firefox patch) but since (1) our bookmarks are already part of tor-browser-bundle and (2) we already touch browser/omni.ja to modify the 000-tor-browser.js, we thought it best to handle the bookmarks in a similar way.

Makes sense to me, thanks.

comment:30 Changed 2 years ago by gk

The tor-launcher changes look good to me. Some minor things:

1) regarding the changes in the prefs.js: s/tor_path pref./tor_path/ like you did for the other paths?

2) I wonder whether

// In FF21+, CurProcD is the "browser" directory that is next to
// the firefox binary, e.g., <TorFileBaseDir>/Browser/browser

should be in the Tor Browser code path (as it basically was before). I was confused to read that comment and then the code continued to be concerned with Thunderbird/Instantbird

3) What is the reason for treating torrc-defaults differently here, i.e. moving that one out of the data dir?

Last edited 2 years ago by gk (previous) (diff)

comment:31 in reply to:  29 Changed 2 years ago by gk

Replying to gk:

Replying to mcs:

Replying to gk:

Comments to the tor-browser-bundle changes:

1) It seems to me we can get rid of creating Caches altogether without the need for treating that step specially in the non-signed bundles? I tested it a bit and Caches seems to get created anyway.

Yes, you are correct that we can just let the browser create it when it is opened for the first time. Do you want us to make that change (skip creation of the Caches directory) for all platforms at this time or just for Mac OS?

I think we can get rid of it for all platforms.

Ah, sorry. I think I meant the opposite. Let's not touch the other bundle descriptors in this bug. We can do this later in the respective bugs for Linux and Windows or separately.

comment:32 in reply to:  29 Changed 2 years ago by mcs

Replying to gk:

...
Sounds good. But what is the purpose of the first rm -rf $SKELETON_TMP then? I thought that one was there for making sure everything is in a clean state before starting with the skeletion extraction? Do we really need a clean-up twice in that code snippet?

We can remove the second one.

comment:33 in reply to:  30 ; Changed 2 years ago by mcs

Replying to gk:

The tor-launcher changes look good to me. Some minor things:

1) regarding the changes in the prefs.js: s/tor_path pref./tor_path/ like you did for the other paths?

Sure. The comments are probably easier to read that way.

2) I wonder whether

// In FF21+, CurProcD is the "browser" directory that is next to
// the firefox binary, e.g., <TorFileBaseDir>/Browser/browser

should be in the Tor Browser code path (as it basically was before). I was confused to read that comment and then the code continued to be concerned with Thunderbird/Instantbird

You are right. We will move or remove that comment. We are already proposing that we move the check for FF >= 21:

if (TorLauncherUtil.isAppVersionAtLeast("21.0"))

3) What is the reason for treating torrc-defaults differently here, i.e. moving that one out of the data dir?

The updater will not be able to touch anything inside TorBrowser-Data (at least that is our current plan). Therefore we need to put torrc-defaults somewhere inside the app bundle so it can be updated. Also, we do not really want users to edit torrc-defaults.

comment:34 in reply to:  33 Changed 2 years ago by gk

Replying to mcs:

Replying to gk:

3) What is the reason for treating torrc-defaults differently here, i.e. moving that one out of the data dir?

The updater will not be able to touch anything inside TorBrowser-Data (at least that is our current plan). Therefore we need to put torrc-defaults somewhere inside the app bundle so it can be updated. Also, we do not really want users to edit torrc-defaults.

Sounds good to me.

comment:35 Changed 2 years ago by gk

Commits 4d8e33f4dca21923f3dfef4e740c3c01f395ec1e, 4ea6a818614ec50a62e1bf683baed931d33586ff and f5f6dd2b7d6358343917dba64f722896aa6b79a3 in your tor-browser branch look good to me.

Last edited 2 years ago by gk (previous) (diff)

comment:36 in reply to:  35 ; Changed 2 years ago by mcs

Replying to gk:

Commits 4d8e33f4dca21923f3dfef4e740c3c01f395ec1e, 4ea6a818614ec50a62e1bf683baed931d33586ff and f5f6dd2b7d6358343917dba64f722896aa6b79a3 in your tor-browser branch look good to me.

Thank you for all the reviews! I think the only remaining patch is b03f511d38631243fec0e6c5427d9a50e602a762 (also on our tor-browser branch). After you have a chance to review that one, Kathy and I will create new tor-launcher, tor-browser-bundle, and tor-browser patches. We will take your feedback into account and probably combine 4d8e33f4dca21923f3dfef4e740c3c01f395ec1e and b03f511d38631243fec0e6c5427d9a50e602a762 into one patch while we are at it.

comment:37 in reply to:  36 ; Changed 2 years ago by gk

Keywords: TorBrowserTeam201603 added; TorBrowserTeam201603R removed
Status: needs_reviewneeds_revision

Replying to mcs:

Replying to gk:

Commits 4d8e33f4dca21923f3dfef4e740c3c01f395ec1e, 4ea6a818614ec50a62e1bf683baed931d33586ff and f5f6dd2b7d6358343917dba64f722896aa6b79a3 in your tor-browser branch look good to me.

Thank you for all the reviews! I think the only remaining patch is b03f511d38631243fec0e6c5427d9a50e602a762 (also on our tor-browser branch).

Yes. I just wanted to indicate to you where I was in my review process and that I probably won't get to that commit anymore on Friday. :)

After you have a chance to review that one, Kathy and I will create new tor-launcher, tor-browser-bundle, and tor-browser patches. We will take your feedback into account and probably combine 4d8e33f4dca21923f3dfef4e740c3c01f395ec1e and b03f511d38631243fec0e6c5427d9a50e602a762 into one patch while we are at it.

Sounds good to me. Re the missing commit it looks good to me. I'd like to understand better the following, though:

+        // Display an error alert and continue startup. Since XPCOM was
+        // initialized in a limited way inside ProfileErrorDialog() and
+        // because it cannot be reinitialized, use LaunchChild() to start
+        // the browser.

What exactly is happening in this case? What is the user experiencing? I assume no IP leakage? But what else?

arthuredelstein: Could you please have a look at the C++ bits as well (especially commit b03f511d38631243fec0e6c5427d9a50e602a762 but commits 4d8e33f4dca21923f3dfef4e740c3c01f395ec1e, 4ea6a818614ec50a62e1bf683baed931d33586ff as well)

mcs, brade: Thanks for all this, nice job. I plan to test the profile migration stuff a bit (that's the remaining testing bit I have on my ToDo list) this week. The needs_revision is for the pieces I found so far which are mentioned in previous comments.

Last edited 2 years ago by gk (previous) (diff)

comment:38 in reply to:  37 Changed 2 years ago by mcs

Replying to gk:

[snip] Re the missing commit it looks good to me. I'd like to understand better the following, though:

+        // Display an error alert and continue startup. Since XPCOM was
+        // initialized in a limited way inside ProfileErrorDialog() and
+        // because it cannot be reinitialized, use LaunchChild() to start
+        // the browser.

What exactly is happening in this case? What is the user experiencing? I assume no IP leakage? But what else?

If this code path is followed (because migration of the old/existing profile failed), the user will see an error alert that looks like this:

Tor Browser Profile Problem

Migration of your existing Tor Browser profile failed.
New settings will be used.

After they click OK, the browser will appear to continue to start up, a new profile will be created, and they will see the language prompt or network settings wizard. To the user, it will be as if they downloaded a new copy of Tor Browser, although if they poke inside TorBrowser.app they can find an old TorBrowser/ directory that contains their old profile, their old Tor data directory, and their old UpdateInfo.

Above I said "appear to continue to start up" because firefox is actually re-exec'd inside LaunchChild(). Doing so ensures a clean startup. This same technique is used by Mozilla whenever a dialog is displayed early on, e.g., there is a LaunchChild() call at the very end of ShowProfileManager() (profile picker dialog).

Changed 2 years ago by mcs

Attachment: signit.sh added

Mark and Kathy's codesign test script

comment:39 Changed 2 years ago by mcs

I attached the script Kathy and I have been using to test code signing in case it is useful to anyone else. You will need to set the CERTNAME variable to an appropriate value.

comment:40 Changed 2 years ago by mcs

We pushed revised patches for tor-browser-bundle and tor-launcher (taking Georg's comments into account):

https://gitweb.torproject.org/user/brade/tor-browser-bundle.git/commit/?h=bug13252-02&id=b87c52d313977a531703e79711824c3ef1f92f17
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug13252-02&id=64d3139e72587372223d0795ca715991d7423adb

We will wait for Arthur's comments on the tor-browser patches.

comment:41 Changed 2 years ago by arthuredelstein

Sponsor: None

I read through each of the three tor-browser patches carefully and I didn't find any problems, although I am pretty unfamiliar with this part of the codebase so I may not understand all of the ramifications. Very nice work! One question I have for patch 4d8e33f4dca21923f3dfef4e740c3c01f395ec1e:

--- a/toolkit/mozapps/extensions/AddonManager.jsm
+++ b/toolkit/mozapps/extensions/AddonManager.jsm
@@ -42,6 +42,11 @@ const PREF_MATCH_OS_LOCALE            = "intl.locale.matchOS";
 const PREF_SELECTED_LOCALE            = "general.useragent.locale";
 const UNKNOWN_XPCOM_ABI               = "unknownABI";
 
+#ifdef TOR_BROWSER_VERSION
+#expand const TOR_BROWSER_VERSION = __TOR_BROWSER_VERSION__;
+const PREF_EM_LAST_TORBROWSER_VERSION = "extensions.lastTorBrowserVersion";
+#endif
+
 const UPDATE_REQUEST_VERSION          = 2;
 const CATEGORY_UPDATE_PARAMS          = "extension-update-params";
 
@@ -866,6 +871,30 @@ var AddonManagerInternal = {
         this.validateBlocklist();
       }
 
+#ifdef TOR_BROWSER_VERSION
+      // To ensure that extension override prefs are reinstalled into the
+      // user's profile after each update, set appChanged = true if the
+      // Mozilla app version has not changed but the Tor Browser version
+      // has changed.
+      let tbChanged = undefined;
+      try {
+        tbChanged = TOR_BROWSER_VERSION !=
+                   Services.prefs.getCharPref(PREF_EM_LAST_TORBROWSER_VERSION);
+      }
+      catch (e) { }
+      if (tbChanged !== false) {
+        // Because PREF_EM_LAST_TORBROWSER_VERSION was not present in older
+        // versions of Tor Browser, an app change is indicated when tbChanged
+        // is undefined or true.
+        if (appChanged === false) {
+          appChanged = true;
+        }
+
+        Services.prefs.setCharPref(PREF_EM_LAST_TORBROWSER_VERSION,
+                                   TOR_BROWSER_VERSION);
+      }
+#endif

Is it true here that the TOR_BROWSER_VERSION preprocessor directive already contains quotation marks? I noticed in some other places in a previous patch Kathy and Mark used

+#ifdef TOR_BROWSER_VERSION
+# Add double-quotes back on (stripped by JarMaker.py).
+#expand const TOR_BROWSER_VERSION = "__TOR_BROWSER_VERSION__";
+#endif

but perhaps the quotes are not stripped on AddonManager.jsm?

One other observation: in b03f511d38631243fec0e6c5427d9a50e602a762 it would be nice to have a doc comment at the head of the migrateOneTorBrowserDataDir function describing what it does. I also have difficulty parsing the comment inside that function:

+    // The destination directory exists, which is expected in the case of
+    // migration of the browser profile. Set the new directory aside and set
+    // tmpDir to point to the new, temporary location.

Could you make it explicit why an existing destination directory is expected in migrating the browser profile? And, should the next sentence be something like Set the old directory aside and set tmpDir to point to its new temporary location? (Sorry if I'm misunderstanding.)

comment:43 Changed 2 years ago by gk

I tested the update (a full one, not an incremental one) as good as I could and it works as far as I can see. Nice!

One thing I noticed is that we might want to have a patch for not showing FTE as a PT option for OS X users in case we aren't able to get #18495 fixed until the alpha with the code signing gets out of the door.

comment:44 in reply to:  41 ; Changed 2 years ago by mcs

Replying to arthuredelstein:

I read through each of the three tor-browser patches carefully and I didn't find any problems, although I am pretty unfamiliar with this part of the codebase so I may not understand all of the ramifications. Very nice work!

Thanks!

One question I have for patch 4d8e33f4dca21923f3dfef4e740c3c01f395ec1e:
[snip]
Is it true here that the TOR_BROWSER_VERSION preprocessor directive already contains quotation marks? I noticed in some other places in a previous patch Kathy and Mark used

+#ifdef TOR_BROWSER_VERSION
+# Add double-quotes back on (stripped by JarMaker.py).
+#expand const TOR_BROWSER_VERSION = "__TOR_BROWSER_VERSION__";
+#endif

but perhaps the quotes are not stripped on AddonManager.jsm?

Yes, that is the case (strange as it is). I checked both toolkit/mozapps/extensions/AddonManager.jsm and toolkit/mozapps/update/content/updates.js (which has a code snippet like you showed above) in a built copy of the browser and the quotes are correct in the processed JavaScript files. I think the difference is that a jar.mn file is used to package update.js but not AddonManager.jsm. But I admit that I do not understand all of the quirks of Mozilla's build system.

One other observation: in b03f511d38631243fec0e6c5427d9a50e602a762 it would be nice to have a doc comment at the head of the migrateOneTorBrowserDataDir function describing what it does.

OK. We will add a comment. We tried to write a descriptive comment for migrateInAppTorBrowserProfile() but later we pulled some common code out into migrateOneTorBrowserDataDir() but neglected to document that second function.

I also have difficulty parsing the comment inside that function:

+    // The destination directory exists, which is expected in the case of
+    // migration of the browser profile. Set the new directory aside and set
+    // tmpDir to point to the new, temporary location.

Could you make it explicit why an existing destination directory is expected in migrating the browser profile? And, should the next sentence be something like Set the old directory aside and set tmpDir to point to its new temporary location? (Sorry if I'm misunderstanding.)

Sorry for the confusing comment. Here is an attempt at clearer wording:

// The destination directory exists. When we are migrating an old Tor Browser
// profile, we expect this to be the case because we first allow the standard
// Mozilla startup code to create a new profile as usual, and then later (here)
// we set aside that profile directory and replace it with the old Tor Browser
// profile that we need to migrate. For now, move the Mozilla profile directory
// aside and set tmpDir to point to its new, temporary location in case migration
// fails and we need to restore the profile that was created by the Mozilla code.

We will produce a new tor-browser branch that contains updated patches. Thanks again for your review!

comment:45 in reply to:  43 Changed 2 years ago by mcs

Replying to gk:

One thing I noticed is that we might want to have a patch for not showing FTE as a PT option for OS X users in case we aren't able to get #18495 fixed until the alpha with the code signing gets out of the door.

Good catch. We revised our tor-browser-bundle patch to includes removal of the FTE-related configuration:

https://gitweb.torproject.org/user/brade/tor-browser-bundle.git/commit/?h=bug13252-03&id=de8b74299c2235cfe42d04df15b621b8807903e2

comment:46 in reply to:  44 Changed 2 years ago by mcs

Keywords: TorBrowserTeam201603R added; TorBrowserTeam201603 removed
Status: needs_revisionneeds_review

Replying to mcs:

We will produce a new tor-browser branch that contains updated patches.

This is done now, on a new branch named bug13252-02:
https://gitweb.torproject.org/user/brade/tor-browser.git/log/?h=bug13252-02

There are 3 commits there for this ticket: a9354c824b2435054c3d8d853606a2c7f8622f0b, 38e31a62d48dd8ea8bee02673b7050338fc1c22a, and c1b7d7e2f51f89ab19018e99fe62f8654d3cecb8.

We combined the two #13252 patches and fixed up the comments as we said we would in #comment:44.

comment:48 Changed 2 years ago by gk

Alright. The Torbutton patch is ccf71174bcd6037560436280b93a7dfed84a7d3f on master. The TorLauncher one 64d3139e72587372223d0795ca715991d7423adb on master. Commit de8b74299c2235cfe42d04df15b621b8807903e2 has the changes for the tor-browser-bundle repo. And the changes for tor-browser are the three commits specified above, a9354c824b2435054c3d8d853606a2c7f8622f0b, 38e31a62d48dd8ea8bee02673b7050338fc1c22a, and c1b7d7e2f51f89ab19018e99fe62f8654d3cecb on tor-browser-38.7.1esr-6.0-1.

comment:49 Changed 2 years ago by gk

Hrm, that nightly from https://people.torproject.org/~linus/builds/tbb-nightly-2016-03-20/ does not work for me anymore. It asks me to import settings from IE or Safari at the beginning and is complaining that Tor does not start. Did I forget to merge a commit maybe?

comment:50 Changed 2 years ago by gk

Could be an issue with our nightly build setup as well.

comment:51 in reply to:  50 ; Changed 2 years ago by mcs

Replying to gk:

Could be an issue with our nightly build setup as well.

It looks like the tor-browser code was built with --enable-tor-browser-data-outside-app-dir but the tor-browser-build packaging was done without DATA_OUTSIDE_APP_DIR=1. Maybe add DATA_OUTSIDE_APP_DIR=1 to versions.nightly? But it depends which way you want the nightlies to be built.

comment:52 in reply to:  51 Changed 2 years ago by gk

Replying to mcs:

Replying to gk:

Could be an issue with our nightly build setup as well.

It looks like the tor-browser code was built with --enable-tor-browser-data-outside-app-dir but the tor-browser-build packaging was done without DATA_OUTSIDE_APP_DIR=1. Maybe add DATA_OUTSIDE_APP_DIR=1 to versions.nightly?

Yeah, while I added the proper meek tag I forgot about that one. Fixing this produces a working bundle for me.

comment:53 Changed 2 years ago by mcs

Status: needs_reviewneeds_revision

An issue was raised on IRC yesterday that we need to address: if a non-admin user installs TorBrowser.app into /Applications (as suggested by our disk image), they will not have permission to create /Applications/TorBrowser-Data. In that situation, we need to put the user data somewhere else.

Kathy and I suggest we use ~/Library/Application\ Support/TorBrowser-Data

If everyone agrees with that choice, then we need to decide what criteria to use at runtime to determine whether to use the ~/Library/Application\ Support approach or the "side-by-side" approach. We could do the same thing that Ricochet does, which is to put user data under ~/Library/Application\ Support whenever the .app's path contains "/Applications". We could also use an algorithm that is purely based on whether Tor Browser can write to the location it would use for side-by-side data (in other words, the strategy would be to use side-by-side whenever possible and use ~/Library/Application\ Support/ only if necessary).

The best solution is probably a combination: if the .app path contains /Applications or if we do not have write access, store the user data under~/Library/Application\ Support/TorBrowser-Data.

comment:54 in reply to:  53 Changed 2 years ago by gk

Replying to mcs:

The best solution is probably a combination: if the .app path contains /Applications or if we do not have write access, store the user data under~/Library/Application\ Support/TorBrowser-Data.

Sounds good to me.

comment:55 Changed 2 years ago by mcs

Status: needs_revisionneeds_review

Kathy and I created a couple of fix up patches to cause data to be placed under ~/Library/Application Support/TorBrowser-Data when appropriate. The Tor Launcher one is fairly simple:

https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug13252-fixup-01&id=c4d03f9b16b625f11a08fe0f6dfc5ed6b2467638

And the browser patch is here:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug13252-fixup-01&id=673d1ba99ad5992a5838b808764a8b0ee4fd1056

Note that we refactored the code to reduce redundancy and added a TorBrowser_GetUserDataDir() function. Also, the diffs for nsXREDirProvider::GetUpdateRootDir() are somewhat large because it made more sense to revert that function to the original Mozilla code and then modify it to use the new function. We also realized that on Mac OS we should add the application path under the UpdateInfo directory on because that directory may be shared across different copies of the app (this is especially likely when the data is under ~/Library/Application Support).

Please review.

comment:56 Changed 2 years ago by gk

arthuredelstein: could you please have a look at the updated tor-browser patch, too?

comment:57 in reply to:  56 ; Changed 2 years ago by arthuredelstein

Replying to mcs:

xpcom/io/TorFileUtils.cpp:

+ // When crawling up the hierarchy, components named "." do not count.

Is this a bug in Mozilla's nsILocalFile::GetParent implementation?

When useOSLocation==false, you actively create the "TorBrowser-Data" directory:

+    rv = tbDataDir->AppendNative(tbDataLeafName);
+    NS_ENSURE_SUCCESS(rv, rv);
+    bool exists = false;
+    rv = tbDataDir->Exists(&exists);
+    if (NS_SUCCEEDED(rv) && !exists)
+      rv = tbDataDir->Create(nsIFile::DIRECTORY_TYPE, 0700);

but it looks like you don't create it when useOSLocation==true.

On a possibly related issue, I found the following line confusing:

+    nsresult rv = NS_NewNativeLocalFile(EmptyCString(), true,
+                                        getter_AddRefs(tbDataDir));

I guess this is just for checking that it is possible to write to the parent directory where the "TorBrowser-Data" directory will be? A comment here might help.

/toolkit/xre/nsXREDirProvider.cpp:

+  // Since the TorBrowser-Data directory may be shared among different
+  // installations of the application, embed the app path in the update dir
+  // so that the update history is partitioned.

Is this potentially true for Linux or Windows as well? If I install Tor Browser (FR) and Tor Browser (JA) in the same directory, for example. Perhaps that's a pretty unlikely scenario, though.

Apart from these issues, this patch looks OK to me.

Last edited 2 years ago by arthuredelstein (previous) (diff)

comment:58 in reply to:  57 Changed 2 years ago by mcs

Status: needs_reviewneeds_revision

Replying to arthuredelstein:

Replying to mcs:

xpcom/io/TorFileUtils.cpp:

+ // When crawling up the hierarchy, components named "." do not count.

Is this a bug in Mozilla's nsILocalFile::GetParent implementation?

You could argue that it is, but the answer may depend on your point of view. Kathy and I commonly encounter dots in the executable path because we start from the command line a lot when debugging; most people are not affected by this kind of issue. The code in TorFileUtils.cpp is not new but relocated from elsewhere.

When useOSLocation==false, you actively create the "TorBrowser-Data" directory:

+    rv = tbDataDir->AppendNative(tbDataLeafName);
+    NS_ENSURE_SUCCESS(rv, rv);
+    bool exists = false;
+    rv = tbDataDir->Exists(&exists);
+    if (NS_SUCCEEDED(rv) && !exists)
+      rv = tbDataDir->Create(nsIFile::DIRECTORY_TYPE, 0700);

but it looks like you don't create it when useOSLocation==true.

You are correct. Firefox will create that directory; the only reason we try to create TorBrowser-Data when useOSLocation==false is to find out whether we can. We will add a comment to explain this.

On a possibly related issue, I found the following line confusing:

+    nsresult rv = NS_NewNativeLocalFile(EmptyCString(), true,
+                                        getter_AddRefs(tbDataDir));

I guess this is just for checking that it is possible to write to the parent directory where the "TorBrowser-Data" directory will be? A comment here might help.

We will add a comment. That code is the same as some existing Mozilla code that is nearby. The call to NS_NewNativeLocalFile() gives us an nsIFile object whose path is soon after set to point to the correct location by the calls to InitWithFSRef() and AppendNative().

/toolkit/xre/nsXREDirProvider.cpp:

+  // Since the TorBrowser-Data directory may be shared among different
+  // installations of the application, embed the app path in the update dir
+  // so that the update history is partitioned.

Is this potentially true for Linux or Windows as well? If I install Tor Browser (FR) and Tor Browser (JA) in the same directory, for example. Perhaps that's a pretty unlikely scenario, though.

It is possible but much less likely on Linux or Windows. When we switch to the "side-by-side" approach on those platforms, the TorBrowser-Data directory will be created inside the directory that contains the Browser directory and startup script/Windows shortcut. Because our packaging already includes that top level "container" directory (which was not true on Mac OS), Kathy and I do not think people will install another copy of Tor Browser inside the same directory.

Apart from these issues, this patch looks OK to me.

Thanks for your review! We will post a new patch with the comment changes you suggested.

comment:59 Changed 2 years ago by gk

Just one (additional) thing: "Application Defaults" -> "Application Support" in nsAppFileLocationProvider.cpp.

comment:60 in reply to:  59 Changed 2 years ago by mcs

Status: needs_revisionneeds_review

Replying to gk:

Just one (additional) thing: "Application Defaults" -> "Application Support" in nsAppFileLocationProvider.cpp.

Oops. Good catch.
A revised patch is available here (comment additions and changes only):
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug13252-fixup-02&id=b9a6bef70d96a5aab0cea088d2dc3da4a17354a0

comment:61 Changed 2 years ago by gk

The revised patches are commit b9a6bef70d96a5aab0cea088d2dc3da4a17354a0 on tor-browser-38.7.1esr-6.0-1 and commit c4d03f9b16b625f11a08fe0f6dfc5ed6b2467638 on tor-launcher master. Let's see if the codesigning tool and the OS X systems are happy with the result.

comment:62 Changed 2 years ago by gk

Keywords: TorBrowserTeam201604R added; TorBrowserTeam201603R removed

No easy way to do reviews in March anymore.

comment:63 Changed 2 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Seems to work \o/. Declaring victory here.

Note: See TracTickets for help on using tickets.