bug 1123671 - if a non-overridable error is encountered when processing an overridable certificate error, report the non-overridable error r=mmc r=jcj

Also, SEC_ERROR_UNTRUSTED_ISSUER and SEC_ERROR_UNTRUSTED_CERT are not actually overridable, so don't pretend they are.

security/manager/ssl/src/NSSErrorsService.cpp

@@ -131,13 +131,23 @@ NSSErrorsService::GetErrorClass(nsresult aXPCOMErrorCode, uint32_t *aErrorClass)
     return NS_ERROR_FAILURE;
   }
 
-  switch (aNSPRCode)
+  if (mozilla::psm::ErrorIsOverridable(aNSPRCode)) {
+    *aErrorClass = ERROR_CLASS_BAD_CERT;
+  } else {
+    *aErrorClass = ERROR_CLASS_SSL_PROTOCOL;
+  }
+
+  return NS_OK;
+}
+
+bool
+ErrorIsOverridable(PRErrorCode code)
+{
+  switch (code)
   {
     // Overridable errors.
     case SEC_ERROR_UNKNOWN_ISSUER:
-    case SEC_ERROR_UNTRUSTED_ISSUER:
     case SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE:
-    case SEC_ERROR_UNTRUSTED_CERT:
     case SSL_ERROR_BAD_CERT_DOMAIN:
     case SEC_ERROR_EXPIRED_CERTIFICATE:
     case SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED:
@@ -146,14 +156,11 @@ NSSErrorsService::GetErrorClass(nsresult aXPCOMErrorCode, uint32_t *aErrorClass)
     case mozilla::pkix::MOZILLA_PKIX_ERROR_V1_CERT_USED_AS_CA:
     case mozilla::pkix::MOZILLA_PKIX_ERROR_NOT_YET_VALID_CERTIFICATE:
     case mozilla::pkix::MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE:
-      *aErrorClass = ERROR_CLASS_BAD_CERT;
-      break;
+      return true;
     // Non-overridable errors.
     default:
-      *aErrorClass = ERROR_CLASS_SSL_PROTOCOL;
-      break;
+      return false;
   }
-  return NS_OK;
 }
 
 NS_IMETHODIMP

security/manager/ssl/src/NSSErrorsService.h

@@ -38,6 +38,7 @@ private:
 
 bool IsNSSErrorCode(PRErrorCode code);
 nsresult GetXPCOMFromNSSError(PRErrorCode code);
+bool ErrorIsOverridable(PRErrorCode code);
 
 } // psm
 } // mozilla

security/manager/ssl/src/SSLServerCertVerification.cpp

@@ -351,7 +351,10 @@ DetermineCertOverrideErrors(CERTCertificate* cert, const char* hostName,
 
       SECCertTimeValidity validity = CERT_CheckCertValidTimes(cert, now, false);
       if (validity == secCertTimeUndetermined) {
-        PR_SetError(defaultErrorCodeToReport, 0);
+        // This only happens if cert is null. CERT_CheckCertValidTimes will
+        // have set the error code to SEC_ERROR_INVALID_ARGS. We should really
+        // be using mozilla::pkix here anyway.
+        MOZ_ASSERT(PR_GetError() == SEC_ERROR_INVALID_ARGS);
         return SECFailure;
       }
       if (validity == secCertTimeExpired) {
@@ -404,7 +407,7 @@ DetermineCertOverrideErrors(CERTCertificate* cert, const char* hostName,
       collectedErrors |= nsICertOverrideService::ERROR_MISMATCH;
       errorCodeMismatch = SSL_ERROR_BAD_CERT_DOMAIN;
     } else if (result != Success) {
-      PR_SetError(defaultErrorCodeToReport, 0);
+      PR_SetError(MapResultToPRErrorCode(result), 0);
       return SECFailure;
     }
   }
@@ -593,6 +596,13 @@ CreateCertErrorRunnable(CertVerifier& certVerifier,
                                   defaultErrorCodeToReport, collected_errors,
                                   errorCodeTrust, errorCodeMismatch,
                                   errorCodeExpired) != SECSuccess) {
+    // Attempt to enforce that if DetermineCertOverrideErrors failed,
+    // PR_SetError was set with a non-overridable error. This is because if we
+    // return from CreateCertErrorRunnable without calling
+    // infoObject->SetStatusErrorBits, we won't have the required information
+    // to actually add a certificate error override. This results in a broken
+    // UI which is annoying but not a security disaster.
+    MOZ_ASSERT(!ErrorIsOverridable(PR_GetError()));
     return nullptr;
   }
 

security/manager/ssl/tests/unit/test_cert_overrides.js

@@ -128,6 +128,22 @@ function add_simple_tests() {
   add_non_overridable_test("inadequatekeyusage.example.com",
                            SEC_ERROR_INADEQUATE_KEY_USAGE);
 
+  // This is intended to test the case where a verification has failed for one
+  // overridable reason (e.g. unknown issuer) but then, in the process of
+  // reporting that error, a non-overridable error is encountered. The
+  // non-overridable error should be prioritized.
+  add_test(function() {
+    let rootCert = constructCertFromFile("tlsserver/test-ca.der");
+    setCertTrust(rootCert, ",,");
+    run_next_test();
+  });
+  add_non_overridable_test("badSubjectAltNames.example.com", SEC_ERROR_BAD_DER);
+  add_test(function() {
+    let rootCert = constructCertFromFile("tlsserver/test-ca.der");
+    setCertTrust(rootCert, "CTu,,");
+    run_next_test();
+  });
+
   // Bug 990603: Apache documentation has recommended generating a self-signed
   // test certificate with basic constraints: CA:true. For compatibility, this
   // is a scenario in which an override is allowed.
@@ -205,20 +221,18 @@ function add_distrust_tests() {
   // Before we specifically distrust this certificate, it should be trusted.
   add_connection_test("untrusted.example.com", Cr.NS_OK);
 
-  add_distrust_override_test("tlsserver/default-ee.der",
-                             "untrusted.example.com",
-                             getXPCOMStatusFromNSS(SEC_ERROR_UNTRUSTED_CERT));
+  add_distrust_test("tlsserver/default-ee.der", "untrusted.example.com",
+                    SEC_ERROR_UNTRUSTED_CERT);
 
-  add_distrust_override_test("tlsserver/other-test-ca.der",
-                             "untrustedissuer.example.com",
-                             getXPCOMStatusFromNSS(SEC_ERROR_UNTRUSTED_ISSUER));
+  add_distrust_test("tlsserver/other-test-ca.der",
+                    "untrustedissuer.example.com", SEC_ERROR_UNTRUSTED_ISSUER);
 
-  add_distrust_override_test("tlsserver/test-ca.der",
-                             "ca-used-as-end-entity.example.com",
-                             getXPCOMStatusFromNSS(SEC_ERROR_UNTRUSTED_ISSUER));
+  add_distrust_test("tlsserver/test-ca.der",
+                    "ca-used-as-end-entity.example.com",
+                    SEC_ERROR_UNTRUSTED_ISSUER);
 }
 
-function add_distrust_override_test(certFileName, hostName, expectedResult) {
+function add_distrust_test(certFileName, hostName, expectedResult) {
   let certToDistrust = constructCertFromFile(certFileName);
 
   add_test(function () {
@@ -227,26 +241,9 @@ function add_distrust_override_test(certFileName, hostName, expectedResult) {
     clearSessionCache();
     run_next_test();
   });
-  add_connection_test(hostName, expectedResult, null,
-                      function (securityInfo) {
-                        securityInfo.QueryInterface(Ci.nsISSLStatusProvider);
-                        // XXX(Bug 754369): SSLStatus isn't available for
-                        // non-overridable errors.
-                        if (securityInfo.SSLStatus) {
-                          certOverrideService.rememberValidityOverride(
-                              hostName, 8443, securityInfo.SSLStatus.serverCert,
-                              Ci.nsICertOverrideService.ERROR_UNTRUSTED, true);
-                        } else {
-                          // A missing SSLStatus probably means (due to bug
-                          // 754369) that the error was non-overridable, which
-                          // is what we're trying to test, though we'd rather
-                          // not test it this way.
-                          do_check_neq(expectedResult, Cr.NS_OK);
-                        }
-                        clearSessionCache();
-                      });
-  add_connection_test(hostName, expectedResult, null,
-                      function () {
-                        setCertTrust(certToDistrust, "u,,");
-                      });
+  add_non_overridable_test(hostName, expectedResult);
+  add_test(function () {
+    setCertTrust(certToDistrust, "u,,");
+    run_next_test();
+  });
 }

security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp

@@ -66,6 +66,7 @@ const BadCertHost sBadCertHosts[] =
   { "nsCertTypeCritical.example.com", "nsCertTypeCritical" },
   { "end-entity-issued-by-v1-cert.example.com", "eeIssuedByV1Cert" },
   { "inadequate-key-size-ee.example.com", "inadequateKeySizeEE" },
+  { "badSubjectAltNames.example.com", "badSubjectAltNames" },
   { nullptr, nullptr }
 };
 

security/manager/ssl/tests/unit/tlsserver/generate_certs.sh

@@ -330,4 +330,6 @@ make_EE eeIssuedByV1Cert 'CN=EE Issued by V1 Cert' v1Cert "localhost,*.example.c
 make_EE eeIssuedByIntermediate 'CN=EE issued by intermediate' testINT "localhost"
 export_cert eeIssuedByIntermediate test-int-ee.der
 
+make_EE badSubjectAltNames 'CN=EE with bad subjectAltNames' testCA "*.*.example.com"
+
 cleanup