#16269 closed defect (fixed)

add-on compatibility check occurs repeatedly

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

Description

This is a spinoff of ticket #16014. Georg noticed that after he updated Tor Browser 4.5a5 to 5.0a1, he saw a "Checking Compatibility of Add-ons" window each time he started the browser. Kathy and I debugged this and found that this window is coming from the meek helper browser. It shows up repeatedly because the prefs.js file is not being written to the profile (presumably because the meek browser is killed and does not exit in a clean manner).

One way to fix this is to add code to the meek HTTP helper extension that flushes the browser prefs. to disk before it enters the blocking event loop. There may be a better solution, but this seems to solve the problem.

Child Tickets

Attachments (1)

0001-Bug-16269-add-on-compatibility-check-occurs-repeated.patch (1.4 KB) - added by mcs 22 months ago.
a revised fix

Download all attachments as: .zip

Change History (7)

comment:1 in reply to: ↑ description ; follow-up: Changed 22 months ago by dcf

Replying to mcs:

One way to fix this is to add code to the meek HTTP helper extension that flushes the browser prefs. to disk before it enters the blocking event loop. There may be a better solution, but this seems to solve the problem.

Does attachment:0001-Bug-16269-add-on-compatibility-check-occurs-repeated.patch still work if savePrefFile(null) comes before, not after, setBoolPref("network.proxy.socks_remote_dns", false)? We actually don't want to save network.proxy.socks_remote_dns=false in prefs.js; it's meant to be a change in memory only.

The reason we want network.proxy.socks_remote_dns=true in prefs.js, even though we always override that setting immediately, is to prevent DNS leaks in case something goes wrong and someone manages to run the meek headless browser, thinking it's a Tor Browser. See:

Last edited 22 months ago by dcf (previous) (diff)

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

Replying to dcf:

Does attachment:0001-Bug-16269-add-on-compatibility-check-occurs-repeated.patch still work if savePrefFile(null) comes before, not after, setBoolPref("network.proxy.socks_remote_dns", false)?

It should work.

We actually don't want to save network.proxy.socks_remote_dns=false in prefs.js; it's meant to be a change in memory only.

Hmmm. I wish there was a way to do this in a less fragile way. Would it be acceptable to use a default preferences file inside the meek HTTP extension to set network.proxy.socks_remote_dns = false?

(I tried and failed to get that to work just now, although it should be possible)

comment:3 in reply to: ↑ 2 Changed 22 months ago by dcf

Replying to mcs:

Replying to dcf:

We actually don't want to save network.proxy.socks_remote_dns=false in prefs.js; it's meant to be a change in memory only.

Hmmm. I wish there was a way to do this in a less fragile way. Would it be acceptable to use a default preferences file inside the meek HTTP extension...

That's exactly how it works now. The helper browser profile has its own prefs that are separate from the normal Tor Browser prefs. (It might inherit Tor Browser settings for anything that's unset, I'm not sure, but it overrides everything that matters for this ticket.)

But I want the default in the helper's prefs to be network.proxy.socks_remote_dns=true, because that's a safe setting if something goes wrong (like the extension fails to load) and the headless browser unexpectedly appears on screen. If that happens, we at least want the browser to be non-functional (which it is, thanks to a default blackhole proxy setting) and not leak DNS (which it does not, thanks to network.proxy.socks_remote_dns=true). The main idea is that only the extension should be able to disable the safe default and make local DNS requests, because the extension knows what it's doing.

We used to have network.proxy.socks_remote_dns=false, an unsafe fallback, but changed it in #12674. (I meant to link to #12674 in comment:1 but messed it up.) The way it works now is intended to make it so that if something breaks, it breaks in a safe way.

https://gitweb.torproject.org/builders/tor-browser-bundle.git/tree/Bundle-Data/PTConfigs/meek-http-helper-user.js?id=0119a3c15711a66c76496d6e8e55511782140ec1#n21

In sum, we want to save the updated prefs after an upgrade (to solve this ticket), but we don't want to save network.proxy.socks_remote_dns=false.

comment:4 follow-up: Changed 22 months ago by mcs

I understood what you were trying to do; I was just trying to find a more robust way to accomplish it. I did not know about the use of a user.js file. I think that will actually cause the network.proxy.socks_remote_dns pref. to be reset to true each time the browser starts up.

In any case, Kathy and I tested a revised patch that flushes the prefs. to disk before the code in components/main.js resets network.proxy.socks_remote_dns. It seems to work fine and solve this issue. I will attach a patch for review.

comment:5 Changed 22 months ago by mcs

  • Status changed from new to needs_review

comment:6 in reply to: ↑ 4 Changed 22 months ago by dcf

  • Resolution set to fixed
  • Status changed from needs_review to closed

Replying to mcs:

I understood what you were trying to do; I was just trying to find a more robust way to accomplish it. I did not know about the use of a user.js file. I think that will actually cause the network.proxy.socks_remote_dns pref. to be reset to true each time the browser starts up.

Oh, I think you are right. The user.js file should take precedence even if we accidentally write socks_remote_dns=false to prefs.js. That's fine, it's just another layer in the failsafe.

In any case, Kathy and I tested a revised patch that flushes the prefs. to disk before the code in components/main.js resets network.proxy.socks_remote_dns. It seems to work fine and solve this issue. I will attach a patch for review.

Thanks! I applied it and tagged 0.19 for you.

For the record, this is what was added to my profile.meek-http-helper/prefs.js after I applied the patch. Before that, it was empty except for comments.

user_pref("app.update.enabled", false);
user_pref("browser.cache.frecency_experiment", 1);
user_pref("browser.dom.window.dump.enabled", true);
user_pref("datareporting.healthreport.nextDataSubmissionTime", "1433523627528");
user_pref("datareporting.policy.firstRunTime", "1433437227527");
user_pref("extensions.blocklist.pingCountVersion", 0);
user_pref("extensions.enabledAddons", "meek-http-helper%40bamsoftware.com:1.0");
user_pref("extensions.installCache", "[{\"name\":\"app-profile\",\"addons\":{\"meek-http-helper@bamsoftware.com\":{\"descriptor\":\"/home/david/tor-browser_en-US/Browser/TorBrowser/Data/Browser/profile.meek-http-helper/extensions/meek-http-helper@bamsoftware.com\",\"mtime\":1433437213000,\"rdfTime\":946713600000}}}]");
user_pref("extensions.lastAppVersion", "31.7.0");
user_pref("extensions.lastPlatformVersion", "31.7.0");
user_pref("extensions.shownSelectionUI", true);
user_pref("network.proxy.socks_port", 9);
user_pref("security.ssl.disable_session_identifiers", false);
user_pref("toolkit.startup.max_resumed_crashes", -1);
Note: See TracTickets for help on using tickets.