1769669 - Signature verifier relies on preference for hash of root certificate

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

[Mathieu Leplatre [:leplatrem]]

The signature verifier reads the security.content.signature.root_hash to get the hash of the root of the certificates chain.

Remote Settings data updates rely on this verifier, and can thus be "disabled" by changing the value of this pref.

In Bug 1702759, we are removing the ability to flip critical configuration from preferences on beta and release (tagged as [sec-want]).
We read from hardcoded value unless the environment variable MOZ_REMOTE_SETTINGS_DEVTOOLS is set at runtime (to be able to let our addon toggle stuff using priviledged code).

Should we apply the same logics and hardcode the root hash or does the security model assume that anything can be disabled/broken when preferences are owned?

Seeing there's a pref called security.turn_off_all_security_so_that_viruses_can_take_over_this_computer, we may be tilting at windmills here.

Comment 1

[Mathieu Leplatre [:leplatrem]]

Would it be reasonable to reset the critical preferences (clearUserPref()) if we detect that the Remote Settings synchronization keeps on failing?

Comment 2

[Andrew McCreight [:mccr8]]

(In reply to Mathieu Leplatre [:leplatrem] from comment #0)

Seeing there's a pref called security.turn_off_all_security_so_that_viruses_can_take_over_this_computer, we may be tilting at windmills here.

For what it is worth, I think if you set that pref to true, the browser will crash if we aren't in automation, so there's at least some hardening of it against abuse.

Comment 3

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

(In reply to Andrew McCreight [:mccr8] from comment #2)

For what it is worth, I think if you set that pref to true, the browser will crash if we aren't in automation, so there's at least some hardening of it against abuse.

I can't find that in the code but I could be missing something...

In general - where we can limit the bad things that can be done by flipping preference without impacting our workflow it would be great to do so. A past sandbox escape happened by flipping a pref in an inventive way. Blocking updates to the cert blocklist would be a high-value attack I think. So if we can easily remove this pref in favor of an environment variable and/or limiting its customization to pre-release branches that would be great.

Comment 4

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

We could make the content signature verifier work more like app (add-on) signature verification, where the roots are hard-coded and the caller specifies which one to use (prod, stage, testing, etc.). Would that work?

Comment 5

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

Attachment: Bug 1769669 - move app signature verification to security/manager/ssl/ r?jschanck

Before this patch, the app signature verification code lived in security/apps/.
The majority of the rest of PSM is in security/manager/ssl/ and there's little
reason to have that extra directory for the app signature verification
implementation alone.

Comment 6

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

Attachment: Bug 1769669 - require specifying the trusted root in content signature verifier r?jschanck!,leplatrem!,barret!,robwu!

Before this patch, the content signature verifier
(nsIContentSignatureVerifier/ContentSignatureVerifier) would identify the root
it trusted based on the value of a preference. This patch changes the
implementation to require a specified hard-coded root to trust as with app
signature verification.

Depends on D146644

Comment 7

[Mathieu Leplatre [:leplatrem]]

We could make the content signature verifier work more like app (add-on) signature verification, where the roots are hard-coded and the caller specifies which one to use (prod, stage, testing, etc.). Would that work?

In our DevTools addon, we would need a way to change from prod to stage.

I'm not familiar with the concept of app/add-on, and how we would call it, but using an environment "name" would work yes.
Could you show us pseudo-code please?

Comment 8

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

Right now, remote settings calls nsIContentSignatureVerifier.asyncVerifyContentSignature and relies on the value of the preference to determine the correct root certificate. The change I'm proposing is to pass in an identifier corresponding to a hard-coded root in the call to asyncVerifyContentSignature and to remove the preference. That way, remote settings can specify which root should be trusted, depending on if it's verifying information from prod vs. stage.

For comparison, addon signature verification works in a very similar way:
https://searchfox.org/mozilla-central/rev/4719c903af605c2caeda3bb1ce4edb9ca58bf68e/toolkit/mozapps/extensions/internal/XPIInstall.jsm#365
(the root parameter is specified here: https://searchfox.org/mozilla-central/rev/4719c903af605c2caeda3bb1ce4edb9ca58bf68e/toolkit/mozapps/extensions/internal/XPIInstall.jsm#252,257)

Comment 9

[Mathieu Leplatre [:leplatrem]]

Gotcha, that works for us! Thanks.

Comment 10

[Mathieu Leplatre [:leplatrem]]

Should I prepare a patch for the Remote Settings counter part or will you do it as part of this bug?

Basically, I'm thinking of adding an AppConstant with the prod root hash, and then rely on the same preference as before on Dev/Nightly.

This would roughly look like this:

diff --git a/services/settings/Utils.jsm b/services/settings/Utils.jsm
--- a/services/settings/Utils.jsm
+++ b/services/settings/Utils.jsm
@@ -106,6 +106,12 @@ var Utils = {
       : AppConstants.REMOTE_SETTINGS_SERVER_URL;
   },

+  get ROOT_HASH() {
+    return allowServerURLOverride
+      ? Services.prefs.getCharPref("security.content.signature.root_hash")
+      : AppConstants.REMOTE_SETTINGS_ROOT_HASH;
+  },
+

Comment 11

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

It would be great if you could prepare a patch. That said, we might not need to use the security.content.signature.root_hash preference or hash values at all. The way add-on signatures are verified, the root certificates are actually compiled into Firefox (see the .crt files at [0]). The API refers to them by the identifiers nsIX509CertDB.AddonsPublicRoot, nsIX509CertDB.AddonsStageRoot, etc. [1]. Presumably the content signature roots don't change often - would it be possible to compile them into FIrefox as well, and refer to them by identifiers such as nsIX509CertDB.ContentSignaturePublicRoot, nsIX509CertDB.ContentSignatureStageRoot, etc.?

[0] https://searchfox.org/mozilla-central/source/security/apps/
[1] https://searchfox.org/mozilla-central/rev/c6620104602decf1af7c6a9f78692426db6a5da2/security/manager/ssl/nsIX509CertDB.idl#255-257

Comment 12

[Mathieu Leplatre [:leplatrem]]

Ok. We just want to be able to switch the environment from the Dev Tools, so I believe we could deduce its name from the server URL, and use the appropriate nsIX509CertDB.ContentSignature${ENV}Root constant.

So far we have 4 environments:

• PROD
• STAGE
• DEV (hash root is same as STAGE)
• LOCAL (hash root comes from default Autograph configuration)

For any other unknown environment, I think it's acceptable to just disable signatures verification. And we can revisit this later if needed.

I'll prepare a patch in another ticket, we should be able to link them in phabricator.

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

move app signature verification to security/manager/ssl/ r=jschanck
https://hg.mozilla.org/integration/autoland/rev/0f24f538f61e0c0bc8afdc94140f2da9186cb24a
https://hg.mozilla.org/mozilla-central/rev/0f24f538f61e

require specifying the trusted root in content signature verifier r=jschanck,leplatrem,robwu,barret
https://hg.mozilla.org/integration/autoland/rev/42a09d35dd7b2df574e643ce91a8575a94307fd5
https://hg.mozilla.org/mozilla-central/rev/42a09d35dd7b

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Seems like something we may want to backport to Beta so it's in the next ESR release as well?

Comment 16

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

Attachment: Bug 1769669 - move app signature verification to security/manager/ssl/ (patch for beta) r?jschanck

Before this patch, the app signature verification code lived in security/apps/.
The majority of the rest of PSM is in security/manager/ssl/ and there's little
reason to have that extra directory for the app signature verification
implementation alone.

Comment 17

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

Attachment: Bug 1769669 - require specifying the trusted root in content signature verifier (patch for beta) r?jschanck!,leplatrem!,barret!,robwu!

Before this patch, the content signature verifier
(nsIContentSignatureVerifier/ContentSignatureVerifier) would identify the root
it trusted based on the value of a preference. This patch changes the
implementation to require a specified hard-coded root to trust as with add-on
signature verification.

Depends on D148428

Comment 18

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

Comment on attachment 9279916 [details]
Bug 1769669 - move app signature verification to security/manager/ssl/ (patch for beta) r?jschanck

Beta/Release Uplift Approval Request

• User impact if declined: Changing preferences could result in remote settings/normandy being disabled (e.g. if something malicious changed the affected preferences).
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This affects content signature verification, so I wouldn't say it's low-risk. That said, this changes the code to work like add-on signature verification, so there's precedent for this. Also, the tests seem reasonably comprehensive.
• String changes made/needed:
• Is Android affected?: No

Comment 19

[Pascal Chevrel:pascalc]

Comment on attachment 9279916 [details]
Bug 1769669 - move app signature verification to security/manager/ssl/ (patch for beta) r?jschanck

Approved for 102 beta 6, thanks.

Comment 20

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/4e6a8b7a1200
https://hg.mozilla.org/releases/mozilla-beta/rev/5317b48c84b9