Bug 973268: Return better error codes and make simple cert error override processing work for insanity::pkix, r=keeler, r=cviecco

--HG--
extra : rebase_source : 596e7a67b8631bb6a52c20d569fe433aa5e86cec

security/insanity/include/insanity/pkix.h

@@ -23,6 +23,48 @@
 
 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,

security/insanity/lib/pkixbuild.cpp

@@ -164,7 +164,12 @@ BuildForwardInner(TrustDomain& trustDomain,
   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,
@@ -186,14 +191,23 @@ BuildForward(TrustDomain& trustDomain,
   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) {
@@ -219,6 +233,8 @@ BuildForward(TrustDomain& trustDomain,
     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,
@@ -226,6 +242,14 @@ BuildForward(TrustDomain& trustDomain,
                            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,
@@ -240,9 +264,31 @@ BuildForward(TrustDomain& trustDomain,
     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

security/insanity/lib/pkixcheck.cpp

@@ -231,9 +231,13 @@ CheckNameConstraints(BackCert& cert)
     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);
@@ -354,11 +358,6 @@ CheckIssuerIndependentProperties(TrustDomain& trustDomain,
   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
 
@@ -402,6 +401,13 @@ CheckIssuerIndependentProperties(TrustDomain& trustDomain,
   // 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;
 }