Remove built-in certificate requirement for addon updates

NEW
Unassigned

Status

()

defect
P2
normal
3 years ago
10 months ago

People

(Reporter: chuck, Unassigned)

Tracking

(Depends on 2 bugs, Blocks 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54+ wontfix, firefox55+ fix-optional, firefox56 fix-optional)

Details

()

Attachments

(1 attachment)

We've had reports of add-on installation on Test Pilot being broken by users of security software suites. After some investigation, I found that these suites (Avast Free and Kaspersky were the ones observed, but there are likely others) installed their own root certificate in Firefox's store (in order to monitor encrypted traffic), and MITMed all outgoing connections.

Some testing showed that the add-on install failures was due to built-in certificate check failing.

Of note:

- This does not break installs on AMO, though both AMO and its CDN are MITMed.
- This does not break installs from the Discopane.
- This does break add-on updates.
- This does break system add-on updates.
- This breaks installs of the Test Pilot add-on, which uses mozAddonManager.
- This breaks installs of Test Pilot experiments, which uses AddonManager.

Probably not a security bug, but since there may be potentially-exploitable holes, I figured I'd let someone with better expertise in that area make that call.


[1] https://github.com/mozilla/testpilot/issues/1474
[2] https://github.com/mozilla/testpilot/issues/1474#issuecomment-251997267
Group: client-services-security
We do have certificate pinning enabled for these domains, but local roots should bypass certificate pins. I know Mark Goodwin has investigated such issues earlier this year, so maybe he has some insight on the root cause?

If not certificate related, it's possible, although highly unlikely, that those security suites can only negotiate a particular TLS configuration that isn't supported by our sites.
Flags: needinfo?(mgoodwin)
Add-on installs/updates have their own built-in root check that is separate from pinning checks. We would have to finish off bug 880269 if we want add-on installs/updates to work when users have these sorts of systems intercepting their traffic.
Did something change recently on our side (i.e. more pinning became enforced, or certificates changed, or I don't know..) that would point this as a recent-ish problem? Or is it likely that this has been happening for a long time?
The built-in certificate checks aren't performed when we have a hash for the file being downloaded, so to to fix the (non-third-party) update and download cases, we just need to finish migrating to signed update manifests on AMO, and provide hashes for all installs from testpilot.
(In reply to :Felipe Gomes (needinfo me!) from comment #3)
> Did something change recently on our side (i.e. more pinning became
> enforced, or certificates changed, or I don't know..) that would point this
> as a recent-ish problem? Or is it likely that this has been happening for a
> long time?

No, this is not a new issue. It's currently breaking updates (but not downloads) from AMO. We're only noticing it now for the initial download case because testpilot doesn't supply hashes for its installs.
The system add-on updates do provide the hash, and the original reporter said that these updates are also broken. We should look more into that too.
The problem with updates is that we currently block fetching the update manifest if it isn't secured by a built-in root. That's why this is blocked on content signing. But system add-on updates are a separate case from AMO updates.
Flags: needinfo?(mgoodwin)
(In reply to Kris Maglione [:kmag] from comment #7)
> The problem with updates is that we currently block fetching the update
> manifest if it isn't secured by a built-in root.

I think we could safely remove that control (as :keeler said, finish bug 880269) and use preloaded key pins instead (bug 1301956). That would limit valid CAs to whitelisted ones and locally imported ones.
We already use pre-loaded key pins. The built-in cert check currently serves to prevent the pins from being ignored when a local root is being used. Which is why removal of the built-in root check is currently blocked on enabling content signing for update manifests.
While I fully support signing the update manifest, I don't think we need to block on it. Users that get MITMed still check addons signatures, which use our private AMO PKI. An attacker could only really exploit this mechanism if a bug is found on our addon update implementation (see bug 1303418). That's a small enough attack surface that we could reasonably remove the custom built-in root check and unblock those users.
We may want to track this bug at least for 52, if this is something we hope to fix in that time frame.
Tracking 52+ - as Liz says in Comment 11, so we don't lose track of it for 52.
Assignee: nobody → rhelmer
(In reply to Chuck Harmston [:chuck] from comment #0)
> Created attachment 8798485 [details]
> WhimsyPro update failing
> 
> We've had reports of add-on installation on Test Pilot being broken by users
> of security software suites. After some investigation, I found that these
> suites (Avast Free and Kaspersky were the ones observed, but there are
> likely others) installed their own root certificate in Firefox's store (in
> order to monitor encrypted traffic), and MITMed all outgoing connections.
> 
> Some testing showed that the add-on install failures was due to built-in
> certificate check failing.
> 
> Of note:
> 
> - This does not break installs on AMO, though both AMO and its CDN are
> MITMed.
> - This does not break installs from the Discopane.
> - This does break add-on updates.
> - This does break system add-on updates.

GMP and system add-ons share a common update mechanism so it's very likely these are broken as well.
Adding a few folks that I discussed this issue with following a "go faster" meeting yesterday - MITM from "security" software is breaking updates for system add-ons (and almost certainly GMP) as well as test pilot add-ons. Bug 787874 suggests that it's breaking the existing hotfix mechanism as well.

Per comment 8 comment 9 and comment 10, is the path forward here to fix bug 880269 (explicit `isBuiltInToken()` check in CertUtils.jsm), and we can safely do that before content signing is ready?

If so, who can work on bug 880269? Kai was working on it a few years ago but the patch has bitrotted.

David, it looks like you did the last meaningful review on CertUtils.jsm - can you suggest who could take that bug?
Flags: needinfo?(dkeeler)
To be clear, we still need to at least prevent downloads from redirecting through insecure URLs when some sort signing or hash verification is not enabled.
(In reply to Robert Helmer [:rhelmer] from comment #14)
> If so, who can work on bug 880269? Kai was working on it a few years ago but
> the patch has bitrotted.
> 
> David, it looks like you did the last meaningful review on CertUtils.jsm -
> can you suggest who could take that bug?

I wouldn't call that review particularly meaningful - it only removed a single line of code :)
I think Dave Townsend or Robert Strong would have a better idea of who could take this on. I'd be happy to provide input on the security ramifications of any changes, though.
Flags: needinfo?(dkeeler) → needinfo?(robert.strong.bugs)
It has been so long since I've so much as looked at that code since I stopped using it in app update a long time ago. Dan Veditz provided the details of the original implementation in bug 544442 and I suspect that keeler or dveditz should be able to provide guidance to someone on the add-ons team regarding what would need to change.

Alternatively you could always add a new host name for aus and use cert pinning instead if you want but I don't know the specific cases where that would fail or the percentage of users that would fail for.
Flags: needinfo?(robert.strong.bugs)
Selena - we worked around bug 1300633 with bug 1267495. We still need signed GMP updates in order to safely remove pinning on aus5.mozilla.org.
Flags: needinfo?(sdeckelmann)
As can be seen at the top of the bug, multiple mechanisms go through that built-in check and they all have different security properties. For add-on updates (only!) we could now turn off that check and be OK because of the work done elsewhere like bug 1303408. Will be better when the update snippets themselves are content-signed but in this one case it's OK without because 1) we know specifically what signature we're looking for on the content, and 2) version numbers must go up.

The GMP updates apparently worked around this anyway (with a hardcoded install path, but no updates), but they really need the content-signing bit to remove that check. If the GMP downloads contain a copy of their metadata (e.g. name, version) inside the signed content, and if the GMP installer verifies that it matches the thing it -thinks- it's updating, and we ensure versions only go up, then like add-ons we could relax that requirement even without signed meta data. In both cases protection seems weak but OK in the short run.

I'm pretty sure system addons work like regular add-ons, and the hotfix was already OK when we checked after that Tor-addon-upgrade attack a while back. Those don't need the built-in check. Whether to enforce the built-ins is a pref so we may be able to turn it off for specific users, but we certainly can't remove the whole feature like the patch in bug 880269 until all the features have other mechanisms.

clearing the ni?selena from comment 18 because maybe this is the info being sought? The needed info is unclear.
Depends on: 880269, CVE-2016-9064
Flags: needinfo?(sdeckelmann)
From conversations yesterday with :Mossop, it seems like this is relevant to some work :rhelmer is currently doing (unsure which bug).
Flags: needinfo?(rhelmer)
(In reply to Selena Deckelmann :selenamarie :selena use ni? from comment #20)
> From conversations yesterday with :Mossop, it seems like this is relevant to
> some work :rhelmer is currently doing (unsure which bug).

Bug 1323547 is a tracking bug about identifying and measuring uptake of important updates.

Bug 1325501 is introducing a new module for update requests (ServiceRequest) which can be used as a drop-in replacement for XMLHttpRequest, so we can share the same connection settings for different types of update requests.

In bug 1307568 Felipe and I are working on shipping diagnostic code with Firefox so we can get telemetry on what part of the update process is breaking for some users, before we change the actual update code.

The missing piece here is specific bugs to disable cert check when appropriate as we've discussed in this bug. We're going to need to track down each update mechanism and move them over as they are ready - I'll file a tracking bug and start getting these together.

I'd like to make sure the measurement bit is in place before we start shipping significant changes to release users, so we can see if we're really helping uptake or not.
Flags: needinfo?(rhelmer)
(In reply to Daniel Veditz [:dveditz] from comment #19)
> because of the work done elsewhere like bug 1303408.

I meant bug 1303408

> Whether to enforce the built-ins is a pref so we may be able to turn
> it off for specific users,

I meant argument. Prefs are global.
(In reply to Daniel Veditz [:dveditz] from comment #19)
> As can be seen at the top of the bug, multiple mechanisms go through that
> built-in check and they all have different security properties. For add-on
> updates (only!) we could now turn off that check and be OK because of the
> work done elsewhere like bug 1303408. Will be better when the update
> snippets themselves are content-signed but in this one case it's OK without
> because 1) we know specifically what signature we're looking for on the
> content, and 2) version numbers must go up.
> 
> The GMP updates apparently worked around this anyway (with a hardcoded
> install path, but no updates), but they really need the content-signing bit
> to remove that check. If the GMP downloads contain a copy of their metadata
> (e.g. name, version) inside the signed content, and if the GMP installer
> verifies that it matches the thing it -thinks- it's updating, and we ensure
> versions only go up, then like add-ons we could relax that requirement even
> without signed meta data. In both cases protection seems weak but OK in the
> short run.
> 
> I'm pretty sure system addons work like regular add-ons, and the hotfix was
> already OK when we checked after that Tor-addon-upgrade attack a while back.
> Those don't need the built-in check. Whether to enforce the built-ins is a
> pref so we may be able to turn it off for specific users, but we certainly
> can't remove the whole feature like the patch in bug 880269 until all the
> features have other mechanisms.


System add-ons and GMP share the same install code - they both hit Balrog, and (IIRC) they both force cert pinning for the AUS connection. Currently we use HTTPS for system add-on download, and also require the XPIs to be signed.

System add-ons don't currently enforce that the version numbers always go up, so I think we need that in order to turn off the cert pinning to avoid downgrade attacks.
Moving the tracking flag, this doesn't look like it's getting fixed in the 52 time frame.
See Also: → 1307568
Mark 54 fix-optional as there are no actions for the moment and carry tracking flags to 55.
Ongoing project too complicated for one bug.  From https://bugzilla.mozilla.org/show_bug.cgi?id=1308251#c21 it looks like we have some other work in support of better measurement of uptake. How far along is that? Is this bug still useful/active or is the real work happening somewhere else?  Do you need more support for this project if we still plan to put these elements into place?
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #26)
> Ongoing project too complicated for one bug.  From
> https://bugzilla.mozilla.org/show_bug.cgi?id=1308251#c21 it looks like we
> have some other work in support of better measurement of uptake. How far
> along is that? Is this bug still useful/active or is the real work happening
> somewhere else?  Do you need more support for this project if we still plan
> to put these elements into place?

There's a project ongoing to fix this - add-on udpates are not the only type of updates impacted here.

The first wtep we're working on is to make sure we're collecting Telemetry consistently for different types of updates, so we can safely make big changes like this: bug 1323547

The next step after that is to change updates to use common request settings, and to favor signed content over cert pinning (the underlying problem here is that we're forcing cert pinning and depending on the network channel to be secure, which isn't the case when there's an active TLS mitm).

Each component is responsible for its own updates right now so we'll need to go through them one-by-one, but add-on updates are relatively high in the list of priorities.
Flags: needinfo?(rhelmer)
Depends on: 1403075
Any movement on this?

Seems odd to leave corporate users unable to update their add-ons for two years. Abby chance of it being fixed soon?
Updating title in light of comment 19 (which meant to reference bug 1303418)
Component: Security → Add-ons Manager
Priority: -- → P2
Product: addons.mozilla.org → Toolkit
Summary: Add-on installs and updates broken by security software → Remove built-in certificate requirement for addon updates
(In reply to Andrew Ducker from comment #28)
> Any movement on this?
> 
> Seems odd to leave corporate users unable to update their add-ons for two
> years. Abby chance of it being fixed soon?

In order to understand corporate impact can you please clarify if setting security.enterprise_roots.enabled=true fixes the problem? ("Some testing showed that the add-on install failures was due to built-in certificate check failing.") 
Also worth noting is we have a significant share of non corporate users running Avast (14%) or Kaspersky (3%) so this is likely not only a corporate issue.
I set security.enterprise_roots.enabled to true, did a restart, did a "check for updates", and still got "No updates found".
Thanks Andrew - overall it seems this issue would impact consumers and corporate similarly for set-ups where 3rd party softwares break add-ons or system add-ons updates (large corporations typically would validate and deploy add-ons manually whereas smaller ones could probably be more impacted in a similar way to what consumers would using Avast).
Michael, is this something that could impact Normandy system add-on roll-outs?
Flags: needinfo?(mcooper)
Normandy doesn't power system add-ons, or any sort of permanent add-on changes in Firefox. Balrog currently manages system add-ons.

Normandy does use add-ons for studies, which are temporary, and less sensitive to some users not getting access to them. We use  (or should be using) a similar method for getting our data as Balrog, so I'm not concerned about this.
Flags: needinfo?(mcooper)
Duplicate of this bug: 1493113
Not actively working on this, but can help out if it is actively blocking anyone.
Assignee: rhelmer → nobody
Apparently, in the past there were 2 options to disable builtin cert check for addon updates(extensions.install.requireBuiltInCerts and extensions.update.requireBuiltInCerts)  - would it be possible to reintroduce them?
(In reply to Ladislav Furman from comment #36)
> Apparently, in the past there were 2 options to disable builtin cert check
> for addon updates(extensions.install.requireBuiltInCerts and
> extensions.update.requireBuiltInCerts)  - would it be possible to
> reintroduce them?

Those preferences both still exist and, as far as I know, work as they always have.
The thing is that in current stable (at least according to my tests) even with those options set to false and using our corporate Zscaler proxy, addon updates fail according to browser console with following messages:(I use Slovak language version):

 [Zobraziť/skryť podrobnosti o správe.] Expected certificate attribute 'issuerName' value incorrect, expected: 'CN=DigiCert SHA2 Secure Server CA,O=DigiCert Inc,C=US', got: 'CN=johnsoncontrols.com,OU=Zscaler,O=Johnson Controls International plc,L=Cork,ST=Ireland,C=IE'. CertUtils.jsm:105
[Zobraziť/skryť podrobnosti o správe.] Expected certificate attribute 'issuerName' value incorrect, expected: 'CN=thawte SSL CA - G2,O="thawte, Inc.",C=US', got: 'CN=johnsoncontrols.com,OU=Zscaler,O=Johnson Controls International plc,L=Cork,ST=Ireland,C=IE'. CertUtils.jsm:105
[Zobraziť/skryť podrobnosti o správe.] Certificate checks failed. See previous errors for details. CertUtils.jsm:108
[Zobraziť/skryť podrobnosti o správe.] 1537858928456	addons.productaddons	ERROR	Request failed certificate checks: [Exception... "Certificate checks failed. See previous errors for details."  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://gre/modules/CertUtils.jsm :: validateCert :: line 109"  data: no] Log.jsm:853
[Zobraziť/skryť podrobnosti o správe.] Expected certificate attribute 'issuerName' value incorrect, expected: 'CN=DigiCert SHA2 Secure Server CA,O=DigiCert Inc,C=US', got: 'CN=johnsoncontrols.com,OU=Zscaler,O=Johnson Controls International plc,L=Cork,ST=Ireland,C=IE'. CertUtils.jsm:105
[Zobraziť/skryť podrobnosti o správe.] Expected certificate attribute 'issuerName' value incorrect, expected: 'CN=thawte SSL CA - G2,O="thawte, Inc.",C=US', got: 'CN=johnsoncontrols.com,OU=Zscaler,O=Johnson Controls International plc,L=Cork,ST=Ireland,C=IE'. CertUtils.jsm:105
[Zobraziť/skryť podrobnosti o správe.] Certificate checks failed. See previous errors for details. CertUtils.jsm:108
[Zobraziť/skryť podrobnosti o správe.] 1537858928460	addons.productaddons	ERROR	Request failed certificate checks: [Exception... "Certificate checks failed. See previous errors for details."  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://gre/modules/CertUtils.jsm :: validateCert :: line 109"  data: no]
That looks like your certificate simply cannot be validated, which is not the issue covered by this bug.  More information about troubleshooting certificate validation errors is at:
https://support.mozilla.org/en-US/kb/secure-connection-failed-error-message
Andrew, I would argue that this is precisely the bug discussed - these Zscaler MITM corporate certificates otherwise work, in other contexts I don't get any error, just when performing add-on updates are these messages shown. Basically I assume that add-on updater expects that your server has certificates from certain hard-coded issuers and checks issuerName attribute against some list and since in corporate setup we have, Zscaler proxy poses as issuer and CA for  add-on server as well and therefore I get these errors.
I don't have either of those options in About:config.
But when I added both of them, and set them to false, it worked!

Which doesn't help all of those people behind a corporate firewall who have no idea that their addons aren't updating, because the application doesn't tell them that - it just says "No updates found"...
(In reply to Ladislav Furman from comment #40)
> Basically I assume that add-on updater expects that your server has
> certificates from certain hard-coded issuers and checks issuerName attribute
> against some list and since in corporate setup we have, Zscaler proxy poses
> as issuer and CA for  add-on server as well and therefore I get these errors.

That's not exactly how it works but any errors you see after setting extensions.update.requireBuiltInCerts to false are unrelated to this bug.  Feel free to open a separate bug if you think there is one.
Ah yes, this was caused by different issue - media.gmp-manager.cert.requireBuiltIn wasn't set to false; after changing that, these messages disappeared. Now I only see messages about pinning, which might be different (although possibly related)issue;messages are in Slovak:

Public-Key-Pins: stránka špecifikovala hlavičku, ktorá neobsahuje zhodný pin.[Ďalšie informácie] VersionCheck.php
Public-Key-Pins: stránka špecifikovala hlavičku, ktorá neobsahuje zhodný pin.[Ďalšie informácie] VersionCheck.php
Public-Key-Pins: stránka špecifikovala hlavičku, ktorá neobsahuje zhodný pin.[Ďalšie informácie] VersionCheck.php
Public-Key-Pins: stránka špecifikovala hlavičku, ktorá neobsahuje zhodný pin.[Ďalšie informácie] VersionCheck.php
Public-Key-Pins: stránka špecifikovala hlavičku, ktorá neobsahuje zhodný pin.[Ďalšie informácie] VersionCheck.php
Public-Key-Pins: stránka špecifikovala hlavičku, ktorá neobsahuje zhodný pin.[Ďalšie informácie] VersionCheck.php
Public-Key-Pins: stránka špecifikovala hlavičku, ktorá neobsahuje zhodný pin.[Ďalšie informácie] VersionCheck.php
Public-Key-Pins: stránka špecifikovala hlavičku, ktorá neobsahuje zhodný pin.[Ďalšie informácie] VersionCheck.php
Public-Key-Pins: stránka špecifikovala hlavičku, ktorá neobsahuje zhodný pin.[Ďalšie informácie] VersionCheck.php
Public-Key-Pins: stránka špecifikovala hlavičku, ktorá neobsahuje zhodný pin.[Ďalšie informácie] VersionCheck.php
Public-Key-Pins: stránka špecifikovala hlavičku, ktorá neobsahuje zhodný pin.[Ďalšie informácie] VersionCheck.php
Public-Key-Pins: stránka špecifikovala hlavičku, ktorá neobsahuje zhodný pin.[Ďalšie informácie] VersionCheck.php
Public-Key-Pins: stránka špecifikovala hlavičku, ktorá neobsahuje zhodný pin.[Ďalšie informácie] VersionCheck.php
[Zobraziť/skryť podrobnosti o správe.] 1538482447075	addons.productaddons	ERROR	Request failed certificate checks: [Exception... "Certificate issuer is not built-in."  nsresult: "0x80004004 (NS_ERROR_ABORT)"  location: "JS frame :: resource://gre/modules/CertUtils.jsm :: checkCert :: line 167"  data: no] Log.jsm:853
[Zobraziť/skryť podrobnosti o správe.] 1538482447106	addons.productaddons	ERROR	Request failed certificate checks: [Exception... "Certificate issuer is not built-in."  nsresult: "0x80004004 (NS_ERROR_ABORT)"  location: "JS frame :: resource://gre/modules/CertUtils.jsm :: checkCert :: line 167"  data: no] Log.jsm:853

Not sure whether last ones are not related to those DRM addons from google, though as these are only 2 of them.
You need to log in before you can comment on or make changes to this bug.