1714621 - remove cert issuer pinning from GMP update system

Status: RESOLVED FIXED

(Core :: Audio/Video: GMP, enhancement, P1)

Description

[[e-mail] (:bhearsum)]

As part of GMP updates we hard-pin the Issuer of the aus5.mozilla.org certificate (see https://searchfox.org/mozilla-central/source/modules/libpref/init/all.js#4175) to a specific value.

This is not a great way of doing things, because we have no control (or even notifications) of what certificate our own get issued against. It caused a minor panic today, when we thought we were unable to to get a new, compatible-with-our-pinning-requirements, certificate for aus5.mozilla.org.

We removed this pinning long ago for Firefox in favour of MAR signatures. More recently, we've used content signatures for MozillaVPN updates -- where we sign the HTTP response, and verify that with the client. This is probably a good option for GMP, as signing the actual update packages is not possible.

The new certificate expires early in July next year, and I think we should try to deal with this well ahead of that, to make sure most of our users are not longer pinning.

Comment 1

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Assigning to me so we have eyes on this. The media team is currently thinking about some of the architecture around GMP and I'll make sure this is is included in discussions.

Comment 2

[[e-mail] (:bhearsum)]

I was thinking about this a bit more today and realized that we should probably get moving on it ASAP. The current aus5 cert expires in July 2022, and it is extremely unlikely we'll be able to get another cert that works with our pinning. This means that any version of Firefox still requiring a specific cert will no longer be able to get GMP updates. Even if we shipped this today on Nightly, it means that no version older than 91.0 would be capable of receiving GMP updates next July.

Comment 3

[Jim Mathies [:jimm]]

Hey Ben, can we get some additional detail on what's required here? Not sure what we need to change to address the issue.

Comment 4

[[e-mail] (:bhearsum)]

(In reply to Jim Mathies [:jimm] from comment #3)

Hey Ben, can we get some additional detail on what's required here? Not sure what we need to change to address the issue.

IMO, we need to remove the current certificate pinning that we use, which breaks periodically when our certificate provides change, or are unable to provide us with compatible certs (we usually have to control or notification of such events, until we renew the cert).

If we do remove the cert pinning, we need to replace it with something else that provides us with a guarantee that what the GMP update client is receiving is truly from us. My suggestion is to use HTTP content signatures for this -- where the body of the HTTP response is signed, and a signature is returned as a header. The GMP client will need to be taught how to verify such signatures for this to work. This is a model we already use for Mozilla VPN updates, for example, so the plumbing largely already exists.

There may be some other options as well, and we should consult with the security team before moving forward (I'm going to send them some mail about it).

Comment 5

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

So for changes on the Firefox side we'd need to teach the toolkit code to do the verification (the GMP client code), is that right?

Comment 6

[[e-mail] (:bhearsum)]

(In reply to Bryce Seager van Dyk (:bryce) from comment #5)

So for changes on the Firefox side we'd need to teach the toolkit code to do the verification (the GMP client code), is that right?

That's my understanding, yeah. There's some code in Firefox that does this already (for Kinto IIRC).

Comment 8

[[e-mail] (:bhearsum)]

We got signoff from Tom Ritter and Dan Veditz to proceed with this. Dan also urged us to backport the solution to ESR91, since that will be supported past the expiration date of the current cert.

For the Balrog side, there's no harm in starting the sign the responses before the client is ready, so I've opened https://github.com/mozilla-releng/balrog/issues/2106 and given a heads-up to the Autograph folks.

Comment 9

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Trying to get a handle on how this is going to look on the toolkit side. Gathering info at the moment. Notes for me in future

• a permalink for the link in comment 0 - https://searchfox.org/mozilla-central/rev/7543ddeb51415cb3f59dacf9e6c88dd79b0e3f37/modules/libpref/init/all.js#4175
• Content signature readme from autograph github

Comment 10

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Remote settings code that validates signatures -- something to reference for wrangling this in GMP toolkit code.

Comment 11

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Looking at this now, think I've got a fairly good handle on most of what needs to change in toolkit.

My plan is to migrate from the current path to the new code over time and with telemetry to monitor that the new code is working. I think there's some code we can yank once we're sure the new code is working, but I'd prefer to have the old code to fall back to until we have verification the new stuff is working.

:bhearsum, my understanding is that we want to drop the pinning requirements tied to the values associated with certain prefs[0]. There's also a requirement that certs are built in, even when not pinned[1]. Do we wish to drop that latter requirement also, or keep it? My, limited, understanding is that the content signature should protect us even if we drop that requirement also (because even then an attacker can't forge the signature). Is that correct?

[0] https://searchfox.org/mozilla-central/rev/d10188f9b4f1c4974264f3925505a0498d346c57/modules/libpref/init/all.js#4162-4166
[1] https://searchfox.org/mozilla-central/rev/d10188f9b4f1c4974264f3925505a0498d346c57/modules/libpref/init/all.js#4144

Comment 12

[[e-mail] (:bhearsum)]

(In reply to Bryce Seager van Dyk (:bryce) from comment #11)

:bhearsum, my understanding is that we want to drop the pinning requirements tied to the values associated with certain prefs[0]. There's also a requirement that certs are built in, even when not pinned[1]. Do we wish to drop that latter requirement also, or keep it? My, limited, understanding is that the content signature should protect us even if we drop that requirement also (because even then an attacker can't forge the signature). Is that correct?

That's my understanding as well, but I think I'll have to defer to the security team on whether or not we should require the cert to be built in. (It seems like a good idea to me still, but there may be something I'm missing.)

Comment 13

[Daniel Veditz [:dveditz]]

If the signed-content mechanism is done correctly, please drop all cert-pinning as it will only get in the way.

Our ancient roll-our-own "cert-pinning" code was nothing like HKPK that pins to an actual specific certificate, but rather approximates that through matching attribute strings in the cert itself. That makes it trivially easy to bypass with any kind of MITM since they just issue any old cert with those strings in it, thus the "require builtins" pref. Even then the "pin" is still trivially by-passable by a MITM, but at least it's got to be a mis-issued/fraudulent intermediate from an approved, presumably well-run, CA. But a very large percentage of folks talk to us through intentional MITM such as anti-virus or enterprise security proxies. Set the bit and we break lots and lots of people; don't set it and we're not offering much protection from the problem we're trying to solve!

The requireBuiltIn check does nothing unless checkAttributes is true. Just use content signatures and get rid of all that.

Now the question is what do you sign? Ideally we'd sign the Balrog response. If we can trust that then we know we're downloading the intended update, and can use the hash in that response to validate the downloaded GMP plugin. Since the PR in comment 8 is complete I can see that we do, indeed, sign the Balrog response. e.g. https://aus5.mozilla.org/update/3/GMP/92.0.1/20210922161155/Darwin_x86_64-gcc3/en-US/release/Darwin%2020.6.0/default/default/update.xml

Note: That's not what Firefox updates do since that was implemented prior to our content-signature infrastructure. They sign the .MAR files themselves. That means we are confident the .MAR files are legitimate, but because the Balrog connection could be tampered with by a MITM we can't be sure it's the intended update. The installer has to do extra checks to prevent spoofing attacks, such as making sure it's not a lower version number and that the update is for the right product, platform, channel and that kind of thing. It can do that because that kind of information can be embedded in the signed .MAR archive.

Comment 14

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

(In reply to Daniel Veditz [:dveditz] from comment #13)

Snip

Great, thank you for the knowledge. I'll only check the content signature for the new code path.

---

I'm going to be away from my normal dev setup for the rest of this week. Pushing my WIP in case I get a chance to work on it while I'm away, otherwise I'll return to this early next week.

Comment 15

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1714621 - Add functionality to verify GMP's update xml content signatures. r?gijs

Also driveby fix a log string to print the proper function name.

Comment 16

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1714621 - Add telemetry to record if GMP update XML is successfully downloaded from balrog. r?gijs

Depends on D127566

Comment 17

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Data collection review request

Comment 18

[Chris H-C :chutten]

Comment on attachment 9245549 [details]
Data collection review request

DATA COLLECTION REVIEW RESPONSE:

Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes.

Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

If the request is for permanent data collection, is there someone who will monitor the data over time?

No. This collection will expire in Firefox 100.

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

Is the data collection request for default-on or default-off?

Default on for all channels.

Does the instrumentation include the addition of any new identifiers?

No.

Is the data collection covered by the existing Firefox privacy notice?

Yes.

Does the data collection use a third-party collection tool?

No.

---

Result: datareview+

Comment 19

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

For posterity + bus factor -- if we fail to download the update.xml from balrog, we'll fall back to sources hard coded into the browser (GMPInstallManager handles this).

This needs to be kept in mind when testing, as it can hide failures (the worst case scenario is we may think we have something working, when it's just the fallback mechanism doing the download). We don't want to rely on the fallback mechanism because then we can no longer ship updates via balrog, they'll just be hardcoded from the json source.

I've been mindful of this while testing my current patches, but wanted to flag it here as it's not immediately obvious that it will happen.

Comment 20

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1714621 - Add tests to cover ProductAddonChecker content signature verification. r?gijs

Depends on D127566

Comment 21

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1714621 - Add test case to cover GMPInstallerManager content signature usage. r?gijs

This tests the new content signature path for the GMPInstaller manager. It also
tests the telemetry gathered for the new content signature path works correctly
when that path succeeds or fails validation.

These tests differ from the existing tests in that they use a live HTTPServer,
rather than using the XML override test mechanism. This is needed to avoid
adding further test specific mechanisms for content signature, but is also cool
IMO as it makes the test cases a little more realistic (at the cost of some
verbosity).

Depends on D127567

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/753d457df764
Add functionality to verify GMP's update xml content signatures. r=Gijs,robwu
https://hg.mozilla.org/integration/autoland/rev/5eaba5f37b3f
Add tests to cover ProductAddonChecker content signature verification. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/5791dc3eaa95
Add telemetry to record if GMP update XML is successfully downloaded from balrog. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/700c2785dba6
Add test case to cover GMPInstallerManager content signature usage. r=Gijs

Comment 23

[Narcis Beleuzu [:NarcisB]]

Backed out for xpcshell failures on test_GMPInstallManager.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/1750da2d7f9db490b9d15b3ee696e89e6aa68cb7
Log link: https://treeherder.mozilla.org/logviewer?job_id=356147622&repo=autoland&lineNumber=3160

Comment 24

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Thanks for NI. Think I see what's going on -- the test that failed is checking that we get the expected number of GMPs from hardcoded data when we fail to download GMPs from balrog (or the test server in this case). The expected number of GMPs is 2 for most platforms, but we don't code fallbacks for all platforms, so it's not always 2.

Let me relax the test to accommodate this + try push to all platforms before I reland.

Comment 25

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Current try runs look good, let's try this again.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/885963ade64f
Add functionality to verify GMP's update xml content signatures. r=Gijs,robwu
https://hg.mozilla.org/integration/autoland/rev/97f1c562aa06
Add tests to cover ProductAddonChecker content signature verification. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/f8b03c0d4ea2
Add telemetry to record if GMP update XML is successfully downloaded from balrog. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/920e03e506ab
Add test case to cover GMPInstallerManager content signature usage. r=Gijs

Comment 27

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/885963ade64f
https://hg.mozilla.org/mozilla-central/rev/97f1c562aa06
https://hg.mozilla.org/mozilla-central/rev/f8b03c0d4ea2
https://hg.mozilla.org/mozilla-central/rev/920e03e506ab

Comment 28

[:Gijs (he/him)]

(In reply to [e-mail] (:bhearsum) from comment #0)

The new certificate expires early in July next year, and I think we should try to deal with this well ahead of that, to make sure most of our users are not longer pinning.

Do we need to do something about esr91? The fix here landed for 95, but esr91 won't be EOL until well after July.

Comment 29

[[e-mail] (:bhearsum)]

(In reply to :Gijs (he/him) from comment #28)

(In reply to [e-mail] (:bhearsum) from comment #0)

The new certificate expires early in July next year, and I think we should try to deal with this well ahead of that, to make sure most of our users are not longer pinning.

Do we need to do something about esr91? The fix here landed for 95, but esr91 won't be EOL until well after July.

Yes -- up in https://bugzilla.mozilla.org/show_bug.cgi?id=1714621#c8 we talked about this a bit, and my hope was that this would be backported there, and I still strongly urge this if it's not too much of a pain.

If we can't backport for some reason, RelEng (probably gbrown or jcristau) and CloudOps will need to keep this in mind when dealing with the cert renewal.

Comment 30

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

I'll request uplift. I'd hoped to land something in bug 1739664, but I've been away for a bit and now other things are taking my attention. The telemetry we have indicates this new method isn't failing (significantly) more than the old method of verifying updates. While I still have some questions I want to try and answer around why GMP updates fail, it makes sense to land these changes in ESR (we'll need to flip the pref there to use the new method, but that should be easy enough).

Comment 31

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 9244378 [details]
Bug 1714621 - Add functionality to verify GMP's update xml content signatures. r?gijs

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Uplift of these patches allows us to avoid an incoming issue where GMP updates will fail if we can't get a new cert that meets certain requirements. I.e. Widevine + openh264 updates could become busted, which would break premium media + certain web RTC functionality.
• User impact if declined: If we can't get a suitable cert -- loss of premium media playback functionality + loss of webRTC functionality.
• Fix Landed on Version: 95
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch as written should be low risk as the new functionality is behind a pref. I.e. the patch changes very little behaviour without the pref being flipped. Considering we may flip the pref later, the risk is low - medium. That will be a functionality change, but it's one we've had bake in nightly + early beta for some time now.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9244378 [details]
Bug 1714621 - Add functionality to verify GMP's update xml content signatures. r?gijs

Basically a no-op at the moment since the majority of the functionality is behind an early_beta flag, but it sounds like we'll probably end up needing this down the road later on in the ESR lifecycle. Approved for 91.7esr.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/7572a9afdfe3
https://hg.mozilla.org/releases/mozilla-esr91/rev/da6a1aee9f95
https://hg.mozilla.org/releases/mozilla-esr91/rev/1834ba383ee0
https://hg.mozilla.org/releases/mozilla-esr91/rev/aa653d9f3a77