Bug 973268: Return better error codes and make simple cert error override processing work for insanity::pkix, r=keeler, r=cviecco
authorBrian Smith <brian@briansmith.org>
Tue, 11 Feb 2014 00:46:05 -0800
changeset 169458 b8fd613d94d560daeab160b6e8efbbb33668a971
parent 169457 302def56019a278411ed9d71e3de7126d1729811
child 169459 571a1bf8c8abc5203b9026cdacdfd36321183533
push id39957
push userbrian@briansmith.org
push dateWed, 19 Feb 2014 07:28:25 +0000
treeherdermozilla-inbound@571a1bf8c8ab [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler, cviecco
bugs973268
milestone30.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 973268: Return better error codes and make simple cert error override processing work for insanity::pkix, r=keeler, r=cviecco
security/insanity/include/insanity/pkix.h
security/insanity/lib/pkixbuild.cpp
security/insanity/lib/pkixcheck.cpp
--- a/security/insanity/include/insanity/pkix.h
+++ b/security/insanity/include/insanity/pkix.h
@@ -18,16 +18,58 @@
 #ifndef insanity_pkix__pkix_h
 #define insanity_pkix__pkix_h
 
 #include "pkixtypes.h"
 #include "prtime.h"
 
 namespace insanity { namespace pkix {
 
+// ----------------------------------------------------------------------------
+// ERROR RANKING
+//
+// BuildCertChain prioritizes certain checks ahead of others so that when a
+// certificate chain has multiple errors, the "most serious" error is
+// returned. In practice, this ranking of seriousness is tied directly to how
+// Firefox's certificate error override mechanism.
+//
+// The ranking is:
+//
+//    1. Active distrust (SEC_ERROR_UNTRUSTED_CERT).
+//    2. Problems with issuer-independent properties other than
+//       notBefore/notAfter.
+//    3. For CA certificates: Expiration.
+//    4. Unknown issuer (SEC_ERROR_UNKNOWN_ISSUER).
+//    5. For end-entity certificates: Expiration.
+//    6. Revocation.
+//
+// In particular, if BuildCertChain returns SEC_ERROR_UNKNOWN_ISSUER then the
+// caller can call CERT_CheckCertValidTimes to determine if the certificate is
+// ALSO expired.
+//
+// It would be better if revocation were prioritized above expiration and
+// unknown issuer. However, it is impossible to do revocation checking without
+// knowing the issuer, since the issuer information is needed to validate the
+// revocation information. Also, generally revocation checking only works
+// during the validity period of the certificate.
+//
+// In general, when path building fails, BuildCertChain will return
+// SEC_ERROR_UNKNOWN_ISSUER. However, if all attempted paths resulted in the
+// same error (which is trivially true when there is only one potential path),
+// more specific errors will be returned.
+//
+// ----------------------------------------------------------------------------
+// Meaning of specific error codes
+//
+// SEC_ERROR_UNTRUSTED_CERT means that the end-entity certificate was actively
+//                          distrusted.
+// SEC_ERROR_UNTRUSTED_ISSUER means that path building failed because of active
+//                            distrust.
+// TODO(bug 968451): Document more of these.
+
 SECStatus BuildCertChain(TrustDomain& trustDomain,
                          CERTCertificate* cert,
                          PRTime time,
                          EndEntityOrCA endEntityOrCA,
             /*optional*/ KeyUsages requiredKeyUsagesIfPresent,
             /*optional*/ SECOidTag requiredEKUIfPresent,
             /*optional*/ const SECItem* stapledOCSPResponse,
                  /*out*/ ScopedCERTCertList& results);
--- a/security/insanity/lib/pkixbuild.cpp
+++ b/security/insanity/lib/pkixbuild.cpp
@@ -159,17 +159,22 @@ BuildForwardInner(TrustDomain& trustDoma
   if (trustDomain.VerifySignedData(&subject.GetNSSCert()->signatureWrap,
                                    potentialIssuer.GetNSSCert()) != SECSuccess) {
     return MapSECStatus(SECFailure);
   }
 
   return Success;
 }
 
-// Caller must check for expiration before calling this function
+// Recursively build the path from the given subject certificate to the root.
+//
+// Be very careful about changing the order of checks. The order is significant
+// because it affects which error we return when a certificate or certificate
+// chain has multiple problems. See the error ranking documentation in
+// insanity/pkix.h.
 static Result
 BuildForward(TrustDomain& trustDomain,
              BackCert& subject,
              PRTime time,
              EndEntityOrCA endEntityOrCA,
              KeyUsages requiredKeyUsagesIfPresent,
              SECOidTag requiredEKUIfPresent,
              /*optional*/ const SECItem* stapledOCSPResponse,
@@ -181,24 +186,33 @@ BuildForward(TrustDomain& trustDomain,
   static const size_t MAX_DEPTH = 8;
   if (subCACount >= MAX_DEPTH - 1) {
     return RecoverableError;
   }
 
   Result rv;
 
   TrustDomain::TrustLevel trustLevel;
-
+  bool expiredEndEntity = false;
   rv = CheckIssuerIndependentProperties(trustDomain, subject, time,
                                         endEntityOrCA,
                                         requiredKeyUsagesIfPresent,
                                         requiredEKUIfPresent, subCACount,
                                         &trustLevel);
   if (rv != Success) {
-    return rv;
+    // CheckIssuerIndependentProperties checks for expiration last, so if
+    // it returned SEC_ERROR_EXPIRED_CERTIFICATE we know that is the only
+    // problem with the cert found so far. Keep going to see if we can build
+    // a path; if not, it's better to return the path building failure.
+    expiredEndEntity = endEntityOrCA == MustBeEndEntity &&
+                       trustLevel != TrustDomain::TrustAnchor &&
+                       PR_GetError() == SEC_ERROR_EXPIRED_CERTIFICATE;
+    if (!expiredEndEntity) {
+      return rv;
+    }
   }
 
   if (trustLevel == TrustDomain::TrustAnchor) {
     // End of the recursion. Create the result list and add the trust anchor to
     // it.
     results = CERT_NewCertList();
     if (!results) {
       return FatalError;
@@ -214,40 +228,72 @@ BuildForward(TrustDomain& trustDomain,
                                        candidates) != SECSuccess) {
     return MapSECStatus(SECFailure);
   }
   PORT_Assert(candidates.get());
   if (!candidates) {
     return Fail(RecoverableError, SEC_ERROR_UNKNOWN_ISSUER);
   }
 
+  PRErrorCode errorToReturn = 0;
+
   for (CERTCertListNode* n = CERT_LIST_HEAD(candidates);
        !CERT_LIST_END(n, candidates); n = CERT_LIST_NEXT(n)) {
     rv = BuildForwardInner(trustDomain, subject, time, endEntityOrCA,
                            requiredEKUIfPresent,
                            n->cert, stapledOCSPResponse, subCACount,
                            results);
     if (rv == Success) {
+      if (expiredEndEntity) {
+        // We deferred returning this error to see if we should return
+        // "unknown issuer" instead. Since we found a valid issuer, it's
+        // time to return "expired."
+        PR_SetError(SEC_ERROR_EXPIRED_CERTIFICATE, 0);
+        return RecoverableError;
+      }
+
       SECStatus srv = trustDomain.CheckRevocation(endEntityOrCA,
                                                   subject.GetNSSCert(),
                                                   n->cert, time,
                                                   stapledOCSPResponse);
       if (srv != SECSuccess) {
         return MapSECStatus(SECFailure);
       }
 
       // We found a trusted issuer. At this point, we know the cert is valid
       return subject.PrependNSSCertToList(results.get());
     }
     if (rv != RecoverableError) {
       return rv;
     }
+
+    PRErrorCode currentError = PR_GetError();
+    switch (currentError) {
+      case 0:
+        PR_NOT_REACHED("Error code not set!");
+        PR_SetError(PR_INVALID_STATE_ERROR, 0);
+        return FatalError;
+      case SEC_ERROR_UNTRUSTED_CERT:
+        currentError = SEC_ERROR_UNTRUSTED_ISSUER;
+        break;
+      default:
+        break;
+    }
+    if (errorToReturn == 0) {
+      errorToReturn = currentError;
+    } else if (errorToReturn != currentError) {
+      errorToReturn = SEC_ERROR_UNKNOWN_ISSUER;
+    }
   }
 
-  return Fail(RecoverableError, SEC_ERROR_UNKNOWN_ISSUER);
+  if (errorToReturn == 0) {
+    errorToReturn = SEC_ERROR_UNKNOWN_ISSUER;
+  }
+
+  return Fail(RecoverableError, errorToReturn);
 }
 
 SECStatus
 BuildCertChain(TrustDomain& trustDomain,
                CERTCertificate* certToDup,
                PRTime time,
                EndEntityOrCA endEntityOrCA,
                /*optional*/ KeyUsages requiredKeyUsagesIfPresent,
--- a/security/insanity/lib/pkixcheck.cpp
+++ b/security/insanity/lib/pkixcheck.cpp
@@ -226,19 +226,23 @@ CheckNameConstraints(BackCert& cert)
     const CERTGeneralName* names = nullptr;
     Result rv = prev->GetConstrainedNames(&names);
     if (rv != Success) {
       return rv;
     }
     PORT_Assert(names);
     CERTGeneralName* currentName = const_cast<CERTGeneralName*>(names);
     do {
-      rv = MapSECStatus(CERT_CheckNameSpace(arena, constraints, currentName));
-      if (rv != Success) {
-        return rv;
+      if (CERT_CheckNameSpace(arena, constraints, currentName) != SECSuccess) {
+        // XXX: It seems like CERT_CheckNameSpace doesn't always call
+        // PR_SetError when it fails. We set the error code here, though this
+        // may be papering over some fatal errors. NSS's
+        // cert_VerifyCertChainOld does something similar.
+        PR_SetError(SEC_ERROR_CERT_NOT_IN_NAME_SPACE, 0);
+        return RecoverableError;
       }
       currentName = CERT_GetNextGeneralName(currentName);
     } while (currentName != names);
   }
 
   return Success;
 }
 
@@ -349,21 +353,16 @@ CheckIssuerIndependentProperties(TrustDo
   }
   if (trustLevelOut) {
     *trustLevelOut = trustLevel;
   }
 
   bool isTrustAnchor = endEntityOrCA == MustBeCA &&
                        trustLevel == TrustDomain::TrustAnchor;
 
-  rv = CheckTimes(cert.GetNSSCert(), time);
-  if (rv != Success) {
-    return rv;
-  }
-
   // 4.2.1.1. Authority Key Identifier dealt with as part of path building
   // 4.2.1.2. Subject Key Identifier dealt with as part of path building
 
   PLArenaPool* arena = cert.GetArena();
   if (!arena) {
     return FatalError;
   }
 
@@ -397,12 +396,19 @@ CheckIssuerIndependentProperties(TrustDo
                              requiredEKUIfPresent);
   if (rv != Success) {
     return rv;
   }
 
   // 4.2.1.13. CRL Distribution Points will be dealt with elsewhere
   // 4.2.1.14. Inhibit anyPolicy
 
+  // IMPORTANT: This check must come after the other checks in order for error
+  // ranking to work correctly.
+  rv = CheckTimes(cert.GetNSSCert(), time);
+  if (rv != Success) {
+    return rv;
+  }
+
   return Success;
 }
 
 } } // namespace insanity::pkix