Closed Bug 1308251 (CVE-2020-12421) Opened 4 years ago Closed 2 months ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla79
Tracking Status
firefox-esr68 78+ verified
firefox-esr78 78+ verified
firefox54 + wontfix
firefox55 + wontfix
firefox56 --- wontfix
firefox77 --- wontfix
firefox78 + verified
firefox79 + verified

People

(Reporter: chuck, Assigned: mixedpuppy)

References

(Depends on 2 open bugs, Blocks 1 open bug, )

Details

(Keywords: sec-moderate, Whiteboard: [STR in comment 67][adv-main78+][adv-esr68.10+])

Attachments

(5 files, 2 obsolete files)

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.
Duplicate of this bug: 1641482

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?

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

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.

Severity: normal → S2
Keywords: sec-high
Summary: Remove built-in certificate requirement for addon updates → Remove built-in certificate requirement for addon updates (no security updates for corporate or AntiVirus users)

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

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]

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.

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

Keywords: sec-highsec-moderate

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

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.

Assignee: nobody → mixedpuppy
Status: NEW → ASSIGNED
Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c8be39c5c14d
relax builtincert requirement if we require signed extensions r=mossop,dveditz

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
Attachment #9157804 - Flags: approval-mozilla-beta?
Attachment #9157805 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Attachment #9157805 - Flags: approval-mozilla-beta?

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

Approved for 78.0b9.

Attachment #9157804 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9157805 - Attachment is obsolete: true

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

Per Slack discussion, approving for 68.10esr also.

Attachment #9157804 - Flags: approval-mozilla-esr68+
Pushed by rvandermeulen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9df493e7b89e
fix builtin cert testing for addon install and update r=RyanVM
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla79

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.

Has STR: --- → yes
Whiteboard: STR in comment 67
QA Whiteboard: [qa-triaged]

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.

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.

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

Blocks: 1647360

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.

Attachment #9158228 - Attachment is obsolete: true

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:
Attachment #9158210 - Flags: approval-mozilla-esr78?
Attachment #9158210 - Flags: approval-mozilla-esr68?

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.

Attachment #9158210 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

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

Approved for 78.0esr.

Attachment #9158210 - Flags: approval-mozilla-esr78? → approval-mozilla-esr78+
Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d01bc63942f3
fix builtin cert setting for esr r=dveditz
Whiteboard: STR in comment 67 → [STR in comment 67][adv-main78+]
Whiteboard: [STR in comment 67][adv-main78+] → [STR in comment 67][adv-main78+][adv-esr68.10+]

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.

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

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

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
Depends on: 1648815
You need to log in before you can comment on or make changes to this bug.