1002586 - CertUtils.checkCert should remove certificate validation

Status: RESOLVED DUPLICATE of bug 1454504

(Toolkit :: General, defect)

Description

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

https://mxr.mozilla.org/mozilla-central/source/toolkit/modules/CertUtils.jsm:
162   var issuerCert = cert;
163   while (issuerCert.issuer && !issuerCert.issuer.equals(issuerCert))
164     issuerCert = issuerCert.issuer;

This is slow (an entire certificate chain validation is performed each time issuer is accessed) and problematic in terms of checking for key pins.

(Ideally this entire function would be asynchronous, but that's a lot more work. Incremental progress is the way to go here.)

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

Pinning is landed in Nightly (w00t!) and the pinned domains include addons.mozilla.{org,net}, so we should be able to just remove this function. Besides, it was not checking a verified cert anyway.

https://mxr.mozilla.org/mozilla-central/source/security/manager/boot/src/PreloadedHPKPins.json#96

96 { "name": "addons.mozilla.org", "include_subdomains": true, "pins": "mozilla" },
97     { "name": "addons.mozilla.net", "include_subdomains": true, "pins": "mozilla" },

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #1)
> Pinning is landed in Nightly (w00t!) and the pinned domains include
> addons.mozilla.{org,net}, so we should be able to just remove this function.

FYI for anyone following the bug: we had a regression on AMO in https://bugzilla.mozilla.org/show_bug.cgi?id=1005364 and rolled back pinning on AMO to test-mode only while we fixed the pinset.

Comment 3

[Benjamin Smedberg]

Note that you'll also need to deal with the telemetry experiment pinning which uses CertUtils.jsm.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #1)
> Besides, it was not checking a verified cert anyway.

What does this mean?

Comment 5

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

(In reply to :Gavin Sharp (email [e-mail]) from comment #4)
> (In reply to [:mmc] Monica Chew (please use needinfo) from comment #1)
> > Besides, it was not checking a verified cert anyway.
> 
> What does this mean?

If I recall correctly, it's possible for .issuer to return a certificate that has not been verified (i.e. it doesn't chain to a trusted root, and it's not actually guaranteed that the "issuer" certificate signed the certificate in question).

Comment 6

[[:mmc] Monica Chew (no longer reading bugmail)]

Now I remember. This code propagates down to nsNSSCertificate::GetChain(nsIArray** _rvChain)

https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCertificate.cpp#865

Notice that the linked line returns a certificate chain *even if the chain does not verify*. This is an example of how using NSS for verification has been a thorn in our side, and why we are switching to a new verification library with saner APIs.