bug 1312827 - make the certificate blocklist only apply to TLS server certificates r?mgoodwin,jcj draft
authorDavid Keeler <dkeeler@mozilla.com>
Thu, 22 Dec 2016 16:57:20 -0800
changeset 453557 3c3f68285ee726efe3a20a504bc472873d29b4e6
parent 453490 bbbd2f7539f224a482cc6d2dd10e6a5f31c8baf3
child 540501 073c2beaef63fcba07f07049e23b128e64fd2030
push id39707
push userdkeeler@mozilla.com
push dateFri, 23 Dec 2016 21:19:56 +0000
reviewersmgoodwin, jcj
bugs1312827
milestone53.0a1
bug 1312827 - make the certificate blocklist only apply to TLS server certificates r?mgoodwin,jcj (Note that content signature verification does not use the unified certificate verifier and thus will still consult OneCRL.) MozReview-Commit-ID: 6KvHOngpabT
security/certverifier/NSSCertDBTrustDomain.cpp
security/manager/ssl/tests/mochitest/browser/browser_certViewer.js
security/manager/ssl/tests/unit/test_cert_blocklist.js
--- a/security/certverifier/NSSCertDBTrustDomain.cpp
+++ b/security/certverifier/NSSCertDBTrustDomain.cpp
@@ -187,35 +187,39 @@ NSSCertDBTrustDomain::GetCertTrust(EndEn
     return MapPRErrorCodeToResult(PR_GetError());
   }
 
   // Check the certificate against the OneCRL cert blocklist
   if (!mCertBlocklist) {
     return Result::FATAL_ERROR_LIBRARY_FAILURE;
   }
 
-  bool isCertRevoked;
-  nsresult nsrv = mCertBlocklist->IsCertRevoked(
-                    candidateCert->derIssuer.data,
-                    candidateCert->derIssuer.len,
-                    candidateCert->serialNumber.data,
-                    candidateCert->serialNumber.len,
-                    candidateCert->derSubject.data,
-                    candidateCert->derSubject.len,
-                    candidateCert->derPublicKey.data,
-                    candidateCert->derPublicKey.len,
-                    &isCertRevoked);
-  if (NS_FAILED(nsrv)) {
-    return Result::FATAL_ERROR_LIBRARY_FAILURE;
-  }
+  // The certificate blocklist currently only applies to TLS server
+  // certificates.
+  if (mCertDBTrustType == trustSSL) {
+    bool isCertRevoked;
+    nsresult nsrv = mCertBlocklist->IsCertRevoked(
+                      candidateCert->derIssuer.data,
+                      candidateCert->derIssuer.len,
+                      candidateCert->serialNumber.data,
+                      candidateCert->serialNumber.len,
+                      candidateCert->derSubject.data,
+                      candidateCert->derSubject.len,
+                      candidateCert->derPublicKey.data,
+                      candidateCert->derPublicKey.len,
+                      &isCertRevoked);
+    if (NS_FAILED(nsrv)) {
+      return Result::FATAL_ERROR_LIBRARY_FAILURE;
+    }
 
-  if (isCertRevoked) {
-    MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
-           ("NSSCertDBTrustDomain: certificate is in blocklist"));
-    return Result::ERROR_REVOKED_CERTIFICATE;
+    if (isCertRevoked) {
+      MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
+             ("NSSCertDBTrustDomain: certificate is in blocklist"));
+      return Result::ERROR_REVOKED_CERTIFICATE;
+    }
   }
 
   // XXX: CERT_GetCertTrust seems to be abusing SECStatus as a boolean, where
   // SECSuccess means that there is a trust record and SECFailure means there
   // is not a trust record. I looked at NSS's internal uses of
   // CERT_GetCertTrust, and all that code uses the result as a boolean meaning
   // "We have a trust record."
   CERTCertTrust trust;
--- a/security/manager/ssl/tests/mochitest/browser/browser_certViewer.js
+++ b/security/manager/ssl/tests/mochitest/browser/browser_certViewer.js
@@ -101,18 +101,21 @@ add_task(function* testRevoked() {
   // likely) and subject (less likely).
   let certBlocklist = Cc["@mozilla.org/security/certblocklist;1"]
                         .getService(Ci.nsICertBlocklist);
   certBlocklist.revokeCertBySubjectAndPubKey(
     "MBIxEDAOBgNVBAMMB3Jldm9rZWQ=", // CN=revoked
     "VCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8="); // hash of the shared key
   let cert = yield readCertificate("revoked.pem", ",,");
   let win = yield displayCertificate(cert);
-  checkError(win,
-             "Could not verify this certificate because it has been revoked.");
+  // As of bug 1312827, OneCRL only applies to TLS web server certificates, so
+  // this certificate will actually verify successfully for every end-entity
+  // usage except TLS web server.
+  checkUsages(win, ["Email Recipient Certificate", "Email Signer Certificate",
+                    "Object Signer", "SSL Client Certificate"]);
   yield BrowserTestUtils.closeWindow(win);
 });
 
 add_task(function* testInvalid() {
   // This certificate has a keyUsage extension asserting cRLSign and
   // keyCertSign, but it doesn't have a basicConstraints extension. This
   // shouldn't be valid for any usage. Sadly, we give a pretty lame error
   // message in this case.
--- a/security/manager/ssl/tests/unit/test_cert_blocklist.js
+++ b/security/manager/ssl/tests/unit/test_cert_blocklist.js
@@ -134,16 +134,27 @@ var converter = Cc["@mozilla.org/intl/sc
                   .createInstance(Ci.nsIScriptableUnicodeConverter);
 converter.charset = "UTF-8";
 
 function verify_cert(file, expectedError) {
   let ee = constructCertFromFile(file);
   checkCertErrorGeneric(certDB, ee, expectedError, certificateUsageSSLServer);
 }
 
+// The certificate blocklist currently only applies to TLS server certificates.
+function verify_non_tls_usage_succeeds(file) {
+  let ee = constructCertFromFile(file);
+  checkCertErrorGeneric(certDB, ee, PRErrorCodeSuccess,
+                        certificateUsageSSLClient);
+  checkCertErrorGeneric(certDB, ee, PRErrorCodeSuccess,
+                        certificateUsageEmailSigner);
+  checkCertErrorGeneric(certDB, ee, PRErrorCodeSuccess,
+                        certificateUsageEmailRecipient);
+}
+
 function load_cert(cert, trust) {
   let file = "bad_certs/" + cert + ".pem";
   addCertFromFile(certDB, file, trust);
 }
 
 function test_is_revoked(certList, issuerString, serialString, subjectString,
                          pubKeyString) {
   let issuer = converter.convertToByteArray(issuerString || "", {});
@@ -289,24 +300,27 @@ function run_test() {
 
     // Check the blocklist entry has been persisted properly to the backing
     // file
     check_revocations_txt_contents(expected);
 
     // Check the blocklisted intermediate now causes a failure
     let file = "test_onecrl/test-int-ee.pem";
     verify_cert(file, SEC_ERROR_REVOKED_CERTIFICATE);
+    verify_non_tls_usage_succeeds(file);
 
     // Check the ee with the blocklisted root also causes a failure
     file = "bad_certs/other-issuer-ee.pem";
     verify_cert(file, SEC_ERROR_REVOKED_CERTIFICATE);
+    verify_non_tls_usage_succeeds(file);
 
     // Check the ee blocked by subject / pubKey causes a failure
     file = "test_onecrl/same-issuer-ee.pem";
     verify_cert(file, SEC_ERROR_REVOKED_CERTIFICATE);
+    verify_non_tls_usage_succeeds(file);
 
     // Check a non-blocklisted chain still validates OK
     file = "bad_certs/default-ee.pem";
     verify_cert(file, PRErrorCodeSuccess);
 
     // Check a bad cert is still bad (unknown issuer)
     file = "bad_certs/unknownissuer.pem";
     verify_cert(file, SEC_ERROR_UNKNOWN_ISSUER);