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

net/cert/cert_verify_proc_win.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/cert/cert_verify_proc_win.h"
 
 #include <string>
 #include <vector>
 
 #include "base/memory/scoped_ptr.h"
@@ skipping 617 matching lines @@
            NULL,  // reserved
            &chain_context)) {
     verify_result->cert_status |= CERT_STATUS_INVALID;
     return MapSecurityError(GetLastError());
   }
 
   CRLSetResult crl_set_result = kCRLSetUnknown;
   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 &&
              (flags & CertVerifier::VERIFY_REV_CHECKING_ENABLED_EV_ONLY) &&
              !rev_checking_enabled &&
              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.

       CERT_TRUST_IS_NOT_VALID_FOR_USAGE) {
     ev_policy_oid = NULL;
     chain_para.RequestedIssuancePolicy.Usage.cUsageIdentifier = 0;
     chain_para.RequestedIssuancePolicy.Usage.rgpszUsageIdentifier = NULL;
     CertFreeCertificateChain(chain_context);
     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());
     }
   }
 
   CertVerifyResult temp_verify_result = *verify_result;
   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;
     verify_result->cert_status |= CERT_STATUS_REV_CHECKING_ENABLED;
     chain_flags &= ~CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY;
 
     CertFreeCertificateChain(chain_context);
     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());
     }
     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;
     }
   }
 
   ScopedPCCERT_CHAIN_CONTEXT scoped_chain_context(chain_context);
 
   verify_result->cert_status |= MapCertChainErrorStatusToCertStatus(
       chain_context->TrustStatus.dwErrorStatus);
 
   // Flag certificates that have a Subject common name with a NULL character.
@@ skipping 60 matching lines @@
     return MapCertStatusToNetError(verify_result->cert_status);
 
   if (ev_policy_oid &&
       CheckEV(chain_context, rev_checking_enabled, ev_policy_oid)) {
     verify_result->cert_status |= CERT_STATUS_IS_EV;
   }
   return OK;
 }
 
 }  // namespace net