net: mark cert as revoked if EV revocation check receives revoked response (Win).

net/cert/cert_verify_proc_win.cc

Index: net/cert/cert_verify_proc_win.cc
diff --git a/net/cert/cert_verify_proc_win.cc b/net/cert/cert_verify_proc_win.cc
index d3e8b6210398e4ac18e02a2161b62a24b0c7a035..d79f77d238fcb10cf5bfdd0cb97f85a5168f8506 100644
--- a/net/cert/cert_verify_proc_win.cc
+++ b/net/cert/cert_verify_proc_win.cc
@@ -635,6 +635,13 @@ int CertVerifyProcWin::VerifyInternal(
   if (crl_set)
     crl_set_result = CheckRevocationWithCRLSet(chain_context, crl_set);
 
+  // redo_with_revocation_checking controls whether we verify the chain a
+  // second time, but with online revocation checking. If
+  // hard_fail_revocation_checking_when_redoing is also true then revocation
+  // information must be returned.
+  bool redo_with_revocation_checking = false;
+  bool hard_fail_revocation_checking_when_redoing = false;
+
   if (crl_set_result == kCRLSetRevoked) {
     verify_result->cert_status |= CERT_STATUS_REVOKED;
   } else if (crl_set_result == kCRLSetUnknown &&
@@ -643,22 +650,7 @@ int CertVerifyProcWin::VerifyInternal(
              ev_policy_oid != NULL) {
     // We don't have fresh information about this chain from the CRLSet and
     // it's probably an EV certificate. Retry with online revocation checking.
-    rev_checking_enabled = true;
-    chain_flags &= ~CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY;
-    verify_result->cert_status |= CERT_STATUS_REV_CHECKING_ENABLED;
-
-    if (!CertGetCertificateChain(
-             chain_engine,
-             cert_list.get(),
-             NULL,  // current system time
-             cert_list->hCertStore,
-             &chain_para,
-             chain_flags,
-             NULL,  // reserved
-             &chain_context)) {
-      verify_result->cert_status |= CERT_STATUS_INVALID;
-      return MapSecurityError(GetLastError());
-    }
+    redo_with_revocation_checking = true;
   }
 
   if (chain_context->TrustStatus.dwErrorStatus &

[Ryan Sleevi, 2013/08/27 19:25:07]

So, there's a slight side-effect of changing the order here that I'm not sure
about.

In the past, if we had an EV candidate (eg: has the OID), then we'd verify
online first, then try verifying (as non-EV) if the EV check failed.

If you have two versions of a CA - one EV enabled and one not - and the EV
enabled CA is revoked, the "old" behaviour is that CAPI would pick up the
revocation, and then try an alternate (non-revoked) path. The alternate path
would fail the EV policy OID  check, and thus come back with
NOT_VALID_FOR_USAGE.

We'd then downgrade to non-EV, and the path building would/should succeed.

The new behaviour would, by virtue of the ordering, have us treat the cert as
revoked, even though a valid path may exist. Alternatively, we'd flag it
NOT_VALID_FOR_USAGE, thus triggering an ERR_CERT_INVALID (IIRC), rather than an
ERR_CERT_REVOKED.

In practice, I don't think this matters, but it's something we should keep an
eye out for in bug reports. Only the largest of CAs would likely be playing with
both EV and non-EV enabled cross-signings, as I think it's a pretty crazy edge
case.

[agl, 2013/08/28 16:42:58]

Ok, that's a fair point and I was really shuffling code around to try and avoid
some duplication, but I'll just leave it be and just fix the memory leak.

@@ -685,6 +677,13 @@ int CertVerifyProcWin::VerifyInternal(
   GetCertChainInfo(chain_context, verify_result);
   if (!verify_result->is_issued_by_known_root &&
       (flags & CertVerifier::VERIFY_REV_CHECKING_REQUIRED_LOCAL_ANCHORS)) {
+    // Hard-fail revocation checking has been requested for locally installed
+    // CAs.
+    redo_with_revocation_checking = true;
+    hard_fail_revocation_checking_when_redoing = true;
+  }
+
+  if (redo_with_revocation_checking) {
     *verify_result = temp_verify_result;
 
     rev_checking_enabled = true;
@@ -706,8 +705,9 @@ int CertVerifyProcWin::VerifyInternal(
     }
     GetCertChainInfo(chain_context, verify_result);
 
-    if (chain_context->TrustStatus.dwErrorStatus &
-        CERT_TRUST_IS_OFFLINE_REVOCATION) {
+    if (hard_fail_revocation_checking_when_redoing &&
+        (chain_context->TrustStatus.dwErrorStatus &
+         CERT_TRUST_IS_OFFLINE_REVOCATION)) {
       verify_result->cert_status |= CERT_STATUS_REVOKED;
     }
   }