Opened 3 years ago

Last modified 2 months ago

#12716 needs_revision defect

Make meek-client-torbrowser take the firefox command as a parameter

Reported by: dcf Owned by: dcf
Priority: Low Milestone:
Component: Obfuscation/meek Version:
Severity: Normal Keywords:
Cc: brade, mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

meek-client-torbrowser hardcodes the firefox binary and profile paths: linux mac windows. The problem is that when Tor Browser is reorganized, as it was in #11641, you need to make the corresponding change in meek-client-torbrowser, for example 178572f5.

meek-client-torbrowser already takes the full meek-client command on the command line; it looks like:

ClientTransportPlugin meek exec ./TorBrowser/Tor/PluggableTransports/meek-client-torbrowser -- ./TorBrowser/Tor/PluggableTransports/meek-client --url=https://meek-reflect.appspot.com/ --front=www.google.com

I don't know of a good clear way to encode two separate command lines into the command line of another program, except maybe to do them both as long strings and then parse the strings before calling exec.

Child Tickets

Attachments (5)

0001-Add-firefox-and-profile-options-to-meek-client-torbr.patch (4.5 KB) - added by dcf 3 years ago.
12716-1.patch.gz (3.7 KB) - added by dcf 2 years ago.
Patch to build tor-browser-bundle with meek-client-wrapper (gzipped because f'ing Trac thinks it contains too many external links)
12716-2.patch.gz (4.0 KB) - added by infinity0 2 years ago.
Bring in fixes for 15125 and make windows clean up properly
12716-3.patch.gz (4.0 KB) - added by infinity0 2 years ago.
Also fix path finding on FreeBSD-based systems
12716-4.patch.gz (4.2 KB) - added by infinity0 2 years ago.
Fix windows wrapper script for the case where firefox decides to output extra things to StdOut

Download all attachments as: .zip

Change History (37)

comment:1 follow-up: Changed 3 years ago by dcf

attachment:0001-Add-firefox-and-profile-options-to-meek-client-torbr.patch​ adds --firefox and --profile options to meek-client-torbrowser. I'm not sure that's the best way to go. It looks like this:

./meek-client-torbrowser --firefox ./firefox --profile TorBrowser/Data/Browser/profile.meek-http-helper -- ./meek-client

I thought of passing both complete commands as strings, like

./meek-client-torbrowser --firefox "./firefox -no-remote -profile TorBrowser/Data/Browser/profile.meek-http-helper" --meek-client "./meek-client"

But then there would need to be code to split up a quoted string, and I'm not sure I want that. Also I think tor might split up a ClientTransportPlugin line on whitespace, and chop up the quoted strings.

Another option is to slightly abuse -- as an additional separator between the firefox command and the meek-client command:

./meek-client-torbrowser -- ./firefox -no-remote -profile TorBrowser/Data/Browser/profile.meek-http-helper -- ./meek-client

You can get away with that because firefox doesn't use --.

But there's an additional problem with passing a complete firefox command line. Mac OS X needs an absolute path for the profile directory, so meek-client-torbrowser absolutizes it. It couldn't do that easily if the whole command were to come as a unit.

comment:2 in reply to: ↑ 1 ; follow-up: Changed 3 years ago by mcs

Replying to dcf:

But there's an additional problem with passing a complete firefox command line. Mac OS X needs an absolute path for the profile directory, so meek-client-torbrowser absolutizes it. It couldn't do that easily if the whole command were to come as a unit.

Why does Mac OS need an absolute path to the profile directory? Is that a Firefox limitation?
Nevermind. I found this: https://bugzilla.mozilla.org/show_bug.cgi?id=673955

How inconvenient.

comment:3 Changed 3 years ago by mcs

  • Cc brade mcs added

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

Replying to mcs:

Replying to dcf:

But there's an additional problem with passing a complete firefox command line. Mac OS X needs an absolute path for the profile directory, so meek-client-torbrowser absolutizes it. It couldn't do that easily if the whole command were to come as a unit.

Why does Mac OS need an absolute path to the profile directory? Is that a Firefox limitation?
Nevermind. I found this: https://bugzilla.mozilla.org/show_bug.cgi?id=673955

Hmm, I see. I wasn't aware of that Mozilla bug. I had assumed the reason was how OS X 10.9 started setting the working directory of applications to "/"; i.e., the same reason we do the silly tor.real trick a.k.a. #10030. But I could be wrong.

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

comment:5 Changed 2 years ago by infinity0

How about the following:

meek-client-helper /path/to/meek/client <meek-client args> -- <helper command line>

We know meek-client doesn't use --, so this is safe in all cases. Then, meek-client-helper doesn't need any args of its own.

To make things work on OSX, we do this in the Bridge line (until that Mozilla bug is fixed):

meek-client-helper <etc> -- sh -c 'exec "$1" -no-remote -profile "$(realpath "$2")"' TBB ./rel/path/to/firefox ./rel/path/to/profile

"TBB" can be anything, it's just what would show up in process lists as the "command name".

This means we can remove all firefox/tbb specificness from this program, as well as the OSX workaround. I am hoping the Bridge line supports args with spaces in. If it doesn't, we can instead write a wrapper shell script that calls firefox and does $(realpath) on the profile arg.

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

comment:6 Changed 2 years ago by dcf

I'm not in love with any idea that has been proposed so far. The double-dash idea from comment:1 and comment:5 is workable, but I think we would grow to dislike it.

The sh -c exec "$1" idea in comment:5 is about 1.2× as crazy I usually like solutions to be :) But it's headed in a good direction. What infinity0 wrote "we can instead write a wrapper shell script that calls firefox" is what we should do, I think.

Here's what I'm thinking:

meek-client-helper --helper ./path/to/helper.sh -- ./path/to/meek-client

The --helper option is the program that runs the helper. However the catch is that you only get to specify the name of a single file to run: no arguments, no shell expansion, and no string splitting. Anything that the helper needs, for example the path to the profile, or a call to realpath, has to be encoded into the script.

In the browser bundle, we'll have a different helper script for each OS, corresponding to the current linux.go, mac.go, and windows.go in meek-client-torbrowser. On windows we can use, I don't know, a batch file or something, or in the worst case yet another compiled program. The customization for Debian will be just one Debian-specific shell script.

The reason for having a --helper option instead of having it being implied by position is to leave room for other options in meek-client-helper like --log. It keeps the usual meaning of --; i.e., everything before -- "belongs" to meek-client-helper and everything after is a non-option. Omitting the --helper option could mean to just run meek-client directly without a helper (which is sometimes useful).

comment:7 follow-up: Changed 2 years ago by infinity0

Here's a first attempt at doing as suggested in David's previous comment:

https://github.com/infinity0/meek/compare/upstream...master

comment:8 in reply to: ↑ 7 Changed 2 years ago by dcf

Replying to infinity0:

Here's a first attempt at doing as suggested in David's previous comment:

https://github.com/infinity0/meek/compare/upstream...master

I took a quick look and it looks like what I expected. Thanks.

Two questions:

  1. Does this work for what Debian/Tails/Whonix wants?
  2. What is the meek-browser-helper script for? My first instinct is not to add new functionality (auto-installing a profile).

comment:9 Changed 2 years ago by infinity0

meek-browser-helper is similar in functionality to tbb-linux.sh, except it also automatically creates a user profile with that extension enabled. That is, you can use it like ClientTransportPlugin meek exec meek-client-wrapper --helper meek-browser-helper -- meek-client <args> etc. It's meant to work on a clean linux system where you don't already have a pre-made profile for meek-http-helper (unlike TBB).

I'm planning to have it in the Debian package, but it should be generic enough to work for other Linux distros as well (the hardcoded paths are same across linux distros) - which is why I didn't add a -debian suffix to the filename.

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

comment:10 Changed 2 years ago by dcf

I'm starting a tor-browser-bundle build that's modified to use meek-client-wrapper.

comment:11 Changed 2 years ago by dcf

  • Summary changed from Make meek-client-torbrower take the firefox command as a parameter to Make meek-client-torbrowser take the firefox command as a parameter

Changed 2 years ago by dcf

Patch to build tor-browser-bundle with meek-client-wrapper (gzipped because f'ing Trac thinks it contains too many external links)

comment:12 follow-up: Changed 2 years ago by dcf

Here is a build of Tor Browser that uses the branch from comment:7.

https://people.torproject.org/~dcf/pt-bundle/4.5a4-12716-1/

It's built using this patch to tor-browser-bundle:

attachment:12716-1.patch.gz

I tested it on linux64 but not any other platforms. I asked tor-qa for testing here:

https://lists.torproject.org/pipermail/tor-qa/2015-March/000555.html

comment:13 in reply to: ↑ 12 Changed 2 years ago by dcf

Replying to dcf:

I tested it on linux64 but not any other platforms. I asked tor-qa for testing here:

https://lists.torproject.org/pipermail/tor-qa/2015-March/000555.html

Wilton and Mark say it doesn't work on OS X, giving an error "Your Firefox profile cannot be loaded." Mark suggests it's because there is no realpath on OS X.

Here is the meek-browser-helper-mac.sh script:

# Mac OS X needs an absolute profile path.
# https://bugzilla.mozilla.org/show_bug.cgi?id=673955
exec "PluggableTransports/TorBrowser.app.meek-http-helper/Contents/MacOS/firefox" \
  -no-remote -profile "$(realpath "../Data/Browser/profile.meek-http-helper")"

Maybe it could use dirname like mac-tor.sh does?

comment:14 Changed 2 years ago by dcf

  • Status changed from new to needs_revision

Oh! infinity0, please do not add a default browser helper argument like this. I'm fine with having the meek-browser-helper script in the repo as an example, but I do not want to make it the default behavior for everybody. I was surprised when I ran it and got this error:

2015/03/13 09:22:17 running browser-helper command ["meek-browser-helper"]
2015/03/13 09:22:17 exec: "meek-browser-helper": executable file not found in $PATH

I want this to be a place where distributions set policy, not us.

comment:15 follow-up: Changed 2 years ago by infinity0

readlink -f should work instead of realpath. I've asked tor-qa about this.

The Go options library does not allow "required options" because they are an oxymoron - they recommend that required options should instead be a positional argument - https://groups.google.com/forum/#!topic/golang-nuts/agYvRPNaBNA

So I thought a default value of "meek-browser-helper" was the best option - it would work at least some of the time (as opposed to none of the time), and distros can still install their own /usr/bin/meek-browser-helper if they disagreed with the example script given.

An alternative is to set the default value to "", and if this is not set by the user then give a custom error message like "either set --helper or use meek-client directly". Will do that in a commit tomorrow if you prefer.

comment:16 in reply to: ↑ 15 Changed 2 years ago by dcf

Replying to infinity0:

readlink -f should work instead of realpath. I've asked tor-qa about this.

$(dirname "$0") might work. You don't need a canonical path, just an absolute path.

The Go options library does not allow "required options" because they are an oxymoron - they recommend that required options should instead be a positional argument - https://groups.google.com/forum/#!topic/golang-nuts/agYvRPNaBNA

So I thought a default value of "meek-browser-helper" was the best option - it would work at least some of the time (as opposed to none of the time), and distros can still install their own /usr/bin/meek-browser-helper if they disagreed with the example script given.

An alternative is to set the default value to "", and if this is not set by the user then give a custom error message like "either set --helper or use meek-client directly". Will do that in a commit tomorrow if you prefer.

The last one is what I had in mind—just output an error. Or we could just make it really optional, and not run any helper (nor pass --helper to meek-client) if it's missing.

comment:17 Changed 2 years ago by infinity0

OK, I've updated the branch. I also removed the tbb-* scripts since we'll be moving them to the TB repo instead of the meek repo.

dirname doesn't resolve relative paths by itself, but "$(dirname "$(pwd -P)")/Data/Browser/profile.meek-http-helper" should work.

Changed 2 years ago by infinity0

Bring in fixes for 15125 and make windows clean up properly

comment:18 follow-up: Changed 2 years ago by infinity0

I've tweaked your patch; you can apply it with zcat 12716-3.patch.gz | git am --keep-cr. Could you start a build with it, please?

A quicker way of trying this is to copy m-c-w.exe and m-b-h-w.{js,bat} from https://people.torproject.org/~infinity0/bin/ into Browser\TorBrowser\Tor\PluggableTransports in your installation of 4.5a4-12716-1, overwriting what's already there.

(edit: updated the patch to bring in the other fix too)

(edit: updated the patch to fix the problem of "However, when I try this, the first time that meek-client-wrapper starts, it doesn't spawn meek-client so connection to Tor fails. But if I restart TBB, it then works. I'm hoping the "properly-built" version won't have this bug.")

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

Changed 2 years ago by infinity0

Also fix path finding on FreeBSD-based systems

comment:19 in reply to: ↑ 18 Changed 2 years ago by dcf

Replying to infinity0:

I've tweaked your patch; you can apply it with zcat 12716-2.patch.gz | git am --keep-cr. Could you start a build with it, please?

Thanks, but I do not want 36bcdcab. I won't merge that commit. (It's out of scope for this ticket anyway?)

My position is that we're waiting to find out whether https://lists.torproject.org/pipermail/tor-qa/2015-March/000562.html works on OS X. If it does, then I plan to merge meek-client-wrapper, tag a release, and ask the Tor Browser maintainers to start using it in a separate ticket. After that, turn my attention to #15125.

comment:20 Changed 2 years ago by infinity0

The solution to this ticket, on Windows, forces one more child process between meek-client-wrapper and firefox, because windows inherently doesn't have "exec" (or something that replaces the current process). So that commit is necessary for this new situation to be cleaned up properly, because the direct child of the wrapper needs *time* to clean up its own children (firefox), and meek-client-wrapper currently doesn't give it this time. (Explained in more detail in ticket:15125#comment:3.)

So, yes you could merge what we have so far, but I think for a "clean release" with fewer issues it's better to also merge the fix for 15125. I appreciate that it's more complex, which I can help to reduce.

We can wait for tor-qa to report back the situation on windows, but I'm pretty sure I'm accurate in my theorising. I have also tested this stuff on a Windows 7 VM myself.

Changed 2 years ago by infinity0

Fix windows wrapper script for the case where firefox decides to output extra things to StdOut

comment:21 follow-up: Changed 2 years ago by infinity0

An alternative that does not require the timer/signal part of #15125, but would still require the keep-stdin-open part of it, is to have meek-http-helper close itself when it detects that its own stdin is closed.

However, this is not trivial. I am not an expert in the extensions API, but I could not find a direct API for looking at firefox's stdin. The closest I could think of is using js-ctypes to call C functions directly to look at it, but the documentation for that is non-existent. I did find a relatively good and complete example of how to use it to do useful things here: https://github.com/bit/subprocess/blob/master/subprocess.jsm but it would still take a fair bit of work to implement the behaviour we want. But perhaps I can find someone who knows a lot about js-ctypes to help me with this.

comment:22 in reply to: ↑ 21 Changed 2 years ago by mcs

Replying to infinity0:

However, this is not trivial. I am not an expert in the extensions API, but I could not find a direct API for looking at firefox's stdin. The closest I could think of is using js-ctypes to call C functions directly to look at it, but the documentation for that is non-existent. I did find a relatively good and complete example of how to use it to do useful things here: https://github.com/bit/subprocess/blob/master/subprocess.jsm but it would still take a fair bit of work to implement the behaviour we want. But perhaps I can find someone who knows a lot about js-ctypes to help me with this.

The js-ctypes unit test code might be helpful if you need examples of how to declare C types and functions (and use them):

http://mxr.mozilla.org/mozilla-esr31/source/toolkit/components/ctypes/tests/unit/test_jsctypes.js

Note that at one point Mike Perry removed js-ctypes support from Tor Browser (more secure not to have it), but we ended up re-enabling it because some internal Mozilla code relies on it.

comment:23 Changed 23 months ago by dcf

So we have a pretty good solution that works on linux and mac, which is that the meek-client-wrapper program takes as an option a single pathname that points to a shell script that execs firefox. But the same idea does not work automatically on windows because of a lack of exec--when meek-client-wrapper kills its child process (the shell script/batch file), it does not kill the firefox process.

Potential workarounds are to make the windows shell script into a JScript file that does the trick where it watches its own stdin for closure (attachment:12716-4.patch.gz). This solution is pretty reasonable but it makes me uncomfortable. I'd never heard of this JScript before now. I also don't want to dive into js-ctypes for this problem.

It seems there are two motivations behind this ticket, #12716:

  1. To reduce coupling between the tor-browser and meek repositories (don't need to change something in the meek repository when the layout of Tor Browser changes).
  2. To enable packaging on e.g. Debian, where firefox is in a different place than it is in Tor Browser.

So I'm thinking of preserving the status quo with respect to how it works on windows, which fails to solve #1, but doesn't prevent us from making a wrapper program that works on other platforms (what infinity0 already wrote) that can solve #2. I'd even be willing to replace meek-client-torbrowser with a meek-client-wrapper that executes a shell script on linux and mac, concede that it doesn't work on windows, and provide a --magic-torbrowser-windows-exec option that makes it do what meek-client-torbrowser currently does on windows; i.e., execute firefox directly at a hardcoded path.

comment:24 Changed 23 months ago by infinity0

That sounds good. We can still decouple the repositories however - have the Makefile take path variables that then gets hardcoded into the final binary, which could also have a different name. The TBB repo can call meek's Makefile with the TBB specific paths.

So most platforms we'd build the generic wrapper, and on windows we can build one with hardcoded paths.

I hear conditional compilation on go is awkward though, I will look into it and maybe submit a patch.

comment:25 follow-up: Changed 23 months ago by infinity0

Or simpler: if the argument to --helper is a .args file (or whatever), then instead of passing this as a filename to exec.Command, read the file and parse it as a line-separated list of arguments to pass to exec.Command.

Actually, this is so simple I will just write a patch for it soon, on top of my existing previous patches.

comment:26 Changed 23 months ago by infinity0

https://github.com/infinity0/meek/commit/91a476c38ddb1f405412c754ee5fe5eb8abe4aaa
https://github.com/infinity0/meek/compare/upstream...master for context

Using .meek-browser-helper instead of .args. For example, now on windows we can do something like:

$ file windows.meek-browser-helper 
windows.meek-browser-helper: ASCII text, with CRLF line terminators

$ cat windows.meek-browser-helper 
./firefox.exe
-no-remote
-profile
TorBrowser/Data/Browser/profile.meek-http-helper

$ make && ./meek-client-wrapper --helper windows.meek-browser-helper -- meek-client
go build
2015/05/22 17:15:56 running browser-helper command ["./firefox.exe" "-no-remote" "-profile" "TorBrowser/Data/Browser/profile.meek-http-helper"]
2015/05/22 17:15:56 fork/exec ./firefox.exe: no such file or directory
Last edited 23 months ago by infinity0 (previous) (diff)

comment:27 in reply to: ↑ 25 Changed 23 months ago by dcf

Replying to infinity0:

Or simpler: if the argument to --helper is a .args file (or whatever), then instead of passing this as a filename to exec.Command, read the file and parse it as a line-separated list of arguments to pass to exec.Command.

Actually I like this idea. Only I would not want to then support both a script argument or an args argument--I would want there to be only one way. Only then we run into the same problem with mac needing an absolute profile path... hmm.

As to the idea with baking in the paths at compile time: we are already doing something similar in meek-client-torbrowser. E.g. linux.go has at the top

// +build linux

which causes it to be built only on linux (http://golang.org/pkg/go/build/#hdr-Build_Constraints). Seeing as firefoxPath and firefoxProfilePath are the only things we need to conditionally define, we could just remove mac.go, linux.go, and windows.go, and force whoever's building to add their own x.go with their own custom paths.

On another hand, we could just move the entirety of meek-client-torbrowser into the tor-browser tree, and establish the convention that other system-specific interfaces between meek-client and a browser will also be maintained outside the meek tree.

comment:28 Changed 22 months ago by infinity0

meek-client-torbrowser is sufficiently useful for other projects that it would be a shame to move it into the tor-browser tree.

I think (a) supporting both a script/args argument is fine, and that you are overthinking the downsides in supporting both.

But, if you really want to, we can (b) have linux.go / mac.go define a config constant that says "--helper is a script" and have windows.go define it that says "--helper is a args file". Only windows needs the latter, everything else is POSIX and has proper interprocess signals.

Either of these options would satisfy both (1) and (2) the goals of this ticket. I prefer (a), but would also be happy with (b) just to get this thing done already. :)

comment:29 Changed 18 months ago by infinity0

Hi, any new opinions on this? I've rebased to current master:

https://github.com/infinity0/meek/compare/upstream...master

The first four commits were talked about previously. That leaves:

  • 43a35d32 This is as per ticket:12716#comment:26, plus I added the ability to specify ENVVAR=val so that we can include the fix for #13247 on TorBrowser-windows. See e.g. b1cb6747
  • 6b21c416 Here I made each platform only support one single behaviour - windows uses the "argument list" format, and the other platforms treats --helper as an executable.
    • I still think this is unnecessary, and that it's more clean to just consistently support both options. One can think of the "argument list" file as a custom script that meek-client-wrapper knows how to interpret and execute. Instead, we can add some comments about why the "single script" option is probably unsuitable for Windows.

Instead of continually attaching patches, I've pushed the corresponding edits to TorBrowser here:

https://github.com/infinity0/tor-browser-bundle/compare/upstream...master

It includes an example of how to use the "argument list" file for TorBrowser on Windows.

comment:30 Changed 9 months ago by infinity0

  • Severity set to Normal

Any updates on this? Yet another option is to drop the complexity of 43a35d32, hardcode things specifically for TorBrowser-windows, but use the generic shell-script option on *NIX systems.

comment:31 follow-up: Changed 2 months ago by 6h72Q484AddGha8H

status on this? ready to deploy fix so the official meek-client can be added to Debian repos?

comment:32 in reply to: ↑ 31 Changed 2 months ago by dcf

Replying to 6h72Q484AddGha8H:

status on this? ready to deploy fix so the official meek-client can be added to Debian repos?

meek-client-torbrowser has only gotten more tightly coupled to Tor Browser, with #18371, #18904, #19646. meek-client-torbrowser is unlikely now ever to work for ordinary Firefox.

My advice is make a separate meek-client-wrapper or meek-client-debian or something, perhaps by forking an older version of meek-client-torbrowser. The code for it could be maintained in the Debian package (it's not much code) along with whatever xvfb or other auxiliary code Debian needs. I'm also willing to have a generic or Debian-specific in the main meek source tree, if that's better for the Debian maintainers.

Another alternative is just to ignore the browser camouflage layer on Debian. That's what every other non–Tor Browser platform already does, as far as I know.

Note: See TracTickets for help on using tickets.