Bug 1303418 (CVE-2016-9064)

Addons update must verify IDs match between current and update versions

VERIFIED FIXED in Firefox -esr45

Status

()

defect
P1
critical
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: ulfr, Assigned: aswan, NeedInfo)

Tracking

(Blocks 1 bug, {addon-compat, sec-high})

unspecified
mozilla52
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox49+ wontfix, firefox-esr4550+ verified, firefox50+ verified, firefox51+ verified, firefox52+ verified)

Details

(Whiteboard: [adv-main50+][adv-esr45.5+] triaged)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
@movrcx reported [1] a vulnerability in the addons update process that allows a Man-In-The-Middle attack to replace an installed addon with a fraudulent version. The attacker needs to first obtain a valid signature for a fraudulent addon, which can be done without review, then hijack the call between the user and AMO to replace the response from /update/VersionCheck.php with the IDs of the fraudulent addon.

This attack works because Firefox does not currently verify that the updated addon ID matches the installed addon ID. Adding this check will require the attacker to break into the AMO service or steal the developer access to AMO to sign a fraudulent addon with the same IDs as the target addon.

The ID verification should be implemented in Firefox.

[1] https://hackernoon.com/tor-browser-exposed-anti-privacy-implantation-at-mass-scale-bd68e9eb1e95#.ey0pm84bd
Is this different, or worse, in 49 than it is in 48?
(Assignee)

Comment 2

3 years ago
Posted patch quick hack (obsolete) — Splinter Review
MozReview-Commit-ID: JHINo8ShmeI
(Assignee)

Comment 3

3 years ago
Support for changing IDs was explicitly added in bug 412819, but AMO no longer supports changing IDs (not deliberately anyway).  Consensus (between Andy and me so far) is to remove this support from Firefox rather than try to design a method for securely changing an ID.
AMO has never really supported changing the ID, but it has been possible for non-AMO add-ons through the update mechanism. I don't think that changing IDs is a very common use case, and there are other ways to do it than via automatic updates (at least with legacy APIs), so I also support this change.
Comment on attachment 8792131 [details] [diff] [review]
quick hack

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

Surprised that this is rated so highly, the only way of circumventing I can think of involves either compromising AMO itself or getting a built-in CA to issue you a valid cert for AMO and then MITM.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +6641,5 @@
>    }
>  
>    let install = new AddonInstall(aAddon._installLocation, url,
> +                                 aUpdate.updateHash, releaseNotesURI, aAddon,
> +                                 null, aAddon.id);

No need for the extra parameter, the AddonInstall already has the existing add-on to compare against here.
(Assignee)

Comment 6

3 years ago
Posted patch quick hack (obsolete) — Splinter Review
MozReview-Commit-ID: JHINo8ShmeI
(Assignee)

Updated

3 years ago
Attachment #8792131 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
The test isn't quite passing yet and I think that this has now orphaned some code that can be pruned as well but here's the work-in-progress before I step away for a little while...

Updated

3 years ago
Group: addons-security, client-services-security → toolkit-core-security
Component: Security → Add-ons Manager
Product: addons.mozilla.org → Toolkit
This patch checks that the downloaded add-on (.xpi) matches the ID of the thing it's supposed to be updating. Would it be worthwhile to check the metadata returned by versioncheck and not even download the thing if it doesn't match?

I mean in addition (multi-layer security); the install-time check is a good thing.

Signed update.rdf responses means it came from us, even if TLS gets MITM'd.
 * prevents changing the hash to match a different add-on
 * prevents changing the download URL to point to a different download server
 (signing these is another set of bugs)

But a MITM could still substitute one signed update.rdf for another
 * version check prevents downgrade attack (I think we do this already)
 * ID check could prevent substitution

Currently the add-on ID doesn't appear in the body of the update.rdf, but it is part of "urn:mozilla:extension:%ADDON-ID%" in the RDF junk. We could pull it out of there, but if we want to do this it might be worth adding <em:id> in the body next to <em:version> to make things easier (Server-side change required).
(In reply to Daniel Veditz [:dveditz] from comment #8)
> Currently the add-on ID doesn't appear in the body of the update.rdf, but it
> is part of "urn:mozilla:extension:%ADDON-ID%" in the RDF junk. We could pull
> it out of there, but if we want to do this it might be worth adding <em:id>
> in the body next to <em:version> to make things easier (Server-side change
> required).

We do already ID check the "urn:mozilla:extension:%ADDON-ID%" since that is what allows you to embed update info for multiple add-ons in the same file.

Comment 10

3 years ago
Not sure how much work the id in the RDF is, probably pretty easy. I feel like the better thing to focus on might be bug 1303183 though.
Group: toolkit-core-security
(Assignee)

Comment 11

3 years ago
MozReview-Commit-ID: JHINo8ShmeI
Attachment #8792595 - Flags: review?(dtownsend)
(Assignee)

Updated

3 years ago
Attachment #8792164 - Attachment is obsolete: true
Comment on attachment 8792595 [details] [diff] [review]
Don't allow upgrades that change the addon ID

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

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm
@@ +5820,5 @@
>  
> +    if (this.existingAddon && this.addon.id != this.existingAddon.id) {
> +      let err = new Error(`Refusing to upgrade addon ${this.existingAddon.id} to new ID {this.addon.id}`);
> +      return Promise.reject([AddonManager.ERROR_INCORRECT_ID, err]);
> +    }

If I'm reading this right then you could potentially fool this by using a multipackage XPI with the right ID which then installs a different add-on. We should probably always reject multipackage XPIs when there is an existing add-on being updated.
Attachment #8792595 - Flags: review?(dtownsend)
(Assignee)

Comment 13

3 years ago
(In reply to Dave Townsend [:mossop] from comment #12)
> If I'm reading this right then you could potentially fool this by using a
> multipackage XPI with the right ID which then installs a different add-on.
> We should probably always reject multipackage XPIs when there is an existing
> add-on being updated.

Is the concern about a "regular" addon having an upgrade that points to a multipackage xpi?  Or an addon that originally came from a multipackage xpi being upgraded to ... something?
For the first case, the test should fail since this.addon is the multipackage xpi, and its id property will be null or undefined, which won't match the existing addon id.  I can add a test for this, or did I misunderstand the issue?
(In reply to Andrew Swan [:aswan] from comment #13)
> (In reply to Dave Townsend [:mossop] from comment #12)
> > If I'm reading this right then you could potentially fool this by using a
> > multipackage XPI with the right ID which then installs a different add-on.
> > We should probably always reject multipackage XPIs when there is an existing
> > add-on being updated.
> 
> Is the concern about a "regular" addon having an upgrade that points to a
> multipackage xpi?  Or an addon that originally came from a multipackage xpi
> being upgraded to ... something?
> For the first case, the test should fail since this.addon is the
> multipackage xpi, and its id property will be null or undefined, which won't
> match the existing addon id.  I can add a test for this, or did I
> misunderstand the issue?

multipackage XPIs don't have to have an ID in the root install.rdf but they can and my brief glance suggests it gets passed all the way through to here, so add a test for that case and then if I'm right fix it so it doesn't work.
(Assignee)

Comment 15

3 years ago
MozReview-Commit-ID: JHINo8ShmeI
Attachment #8792645 - Flags: review?(dtownsend)
(Assignee)

Updated

3 years ago
Attachment #8792595 - Attachment is obsolete: true
Attachment #8792645 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 16

3 years ago
Comment on attachment 8792645 [details] [diff] [review]
Don't allow upgrades that change the addon ID

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Quite easily, but this bug is already public and comments in the bug explain the problem clearly.  However, this bug is about adding defense-in-depth so a real exploit would involve other steps, as detailed in the bug description.

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

Which older supported branches are affected by this flaw?
This bug has existed for some time, it is present on all currently supported branches.

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?
The actual code change should be easy to backport, the tests use test code that has been modified recently that may take more effort.

How likely is this patch to cause regressions; how much testing does it need?
The change is simple and straightforward enough that it is unlikely to cause unintended regressions.  The risk is more that somebody somewhere is relying on the previous insecure behavior, but we can handle that if it arises by helping anybody affected to find a supported way to manage addon upgrades.
Attachment #8792645 - Flags: sec-approval?
(Assignee)

Comment 17

3 years ago
Updated to fix eslint errors

MozReview-Commit-ID: JHINo8ShmeI
(Assignee)

Updated

3 years ago
Attachment #8792645 - Attachment is obsolete: true
Attachment #8792645 - Flags: sec-approval?
(Assignee)

Updated

3 years ago
Attachment #8792666 - Flags: sec-approval?
Attachment #8792666 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8792666 - Flags: sec-approval?
(Assignee)

Comment 19

3 years ago
Comment on attachment 8792666 [details] [diff] [review]
Don't allow upgrades that change the addon ID

see comment 16
Attachment #8792666 - Flags: sec-approval?
(Assignee)

Comment 20

3 years ago
Dan, I got the impression over the weekend that there was some urgency to this, but bugzilla tells me is needs sec-approval to land.  Note that the bug is already public.
Also, Mossop's earlier comment clarified a bit about the format of update manifests, I'm happy to answer further questions if there are any.
Flags: needinfo?(dveditz)

Comment 21

3 years ago
I posted a restricted bug about this process and the proposed fix. I think it's important enough to look at before pushing changes. Thanks!

https://bugzilla.mozilla.org/show_bug.cgi?id=1304112

Updated

3 years ago
Whiteboard: triaged
(In reply to Andrew Swan [:aswan] from comment #20)
> Dan, I got the impression over the weekend that there was some urgency to
> this, but bugzilla tells me is needs sec-approval to land.  Note that the
> bug is already public.
> Also, Mossop's earlier comment clarified a bit about the format of update
> manifests, I'm happy to answer further questions if there are any.

I pinged Dan about this and he would like to see this uplifted to 50, but it's not urgent for 49 because of HPKP being enabled on the add-on update servers. I'm guessing based on his schedule, he'll get to the sec-review next week. 

Sorry about the undue urgency, and thank you for the patch.
Tracking across current versions.  
If we end up doing a 49.0.1 this may be a candidate for uplift to release, but we should wait for sec approval and maybe test on 50 first.
Please go ahead and check in.

I'm not against pushing this to a 49 point release if we're confident in it after testing, but this bug doesn't need to be the trigger for a 49 point release. We'll want this on the ESR branch as well.
Flags: needinfo?(dveditz)
Attachment #8792666 - Flags: sec-approval? → sec-approval+
(Assignee)

Comment 26

3 years ago
As of earlier this week, the attached patch applied cleanly to aurora and it was good on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b289af413bd&selectedJob=27684834

Uplift to beta/release is going to take a little more work, I'll try to have a patch ready to test today.

Comment 28

3 years ago
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f162c0586174
Follow-up to fix fat-fingered graft r=me
Blocks: 1304117

Comment 29

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7e4ddc2eb91d
https://hg.mozilla.org/mozilla-central/rev/f162c0586174
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Comment 31

3 years ago
MozReview-Commit-ID: JHINo8ShmeI
(Assignee)

Comment 32

3 years ago
Comment on attachment 8792666 [details] [diff] [review]
Don't allow upgrades that change the addon ID

Approval Request Comment
[Feature/regressing bug #]:
This is a long-standing bug

[User impact if declined]:
The patch adds extra defense against a particular security vulnerability.  Without the patch, we won't have the extra protection.

[Describe test coverage new/current, TreeHerder]:
Tests are included in the patch

[Risks and why]: 
The changes are confined to the processing of addon upgrades and are unlikely to cause unintended regressions.  The larger risk is if any addon developers are relying on the ability to change addon ids during an upgrade.  As discussed in the bug, this is a risk that the add-ons team judges to be acceptable.

[String/UUID change made/needed]:
n/a
Attachment #8792666 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 33

3 years ago
Comment on attachment 8794374 [details] [diff] [review]
Don't allow upgrades that change the addon ID

see comment 32
Attachment #8794374 - Flags: approval-mozilla-beta?
Comment on attachment 8794374 [details] [diff] [review]
Don't allow upgrades that change the addon ID

Sec-high, Beta50+
Attachment #8794374 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Hi Andrew, Dan pointed out that esr45 is affected and therefore we will need a patch for uplift to ESR45.5. Please go ahead and nominate that.
Flags: needinfo?(aswan)
Comment on attachment 8792666 [details] [diff] [review]
Don't allow upgrades that change the addon ID

Sec-high, take it in 51 aurora
Attachment #8792666 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 39

3 years ago
This try run is a little wacky (no linux debug build?) but
the updated test in this patch does pass on all the other
platforms.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad03f80ca99d
(Assignee)

Updated

3 years ago
Flags: needinfo?(aswan)
Attachment #8795925 - Flags: approval-mozilla-esr45?
Verified as fixed on Firefox 52.0a1 (2016-09-28/29), Firefox 51.0a2 (2016-09-28/29), Firefox 50.0b2 unbranded build (20160926162149) under Windows 10 64-bit, Windows 7 64-bit and Ubuntu 16.04 32-bit. Tested various scenarios and we confirm that the add-on/webextension is not updated to a new version with different ID.

Covered the following cases:
 - unlisted add-on (with id specified in the install.rdf) cannot be updated if the ids don't match 
 - unlisted webext (with id specified in the manifest.json) cannot be updated if the ids don't match
 - unlisted webext (with id NOT specified in the manifest.json) cannot be updated if the ids don't match (webext 1 without id -> webext 2 without id, webext 1 without id -> webext 2 with id)


Based on this testing I am marking this bug as Verified.
Comment on attachment 8795925 [details] [diff] [review]
Don't allow upgrades that change the addon ID

Sec-high issue, fixed in beta 50, let's take this for esr45.5.0.
Attachment #8795925 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Verified as fixed on Firefox 45.4.1 esr tinderbox-build (20161018094221) under Windows 10 64-bit, Ubuntu 16.04 32-bit and Mac OS X 10.10.4. 
Confirm that an add-on/webextension is not updated to a new version with different ID.
Whiteboard: triaged → [adv-main50+][adv-esr45.5+] triaged
Alias: CVE-2016-9064
Blocks: 1308251
Comment hidden (spam)
You need to log in before you can comment on or make changes to this bug.