Opened 7 years ago

Last modified 4 weeks ago

#10394 needs_review task

Torbrowser's updater updates HTTPS-everywhere

Reported by: StrangeCharm Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-security, https-everywhere, TorBrowserTeam202006R
Cc: mcs, brade, Dbryrtfbcbhgf, tom, boklm, legind Actual Points:
Parent ID: Points:
Reviewer: gk Sponsor:

Description (last modified by gk)

Let's think about shipping HTTPS-Everywhere solely via our updater, disabling update pings for that extension as well.

Child Tickets

Attachments (1)

Bug-10394-Disable-HTTPS-Everywhere-addon-updates.patch (1.1 KB) - added by rustybird 6 weeks ago.

Download all attachments as: .zip

Change History (48)

comment:1 Changed 6 years ago by nickm

Component: - Select a componentTorBrowserButton

I think all this updater stuff is TBB. If not, please reassign.

comment:2 Changed 6 years ago by erinn

Component: TorBrowserButtonTor Browser
Keywords: tbb-torbutton added
Owner: changed from mikeperry to tbb-team

comment:3 Changed 4 years ago by bugzilla

Keywords: pantheon chronos tbb-torbutton removed
Resolution: worksforme
Severity: Normal
Status: newclosed

comment:4 Changed 4 years ago by gk

Resolution: worksforme
Status: closedreopened
Summary: Torbrowser's updater updates HTTPS-everwhereTorbrowser's updater updates HTTPS-everywhere

We are still getting HTTPS-E updates outside of our updater.

comment:5 in reply to:  4 Changed 4 years ago by mcs

Replying to gk:

We are still getting HTTPS-E updates outside of our updater.

When we ship a new version of HTTPS-E with a new release of Tor Browser, we arrange for it to be "force updated" (files replaced) so that the user is left with a known version of HTTPS-E which has been tested with TB. Interim updates are still retrieved from addons.mozilla.org using the extension update mechanism so users can get updates if desired. We use the same approach for NoScript.

Do we want to do something different? If not, then this bug can be closed.

comment:6 Changed 4 years ago by gk

Description: modified (diff)
Keywords: tbb-security added
Milestone: Chronos: phase two
Type: projecttask

I think we want to have a ticket about shipping HTTPS-E solely via our updater, disabling update pings to EFF. I thought there was already a ticket for this but I did not found one and thought this one might fit.

comment:7 Changed 4 years ago by bugzilla

mcs, thanks for the clarification. But,

Interim updates are still retrieved from addons.mozilla.org using the extension update mechanism

No. From EFF.

so users can get updates if desired.

What does it mean (desired)? Update Add-ons Automatically is selected by default.

We use the same approach for NoScript.

No. But, maybe, it's better to use the same, because recent updates led to 5.2.0 on alpha, 5.1.x on stable and 5.2.1 on AMO.

comment:8 in reply to:  7 ; Changed 4 years ago by mcs

Replying to bugzilla:

mcs, thanks for the clarification. But,

Interim updates are still retrieved from addons.mozilla.org using the extension update mechanism

No. From EFF.

Thanks. My mistake.

so users can get updates if desired.

What does it mean (desired)? Update Add-ons Automatically is selected by default.

It means users do have a way to disable updates if they want to do so. But most will keep the default setting.

We use the same approach for NoScript.

No. But, maybe, it's better to use the same, because recent updates led to 5.2.0 on alpha, 5.1.x on stable and 5.2.1 on AMO.

There is a policy question here: should we disable updates for bundled extensions. By allowing updates from EFF or AMO, we risk that users may get a version of an extension that is somehow incompatible with Tor Browser. But by allowing updates we ensure that users will have the latest (and hopefully most secure) versions of HTTPS-E and NoScript.

comment:9 in reply to:  6 Changed 4 years ago by bugzilla

Replying to gk:

we want to have a ticket about shipping HTTPS-E solely via our updater

Maybe, you mean your update servers instead of AMO, or you'll have to make a new release of TBB for every update.

comment:10 Changed 3 years ago by gk

Cc: Dbryrtfbcbhgf added

#21260 is a duplicate.

comment:11 Changed 3 years ago by yawning

Cc: yawning added

comment:12 in reply to:  11 Changed 3 years ago by Dbryrtfbcbhgf

Replying to yawning:
I made a video showing the updater I'm talking about, because I'm still not sure that I explained it properly.
http://sendvid.com/ito1papi

Version 1, edited 3 years ago by Dbryrtfbcbhgf (previous) (next) (diff)

comment:13 Changed 3 years ago by tom

Cc: tom added

comment:14 in reply to:  8 Changed 3 years ago by yawning

Replying to mcs:

There is a policy question here: should we disable updates for bundled extensions.

Yes.

By allowing updates from EFF or AMO, we risk that users may get a version of an extension that is somehow incompatible with Tor Browser.

#23258, #22974

comment:15 Changed 3 years ago by cypherpunks

+1, as #23772.

comment:16 Changed 3 years ago by gk

Keywords: TorBrowserTeam201711 added
Status: reopenedassigned

I heard we are close to be able to test that. Hopefully this can already happen in the next regular alpha release. Putting it on our radar for November.

comment:17 Changed 3 years ago by gk

Cc: boklm added
Keywords: TorBrowserTeam201711R added; TorBrowserTeam201711 removed
Status: assignedneeds_review

bug_10394_v2 (https://gitweb.torproject.org/user/gk/tor-browser-build.git/commit/?h=bug_10394_v2&id=50982eda6d3687aa5bcc2d088546f82c4fa7e53d) in my tor-browser-build repository has a fix for this bug. Note: this breaks at the time of writing the nightly builds. Given that we need to get alphas out rather soon, that's okay for now. The potential breakage will be addressed in #24179.

comment:18 Changed 3 years ago by boklm

Resolution: fixed
Status: needs_reviewclosed

This looks good to me. I applied it to master as commit 50982eda6d3687aa5bcc2d088546f82c4fa7e53d.

comment:19 Changed 3 years ago by gk

Cc: legind added
Keywords: TorBrowserTeam201711 added; TorBrowserTeam201711R removed
Resolution: fixed
Status: closedreopened

Firefox does not like our trick in a WebExtensions context:

1510514437300	addons.webextension.<unknown>	ERROR	Loading extension 'null': Reading manifest: Error processing applications.gecko.update_url: Error: Access denied for URL data:text/plain,
1510514437500	addons.xpi-utils	WARN	updateMetadata: Add-on https-everywhere-eff@eff.org is invalid: Error: Extension is invalid (resource://gre/modules/addons/XPIProvider.jsm:963:11) JS Stack trace: loadManifestFromWebManifest<@XPIProvider.jsm:963:11 < TaskImpl_run@Task.jsm:319:42 < Handler.prototype.process@Promise-backend.js:932:23 < this.PromiseWalker.walkerLoop@Promise-backend.js:813:7 < this.PromiseWalker.scheduleWalkerLoop/<@Promise-backend.js:747:11 < syncLoadManifestFromFile@XPIProvider.jsm:1621:5 < updateMetadata@XPIProviderUtils.js:1785:21 < processFileChanges@XPIProviderUtils.js:2009:26 < this.XPIProvider.checkForChanges@XPIProvider.jsm:3899:34 < this.XPIProvider.startup@XPIProvider.jsm:2839:25 < callProvider@AddonManager.jsm:242:12 < _startProvider@AddonManager.jsm:795:5 < AddonManagerInternal.startup@AddonManager.jsm:1005:9 < this.AddonManagerPrivate.startup@AddonManager.jsm:3062:5 < amManager.prototype.observe@addonManager.js:65:9

So, we need something better and need to rebuild the alphas.

comment:20 Changed 3 years ago by gk

Moving tickets to December 2017

comment:21 Changed 3 years ago by gk

Keywords: TorBrowserTeam201712 added; TorBrowserTeam201711 removed

Moving tickets to December 2017, for realz.

comment:22 Changed 2 years ago by gk

Keywords: TorBrowserTeam201801 added; TorBrowserTeam201712 removed

Moving tickets to 2018.

comment:23 Changed 2 years ago by gk

Keywords: TorBrowserTeam201802 added; TorBrowserTeam201801 removed

Moving tickets to Feb

comment:24 Changed 2 years ago by gk

Keywords: TorBrowserTeam201803 added; TorBrowserTeam201802 removed

Adding to our March plate.

comment:25 Changed 2 years ago by cypherpunks

Hainish says it should hopefully be ready for the 2018.3.27 release:

I plan on making this the release that incorporates the sign-rulesets branch.

https://github.com/EFForg/https-everywhere/issues/14907

comment:27 Changed 2 years ago by gk

Keywords: TorBrowserTeam201804 added; TorBrowserTeam201803 removed

Moving our tickets to April.

comment:28 Changed 2 years ago by gk

Keywords: TorBrowserTeam201805 added; TorBrowserTeam201804 removed

Moving remaining tickets to May.

comment:29 Changed 23 months ago by traumschule

Keywords: https-everywhere added

comment:30 Changed 23 months ago by traumschule

Today HTTPS Everywhere requested permission to update itself in TB 8.0a10
https://share.riseup.net/#Caex1xF8eY8YSUOksRdKkg

comment:31 Changed 23 months ago by cypherpunks3

Yeah that's #27277

comment:32 Changed 22 months ago by legind

gk, do you see any problem with simply setting the extension update URL to https://0.0.0.0/ rather than data:text/plain,? This doesn't result an the extension load-time error.

comment:33 Changed 22 months ago by gk

Hm. We had this before but ran into scary tor warnings, see: #16427 and #13129. We could check, though, whether they still occur. If not, great, let's do it. But if so, we should think harder about what to do.

comment:34 Changed 22 months ago by legind

I'm not seeing these warnings in the browser console when testing on the latest TB, but perhaps I'm missing something?

comment:35 in reply to:  34 Changed 22 months ago by gk

Replying to legind:

I'm not seeing these warnings in the browser console when testing on the latest TB, but perhaps I'm missing something?

Interesting. Do you get those messages if you use a Torbutton .xpi with the update URL changed to https://0.0.0.0/? One difference could be that this was only problematic for XPCOM extensions but is not an issue anymore for WebExtensions. If that's the case, great! Then let's switch to the HTTPS URL.

comment:36 Changed 22 months ago by legind

I don't see these warnings when modifying the Tor Button .xpi. Maybe they removed this as a warning at some point. To be clear, I'm just looking in the browser console - should I be looking elsewhere as well?

comment:37 in reply to:  36 Changed 22 months ago by gk

Replying to legind:

I don't see these warnings when modifying the Tor Button .xpi. Maybe they removed this as a warning at some point. To be clear, I'm just looking in the browser console - should I be looking elsewhere as well?

[09-12 07:23:47] Torbutton INFO: tor SOCKS: https://0.0.0.0/ via
                       --unknown--:6151cba48e49acf249f6b48fb13ce789
Sep 12 07:23:47.000 [warn] Rejecting SOCKS request for anonymous connection to private address [scrubbed].

is what I get in my terminal. I see the Tor warning in my browser console as well if I open about:addons, click on Extensions -> Torbutton (More button) -> right click -> Find Updates.

comment:38 Changed 22 months ago by legind

Starting the browser with

./start-tor-browser --verbose

I can now see

Sep 28 02:48:58.000 [warn] Rejecting SOCKS request for anonymous connection to private address [scrubbed].

But I don't see that first line.

I'm also seeing this when I update the manifest.json file in HTTPS Everywhere. Though for both, I see it only on the first time I click Check for Updates, not each subsequent time. Maybe this is due to addon update connection throttling?

Is the presence of these warnings really a blocker on moving forward here?

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

comment:39 Changed 6 months ago by gk

Status: reopenednew

comment:40 Changed 6 weeks ago by rustybird

Here's a small patch.

I tested it on top of TB 9.0.10 (rezipped omni.ja), with extensions.update.interval set to 60 seconds, by watching requests via SETEVENTS STREAM on a tor control port: The eff.org version check ping is gone. It's even more obvious if the NoScript ID is added to the patch as well, then there's no update traffic at all.

comment:41 Changed 6 weeks ago by rustybird

Status: newneeds_review

comment:42 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam202005R added; TorBrowserTeam201805 removed
Reviewer: gk

Nice, thanks!

comment:43 Changed 5 weeks ago by gk

Keywords: TorBrowserTeam202006R added; TorBrowserTeam202005R removed

Moving review tickets.

comment:44 in reply to:  40 ; Changed 4 weeks ago by gk

Status: needs_reviewneeds_information

Replying to rustybird:

Here's a small patch.

I tested it on top of TB 9.0.10 (rezipped omni.ja), with extensions.update.interval set to 60 seconds, by watching requests via SETEVENTS STREAM on a tor control port: The eff.org version check ping is gone. It's even more obvious if the NoScript ID is added to the patch as well, then there's no update traffic at all.

The permission path is an interesting idea. I had some hope we could get this ticket fixed without carrying yet another patch for it with us but I like the UX changes etc. we basically get for free with it. Plus no changes needed to the extension whatsoever and no weird console error messages either.

Maybe we could include this patch as part of our "don't block our unsigned extensions" patch where HTTPS-Everywhere is the only extension left anyway. Would be easy to make this to an "treat https-e special" patch.

rustybird: have you checked whether the ruleset updates are unaffected by your patch (because those are updates we want to keep getting)?

Last edited 4 weeks ago by gk (previous) (diff)

comment:45 in reply to:  44 ; Changed 4 weeks ago by rustybird

Replying to gk:

Maybe we could include this patch as part of our "don't block our unsigned extensions" patch where HTTPS-Everywhere is the only extension left anyway. Would be easy to make this to an "treat https-e special" patch.

If the plan still is to eventually disable NoScript updates too, then it might be simpler to keep the patch separate and later add a fixup checking for the NoScript ID as well. Just a thought.

rustybird: have you checked whether the ruleset updates are unaffected by your patch

Yes, they still work: There are connections to www.https-rulesets.org:443 and securedrop.org:443. And when I start with an old HTTPS Everywhere version that includes an outdated ruleset, the rulesets-timestamp fields in browser-extension-data/https-everywhere-eff@eff.org/storage.js show that those updates are applied.

comment:46 in reply to:  45 ; Changed 4 weeks ago by gk

Cc: yawning removed
Status: needs_informationneeds_review

Replying to rustybird:

Replying to gk:

Maybe we could include this patch as part of our "don't block our unsigned extensions" patch where HTTPS-Everywhere is the only extension left anyway. Would be easy to make this to an "treat https-e special" patch.

If the plan still is to eventually disable NoScript updates too, then it might be simpler to keep the patch separate and later add a fixup checking for the NoScript ID as well. Just a thought.

Yes, that's still the plan. I am not overly worried about NoScript having any impact here. Once we disable updates for NoScript we want to make a signature check exception for it, too, because we don't want to be affected again by Mozilla messing up their signing certificate renewal. So, this would fit into a single patch together with HTTPS-Everywhere being exempted and its updates disabled.

What I *am* worried about is the additional review cost this move would imply because I think we should neither disable HTTPS-Everywhere's nor NoScript's update mechanism if we can't manage to track their releases and check whether those contain any new security issues or fixes for older ones.

rustybird: have you checked whether the ruleset updates are unaffected by your patch

Yes, they still work: There are connections to www.https-rulesets.org:443 and securedrop.org:443. And when I start with an old HTTPS Everywhere version that includes an outdated ruleset, the rulesets-timestamp fields in browser-extension-data/https-everywhere-eff@eff.org/storage.js show that those updates are applied.

Great, thanks.

comment:47 in reply to:  46 Changed 4 weeks ago by rustybird

Replying to gk:

Once we disable updates for NoScript we want to make a signature check exception for it, too, because we don't want to be affected again by Mozilla messing up their signing certificate renewal. So, this would fit into a single patch together with HTTPS-Everywhere being exempted and its updates disabled.

Ah, makes sense. Squash away!

What I *am* worried about is the additional review cost this move would imply because I think we should neither disable HTTPS-Everywhere's nor NoScript's update mechanism if we can't manage to track their releases and check whether those contain any new security issues or fixes for older ones.

For new security issues, the status quo could be preserved by making the TB build system default to shipping not necessarily the very latest extension release, but the latest on AMO. This would transform AMO from an authority that can unilaterally approve updates, to just an additional code reviewer (who can be overridden).

For old security issues, the status quo with extensions.update.interval == 86400 is 24h worst case, so 12h on average until an approved update is applied; which comes after however much time AMO approval takes... Hmm, how fast could the TB release process actually upload an update, assuming it's only an extension version bump and nothing else?

Note: See TracTickets for help on using tickets.