Closed Bug 1294483 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla51
Tracking Status
firefox48 blocking verified
firefox49 + verified
relnote-firefox --- 48+
firefox50 + verified
firefox51 + verified

People

(Reporter: mossop, Assigned: mossop)

References

(Depends on 1 open bug)

Details

(Keywords: regression, sec-moderate)

Attachments

(4 files)

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.
Keywords: regression
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.
Turns out it hasn't been around a while, it happened in 48. Suggestion is to get this out on the point release.
Blocks: 1131180
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.
Once we have a patch this would also need sec approval before landing.
Attached patch patchSplinter Review
This is spinning through try right now but we should start the review process as it is passing all tests locally for me anyway.
Attachment #8780303 - Flags: review?(aswan)
Comment on attachment 8780303 [details] [diff] [review]
patch

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

Nice, much more readable as a bonus.
Attachment #8780303 - Flags: review?(aswan) → review+
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?
Attachment #8780303 - Flags: sec-approval?
(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 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
Attachment #8780303 - Flags: sec-approval? → sec-approval+
Is it possible to add a test to make sure this doesn't regress again?
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?
Flags: needinfo?(dtownsend)
Vasilica, you could try with the steps that philipp outlined here: https://bugzilla.mozilla.org/show_bug.cgi?id=1286368#c31.
(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.
Flags: needinfo?(dtownsend)
(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.
Attached patch testcaseSplinter Review
Attachment #8780589 - Flags: review?(aswan)
Attachment #8780588 - Flags: review?(aswan) → review+
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?
Attachment #8780589 - Flags: review?(aswan) → review+
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.
Attachment #8780588 - Flags: sec-approval?
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.
Attachment #8780589 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/666549adf9c6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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
Attachment #8780303 - Flags: approval-mozilla-release?
Attachment #8780303 - Flags: approval-mozilla-beta?
Attachment #8780303 - Flags: approval-mozilla-aurora?
Sylvestre, just to make sure you see this for the dot release. Since this is sec-moderate rated we don't need sec approval.
Flags: needinfo?(sledru)
Comment on attachment 8780303 [details] [diff] [review]
patch

Thanks, taking it as it might address a crash.
Should be in 48.0.1
Flags: needinfo?(sledru)
Attachment #8780303 - Flags: approval-mozilla-release?
Attachment #8780303 - Flags: approval-mozilla-release+
Attachment #8780303 - Flags: approval-mozilla-beta?
Attachment #8780303 - Flags: approval-mozilla-beta+
Attachment #8780303 - Flags: approval-mozilla-aurora?
Attachment #8780303 - Flags: approval-mozilla-aurora+
Attachment #8780588 - Flags: sec-approval? → sec-approval+
Comment on attachment 8780589 [details] [diff] [review]
testcase

We shouldn't check in the test until after this ships.
Attachment #8780589 - Flags: sec-approval?
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
Group: firefox-core-security → core-security-release
Added in the 48.0.1 release notes: "Fix an unsigned add-ons issue on Windows".
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.
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?
Flags: needinfo?(dtownsend)
I've attached a screenshot with the "unexpected error" message.
(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?
Flags: needinfo?(dtownsend)
(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
No longer depends on: 1297028
See Also: → 1296620
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.