1308251 - (CVE-2020-12421) Remove built-in certificate requirement for addon updates (no security updates for corporate or AntiVirus users)

Status: VERIFIED FIXED

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

Description

[Chuck Harmston [:chuck]]

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

Comment 1

[Julien Vehent]

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.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

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?

Comment 4

[Kris Maglione [:kmag]]

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.

Comment 5

[Kris Maglione [:kmag]]

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

Comment 6

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 7

[Kris Maglione [:kmag]]

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.

Comment 8

[Julien Vehent]

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

Comment 9

[Kris Maglione [:kmag]]

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.

Comment 10

[Julien Vehent]

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.

Comment 11

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

We may want to track this bug at least for 52, if this is something we hope to fix in that time frame.

Comment 12

[Marcia Knous [:marcia]]

Tracking 52+ - as Liz says in Comment 11, so we don't lose track of it for 52.

Comment 13

[Robert Helmer [: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.

Comment 14

[Robert Helmer [:rhelmer]]

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?

Comment 15

[Kris Maglione [:kmag]]

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.

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler]]

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

Comment 17

[Robert Strong (they/them - no direct email)]

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.

Comment 18

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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.

Comment 19

[Daniel Veditz [:dveditz]]

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.

Comment 20

[Selena Deckelmann :selenamarie :selena]

From conversations yesterday with :Mossop, it seems like this is relevant to some work :rhelmer is currently doing (unsure which bug).

Comment 21

[Robert Helmer [: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.

Comment 22

[Daniel Veditz [:dveditz]]

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

Comment 23

[Robert Helmer [:rhelmer]]

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

Comment 24

[Julien Cristau [:jcristau]]

Moving the tracking flag, this doesn't look like it's getting fixed in the 52 time frame.

Comment 25

[Gerry Chang [:gchang]]

Mark 54 fix-optional as there are no actions for the moment and carry tracking flags to 55.

Comment 26

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

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?

Comment 27

[Robert Helmer [:rhelmer]]

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

Comment 28

[Andrew Ducker]

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?

Comment 29

[Andrew Swan [:aswan]]

Updating title in light of comment 19 (which meant to reference bug 1303418)

Comment 30

[Romain Testard [:RT]]

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

Comment 31

[Andrew Ducker]

I set security.enterprise_roots.enabled to true, did a restart, did a "check for updates", and still got "No updates found".

Comment 32

[Romain Testard [:RT]]

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?

Comment 33

[Michael Cooper [:mythmon]]

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.

Comment 35

[Robert Helmer [:rhelmer]]

Not actively working on this, but can help out if it is actively blocking anyone.

Comment 36

[Ladislav Furman]

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?

Comment 37

[Andrew Swan [:aswan]]

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

Comment 38

[Ladislav Furman]

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]

Comment 39

[Andrew Swan [:aswan]]

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

Comment 40

[Ladislav Furman]

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.

Comment 41

[Andrew Ducker]

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

Comment 42

[Andrew Swan [:aswan]]

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

Comment 43

[Ladislav Furman]

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.

Comment 45

[yxu]

hi, I want to know what progress this bug has made recently. If we can’t stop man-in-the-middle for the time being, can we add a prompt like the https web page error to inform the user what happened?

Comment 46

[rhardy]

Please consider retagging this as the critical security vulnerability this really is.
SSL interception is not a rare configuration in this day and age.
The current configuration is Firefox is able to update itself without issue, however all addins updates, including critical security updates, are disabled by this bug.
Worse than this it does NOT give an error. It simply says No updates available, which is wrong.
To make matters worse for end users there is no obvious error that communication isn't working unless the user notices:
hey! Why are my addins years out of date!?

Comment 47

[Daniel Veditz [:dveditz]]

Short term fix -- set the following prefs to false (you will need to add them in about:config):
extensions.update.requireBuiltInCerts
extensions.install.requireBuiltInCerts

The first is the main one. The second is for web pages that use InstallTrigger to install extensions (which isn't how https://addons.mozilla.org does it anymore).

Since enterprise users are extremely likely to have a corporate MITM security device this is important to fix in ESR-78. Although a better fix is to remove this code, for ESR we could settle for simply setting the prefs.

Comment 48

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Fx78 goes to RC next week, so it's very unlikely we'll have a fix for this 4 year old bug in time to make that release. We can keep this on the radar for the next cycle, however (i.e. Fx79/78.1esr).

Comment 49

[rhardy]

Please reconsider changing this back from wontfix to something sane. This has a critical impact to corporate, government and military users. Failure to fix this may result in Firefox being banned from networks because it is broken so badly that it currently Firefox cannot apply ANY addon security updates. Normally a bug which outright blocks security updates should BLOCK future releases until it is fixed.
It did not make sense to me to have bug 1493113 be marked a duplicate of this bug and closed.
That should have been a critical security bug that was fixed quickly and separately from all the rest of this.

The ongoing mishandling of this bug is embarrassing. Treating security as optional again is breaking Mozilla Manifesto Principle 4:
“Individuals’ security and privacy on the Internet are fundamental and must not be treated as optional.”

Yes there are all sorts of extra longer term related projects that can be done to make this work better, improve security and implement signed updates etc. but leaving Firefox in a state where some users cannot get ANY addon security updates and are not even warned about it is simply choosing to ignore user security which breaks your own Manifesto.

The minimum critical change is small i.e. change the default value of an existing variable so this broken behaviour isn't on by default. It would be ideal if the addons website also allowed upgrades of out of date addons which it currently does NOT in environments where SSL interception is at play. The change there for minimum operation of addon patching is likely small too.

Comment 50

[Daniel Veditz [:dveditz]]

Since just the pref-flip would be acceptable for 78 I don't want to give up on this yet without talking to the WebExtensions team. Better to get that in than have to ship a Normandy update later (which, due to this bug, won't reach the affected people!). [updated: Normandy uses a completely separate mechanism and isn't affected]

Comment 52

[Andrew Swan [:aswan]]

Better to get that in than have to ship a Normandy update later (which, due to this bug, won't reach the affected people!).

Is that true? Normandy has its own installation and update paths that don't overlap with the "regular" install/update paths. If the Normandy implementation has a similar issue, that should probably be tracked in a separate bug.

Comment 53

[Daniel Veditz [:dveditz]]

Our blocklist should nix the dangerously vulnerable versions of addons and others will affect a smaller audience. This could not be used as a mass attack vector so sec-moderate seems more appropriate by our guidelines

Comment 54

[Daniel Veditz [:dveditz]]

(In reply to Andrew Swan [:aswan] from comment #52)

Better to get that in than have to ship a Normandy update later (which, due to this bug, won't reach the affected people!).

Is that true? Normandy has its own installation and update paths that don't overlap with the "regular" install/update paths.

I was definitely wrong about the installation path. I thought Normandy-installed addons used the normal addon update mechanism, but of course if we did a pref flip the problem would be resolved anyway (I was thinking of Test-Pilot, which did have its installs fail but that is because it was a web page using InstallTrigger and very different from Normandy).

Comment 55

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1308251 relax builtincert requirement if we require signed extensions

This changes the default for requiring builtin certs for extension install and update if we also
require signed extensions. For builds that allow unsigned extensions, the default still requires builtin certs.

Comment 56

[Andrew Swan [:aswan]]

Attachment: Bug 1308251 Remove extensions.{install,update}.requireBuiltinCerts prefs

Comment 57

[Shane Caraveo (:mixedpuppy)]

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

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c8be39c5c14d
relax builtincert requirement if we require signed extensions r=mossop,dveditz

Comment 59

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9157804 [details]
Bug 1308251 relax builtincert requirement if we require signed extensions

Beta/Release Uplift Approval Request

• User impact if declined: users with 3rd party certs installed will fail to get addon updates
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: will post in bug
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch changes the default for a hidden pref in a way that a) preserves any user-set value, b) keeps the old default for builds that do not require addon signing (dev-ed, thunderbird, unbranded, etc).

A number of tests already tested with these prefs explicitly turned off and have done so for a long time.

Turning the pref off skips the builtin cert requirement, but certs must still be valid.

• String changes made/needed: n/a

Comment 60

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Comment on attachment 9157804 [details]
Bug 1308251 relax builtincert requirement if we require signed extensions

Approved for 78.0b9.

Comment 61

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e2aa80644a3b

Comment 62

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Comment on attachment 9157804 [details]
Bug 1308251 relax builtincert requirement if we require signed extensions

Per Slack discussion, approving for 68.10esr also.

Comment 63

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/e73dd77bc3b404b7875e0cc3f2728aba89e86dbe

Comment 64

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1308251 fix builtin cert testing for addon install and update

Comment 65

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9df493e7b89e
fix builtin cert testing for addon install and update r=RyanVM

Comment 66

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/c8be39c5c14d

Comment 67

[Daniel Veditz [:dveditz]]

How to test:
This will need a setup much like that used to test the "enterprise roots" feature (bug 1513069): an intercepting proxy with a CA cert installed in the OS root store. If you already have a test environment with a proxy like ZAP that would work, or you could install an antivirus with an "HTTPS scanning" feature. One I know about is Avast Web Shield.

1. turn on the intercepting proxy.
   1.a Make sure https: sites still work and
   1.b that when you look at their certificates they are issued by the proxy and are not the ones from the site.
2. install an out-of-date version of Facebook container
3. open about:addons
4. click the Gear button and "Check for updates"

In an unfixed version you will be told "No updates found" (rather than an error -- see bug 1556718)

In the fixed version your Facebook Container should be updated to the most recent version.

Update 2020-06-23: According to https://zakird.com/papers/https_interception.pdf Avast only intercepts IE on windows, but should intercept Firefox on Mac according to Fig. 4. That paper is several years old now, though, so some products may have changed. Others popular ones that were known to have an "HTTPS scanning" feature are Bitdefender, Kaspersky, ESET, McAfee, and Norton. Their sites still have support pages on how to disable it so I assume they all still do.

Comment 68

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f57ce5567163

Comment 69

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/9df493e7b89e

Comment 70

[rhardy]

Thank you.
I'm confirming our testing shows this both fixes the inability to check for security updates to add-ons and they successfully install when it finds one out of date.

Comment 71

[rhardy]

There was one anomaly with the work around. On a client which used Lastpass for personal passwords, that addon was actually partially uninstalled without removing the user data when the patching flood gates opened when those about:config options were added. By this I mean the addon disappeared from the addons list and the icon from the tool bar was gone. Re-adding it from addons.mozila.org resolved that issue without losing the existing data.

Comment 72

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1308251 fix builtin cert setting for esr

Comment 73

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

ESR wasn't fixed by the patch that landed. Updating the status accordingly.

Comment 74

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1308251 consolidate logic for requiring builtin certs for addon install/update

Comment 75

[Phabricator Automation]

Comment on attachment 9158228 [details]
Bug 1308251 consolidate logic for requiring builtin certs for addon install/update

Revision D80488 was moved to bug 1647360. Setting attachment 9158228 [details] to obsolete.

Comment 76

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9158210 [details]
Bug 1308251 fix builtin cert setting for esr

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: addon updates/installs will require builtin certs where ESR organizations may be using other certs.
• User impact if declined: no Addon updates
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is adding an additional check specific to esr to correct the default value of a pref read.
• String or UUID changes made by this patch:

Comment 77

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Comment on attachment 9158210 [details]
Bug 1308251 fix builtin cert setting for esr

Approved for 68.10esr. Temporarily holding off on the ESR78 approval until the branch is ready for uplifts.

Comment 78

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Comment on attachment 9158210 [details]
Bug 1308251 fix builtin cert setting for esr

Approved for 78.0esr.

Comment 79

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/20bf65924b20

Comment 80

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d01bc63942f3
fix builtin cert setting for esr r=dveditz

Comment 81

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/d58f002e6dcf
https://hg.mozilla.org/releases/mozilla-esr68/rev/6dd69e9725f3

Comment 82

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/d01bc63942f3

Comment 83

[Tom Ritter [:tjr] (OOTO until mid-August)]

Attachment: advisory.txt

Comment 84

[Alin Ilea, Desktop QA]

Hi,

I have managed to reproduce the initial issue using an old Nightly build 77.0a1 (build id: 20200503093615), Beta 78.0b7, Release version 77.0.1 and ESR 68.9.0 following the steps from comment 67, setting a proxy in Firefox settings and using ESET with HTTPS scanning feature enabled. "No updates found" is displayed when trying to update an older version of the Facebook Container add-on by clicking on "Check for updates".
The issue is verified - Fixed in 68.10.0 ESR (build id: 20200622191537), 78.0 ESR (build id: 20200623021425) and 78.0 RC (build id: 20200622230509). In all these versions the add-on is updated accordingly to the latest version followed by the message "Your add-ons have been updated".

BUT the issue still seems to be reproducible in latest Nightly 79.0a1 (build id: 20200624093107). Still "No updates found" after checking for updates, following the same steps.

Thanks and just let me know if any other information is needed.

Comment 85

[Julien Cristau [:jcristau]]

AIUI this result is expected for nightly, because it has MOZ_REQUIRE_SIGNING disabled.

Comment 86

[Alin Ilea, Desktop QA]

Based on comment 85, I will update the flag for Nightly as verified and change the status accordingly.
Thanks.