1294483 - Unsigned add-ons installed in the local machine section of the windows registry aren't blocked

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

Bug 1255590 was intended to allow the use of unsigned add-ons in the system area of linux distributions but I failed to review it properly and it also applied to the windows registry install location. This is a pretty gaping hole in the signing story as that location is where we tell sideloaders to put their add-ons.

Comment 1

[Andy McKay]

Spoke to Kev and Shell about this whether we should uplift this to 48 and the verdict was let's aim for 49. It's something that's been happening for quite a few releases already, so waiting a few weeks for 49 probably won't make a huge difference, it will also let it bake a while and get some QA on it.

Comment 2

[Andy McKay]

Turns out it hasn't been around a while, it happened in 48. Suggestion is to get this out on the point release.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

From discussion in IRC this is contributing to a top startup crash: https://bugzilla.mozilla.org/show_bug.cgi?id=1131180
Sylvestre, needinfo to you to make sure you consider this for 48.0.1.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Once we have a patch this would also need sec approval before landing.

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch

This is spinning through try right now but we should start the review process as it is passing all tests locally for me anyway.

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8780303 [details] [diff] [review]
patch

Review of attachment 8780303 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, much more readable as a bonus.

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 8780303 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Which older supported branches are affected by this flaw?

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?

Comment 8

[Dave Townsend [:mossop]]

(In reply to Dave Townsend [:mossop] from comment #7)
> Comment on attachment 8780303 [details] [diff] [review]
> patch
> 
> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Requires admin access on the machine in the first place to add something to the registry.

> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?

No

> Which older supported branches are affected by this flaw?

48 and later are affected

> Do you have backports for the affected branches? If not, how different, hard
> to create, and risky will they be?

No backports yet but I expect this to apply cleanly.

> How likely is this patch to cause regressions; how much testing does it need?

We should do manual QA on registry installed add-ons and signing checks in general.

Comment 9

[Dave Townsend [:mossop]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eaa583dc8ea7

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 8780303 [details] [diff] [review]
patch

(In reply to Dave Townsend [:mossop] from comment #8)
> > How easily could an exploit be constructed based on the patch?
> Requires admin access [...] to add something to the registry.

Which on windows is pretty much every installer ever, just what all our side-loaders use :-(

sec-approval=dveditz

Comment 11

[Dave Townsend [:mossop]]

https://hg.mozilla.org/integration/fx-team/rev/666549adf9c6

Comment 12

[Marco Castelluccio [:marco]]

Is it possible to add a test to make sure this doesn't regress again?

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Important issue, tracking!

Comment 14

[Vasilica Tamas, QA [:vtamas]]

We were unable to reproduce this issue on Firefox 51.0a1, Firefox 50.0a2, Firefox 49.0b2 and Firefox 48 under Windows 10 64-bit and Windows 7 64-bit, running the following steps:
- Find an unsigned add-on
- Set up to install this add-on via the Windows registry using instructions from https://developer.mozilla.org/en-US/docs/Archive/Mozilla/Adding_extensions_using_the_Windows_registry
- Restart Firefox and load about:addons

The unsigned add-ons were automatically disabled in Add-ons Manager: http://screencast.com/t/m1VWAHAehs

Is there any additional step or precondition that we should take care of in order to reproduce this issue?

Comment 15

[Marco Castelluccio [:marco]]

Vasilica, you could try with the steps that philipp outlined here: https://bugzilla.mozilla.org/show_bug.cgi?id=1286368#c31.

Comment 16

[Dave Townsend [:mossop]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #14)
> We were unable to reproduce this issue on Firefox 51.0a1, Firefox 50.0a2,
> Firefox 49.0b2 and Firefox 48 under Windows 10 64-bit and Windows 7 64-bit,
> running the following steps:
> - Find an unsigned add-on
> - Set up to install this add-on via the Windows registry using instructions
> from
> https://developer.mozilla.org/en-US/docs/Archive/Mozilla/
> Adding_extensions_using_the_Windows_registry
> - Restart Firefox and load about:addons
> 
> The unsigned add-ons were automatically disabled in Add-ons Manager:
> http://screencast.com/t/m1VWAHAehs
> 
> Is there any additional step or precondition that we should take care of in
> order to reproduce this issue?

Make sure you're using the HKEY_LOCAL_MACHINE section of the registry, it won't reproduce for the HKEY_CURRENT_USER install location.

Comment 17

[Dave Townsend [:mossop]]

(In reply to Marco Castelluccio [:marco] from comment #12)
> Is it possible to add a test to make sure this doesn't regress again?

Yes we were discussing yesterday how to add a test and I'll be working on one today.

Comment 18

[Dave Townsend [:mossop]]

Attachment: Convert test_registry.js to be task based

Comment 19

[Dave Townsend [:mossop]]

Attachment: testcase

Comment 20

[Andrew Swan [:aswan]]

Comment on attachment 8780589 [details] [diff] [review]
testcase

Review of attachment 8780589 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but I assume this will be embargoed for a while?

Comment 21

[Dave Townsend [:mossop]]

Comment on attachment 8780588 [details] [diff] [review]
Convert test_registry.js to be task based

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

This is a refactoring of a testcase, you wouldn't be able to guess the exploit based on it except knowing the general area (add-ons installed in the registry).

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No

Which older supported branches are affected by this flaw?

48 and higher

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This should apply cleanly.

How likely is this patch to cause regressions; how much testing does it need?

Very low risk.

Comment 22

[Dave Townsend [:mossop]]

Comment on attachment 8780589 [details] [diff] [review]
testcase

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

This testcase makes it clear that there is an issue with signing checks for registry installed add-ons so you could probably figure out the problem quickly. You still need escalated privileges to install an add-on in this way.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

It makes it fairly obvious.

Which older supported branches are affected by this flaw?

48 and higher.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This should apply cleanly.

How likely is this patch to cause regressions; how much testing does it need?

Just a testcase so no regressions.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/666549adf9c6

Comment 24

[Dave Townsend [:mossop]]

Comment on attachment 8780303 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1255590
[User impact if declined]: Unsigned add-ons can be injected into Firefox via the windows registry
[Describe test coverage new/current, TreeHerder]: QA are preparing a manual test plan, other automated tests are pretty solid and the new testcases that have yet to land show that the fix works.
[Risks and why]: Low risk, very few changes and if anything the code is less complex than it was before.
[String/UUID change made/needed]: None

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Sylvestre, just to make sure you see this for the dot release. Since this is sec-moderate rated we don't need sec approval.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8780303 [details] [diff] [review]
patch

Thanks, taking it as it might address a crash.
Should be in 48.0.1

Comment 27

[Dave Townsend [:mossop]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/65f69c19b3c7
https://hg.mozilla.org/releases/mozilla-beta/rev/db6f582c5da2
https://hg.mozilla.org/releases/mozilla-release/rev/e33c4c0efaaf

Comment 28

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8780589 [details] [diff] [review]
testcase

We shouldn't check in the test until after this ships.

Comment 29

[Vasilica Tamas, QA [:vtamas]]

Verified this fix and performed regression testing around it on Firefox 48.0.1 (20160815115917) tinderbox build (https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-release-win32/1471287557/) under Windows 10 32-bit, Windows 7 64-bit and Mac OS X 10.10 and we noticed the following:

- The unsigned add-ons which are installed via registry entry are automatically disabled in Addons Manager
- The signed add-ons which are installed via registry entry are enabled and displayed in Addons Manager
- There were no new regressions 
- More details about this testing are available on: https://docs.google.com/spreadsheets/d/1krYI33GJowAfO9i-_wJt2ASttWm_kKRJXzOWdDYqd9M/edit#gid=457466695

Comment 30

[Gerry Chang [:gchang]]

Added in the 48.0.1 release notes: "Fix an unsigned add-ons issue on Windows".

Comment 31

[Vasilica Tamas, QA [:vtamas]]

Verified as fixed on Firefox 48.0.1 build 1 and build 3, Firefox 50.0a2 (2016-08-16/17) and Firefox 51.0a1 (2016-08-16/17) under Windows 10 32-bit, Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.10 and everything worked as expected. We also performed regression testing around this bug but there were no new issues found. 


More details about our testing are available here: https://docs.google.com/spreadsheets/d/1krYI33GJowAfO9i-_wJt2ASttWm_kKRJXzOWdDYqd9M/edit#gid=457466695

As soon as the beta build will be available we will verify the fix on this version as well.

Comment 32

[Vasilica Tamas, QA [:vtamas]]

Confirm that this issue is also fixed on Firefox 49.0b5 (20160818050015) under Windows 7 64-bit, Windows 10 32-bit, Ubuntu 16.04 32-bit and Mac OS X 10.11. 
Testing details are available here: https://docs.google.com/spreadsheets/d/1krYI33GJowAfO9i-_wJt2ASttWm_kKRJXzOWdDYqd9M/edit#gid=1498217123

There was found a new bug but the probability to be encountered is very low, so I do not consider this issue a blocker: Bug 1296620

I’ve also noticed that when trying to uninstall a signed add-on installed via registry key, which also appears in new disco pane (e.g Adblock Plus) will display an unexpected error.
-> Expected Results: A clearer error message should be displayed regarding the add-on uninstall process or the install/uninstall button from disco pane should be grayed out.

Dave, any thoughts about this?

Comment 33

[Vasilica Tamas, QA [:vtamas]]

Attachment: unexpected error message.png

I've attached a screenshot with the "unexpected error" message.

Comment 34

[Kris Maglione [:kmag]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #32)
> I’ve also noticed that when trying to uninstall a signed add-on installed
> via registry key, which also appears in new disco pane (e.g Adblock Plus)
> will display an unexpected error.
> -> Expected Results: A clearer error message should be displayed regarding
> the add-on uninstall process or the install/uninstall button from disco pane
> should be grayed out.

Yes, the uninstall option should not be available for globally-installed add-ons. Can you file a follow-up bug for this?

Comment 35

[Vasilica Tamas, QA [:vtamas]]

(In reply to Kris Maglione [:kmag] from comment #34)
> Yes, the uninstall option should not be available for globally-installed
> add-ons. Can you file a follow-up bug for this?

Thanks Kris! Filled Bug 1297028