657228 - Preload all known intermediate certificates for CAs in our root store

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P3)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

[David E. Ross]

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

Comment 2

[Kai Engert (:KaiE:)]

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

[Wan-Teh Chang]

It is fine by me to mark this bug WONTFIX.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 5

[Devdatta Akhawe [:devd]]

Comment 72 on Bug 657228 has some useful data on the impact of caching. https://bugzilla.mozilla.org/show_bug.cgi?id=399324#c72

Comment 7

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 657228 - Preload all known intermediate certificates in our certificate store

Review commit: https://reviewboard.mozilla.org/r/220538/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220538/

Comment 8

[Code Review Bot [:reviewbot]]

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 9

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

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 `",,"`.

Comment 10

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 657228: Preload all known intermediate certificates for CAs in our root store r?keeler

Comment 11

[Mark Goodwin [:mgoodwin]]

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

Comment 12

[Kristian Klausen]

What are the reasons for implementing this? Performance?

Won't this result in more improperly configured sites?

Comment 13

[J.C. Jones [:jcj] (he/they)]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cac9133e5572
Preload all known intermediate certificates for CAs in our root store r=keeler

Comment 15

[Pulsebot]

Backout by [e-mail]:
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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7477e57523cd
Preload all known intermediate certificates for CAs in our root store r=keeler

Comment 17

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/7477e57523cd