Closed Bug 1769669 Opened 2 years ago Closed 2 years ago

Signature verifier relies on preference for hash of root certificate

Categories

(Core :: Security: PSM, enhancement)

enhancement

Tracking

()

RESOLVED FIXED
103 Branch
Tracking Status
firefox-esr91 --- wontfix
firefox-esr102 --- fixed
firefox101 --- wontfix
firefox102 --- fixed
firefox103 --- fixed

People

(Reporter: leplatrem, Assigned: keeler)

References

(Blocks 1 open bug)

Details

(Keywords: sec-want, Whiteboard: [psm-assigned][adv-main102-])

Attachments

(4 files)

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.

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

See Also: → 1729400
Group: core-security → crypto-core-security

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

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

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?

Flags: needinfo?(mathieu)

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.

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

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?

Flags: needinfo?(mathieu)
Group: mozilla-employee-confidential
Keywords: sec-want

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)

Flags: needinfo?(mathieu)

Gotcha, that works for us! Thanks.

Flags: needinfo?(mathieu)

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;
+  },
+
Flags: needinfo?(dkeeler)

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

Flags: needinfo?(dkeeler)

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:

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.

Assignee: nobody → dkeeler
Attachment #9277012 - Attachment description: WIP: Bug 1769669 - move app signature verification to security/manager/ssl/ r?jschanck → Bug 1769669 - move app signature verification to security/manager/ssl/ r?jschanck
Status: NEW → ASSIGNED
Attachment #9277013 - Attachment description: WIP: Bug 1769669 - require specifying the trusted root in content signature verifier r?jschanck → Bug 1769669 - require specifying the trusted root in content signature verifier r?jschanck!,leplatrem!,Gijs!,robwu!
Whiteboard: [psm-assigned]
Severity: -- → N/A
Attachment #9277013 - Attachment description: Bug 1769669 - require specifying the trusted root in content signature verifier r?jschanck!,leplatrem!,Gijs!,robwu! → Bug 1769669 - require specifying the trusted root in content signature verifier r?jschanck!,leplatrem!,barret!,robwu!
See Also: → 1771992
Group: crypto-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch

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

Flags: needinfo?(dkeeler)

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.

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 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
Flags: needinfo?(dkeeler)
Attachment #9279916 - Flags: approval-mozilla-beta?
Attachment #9279917 - Flags: approval-mozilla-beta?

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.

Attachment #9279916 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9279917 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [psm-assigned] → [psm-assigned][adv-main102-]
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-
Group: core-security-release
Blocks: 1846866
You need to log in before you can comment on or make changes to this bug.