Sebastien Marie <[email protected]> writes:
> On Thu, Jun 23, 2016 at 05:24:58PM -0500, attila wrote:
>> Hi ports@,
>>
>> Here is another try at the Tor Browser Bundle, updated to 6.0.2 just
>> today.
>>
> [...]
>>
>> All feedback most welcome.
>>
>
> Hi,
>
> I would like to ask some questions.
This makes me very happy :-).
>
> - www/tbb/tor-browser/files/tor-browser.cfg file
>
> where does the configuration comes from you ? is it settings from you
> or from TBB ?
>
> I ask due to default bridges configuration (addresses, fingerprints
> and certs), and to default plugins configuration. These configuration
> settings are very sensible.
>
This mostly comes directly from Tor Browser. Our tor-browser.cfg is
actually two things:
(1) the contents of extension-overrides.js from the TBB, lightly
edited to pass muster as a cfg file... for some reason they use
#comments in extension-overrides.js. I don't know why this is
valid syntax in that context but it isn't in a cfg file, which
is pure JS;
(2) The bit at the end is my addition, whose purpose is to initialize
your ~/.tor-browser/torrc to an empty config if it doesn't exist.
This has to happen before the tor-launcher addon tries to start
tor or else we have problems. In the past I tried to solve this
issue by adding a sh script called start-tor-browser that took
care of these things but landry@ pointed out that I should not
need to do this if I'm using moz correctly.
All of the settings that come along with (1) are verbatim what the Tor
project says. They change between releases quite often. I track that
file manually right now, I would like to make this more automated.
The second point needs some explanation. We need ~/.tor-browser/torrc
to exist before tor browser starts for the first time, or else it will
never get created and tor browser won't work correctly. Worse, it
will *appear* to work correctly the first time. This is more an issue
in tor itself. If you say
$ tor -f /some/nonexistent/file --ignore-missing-torrc \
--defaults-torrc /file/that/exists ...
tor will start, using the config in /file/that/exists, and not
complain about /some/nonexistent/file, but it will also forget that
you said -f /some/nonexstent/file. This is a problem because Tor
browser starts and controls its own instance of the tor daemon via the
control port. One of the first things it does once it gets going is
send tor a SAVECONF command, which means: save the non-default
settings in your running config as a torrc file. The question is:
where will it try to write them? If -f specified a non-exstent file
that pathname gets forgotten, so under OpenBSD it ends up trying to
write onto /etc/tor/torrc. In the best case this fails due to perms,
and the user can't really change their config via the normal means
(e.g. add bridge lines to the config when TB starts up using their
dialog, which I generally do in Mexico). In the worst case, the user
is in the same group as the /etc/tor/torrc file and it is
group-writable (happened to me once due to stupidity which I will not
repeat), and, well...
Okay, I guess the worse case is that the user runs tor-browser as
root, which, sadly, will "work" without incident... except that your
system-wide tor config gets stomped on silently. Somehow I have a
feeling most OpenBSD users know not to run stuff as root...
It has been on my list for a while to investigate whether this is
considered a bug by the tor people or not (-f forgets a non-existent
path). I just haven't gotten to it, since I had a patch that dealt
with the issue regardless... death by a thousand patches.
>
> - www/tbb/tor-browser/patches/patch-toolkit_xre_nsAppRunner_cpp file
> "Revert the file back to ESR45.1.1, all diffs were TB-specific and not
> relevant to OpenBSD".
>
> Can you explain why ? The diff is quite large, and reverts elements
> like:
> - In Tor Browser, remoting is disabled by default unless -osint is
> used (so --no-remote isn't the default anymore with TBB).
This was an oversight on my part. I shouldn't have reverted that
part.
> - Set the application-wide C-locale. Needed to resist fingerprinting
> of Date.toLocaleFormat().
This is another oversight. As you point out the patch is too big and
these are good examples of why that's a bad idea, since I missed some
things I should've caught.
In the switch to ESR45 the Tor people changed their hacks to the stuff
in toolkit/xre, probably in part because moz itself changed there
between ESR38 and ESR45. They had hacks strewn through several files
before that were effectively the core of "the bundle": stuff that
hard-coded paths relative to the location of the tor-browser binary
that didn't work how the normal moz code worked when it comes to
finding your profile, etc.
Instead now in ESR45-based Tor Browser they refactored most of their
stuff into a separate module and call into that module from various
places in toolkit/xre. All of the bundle-specific stuff didn't seem
relevant to our port, since we're not using that approach. Instead we
have software installed unde /usr/local and per-user config stuff
under ~/.tor-browser.
The choice of ~/.tor-browser was a conscious decision I made when
doing the port because I felt it was the most natural and logical
thing that an OpenBSD user would expect, given that I had to break up
the bundle one way or another to make this work at all. Now I'm
thinking maybe this was a wrong decision, or at very least something
ports@ needs to agree on... see my response to your comments below on
this for more.
> - www/tbb/tor-browser/patches/patch-toolkit_xre_nsXREDirProvider_cpp file
>
> You revert TBB specific code for cache data dir: so the port will use
> $XDG_CACHE_HOME if defined, or use $HOME/.cache instead of keeping
> stuff in GetTorBrowserUserDataDir()
I think this bit of the patch was another mistake because of the
approach I was taking (reverting the bundle bits). This didn't occur
to me probably because of the way I manage my own ~/.cache, which has
nothing to do with anyone else, it just blinded me to the problem.
Should always do these things in a generic account, yet another
example of why.
Instead it seems I should've addressed this head-on. If the
~/.tor-browser approach is deemed the right one then I guess
~/.tor-browser/Caches would be an okay answer, so assuming this is the
way to go the patch should be changed. In any event this must be
dealt with.
> Shouldn't all TBB stuff to be kept under one directory ?
Yes and that was my intention.
I'm really glad there is some discussion about this patch but it isn't
(yet) on the subject I actually care about. The whole reason for that
patch comes down to this:
- static const char* const sXR = ".mozilla";
+ static const char* const sXR = "." MOZ_APP_NAME;
The issue would be clearer if I had instead done this, which is what I
see now it should be:
+ static const char* const sXR = MOZ_USER_DIR;
This is not a Tor browser issue, this is a Mozilla issue: MOZ_USER_DIR
is set via ./configure --with-user-appdir. I add
--with-user-appdir=.tor-browser to CONFIGURE_ENV in the Makefile, but
the problem is that this is effectively a no-op because
nsXREDirProvider::AppendSysUserExtensionPath ignores MOZ_USER_DIR and
will (I think) always end up spitting out ~/.<vendor>/<product>, so no
matter what you set --with-user-appdir to you still end up with
~/.mozilla/firefox (under Unix).
I have been considering whether I should propose a patch upstream to
mozilla, but I hadn't decided what it should be. The problem, to me,
is that MOZ_USER_DIR is always set to something and is therefore not
useful as a way of overriding a default; in fact it is set to
".mozilla", which is still not what the code will do by default. I
think this has been easy to overlook because what the code actually
does (~/.mozilla/firefox under normal circumstances) makes it look
like the default MOZ_USER_DIR (.mozilla) is being used, but it's just
an accident. This sounds to me like things diverged somehow between
what the autoconf-based config system does and what happened in the
code. I can see how this could happen over the years and nobody might
notice.
If MOZ_USER_DIR was only set when the builder overrides the default
with --with-user-appdir then you could do #ifdef MOZ_USER_DIR to use
it if it were set and fall back to the default if it wasn't. This
would be the cleanest solution to me, but unfortunately I think this
ship sailed a long time ago and patching mozilla to deal with
MOZ_USER_DIR not being set would be bad. Since the default for
MOZ_USER_DIR is ".mozilla" and the default path that the code will
come up with under Unix is ~/.mozilla/firefox, the code could instead
be patched to notice if MOZ_USER_DIR is set to a prefix of the true
(in the code) default, which is the situation normally. If it ISN'T,
then AppendSysUserExtensionPath could just respect MOZ_USER_DIR and
not do anything more. If it IS (~/.mozilla is a prefix of
~/.mozilla/firefox) then the code could tack "/<product>" onto
MOZ_USER_DIR. This is ugly even to explain but it would make
--with-user-appdir work while not changing what happens when you don't
specify it. I'm also probably missing some of the complexities
involved in alternate packaging of mozilla for other reasons, where
the vendor and product strings could be different. People do a lot of
crazy things with repackaging mozilla, I honestly had no idea before I
started digging into this port how much you could do.
In the end I just produced a patch that has the effect I wanted, but
this is not the desired end state. I'm really interested in other
opinions.
>
> If I understood the purpose of patches in www/tbb/tor-browser (if I
> don't consider patches copied from www/firefox-esr or security/nss), it
> is to remove specific code in TBB to use ~/.tor-browser instead of the
> TBB default to TorBrowser/Data/Browser. But when doing that you also
> remove specific code (like anti-fingerprinting or keeping all data under
> one well-know directory).
Yes, this is becoming clear.
> I wonder if it wouldn't be more safe (and facilitating porting on
> long-term) to try to not diverge to much from TBB upstream.
If I understand you correctly, you are proposing that instead of
reverting their bundle-specific code we should patch their additions
to do *almost* what they do, but assume things live in $HOME instead
of relative to somewhere else. There will still have to be some
patches because they find the place where TorBrowser lives based on
the location of the Tor browser binary, but I'm guessing they will be
more minimal than my current patches. I will investigate how this
would look.
I guess in the end I'm too much of an old-school unix.head and this
skews my view of things. I find programs (like mozilla, in fact) that
are so free and easy about mkdir in my home directory (~/Downloads?
~/Desktop?) to be obnoxious. In my brain running a command named foo
means it will be looking for a config in ~/.foorc or, if it's a real
beast, in ~/.foo/. This is a personal bias that probably shouldn't be
sneaking into the port like this, which is why this kind of review is
so good.
> - www/tbb/torbutton/patches/patch-src_defaults_preferences_preferences_js
>
> you change log settings to stdout with less level of verbose. if TBB
> is launched via desktop menu, there is no console to see the potential
> errors. and why less level of verbosity ?
Lower levels are more verbose: I cranked the verbosity up and directed
the spew to stdout for my own debugging in both torbutton and
tor-launcher and should've ditched that before I posted. Will fix.
> you disable version checking. shouldn't be still enable ? I
> understand to no try to update, but if a new version of TBB is
> available, having the information could be valuable: else you could
> put users at risk if they use an old version (with potentially know
> vulnerabilities) due to a no up-to-date port under OpenBSD.
This is a good point. I'm not sure that it actually works, I have
never tried to create a situation where the version check happened to
see how I am notified. I'll do this.
> - www/tbb/tor-launcher/patches/patch-src_defaults_preferences_prefs_js
>
> same question than for torbutton about log level and log method
> (reducing the verbosity and change output to stdout).
>
> - www/tbb/tor-launcher/patches/patch-src_components_tl-process_js
>
> "Let geoip/geoip6 file paths be set by prefs like everything else. Go
> back to old way of munging relative paths, their new way is
> effectively a no-op for us anyway."
>
> Do you know why tor devs doesn't use prefs for these settings ? Why do
> you think diverging from upstream (and maintains the diff) would be
> good if their "new way" is a no-op under OpenBSD ?
>
I *think* the devs did what they did because they ship geoip and
geoip6 themselves and assumed they would only be there, but we already
have them installed with net/tor itself (under /usr/local/share/tor).
I thought that using those files made more sense in our context, since
they'll always be on the system if you install TB via the R-dep on
net/tor.
I just checked and the geoip and geoip6 files that ship with TB 6.0.2
for Linux are identical to the ones we have in /usr/local/share/tor at
least as of today. I have not checked every time I've updated the
port, so this might be a reason to package geoip and geoip6 files with
TB itself that match the ones they ship.
The specific patch I aded was an attempt to avoid patching more of the
code and instead move the changes into prefs.js. Their code assumed
that the geoip and geoip6 data files would be in the same place that
their torrc-defaults file lived, which is wrong in our case anyway, so
I would've had to patch something somewhere. I was actually thinking
of proposing my patch (allow prefs.js to specify the location of all
tor-related config files) to upstream, since it is simpler and less
code: they could also just set those prefs and not have special code
in those two cases.
>
> And after all these elements, I would like to say that I would be glad
> to see support of TBB in OpenBSD :)
I am very happy to hear it. I will work on changing my patches to be
more minimalistic.
>
> Thanks.
Pax, -A
--
http://haqistan.net/~attila | [email protected] | 0x62A729CF
