Preload all known intermediate certificates for CAs in our root store

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P3
normal
RESOLVED FIXED
8 years ago
3 months ago

People

(Reporter: briansmith, Assigned: jcj)

Tracking

(Blocks 4 bugs, {perf})

Trunk
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

(Whiteboard: [psm-assigned])

Attachments

(2 attachments)

After bug 399045, our certificate chain validation behavior is basically non-deterministic. There are many (misconfigured) sites where the only factor in deciding if you will get a security warning is whether or not you've visited a properly-configured site with the same CA.

Fetching intermediate certificates (by default) per bug 399324 is an alternative, but it has significant performance, privacy, security (lots of DoS attack potential), and reliability problems.

Comment 1

8 years ago
A missing intermediate certificate represents an erroneously configured Web server, which in turn indicates a troubling lack of understanding by the site administrator of proper Web security.  I'm not sure implementation of this RFE would enhance security; it might even degrade security.  See <https://bugzilla.mozilla.org/show_bug.cgi?id=399324#c68>.
I don't think the request described in this bug is a good idea.

I don't like the idea of preloading an ever changing set of items.

We need libPKIX which can download missing certs on demand. Other browsers are doing the same thing. Automatic downloading might be inappropriate for certs received by email, but should be fine for surfing the web, IMO.

I vote to close this bug as WONTFIX.

Comment 3

8 years ago
It is fine by me to mark this bug WONTFIX.

Updated

8 years ago
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WONTFIX
I am reopening this because I still want to explore this idea further. Even if we do certificate fetching, it may be a good idea to preload some or all of the intermediate certificates for other reasons, such as performance and/or to enforce our (upcoming) intermediate certificate disclosure policies.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

Updated

4 years ago
Blocks: 1137484
Depends on: 1429795
Depends on: 1429796

Comment 8

a year ago
mozreview-review
Comment on attachment 8951285 [details]
Bug 657228 - Preload all known intermediate certificates in our certificate store

https://reviewboard.mozilla.org/r/220538/#review226494


Code analysis found 7 defects in this patch:
 - 7 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: services/common/blocklist-clients.js:441
(Diff revision 1)
> + *
> + * @param {Object} records   current records in the local db.
> + */
> +async function updateIntermediates(records) {
> +  let nsX509CertDB = "@mozilla.org/security/x509certdb;1";
> +  let nsIX509Cert = Ci.nsIX509Cert;

Error: 'nsix509cert' is assigned a value but never used. allowed unused vars must match /^cc|ci|cu|cr|exported_symbols/. [eslint: no-unused-vars]

::: services/common/blocklist-clients.js:452
(Diff revision 1)
> +    if (item.certData) {
> +      let data = item.certData;
> +
> +      // split off the header and footer, base64 decode, construct the cert
> +      // from the resulting DER data.
> +      let b64data = data.split('-----')[2].replace(/\s/g, '');

Error: Strings must use doublequote. [eslint: quotes]

::: services/common/blocklist-clients.js:452
(Diff revision 1)
> +    if (item.certData) {
> +      let data = item.certData;
> +
> +      // split off the header and footer, base64 decode, construct the cert
> +      // from the resulting DER data.
> +      let b64data = data.split('-----')[2].replace(/\s/g, '');

Error: Strings must use doublequote. [eslint: quotes]

::: services/common/blocklist-clients.js:467
(Diff revision 1)
> +      });
> +      let certVerifies = await promise;
> +
> +      // IF the cert verifies, add the certificate to the cert db
> +      if (true == certVerifies) {
> +        certdb.addCert(certDer,'C,,','NSS ignores nicknames');

Error: A space is required after ','. [eslint: comma-spacing]

::: services/common/blocklist-clients.js:467
(Diff revision 1)
> +      });
> +      let certVerifies = await promise;
> +
> +      // IF the cert verifies, add the certificate to the cert db
> +      if (true == certVerifies) {
> +        certdb.addCert(certDer,'C,,','NSS ignores nicknames');

Error: Strings must use doublequote. [eslint: quotes]

::: services/common/blocklist-clients.js:467
(Diff revision 1)
> +      });
> +      let certVerifies = await promise;
> +
> +      // IF the cert verifies, add the certificate to the cert db
> +      if (true == certVerifies) {
> +        certdb.addCert(certDer,'C,,','NSS ignores nicknames');

Error: A space is required after ','. [eslint: comma-spacing]

::: services/common/blocklist-clients.js:467
(Diff revision 1)
> +      });
> +      let certVerifies = await promise;
> +
> +      // IF the cert verifies, add the certificate to the cert db
> +      if (true == certVerifies) {
> +        certdb.addCert(certDer,'C,,','NSS ignores nicknames');

Error: Strings must use doublequote. [eslint: quotes]
Comment on attachment 8951285 [details]
Bug 657228 - Preload all known intermediate certificates in our certificate store

https://reviewboard.mozilla.org/r/220538/#review226568

Cool - this looks like the right thing to do, so definite f+.
My one reservation about all this is do we have any idea of the performance impact of having 100s (1000s?) of certificates on the certificate db itself? Might be good to look into before we turn this on full blast.

::: security/manager/ssl/tests/unit/test_intermediate_preloads.js:26
(Diff revision 1)
> +  return addCertFromFile(certdb, "test_intermediate_preloads/" + cert_filename,
> +                         trust_string);
> +}
> +
> +add_task(async function test_preload() {
> +  const dummyServerURL = `http://localhost:${server.identity.primaryPort}/v1`;

It feels like there's a fair bit of boilerplate in this test - can this be factored out? (assuming there's similar code elsewhere)

::: security/manager/ssl/tests/unit/test_intermediate_preloads/ca2.pem.certspec:1
(Diff revision 1)
> +issuer:ca2

If the test doesn't actually use ca2 (other than in the issuer field of int2), we probably don't have to include it at all.

::: services/common/blocklist-clients.js:440
(Diff revision 1)
> + * collection.
> + *
> + * @param {Object} records   current records in the local db.
> + */
> +async function updateIntermediates(records) {
> +  let nsX509CertDB = "@mozilla.org/security/x509certdb;1";

Yeah I wouldn't bother defining aliases for any of these - just use `"@mozilla.org/security/x509certdb;1"`, `Ci.nsIX509Cert`, 'Ci.nsIX509CertDB` as appropriate.

::: services/common/blocklist-clients.js:452
(Diff revision 1)
> +    if (item.certData) {
> +      let data = item.certData;
> +
> +      // split off the header and footer, base64 decode, construct the cert
> +      // from the resulting DER data.
> +      let b64data = data.split('-----')[2].replace(/\s/g, '');

We could maybe save on some data if we shipped these without the header/footer/newlines, but not a big deal in the long run, probably.

::: services/common/blocklist-clients.js:461
(Diff revision 1)
> +      // verify the certificate - we don't want to import CA certs that do
> +      // not chain to roots in our program
> +      let promise = new Promise((resolve, reject) => {
> +        let now = (new Date()).getTime() / 1000;
> +        let result = new CertVerificationResult(resolve);
> +        certdb.asyncVerifyCertAtTime(cert, certificateUsageSSLCA, 0, null, now, result);

Might want to pass `Ci.nsIX509CertDB.FLAG_LOCAL_ONLY` to prevent network traffic.

::: services/common/blocklist-clients.js:466
(Diff revision 1)
> +        certdb.asyncVerifyCertAtTime(cert, certificateUsageSSLCA, 0, null, now, result);
> +      });
> +      let certVerifies = await promise;
> +
> +      // IF the cert verifies, add the certificate to the cert db
> +      if (true == certVerifies) {

Just `if (certVerifies) {` should be fine.

::: services/common/blocklist-clients.js:467
(Diff revision 1)
> +      });
> +      let certVerifies = await promise;
> +
> +      // IF the cert verifies, add the certificate to the cert db
> +      if (true == certVerifies) {
> +        certdb.addCert(certDer,'C,,','NSS ignores nicknames');

In fact, the nickname argument was removed from this API :)

We still want to treat these as intermediates, right? So the trust argument should be `",,"`.
Attachment #8951285 - Flags: review?(dkeeler)
(In reply to Mark Goodwin [:mgoodwin] from comment #10)
> Created attachment 9025658 [details]
> Bug 657228: Preload all known intermediate certificates for CAs in our root
> store r?keeler

This is still WIP - but the actual implementation is pretty much what I think we want (in particular, I need to fix the test to use attachments).
Assignee

Updated

5 months ago
Assignee: nobody → jjones
Status: REOPENED → ASSIGNED
Whiteboard: [psm-backlog] → [psm-assigned]
Assignee

Updated

5 months ago
Depends on: 1519256
Assignee

Updated

5 months ago
Blocks: 1519273

Comment 12

4 months ago

What are the reasons for implementing this? Performance?

Won't this result in more improperly configured sites?

Assignee

Comment 13

4 months ago

(In reply to Kristian Klausen from comment #12)

What are the reasons for implementing this? Performance?

Performance is part of it, as is reducing unknown issuer error rates (as an alternative to AIA-chasing). Also, a list of all in-program intermediates is a necessary part of the work we're doing for CRLite, so attaching the DER certs to that data is pretty natural. This plus CRLite lets us stop doing out-of-band network fetches to determine configuration and revocation status w/o the operators stapling OCSP.

Won't this result in more improperly configured sites?

Given the error rate telemetry and conversations we've had here over the years, I think that trend is already underway. Error-page-in-Firefox doesn't seem to be the major alarm it once was.

The first uses of this will be to gather telemetry insights from Nightly users; there's no immediate plans to ship this.

Comment 14

4 months ago
Pushed by jjones@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cac9133e5572
Preload all known intermediate certificates for CAs in our root store r=keeler
Assignee

Updated

4 months ago
Blocks: 1520278

Comment 15

4 months ago
Backout by dluca@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8ffff925bbb6
Backed out changeset cac9133e5572 for xpcshell failure in security/manager/ssl/tests/unit/test_intermediate_preloads.js. CLOSED TREE

Comment 16

4 months ago
Pushed by jjones@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7477e57523cd
Preload all known intermediate certificates for CAs in our root store r=keeler

Comment 17

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Assignee

Updated

4 months ago
Blocks: 1429796, 1519256
No longer depends on: 1429796, 1519256
Assignee

Updated

4 months ago
Blocks: 1404934
You need to log in before you can comment on or make changes to this bug.