Opened 6 years ago

Closed 5 years ago

#9308 closed defect (fixed)

JavaScript's BrowserFeedWriter() and other exceptions leak installation paths on OS X and Windows

Reported by: cypherpunks Owned by: mikeperry
Priority: Very High Milestone:
Component: Firefox Patch Issues Version:
Severity: Keywords: tbb-fingerprinting, tbb-easy, interview, GeorgKoppen201404R, MikePerry201404R
Cc: arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In #5922 it was claimed that the vulnerability uncovered at Defcon 17 by Gregory Fleischer (http://pseudo-flaw.net/tor/torbutton/browserfeedwriter-error.html) doesn't affect TBB on OS X. I have just replicated this bug on 2.3.25-10.

When the TBB is installed in a user's homedir, calling (new BrowserFeedWriter()).close() will leak their username in a JS exception.

Child Tickets

Attachments (5)

0001-Prevent-new-BrowserFeedWriter-.close-exception-from-.patch (1.6 KB) - added by arthuredelstein 6 years ago.
0001-prevent-BrowserFeedWriter-and-sidebar-exceptions-fro.patch (4.4 KB) - added by arthuredelstein 6 years ago.
0001-Emulation-of-https-bugzilla.mozilla.org-show_bug.cgi.patch (1.9 KB) - added by arthuredelstein 6 years ago.
0001-fix-9308-don-t-leak-user-install-path-of-TBB.patch (6.2 KB) - added by arthuredelstein 5 years ago.
0001-fixup-fix-9308-and-11433-don-t-leak-user-install-pat.patch (2.1 KB) - added by mikeperry 5 years ago.
Fix memory leak and add some safety checks.

Download all attachments as: .zip

Change History (44)

comment:1 Changed 6 years ago by mikeperry

Keywords: tbb-easy interview added
Parent ID: #5922

Probably somewhere between easy and an interview question in terms of difficulty.

comment:2 Changed 6 years ago by arthuredelstein

Status: newneeds_review

This patch makes sure that the "precompile startup cache" step runs during "make package". This step is required to avoid leaking the browser install location reported in this bug. The step was previously prevented by the line export CROSS_COMPILE=1 in .mozconfig-mac, but this patched version ignores the flag and calls precompile_cache() anyway.

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

comment:3 Changed 6 years ago by gk

Keywords: GeorgKoppen20143R added
Version: Tor: 0.2.3.25

Interesting... I'll look at and test it on Monday.

comment:4 Changed 6 years ago by gk

Keywords: GeorgKoppen201403R added; GeorgKoppen20143R removed

comment:5 Changed 6 years ago by arthuredelstein

Status: needs_reviewnew

Turns out my patch is no good. Please disregard it while I work on a fix.

comment:6 Changed 6 years ago by gk

Keywords: GeorgKoppen201403R removed
Summary: JavaScript's BrowserFeedWriter() leaks installation paths on OS XJavaScript's BrowserFeedWriter() leaks installation paths on OS X and Windows

Btw. this happens on Windows, too. Might be of interest for the fix (and for testing it).

comment:7 Changed 6 years ago by arthuredelstein

Status: newneeds_review

Thanks for the Windows tip!

I've attached a new patch, which simply blocks local file paths from being attached to exceptions. This fixes both privacy leaks found in
new BrowserFeedWriter().close();
and
window.sidebar.addSearchEngine("http://", "http://", null, null);

Note: I corrected a mistake in that last line.

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

comment:8 Changed 6 years ago by gk

Keywords: GeorgKoppen201404R added

comment:9 Changed 6 years ago by gk

Two questions:

1) What was wrong with the former approach?
2) Applying your patch to tor-browser, tagging it and running that through our gitian machinery produces at least Mac OS X bundles (or better: produces at least one Mac OS X bundle, en-US) that still has this issue (Windows builds have not finished yet). Any idea how this could happen? I used the URL in the description of this ticket for testing.

comment:10 Changed 6 years ago by gk

It's the same for the Windows builds.

comment:11 Changed 6 years ago by arthuredelstein

  1. In the former patch, I was trying to turn on precompilation of the startup cache, a step in make package. When the startup cache is precompiled, it masks file:// URLs and provides resource:// URLs instead. Unfortunately, I discovered that precompilation simply doesn't work when the build is cross compiled, because in order to cross compile, we need a native version of xpcshell. In order to get this native version, we would need to run a separate native linux build of mozilla-central. This seemed awkward and unnecessarily slow.
  1. I don't know why it isn't working on the gitian build. What is the text you see on the test website? I will try to reproduce that here, but as I am traveling today it may not get a chance until tomorrow.

comment:12 Changed 6 years ago by arthuredelstein

Cc: arthuredelstein added

comment:13 in reply to:  11 Changed 6 years ago by gk

Replying to arthuredelstein:

  1. I don't know why it isn't working on the gitian build. What is the text you see on the test website?
Date: Wed Apr  2 17:57:53 2014
found location: "jar:file:///Users/release/TorBrowserBundle_en-US.app/Contents/MacOS/TorBrowser.app/Contents/MacOS/browser/omni.ja!/components/FeedWriter.js"

An other thing I was wondering while looking at your patch: What happens in case of an extension producing execptions? Say, like Torbutton and TorLauncher? It seems they would be affected by your changes as well, wouldn't they? If so, that may not be desired as a) extensions are not the problem here and b) it might make it hard to debug stuff without knowing where the error occurs.

comment:14 in reply to:  11 ; Changed 6 years ago by gk

Replying to arthuredelstein:

  1. In the former patch, I was trying to turn on precompilation of the startup cache, a step in make package. When the startup cache is precompiled, it masks file:// URLs and provides resource:// URLs instead. Unfortunately, I discovered that precompilation simply doesn't work when the build is cross compiled, because in order to cross compile, we need a native version of xpcshell. In order to get this native version, we would need to run a separate native linux build of mozilla-central. This seemed awkward and unnecessarily slow.

Why mozilla-central? So, if we are building a new release we compile linux builds anyway. Wouldn't it be much easier if we would save xpcshell from there and use that one for precompilation of the startup cache? I we could get that to work I'd be very happy as we like to get all patches somehow merged into mozilla-central and I fear it would be really hard to convince Mozilla to take your latest patch given a) that they don't have the problem and b) that the issue is not really providing that information in the exception but rather that we miss a sanitizing step due to our build setup.

comment:15 in reply to:  14 Changed 6 years ago by arthuredelstein

Replying to gk:

Replying to arthuredelstein:

  1. In the former patch, I was trying to turn on precompilation of the startup cache, a step in make package. When the startup cache is precompiled, it masks file:// URLs and provides resource:// URLs instead. Unfortunately, I discovered that precompilation simply doesn't work when the build is cross compiled, because in order to cross compile, we need a native version of xpcshell. In order to get this native version, we would need to run a separate native linux build of mozilla-central. This seemed awkward and unnecessarily slow.

Why mozilla-central?

Sorry, I should have said tor-browser.git

So, if we are building a new release we compile linux builds anyway. Wouldn't it be much easier if we would save xpcshell from there and use that one for precompilation of the startup cache? I we could get that to work I'd be very happy as we like to get all patches somehow merged into mozilla-central and I fear it would be really hard to convince Mozilla to take your latest patch given a) that they don't have the problem and b) that the issue is not really providing that information in the exception but rather that we miss a sanitizing step due to our build setup.

I think you're absolutely right. I'm going to try to get the native xpcshell approach to work. Thanks for pointing me in the right direction.

comment:16 Changed 6 years ago by gk

FWIW: the Mozilla folks are thinking about cross-compiling all their Mac builds on Linux machines (see: https://bugzilla.mozilla.org/show_bug.cgi?id=927061). So, they might face the same problem. Hence, I opened a bug (https://bugzilla.mozilla.org/show_bug.cgi?id=991522) and try to get some feedback on a solution they would accept later today. We'll see how it goes.

comment:17 Changed 6 years ago by gk

Okay, interesting Mozilla folks are doubting that this is a cross-compile issue (see my filed bug). So, you might want to chime in there and explain why they are wrong. :) That said, the BrowserFeedWriter thing will be fixed in ESR 31 (https://bugzilla.mozilla.org/show_bug.cgi?id=983845). Not sure about the other test you made. I did not get it to run.

comment:18 in reply to:  17 ; Changed 6 years ago by arthuredelstein

Replying to gk:

Okay, interesting Mozilla folks are doubting that this is a cross-compile issue (see my filed bug). So, you might want to chime in there and explain why they are wrong. :)

:) I've written a comment: https://bugzilla.mozilla.org/show_bug.cgi?id=991522#c5

That said, the BrowserFeedWriter thing will be fixed in ESR 31 (https://bugzilla.mozilla.org/show_bug.cgi?id=983845). Not sure about the other test you made. I did not get it to run.

Sorry about that, I mistyped the other example in the comment. It's fixed now. Try entering
window.sidebar.addSearchEngine("http://", "http://", null, null);
in the web console of Tor Browser on Mac or Windows.

I can imagine there are other exceptions that may cause privacy leaks in the same way, when the startup cache is not precompiled. I think it could be quite challenging to find every privacy leak of this type. So it's probably best for TBB to have the precompilation step to work on the cross-compiled targets. I'm going to try to take the approach you suggested, of borrowing the native xpcshell from the linux build and providing it to the Mac and Windows builds.

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

comment:19 in reply to:  18 ; Changed 6 years ago by gk

Replying to arthuredelstein:

Replying to gk:

Okay, interesting Mozilla folks are doubting that this is a cross-compile issue (see my filed bug). So, you might want to chime in there and explain why they are wrong. :)

:) I've written a comment: https://bugzilla.mozilla.org/show_bug.cgi?id=991522#c5

That said, the BrowserFeedWriter thing will be fixed in ESR 31 (https://bugzilla.mozilla.org/show_bug.cgi?id=983845). Not sure about the other test you made. I did not get it to run.

Sorry about that, I mistyped the other example in the comment. It's fixed now. Try entering
window.sidebar.addSearchEngine("http://", "http://", null, null);
in the web console of Tor Browser on Mac or Windows.

That is fixed on mozilla-central as well, yes!! I've not found the corresponding bug though, yet. So, strictly speaking we could think about closing this bug now. :) And maybe look at backporting those patches. Not sure how difficult this is going to be. Maybe they've fixed those leaks in a more generic way?

I can imagine there are other exceptions that may cause privacy leaks in the same way, when the startup cache is not precompiled. I think it could be quite challenging to find every privacy leak of this type. So it's probably best for TBB to have the precompilation step to work on the cross-compiled targets. I'm going to try to take the approach you suggested, of borrowing the native xpcshell from the linux build and providing it to the Mac and Windows builds.

The major drawback of this approach is that users can't build Mac or Windows bundles anymore without compiling the Tor Browser for Linux as well (and, alas, they'd need both the 32bit and 64bit one)...

comment:20 in reply to:  19 ; Changed 6 years ago by arthuredelstein

Replying to gk:

Replying to arthuredelstein:

Replying to gk:
Sorry about that, I mistyped the other example in the comment. It's fixed now. Try entering
window.sidebar.addSearchEngine("http://", "http://", null, null);
in the web console of Tor Browser on Mac or Windows.

That is fixed on mozilla-central as well, yes!! I've not found the corresponding bug though, yet. So, strictly speaking we could think about closing this bug now. :) And maybe look at backporting those patches. Not sure how difficult this is going to be. Maybe they've fixed those leaks in a more generic way?

How do you know it's fixed? Did you check on a CROSS_COMPILEd version?

I can imagine there are other exceptions that may cause privacy leaks in the same way, when the startup cache is not precompiled. I think it could be quite challenging to find every privacy leak of this type. So it's probably best for TBB to have the precompilation step to work on the cross-compiled targets. I'm going to try to take the approach you suggested, of borrowing the native xpcshell from the linux build and providing it to the Mac and Windows builds.

The major drawback of this approach is that users can't build Mac or Windows bundles anymore without compiling the Tor Browser for Linux as well (and, alas, they'd need both the 32bit and 64bit one)...

That's the reason why I was trying to get the second patch to work. But then you persuaded me I should get the precompile step to work. ;)

comment:21 in reply to:  20 Changed 6 years ago by gk

Replying to arthuredelstein:

Replying to gk:

Replying to arthuredelstein:

Replying to gk:
Sorry about that, I mistyped the other example in the comment. It's fixed now. Try entering
window.sidebar.addSearchEngine("http://", "http://", null, null);
in the web console of Tor Browser on Mac or Windows.

That is fixed on mozilla-central as well, yes!! I've not found the corresponding bug though, yet. So, strictly speaking we could think about closing this bug now. :) And maybe look at backporting those patches. Not sure how difficult this is going to be. Maybe they've fixed those leaks in a more generic way?

How do you know it's fixed? Did you check on a CROSS_COMPILEd version?

So, for the record: Aurora (30.0a2) gives me the following output:

[Exception... "addEngine: Error adding engine:
[Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newChannelFromURI]"  nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)"  location: "JS frame :: resource://gre/components/nsSearchService.js :: SRCH_ENG_initFromURIAndLoad :: line 1255"  data: no]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/components/nsSearchService.js :: FAIL :: line 266"  data: no]

Using the latest Nightly (31.0a1) I get:

[Exception... "Unexpected error"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: debugger eval code :: <TOP_LEVEL> :: line 1"  data: no]

Even though I did not test that with cross-compiled code I think it is safe to assume that this specific problem does not pop up there given that no path at all is leaked in Nightlies anymore.

comment:22 Changed 6 years ago by arthuredelstein

I've confirmed the two Mozilla bugs involved in fixing the BrowserFeedWriter and sidebar privacy leaks are:

  1. "Stop exposing BrowserFeedWriter to the Web", https://bugzilla.mozilla.org/show_bug.cgi?id=983845 (as mentioned by gk, above)

patch: https://hg.mozilla.org/mozilla-central/rev/e9ea1662020a

and

  1. "Port window.sidebar and window.external to WebIDL", https://bugzilla.mozilla.org/show_bug.cgi?id=983920

patch: https://hg.mozilla.org/mozilla-central/rev/d9e6a6c40a57

I tested Firefox nightlies immediately before and after each of these patches and confirmed that the each leak is stopped by the respective patch.

It's interesting to note that these two Mozilla bugs are part of a larger project, "Hunt down all the things that implement DOM_OBJECT classinfo in JS", https://bugzilla.mozilla.org/show_bug.cgi?id=981845 .

My next step is to try backporting these patches to tor-browser.git.

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

comment:23 Changed 6 years ago by arthuredelstein

I've added a new patch that fixes the original vulnerability reported in this ticket (BrowserFeedWriter). Because the patch, ​https://hg.mozilla.org/mozilla-central/rev/e9ea1662020a, requires a number of previous patches, a full backport would be rather complex. But we can get a workable fix simply by imitating the patch's removal of a single line. Deleting this line excises the BrowserFeedWriter constructor from the global JavaScript "window" API. Without the BrowserFeedWriter constructor, the privacy-leaking JS exception is no longer triggerable.

I have opened a separate ticket, #11433, reporting the sidebar bug. Unfortunately the sidebar bug requires a more complex backport. I have a second, unrelated bug I need to work on, so I'll postpone fixing the sidebar issue until after that.

comment:24 in reply to:  23 Changed 6 years ago by gk

Replying to arthuredelstein:

I've added a new patch that fixes the original vulnerability reported in this ticket (BrowserFeedWriter). Because the patch, ​https://hg.mozilla.org/mozilla-central/rev/e9ea1662020a, requires a number of previous patches, a full backport would be rather complex. But we can get a workable fix simply by imitating the patch's removal of a single line. Deleting this line excises the BrowserFeedWriter constructor from the global JavaScript "window" API. Without the BrowserFeedWriter constructor, the privacy-leaking JS exception is no longer triggerable.

While this is a smart move it breaks subscribing to feeds via the browser. Trying to subscribe to one a user gets

ReferenceError: BrowserFeedWriter is not defined

in her error console and a broken website.

comment:25 Changed 5 years ago by arthuredelstein

I've investigated the precompile startup cache mechanism a little more. I ran two builds, one with precompile_cache.js turned off, and one with it turned on.

The zip file TorBrowser.app/Contents/MacOS/browser/omni.ja contains two extra directories, jsloader and jssubloader that are present only if precompile_cache.js has been run. Inside the directory jsloader/resource/app/components/' are a number of .js files, which are (contrary to convention) binary files. These binary .js files are apparently compiled versions of JavaScript sources with the same name. One such compiled file is omni.ja!jsloader/resource/app/components/FeedWriter.js`.

If these compiled .js files are removed from omni.ja, then TorBrowser.app reverts to leaking the file location of an uncompiled version of FeedWriter.js as an absolute file:// URI. Conversely, if we don't precompile the cache, but add a compiled version of FeedWriter.js to the same location in omni.ja, then TorBrowser.app displays the resource:// URI that does not leak the install path.

So one way to stop the leakage would be to take compiled versions of FeedWriter.js and similar files, and supplement the omni.ja file. Unfortunately, we need xpcshell to produce these compiled versions, and a linux-native xpcshell is not available in a cross-compiled version of the tor-browser.git. We could borrow the compiled .js files from the linux build, but then building the Mac or Windows build would require anyone building to run a full linux build, essentially doubling the build time. I don't see any other way to produce the compiled versions of FeedWriter.js, however.

An alternative would be to try to re-write the file:// URIs to resource:// URIs in the thrown exceptions. A class, nsResProtocolHandler, stores a mapping from resource:// URIs to file:// URIs, so we might be able to add some code that reverses this mapping so that we can do the re-writing. Such a patch is perhaps not too likely to be adopted by Mozilla. On the other hand, since the two known bugs (BrowserFeedWriter and sidebar.addSearchEngine) are not present in ESR31, we could simply create a temporary patch that can be discarded when rebasing of TBB to ESR31 happens.

Any advice on which alternative to choose? Thanks!

comment:26 in reply to:  25 ; Changed 5 years ago by gk

Replying to arthuredelstein:

I've investigated the precompile startup cache mechanism a little more. I ran two builds, one with precompile_cache.js turned off, and one with it turned on.

The zip file TorBrowser.app/Contents/MacOS/browser/omni.ja contains two extra directories, jsloader and jssubloader that are present only if precompile_cache.js has been run. Inside the directory jsloader/resource/app/components/' are a number of .js files, which are (contrary to convention) binary files. These binary .js files are apparently compiled versions of JavaScript sources with the same name. One such compiled file is omni.ja!jsloader/resource/app/components/FeedWriter.js`.

If these compiled .js files are removed from omni.ja, then TorBrowser.app reverts to leaking the file location of an uncompiled version of FeedWriter.js as an absolute file:// URI. Conversely, if we don't precompile the cache, but add a compiled version of FeedWriter.js to the same location in omni.ja, then TorBrowser.app displays the resource:// URI that does not leak the install path.

And what happens if we don't precompile the cache but add an *uncompiled* version to resource/app/components? If the Tor Browser is not complaining then we probably have the easiest solution to our problem.

So one way to stop the leakage would be to take compiled versions of FeedWriter.js and similar files, and supplement the omni.ja file. Unfortunately, we need xpcshell to produce these compiled versions, and a linux-native xpcshell is not available in a cross-compiled version of the tor-browser.git. We could borrow the compiled .js files from the linux build, but then building the Mac or Windows build would require anyone building to run a full linux build, essentially doubling the build time. I don't see any other way to produce the compiled versions of FeedWriter.js, however.

What about Mike's idea of doing a python stub that is doing the pre-compilation? And what about compiling those .js files once and ship them as resources via the gitian versions files?

An alternative would be to try to re-write the file:// URIs to resource:// URIs in the thrown exceptions. A class, nsResProtocolHandler, stores a mapping from resource:// URIs to file:// URIs, so we might be able to add some code that reverses this mapping so that we can do the re-writing. Such a patch is perhaps not too likely to be adopted by Mozilla. On the other hand, since the two known bugs (BrowserFeedWriter and sidebar.addSearchEngine) are not present in ESR31, we could simply create a temporary patch that can be discarded when rebasing of TBB to ESR31 happens.

Sounds complicated but if it works why not (I have not looked at the code to estimate how large that patch would have to be, so it might instead be quite easy to provide one). BTW: Are we sure there are no other components exposed to the website that could cause similar issues?

Any advice on which alternative to choose? Thanks!

Given that this is obsolete with ESR 31 choosing the fastest way to solve this ticket seems to be a good approach. :)

comment:27 Changed 5 years ago by gk

Keywords: GeorgKoppen201404R removed
Status: needs_reviewneeds_revision

comment:28 in reply to:  26 ; Changed 5 years ago by arthuredelstein

Replying to gk:

And what happens if we don't precompile the cache but add an *uncompiled* version to resource/app/components? If the Tor Browser is not complaining then we probably have the easiest solution to our problem.

I tried that. It doesn't complain, but it still leaks the install path. TB must not be recognizing an uncompiled version in that location as valid, and falling back on the uncompiled version stored in another location.

What about Mike's idea of doing a python stub that is doing the pre-compilation?

Maybe I'm not understanding this suggestion. I don't see how python can compile JavaScript, unless we write our own JS compiler ;). I'm not an expert on this startup cache, but from my googlings, it seems these files are SpiderMonkey-compatible bytecode.

And what about compiling those .js files once and ship them as resources via the gitian versions files?

Yes, I think that absolutely can work, but don't we then need a way to verify the integrity of those binary files on the build machine? Is there currently a mechanism for doing that? Would checking the binaries into tor-browser.git or tor-brower-bundle.git and relying on git's hashing strategy be acceptable?

An alternative would be to try to re-write the file:// URIs to resource:// URIs in the thrown exceptions. A class, nsResProtocolHandler, stores a mapping from resource:// URIs to file:// URIs, so we might be able to add some code that reverses this mapping so that we can do the re-writing. Such a patch is perhaps not too likely to be adopted by Mozilla. On the other hand, since the two known bugs (BrowserFeedWriter and sidebar.addSearchEngine) are not present in ESR31, we could simply create a temporary patch that can be discarded when rebasing of TBB to ESR31 happens.

Sounds complicated but if it works why not (I have not looked at the code to estimate how large that patch would have to be, so it might instead be quite easy to provide one). BTW: Are we sure there are no other components exposed to the website that could cause similar issues?

This approach would cover exceptions thrown through a similar code path, but not necessarily other unknown path leaks. I'm not sure how tricky it is yet...

Given that this is obsolete with ESR 31 choosing the fastest way to solve this ticket seems to be a good approach. :)

I agree wholeheartedly! :P

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

Replying to arthuredelstein:

Replying to gk:

What about Mike's idea of doing a python stub that is doing the pre-compilation?

Maybe I'm not understanding this suggestion. I don't see how python can compile JavaScript, unless we write our own JS compiler ;). I'm not an expert on this startup cache, but from my googlings, it seems these files are SpiderMonkey-compatible bytecode.

Yes. Frankly, I am not sure what the xpcshell binary is doing under the hood. My guess (and maybe Mike's) was that it is "just" calling some other code that does the hard work (i.e. get the js into bytecode). And maybe replacing the xpcshell binary with some Python code that is just calling the other code as well might have worked.

And what about compiling those .js files once and ship them as resources via the gitian versions files?

Yes, I think that absolutely can work, but don't we then need a way to verify the integrity of those binary files on the build machine? Is there currently a mechanism for doing that?

Yes, having > 1 people build the code on different machines (as we already do now) and make sure the sha256sums match.

Would checking the binaries into tor-browser.git or tor-brower-bundle.git and relying on git's hashing strategy be acceptable?

Well, I think we would put it on a mirror and download it when fetching the build prerequisites (there are mirror URLs in the fetch-inputs.sh we already use). Then we could fiddle with the bundle descriptors and put them manually into the omni.ja files.

comment:30 in reply to:  29 ; Changed 5 years ago by arthuredelstein

Replying to gk:

Replying to arthuredelstein:

Replying to gk:

What about Mike's idea of doing a python stub that is doing the pre-compilation?

Maybe I'm not understanding this suggestion. I don't see how python can compile JavaScript, unless we write our own JS compiler ;). I'm not an expert on this startup cache, but from my googlings, it seems these files are SpiderMonkey-compatible bytecode.

Yes. Frankly, I am not sure what the xpcshell binary is doing under the hood. My guess (and maybe Mike's) was that it is "just" calling some other code that does the hard work (i.e. get the js into bytecode). And maybe replacing the xpcshell binary with some Python code that is just calling the other code as well might have worked.

My experiments showed that the xpcshell is using various C++ libraries
(such as, presumable, the JS compiler and runtime) that are also used
by Firefox. The C++ source for this code is part of the Mozilla build.
So that's why it would be necessary to build tor-browser.git natively
in order to compile the JavaScript: not just to build the xpcshell
binary, but also to build all the needed libraries.

And what about compiling those .js files once and ship them as resources via the gitian versions files?

Yes, I think that absolutely can work, but don't we then need a way to verify the integrity of those binary files on the build machine? Is there currently a mechanism for doing that?

Yes, having > 1 people build the code on different machines (as we already do now) and make sure the sha256sums match.

Would checking the binaries into tor-browser.git or tor-brower-bundle.git and relying on git's hashing strategy be acceptable?

Well, I think we would put it on a mirror and download it when fetching the build prerequisites (there are mirror URLs in the fetch-inputs.sh we already use). Then we could fiddle with the bundle descriptors and put them manually into the omni.ja files.

That will definitely work. A mirror alone is dangerous, though, because anyone building TBB would need to trust the binary files. So for the integrity check, it seems we need
two paths for getting the binary .js files -- one locally, where the
files are borrowed from the linux build, and one from a mirror, for
the case when we don't have time to run the linux build. Then the
resulting TB builds can be compared to make sure nothing nefarious has
happened. Or can you see a simpler way to verify the binary files'
integrity?

comment:31 in reply to:  30 ; Changed 5 years ago by gk

Replying to arthuredelstein:

My experiments showed that the xpcshell is using various C++ libraries
(such as, presumable, the JS compiler and runtime) that are also used
by Firefox. The C++ source for this code is part of the Mozilla build.
So that's why it would be necessary to build tor-browser.git natively
in order to compile the JavaScript: not just to build the xpcshell
binary, but also to build all the needed libraries.

:(

Well, I think we would put it on a mirror and download it when fetching the build prerequisites (there are mirror URLs in the fetch-inputs.sh we already use). Then we could fiddle with the bundle descriptors and put them manually into the omni.ja files.

That will definitely work. A mirror alone is dangerous, though, because anyone building TBB would need to trust the binary files. So for the integrity check, it seems we need
two paths for getting the binary .js files -- one locally, where the
files are borrowed from the linux build, and one from a mirror, for
the case when we don't have time to run the linux build. Then the
resulting TB builds can be compared to make sure nothing nefarious has
happened. Or can you see a simpler way to verify the binary files'
integrity?

Well, what we are currently doing (e.g. when shipping the msvcr100.dll) is putting the resource in question on a mirror we control and adding its sha256 sum to the versions files. When there is a new release we tag it and sign it. That model gives good enough security for the stopgap we are talking about here IMO. A temporary patch for tor-browser.git would be cleaner, though, especially as we would not need to touch tor-browser-bundle related things and revert the changes later when ESR 31 lands. But as I said, dunno how easy that is...

comment:32 in reply to:  31 ; Changed 5 years ago by arthuredelstein

A temporary patch for tor-browser.git would be cleaner, though, especially as we would not need to touch tor-browser-bundle related things and revert the changes later when ESR 31 lands. But as I said, dunno how easy that is...

I don't know how easy it is yet, but I agree that the tor-browser.git change (converting file:// URIs to resource:// URIs) is cleaner for the reason you give, as well as the fact that we aren't having to distribute more binary files. I'm going to give it a try.

One other, slightly less ideal fix that might be good enough, and is certainly fairly easy to carry out, would be to redact the path of a JS file generating an error, and simply show its short name. So, instead of reporting
"jar:file:///Applications/TorBrowserBundle_en-US.app/Contents/MacOS/TorBrowser.app/Contents/MacOS/browser/omni.ja!/components/FeedWriter.js"
or
"resource://app/components/FeedWriter.js"
TBB would simply report
"FeedWriter.js".

Obviously, that makes JS debugging little more difficult, but I think most of the time, the short name is sufficient to find whatever file is producing an exception.

I want to try the full file:// to resource:// conversion, but if that turns out to be too difficult, do you think this alternative "short-name" solution would be acceptable, at least as a stopgap until the ESR31 rebase?

comment:33 in reply to:  32 Changed 5 years ago by gk

Replying to arthuredelstein:

I want to try the full file:// to resource:// conversion, but if that turns out to be too difficult, do you think this alternative "short-name" solution would be acceptable, at least as a stopgap until the ESR31 rebase?

Yes, sounds good.

Changed 5 years ago by arthuredelstein

comment:34 Changed 5 years ago by arthuredelstein

OK, I've added a patch that does the file:// to resource:// conversion. I've tested this on Mac and it looks correct to me. This patch also fixes #11433.

comment:35 Changed 5 years ago by gk

Keywords: GeorgKoppen201404R added
Status: needs_revisionneeds_review

comment:36 Changed 5 years ago by gk

Keywords: MikePerry201404R added

Looks good to me. Tested with the upcoming 24.5.0 on Mac and Windows and it is working fine, thanks!
Mike: I think we can merge and deploy the patch.

comment:37 Changed 5 years ago by mikeperry

I found a memory leak in this patch. The rawFilename string gets allocated by aLocation->GetFilename() and is never freed. I also added some (likely redundant) additional checks to Omnijar::RebaseFilename() for safety. Fixup patch will be attached next.

Changed 5 years ago by mikeperry

Fix memory leak and add some safety checks.

comment:38 in reply to:  37 Changed 5 years ago by arthuredelstein

Replying to mikeperry:

I found a memory leak in this patch. The rawFilename string gets allocated by aLocation->GetFilename() and is never freed.

Damn. Good catch -- thanks.

comment:39 Changed 5 years ago by mikeperry

Resolution: fixed
Status: needs_reviewclosed
Summary: JavaScript's BrowserFeedWriter() leaks installation paths on OS X and WindowsJavaScript's BrowserFeedWriter() and other exceptions leak installation paths on OS X and Windows

Calling #11433 a dup of this, because the fix addresses both.

This fix will appear in TBB 3.6.

Note: See TracTickets for help on using tickets.