1303418 - (CVE-2016-9064) Addons update must verify IDs match between current and update versions

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Julien Vehent]

@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

Comment 1

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

Is this different, or worse, in 49 than it is in 48?

Comment 2

[Andrew Swan [:aswan]]

Attachment: quick hack

MozReview-Commit-ID: JHINo8ShmeI

Comment 3

[Andrew Swan [:aswan]]

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.

Comment 4

[Jorge Villalobos [:jorgev] (he/him)]

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 5

[Dave Townsend [:mossop]]

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.

Comment 6

[Andrew Swan [:aswan]]

Attachment: quick hack

MozReview-Commit-ID: JHINo8ShmeI

Comment 7

[Andrew Swan [:aswan]]

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...

Comment 8

[Daniel Veditz [:dveditz]]

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).

Comment 9

[Dave Townsend [:mossop]]

(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

[Andy McKay]

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.

Comment 11

[Andrew Swan [:aswan]]

Attachment: Don't allow upgrades that change the addon ID

MozReview-Commit-ID: JHINo8ShmeI

Comment 12

[Dave Townsend [:mossop]]

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.

Comment 13

[Andrew Swan [:aswan]]

(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?

Comment 14

[Dave Townsend [:mossop]]

(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.

Comment 15

[Andrew Swan [:aswan]]

Attachment: Don't allow upgrades that change the addon ID

MozReview-Commit-ID: JHINo8ShmeI

Comment 16

[Andrew Swan [:aswan]]

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.

Comment 17

[Andrew Swan [:aswan]]

Attachment: Don't allow upgrades that change the addon ID

Updated to fix eslint errors

MozReview-Commit-ID: JHINo8ShmeI

Comment 18

[Andrew Swan [:aswan]]

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

Comment 19

[Andrew Swan [:aswan]]

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

see comment 16

Comment 20

[Andrew Swan [:aswan]]

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.

Comment 21

[Joshua Yabut]

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

Comment 22

[Selena Deckelmann :selenamarie :selena]

(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.

Comment 23

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

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.

Comment 24

[Daniel Veditz [:dveditz]]

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.

Comment 25

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e4ddc2eb91dc8973ff7ff0b1a56f8fe0d62cacf
Bug 1303418 Don't allow upgrades that change the addon ID r=mossop

Comment 26

[Andrew Swan [:aswan]]

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 27

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f162c05861744c9e6125a936fdf2ffcedfd3d68f
Bug 1303418 Follow-up to fix fat-fingered graft r=me

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f162c0586174
Follow-up to fix fat-fingered graft r=me

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7e4ddc2eb91d
https://hg.mozilla.org/mozilla-central/rev/f162c0586174

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7e4ddc2eb91d
https://hg.mozilla.org/mozilla-central/rev/f162c0586174

Comment 31

[Andrew Swan [:aswan]]

Attachment: Don't allow upgrades that change the addon ID

MozReview-Commit-ID: JHINo8ShmeI

Comment 32

[Andrew Swan [:aswan]]

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

Comment 33

[Andrew Swan [:aswan]]

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

see comment 32

Comment 34

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Sec-high, Beta50+

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/27ce6db0b235

Comment 36

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 37

[Gerry Chang [:gchang]]

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

Sec-high, take it in 51 aurora

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3a82ff00dbb8
https://hg.mozilla.org/releases/mozilla-aurora/rev/9911bd94e79f

Comment 39

[Andrew Swan [:aswan]]

Attachment: Don't allow upgrades that change the addon ID

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

Comment 40

[Vasilica Tamas, QA [:vtamas]]

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 41

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

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.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/00c2b7baaa0b

Comment 43

[Vasilica Tamas, QA [:vtamas]]

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.

Comment 44

[Numan Sheikh]

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] http://www.nbalivemobile.net/nba-live-mobile-hack/
[2] http://www.nbalivemobile.net/nba-2k17-hack/