Opened 3 years ago

Closed 3 years ago

#18371 closed defect (fixed)

TorBrowser.app.meek-http-helper symlinks incompatible with Gatekeeper signing

Reported by: mcs Owned by: mcs
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201603R
Cc: brade, dcf, gk Actual Points:
Parent ID: #13252 Points:
Reviewer: Sponsor: None

Description

Experimentation shows that the symlink approach that we currently use to create a meek-specific "copy" of Tor Browser on Mac OS is not compatible with Apple's Gatekeeper code signing. Apple's codesign command complains about an invalid Info.plist because it is checking that the application binary (firefox) is where the Info.plist says it is and symlinks are apparently not traversed.

One possible solution is to eliminate the TorBrowser.app.meek-http-helper linked app bundle and add support to firefox for a command line option that causes the application to run as a background app. See https://trac.torproject.org/projects/tor/ticket/11429#comment:8 for more info.

Perhaps if we make the call to TransformProcessType() very early during firefox startup the problem that occurred before (dock icon appearing briefly during startup of the meek browser) will not occur. Another possibility is to change the Info.plist for Tor Browser so that the dock icon is hidden by default and then un-hide it when *not* running as the meek helper browser.

Child Tickets

Attachments (3)

dock-icon.gif (57.8 KB) - added by mcs 3 years ago.
Brief "nudge" of other dock icons when turning off the browser icon early during startup
0001-Bug-18371-symlinks-incompatible-with-Gatekeeper-sign.patch (6.5 KB) - added by mcs 3 years ago.
revised patch
0001-fixup-Bug-18371-symlinks-incompatible-with-Gatekeepe.patch (2.7 KB) - added by mcs 3 years ago.
fixup patch

Download all attachments as: .zip

Change History (23)

Changed 3 years ago by mcs

Attachment: dock-icon.gif added

Brief "nudge" of other dock icons when turning off the browser icon early during startup

comment:1 Changed 3 years ago by mcs

Cc: gk added

Kathy and I did some experimentation yesterday. If we make a call to TransformProcessType() early during the startup process to hide the dock icon, the icon never appears in the dock but the animation to make room for it does begin. Here is what it looks like (notice how the Safari icon is briefly nudged to the left):
Brief "nudge" of other dock icons when turning off the browser icon early during startup
Since this is fairly subtle, and I think we can live with this (but other people may disagree). The other problem is that the TransformProcessType() call does not work on Mac OS 10.6, so users will see two dock icons on 10.6.

On the other hand, if we switch the main browser plist to have LSBackgroundOnly=true and then make a TransformProcessType() call to show the icon, it works OK but there is no "bounce" animation at all for the main Tor Browser's dock icon. This seems like a worse solution to Kathy and me because it makes the behavior of Tor Browser during launch different from every other Mac application.

Also, for the record (and so other people don't waste their time), we also tried to use [NSApp setActivationPolicy:] as described here: https://stackoverflow.com/questions/5382932/#5384319
It seems to behave exactly like TransformProcessType() (subtle nudge of the other dock icons during startup of the meek helper browser and it does not work at all on Mac OS 10.6).

comment:2 in reply to:  1 Changed 3 years ago by dcf

Replying to mcs:

Kathy and I did some experimentation yesterday. If we make a call to TransformProcessType() early during the startup process to hide the dock icon, the icon never appears in the dock but the animation to make room for it does begin. Here is what it looks like (notice how the Safari icon is briefly nudged to the left):

Since this is fairly subtle, and I think we can live with this (but other people may disagree). The other problem is that the TransformProcessType() call does not work on Mac OS 10.6, so users will see two dock icons on 10.6.

Yeah, it doesn't look too bad. I think we can live with it.

For visual comparison, here's how it looked when I tried TransformProcessType in #11429. It might have been OS X 10.7?
Momentary appearance of a dock icon when turning off the icon at runtime using ctypes as in comment:8.

comment:3 Changed 3 years ago by mcs

Support for --invisible inside Tor Browser will be included in our forthcoming patches for #13252.

comment:4 Changed 3 years ago by gk

Status: newneeds_review

dcf: Could you review patch + merge it and tag a new release in case you are satisfied with it? This is currently blocking the OS X signing we want to test in the alphas.

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

Here are a few minor revisions I'd like to see:

  • Copy the profile to a temporary directory (ioutil.TempDir) and rename it atomically (os.Rename). Otherwise, the copy could fail halfway through but the os.Stat check in ensureProfileExists will be fooled.
  • Use filepath.Join to join path instead of string concatenation.
  • In copyFile, you can use os.OpenFile to avoid a separate call to os.Chmod.

How do you want the tags to work? I'm assuming that a meek-client-torbrowser using this patch will only work in an alpha TB, not in a stable. So I'm thinking of making it a parallel branch, rather than extending master. (Maybe this is an argument for moving the meek-client-torbrowser code into the tor-browser-bundle repo.)

comment:6 Changed 3 years ago by gk

Status: needs_reviewneeds_revision

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

Replying to dcf:

Here are a few minor revisions I'd like to see:

  • Copy the profile to a temporary directory (ioutil.TempDir) and rename it atomically (os.Rename). Otherwise, the copy could fail halfway through but the os.Stat check in ensureProfileExists will be fooled.
  • Use filepath.Join to join path instead of string concatenation.
  • In copyFile, you can use os.OpenFile to avoid a separate call to os.Chmod.

We made those changes. I will attach a revised patch.

How do you want the tags to work? I'm assuming that a meek-client-torbrowser using this patch will only work in an alpha TB, not in a stable. So I'm thinking of making it a parallel branch, rather than extending master. (Maybe this is an argument for moving the meek-client-torbrowser code into the tor-browser-bundle repo.)

I will leave the branch decision to Georg, but yes, this is is an alpha-only thing for now.

comment:8 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201603R added
Status: needs_revisionneeds_review

comment:9 in reply to:  5 Changed 3 years ago by gk

Replying to dcf:

How do you want the tags to work? I'm assuming that a meek-client-torbrowser using this patch will only work in an alpha TB, not in a stable. So I'm thinking of making it a parallel branch, rather than extending master. (Maybe this is an argument for moving the meek-client-torbrowser code into the tor-browser-bundle repo.)

I have no strong opinions here although I am not so sure about moving meek-clienr-torbrowser into the tor-browser-bundle repo. Giving that the modifications should be for stable users as well once we switch to ESR 45 (which is basically due end of May) a new branch we would use for now might be okay?

comment:10 in reply to:  7 Changed 3 years ago by dcf

Looks good. I made you a branch bug18371 with a signed tag 0.22-18371-1.

comment:11 Changed 3 years ago by gk

Status: needs_reviewneeds_revision

meek does not work with the patch on my OS X 10.6 test machine:

2016/03/09 12:34:20 creating profile by copying files from ../../Resources/TorBrowser/Tor/PluggableTransports/template-profile.meek-http-helper to /Users/release/Desktop/TorBrowser-Data/Tor/PluggableTransports/profile.meek-http-helper
2016/03/09 12:34:20 mkdir /Users/release/Desktop/TorBrowser-Data/Tor/PluggableTransports/tmpMeekProfile667155114: no such file or directory

comment:12 Changed 3 years ago by gk

Keywords: TorBrowserTeam201603 added; TorBrowserTeam201603R removed

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

Keywords: TorBrowserTeam201603R added; TorBrowserTeam201603 removed
Status: needs_revisionneeds_review

Replying to gk:

meek does not work with the patch on my OS X 10.6 test machine:
...

Sorry about that! After Kathy and I made the changes suggested by dcf we did not do a clean test. I attached a fixup patch.

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

Replying to mcs:

Replying to gk:

meek does not work with the patch on my OS X 10.6 test machine:
...

Sorry about that! After Kathy and I made the changes suggested by dcf we did not do a clean test. I attached a fixup patch.

Tag 0.22-18371-2 has the fixup patch.

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

Replying to dcf:

Tag 0.22-18371-2 has the fixup patch.

Thanks! It looks like we need to use a newer tag for goptlib as well. I see these error messages when I build:

./meek-client.go:404: ptInfo.ProxyURL undefined (type pt.ClientInfo has no field or method ProxyURL)
./meek-client.go:410: undefined: pt.ProxyError
./meek-client.go:415: ptInfo.ProxyURL undefined (type pt.ClientInfo has no field or method ProxyURL)
./meek-client.go:416: undefined: pt.ProxyDone

Currently, Tor Browser defines GOPTLIB_TAG=0.2 and uses that when building meek.

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

Replying to mcs:

Replying to dcf:

Tag 0.22-18371-2 has the fixup patch.

Thanks! It looks like we need to use a newer tag for goptlib as well. I see these error messages when I build:

./meek-client.go:404: ptInfo.ProxyURL undefined (type pt.ClientInfo has no field or method ProxyURL)
./meek-client.go:410: undefined: pt.ProxyError
./meek-client.go:415: ptInfo.ProxyURL undefined (type pt.ClientInfo has no field or method ProxyURL)
./meek-client.go:416: undefined: pt.ProxyDone

Currently, Tor Browser defines GOPTLIB_TAG=0.2 and uses that when building meek.

Yes, you will need GOPTLIB_TAG=0.5 (the most recent tag) for the ProxyURL stuff.

Alternatively, I could make you a branch off of 0.20, the current MEEK_TAG and the most recent one that doesn't depend on ProxyURL in goptlib.

But the diffs are pretty minor:

https://gitweb.torproject.org/pluggable-transports/goptlib.git/diff/?id=0.5&id2=0.2
https://gitweb.torproject.org/pluggable-transports/meek.git/diff/?id=0.22&id2=0.20

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

Replying to dcf:

[snip]
Yes, you will need GOPTLIB_TAG=0.5 (the most recent tag) for the ProxyURL stuff.

Alternatively, I could make you a branch off of 0.20, the current MEEK_TAG and the most recent one that doesn't depend on ProxyURL in goptlib.

But the diffs are pretty minor:

https://gitweb.torproject.org/pluggable-transports/goptlib.git/diff/?id=0.5&id2=0.2
https://gitweb.torproject.org/pluggable-transports/meek.git/diff/?id=0.22&id2=0.20

I assume your recommendation is to use the latest meek code? I quickly looked over the diffs and did not see anything that alarmed me, so maybe we should look at this as an opportunity to move Tor Browser to the latest meek code. gk, what do you think?

comment:18 Changed 3 years ago by gk

Sounds good to me.

comment:19 in reply to:  18 Changed 3 years ago by mcs

Sponsor: None

Replying to gk:

Sounds good to me.

OK. I think we just need to set the following in versions.alpha when we update tags for Tor Launcher, Torbutton, etc.:

GOPTLIB_TAG=0.5
MEEK_TAG=0.22-18371-2

comment:20 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Commit 3a5626618641ff45e3431700fe74932644a4e98b has the changes for the nightly builds. Marking this as closed as the work here is done.

Note: See TracTickets for help on using tickets.