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 169820 b8fd613d94d560daeab160b6e8efbbb33668a971
parent 169819 302def56019a278411ed9d71e3de7126d1729811
child 169821 571a1bf8c8abc5203b9026cdacdfd36321183533
push id270
push userpvanderbeken@mozilla.com
push dateThu, 06 Mar 2014 09:24:21 +0000
reviewerskeeler, cviecco
bugs973268
milestone30.0a1
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