Perform online revocation checks when EV certificates aren't covered by a fresh CRLSet.

Previously a fresh CRLSet was sufficient to suppress online revocation checking
for EV certificates because we aimed to have full EV coverage in the CRLSet.

With this change, we'll only suppress online revocation checking for EV
certificates when a fresh CRLSet actually covers the chain in question. We
determine coverage by seeing if the CRLSet contains the issuer SPKI.

There are no changes to the OS X certificate code as I believe that OS X
already does online revocation checking for EV certs no matter what we do in
Chrome.

BUG=none

Review URL: https://chromiumcodereview.appspot.com/11260018

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@214825 0039d316-1c4b-4281-b951-d872f2087c98

12 files changed, 219 insertions, 52 deletions

diff --git a/net/cert/cert_verifier.h b/net/cert/cert_verifier.h
index 1f0b13c..743c835 100644
--- a/net/cert/cert_verifier.h
+++ b/net/cert/cert_verifier.h
@@ -51,9 +51,9 @@ class NET_EXPORT CertVerifier {
     // NSS only.
     VERIFY_CERT_IO_ENABLED = 1 << 2,
 
-    // If set, enables online revocation checking via CRLs or OCSP, but only
-    // for certificates which may be EV, and only when VERIFY_EV_CERT is also
-    // set.
+    // If set, enables online revocation checking via CRLs or OCSP when the
+    // chain is not covered by a fresh CRLSet, but only for certificates which
+    // may be EV, and only when VERIFY_EV_CERT is also set.
     VERIFY_REV_CHECKING_ENABLED_EV_ONLY = 1 << 3,
 
     // If set, this is equivalent to VERIFY_REV_CHECKING_ENABLED, in that it
diff --git a/net/cert/cert_verify_proc.cc b/net/cert/cert_verify_proc.cc
index 3bd3735..eea6a0b 100644
--- a/net/cert/cert_verify_proc.cc
+++ b/net/cert/cert_verify_proc.cc
@@ -87,16 +87,12 @@ int CertVerifyProc::Verify(X509Certificate* cert,
     return ERR_CERT_REVOKED;
   }
 
-  // If EV verification was requested and no CRLSet is present, or if the
-  // CRLSet has expired, then enable online revocation checks. If the online
-  // check fails, EV status won't be shown.
-  //
+  // We do online revocation checking for EV certificates that aren't covered
+  // by a fresh CRLSet.
   // TODO(rsleevi): http://crbug.com/142974 - Allow preferences to fully
   // disable revocation checking.
-  if ((flags & CertVerifier::VERIFY_EV_CERT) &&
-      (!crl_set || crl_set->IsExpired())) {
+  if (flags & CertVerifier::VERIFY_EV_CERT)
     flags |= CertVerifier::VERIFY_REV_CHECKING_ENABLED_EV_ONLY;
-  }
 
   int rv = VerifyInternal(cert, hostname, flags, crl_set,
                           additional_trust_anchors, verify_result);
diff --git a/net/cert/cert_verify_proc_nss.cc b/net/cert/cert_verify_proc_nss.cc
index e9250f9..120f091 100644
--- a/net/cert/cert_verify_proc_nss.cc
+++ b/net/cert/cert_verify_proc_nss.cc
@@ -258,16 +258,18 @@ bool IsAdditionalTrustAnchor(CERTCertList* additional_trust_anchors,
 }
 
 enum CRLSetResult {
-  kCRLSetRevoked,
   kCRLSetOk,
-  kCRLSetError,
+  kCRLSetRevoked,
+  kCRLSetUnknown,
 };
 
 // CheckRevocationWithCRLSet attempts to check each element of |cert_list|
 // against |crl_set|. It returns:
 //   kCRLSetRevoked: if any element of the chain is known to have been revoked.
-//   kCRLSetError: if an error occurs in processing.
-//   kCRLSetOk: if no element in the chain is known to have been revoked.
+//   kCRLSetUnknown: if there is no fresh information about some element in
+//       the chain.
+//   kCRLSetOk: if every element in the chain is covered by a fresh CRLSet and
+//       is unrevoked.
 CRLSetResult CheckRevocationWithCRLSet(CERTCertList* cert_list,
                                        CERTCertificate* root,
                                        CRLSet* crl_set) {
@@ -283,6 +285,8 @@ CRLSetResult CheckRevocationWithCRLSet(CERTCertList* cert_list,
   if (root)
     certs.push_back(root);
 
+  bool covered = true;
+
   // We iterate from the root certificate down to the leaf, keeping track of
   // the issuer's SPKI at each step.
   std::string issuer_spki_hash;
@@ -296,7 +300,8 @@ CRLSetResult CheckRevocationWithCRLSet(CERTCertList* cert_list,
     base::StringPiece spki;
     if (!asn1::ExtractSPKIFromDERCert(der, &spki)) {
       NOTREACHED();
-      return kCRLSetError;
+      covered = false;
+      continue;
     }
     const std::string spki_hash = crypto::SHA256HashString(spki);
 
@@ -315,14 +320,19 @@ CRLSetResult CheckRevocationWithCRLSet(CERTCertList* cert_list,
       case CRLSet::REVOKED:
         return kCRLSetRevoked;
       case CRLSet::UNKNOWN:
+        covered = false;
+        continue;
       case CRLSet::GOOD:
         continue;
       default:
         NOTREACHED();
-        return kCRLSetError;
+        covered = false;
+        continue;
     }
   }
 
+  if (!covered || crl_set->IsExpired())
+    return kCRLSetUnknown;
   return kCRLSetOk;
 }
 
@@ -684,6 +694,7 @@ bool IsEVCandidate(EVRootCAMetadata* metadata,
 bool VerifyEV(CERTCertificate* cert_handle,
               int flags,
               CRLSet* crl_set,
+              bool rev_checking_enabled,
               EVRootCAMetadata* metadata,
               SECOidTag ev_policy_oid,
               CERTCertList* additional_trust_anchors) {
@@ -700,10 +711,6 @@ bool VerifyEV(CERTCertificate* cert_handle,
   cvout[cvout_index].type = cert_po_end;
   ScopedCERTValOutParam scoped_cvout(cvout);
 
-  bool rev_checking_enabled =
-      (flags & CertVerifier::VERIFY_REV_CHECKING_ENABLED) ||
-      (flags & CertVerifier::VERIFY_REV_CHECKING_ENABLED_EV_ONLY);
-
   SECStatus status = PKIXVerifyCert(
       cert_handle,
       rev_checking_enabled,
@@ -817,9 +824,7 @@ int CertVerifyProcNSS::VerifyInternal(
   bool cert_io_enabled = flags & CertVerifier::VERIFY_CERT_IO_ENABLED;
   bool check_revocation =
       cert_io_enabled &&
-      ((flags & CertVerifier::VERIFY_REV_CHECKING_ENABLED) ||
-       ((flags & CertVerifier::VERIFY_REV_CHECKING_ENABLED_EV_ONLY) &&
-        is_ev_candidate));
+      (flags & CertVerifier::VERIFY_REV_CHECKING_ENABLED);
   if (check_revocation)
     verify_result->cert_status |= CERT_STATUS_REV_CHECKING_ENABLED;
 
@@ -863,8 +868,9 @@ int CertVerifyProcNSS::VerifyInternal(
                      verify_result);
   }
 
+  CRLSetResult crl_set_result = kCRLSetUnknown;
   if (crl_set) {
-    CRLSetResult crl_set_result = CheckRevocationWithCRLSet(
+    crl_set_result = CheckRevocationWithCRLSet(
         cvout[cvout_cert_list_index].value.pointer.chain,
         cvout[cvout_trust_anchor_index].value.pointer.cert,
         crl_set);
@@ -895,10 +901,18 @@ int CertVerifyProcNSS::VerifyInternal(
   if (IsCertStatusError(verify_result->cert_status))
     return MapCertStatusToNetError(verify_result->cert_status);
 
-  if ((flags & CertVerifier::VERIFY_EV_CERT) && is_ev_candidate &&
-      VerifyEV(cert_handle, flags, crl_set, metadata, ev_policy_oid,
-               trust_anchors.get())) {
-    verify_result->cert_status |= CERT_STATUS_IS_EV;
+  if ((flags & CertVerifier::VERIFY_EV_CERT) && is_ev_candidate) {
+    check_revocation |=
+        crl_set_result != kCRLSetOk &&
+        cert_io_enabled &&
+        (flags & CertVerifier::VERIFY_REV_CHECKING_ENABLED_EV_ONLY);
+    if (check_revocation)
+      verify_result->cert_status |= CERT_STATUS_REV_CHECKING_ENABLED;
+
+    if (VerifyEV(cert_handle, flags, crl_set, check_revocation, metadata,
+                 ev_policy_oid, trust_anchors.get())) {
+      verify_result->cert_status |= CERT_STATUS_IS_EV;
+    }
   }
 
   return OK;
diff --git a/net/cert/cert_verify_proc_unittest.cc b/net/cert/cert_verify_proc_unittest.cc
index 21b1439..21f79d6 100644
--- a/net/cert/cert_verify_proc_unittest.cc
+++ b/net/cert/cert_verify_proc_unittest.cc
@@ -155,7 +155,7 @@ TEST_F(CertVerifyProcTest, MAYBE_EVVerification) {
       X509Certificate::CreateFromHandle(certs[0]->os_cert_handle(),
                                         intermediates);
 
-  scoped_refptr<CRLSet> crl_set(CRLSet::EmptyCRLSetForTesting());
+  scoped_refptr<CRLSet> crl_set(CRLSet::ForTesting(false, NULL, ""));
   CertVerifyResult verify_result;
   int flags = CertVerifier::VERIFY_EV_CERT;
   int error = Verify(comodo_chain.get(),
diff --git a/net/cert/cert_verify_proc_win.cc b/net/cert/cert_verify_proc_win.cc
index efeb7f4..7e94246 100644
--- a/net/cert/cert_verify_proc_win.cc
+++ b/net/cert/cert_verify_proc_win.cc
@@ -376,17 +376,32 @@ void GetCertPoliciesInfo(PCCERT_CONTEXT cert,
     output->reset(policies_info);
 }
 
-bool CheckRevocationWithCRLSet(PCCERT_CHAIN_CONTEXT chain,
-                               CRLSet* crl_set) {
+enum CRLSetResult {
+  kCRLSetOk,
+  kCRLSetUnknown,
+  kCRLSetRevoked,
+};
+
+// CheckRevocationWithCRLSet attempts to check each element of |chain|
+// against |crl_set|. It returns:
+//   kCRLSetRevoked: if any element of the chain is known to have been revoked.
+//   kCRLSetUnknown: if there is no fresh information about some element in
+//       the chain.
+//   kCRLSetOk: if every element in the chain is covered by a fresh CRLSet and
+//       is unrevoked.
+CRLSetResult CheckRevocationWithCRLSet(PCCERT_CHAIN_CONTEXT chain,
+                                       CRLSet* crl_set) {
   if (chain->cChain == 0)
-    return true;
+    return kCRLSetOk;
 
   const PCERT_SIMPLE_CHAIN first_chain = chain->rgpChain[0];
   const PCERT_CHAIN_ELEMENT* element = first_chain->rgpElement;
 
   const int num_elements = first_chain->cElement;
   if (num_elements == 0)
-    return true;
+    return kCRLSetOk;
+
+  bool covered = true;
 
   // We iterate from the root certificate down to the leaf, keeping track of
   // the issuer's SPKI at each step.
@@ -401,6 +416,7 @@ bool CheckRevocationWithCRLSet(PCCERT_CHAIN_CONTEXT chain,
     base::StringPiece spki;
     if (!asn1::ExtractSPKIFromDERCert(der_bytes, &spki)) {
       NOTREACHED();
+      covered = false;
       continue;
     }
 
@@ -423,17 +439,22 @@ bool CheckRevocationWithCRLSet(PCCERT_CHAIN_CONTEXT chain,
 
     switch (result) {
       case CRLSet::REVOKED:
-        return false;
+        return kCRLSetRevoked;
       case CRLSet::UNKNOWN:
+        covered = false;
+        continue;
       case CRLSet::GOOD:
         continue;
       default:
         NOTREACHED();
+        covered = false;
         continue;
     }
   }
 
-  return true;
+  if (!covered || crl_set->IsExpired())
+    return kCRLSetUnknown;
+  return kCRLSetOk;
 }
 
 void AppendPublicKeyHashes(PCCERT_CHAIN_CONTEXT chain,
@@ -573,9 +594,7 @@ int CertVerifyProcWin::VerifyInternal(
   DWORD chain_flags = CERT_CHAIN_CACHE_END_CERT |
                       CERT_CHAIN_REVOCATION_CHECK_CHAIN_EXCLUDE_ROOT;
   bool rev_checking_enabled =
-      (flags & CertVerifier::VERIFY_REV_CHECKING_ENABLED) ||
-      (ev_policy_oid != NULL &&
-       (flags & CertVerifier::VERIFY_REV_CHECKING_ENABLED_EV_ONLY));
+      (flags & CertVerifier::VERIFY_REV_CHECKING_ENABLED);
 
   if (rev_checking_enabled) {
     verify_result->cert_status |= CERT_STATUS_REV_CHECKING_ENABLED;
@@ -612,6 +631,36 @@ int CertVerifyProcWin::VerifyInternal(
     return MapSecurityError(GetLastError());
   }
 
+  CRLSetResult crl_set_result = kCRLSetUnknown;
+  if (crl_set)
+    crl_set_result = CheckRevocationWithCRLSet(chain_context, crl_set);
+
+  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());
+    }
+  }
+
   if (chain_context->TrustStatus.dwErrorStatus &
       CERT_TRUST_IS_NOT_VALID_FOR_USAGE) {
     ev_policy_oid = NULL;
@@ -672,9 +721,6 @@ int CertVerifyProcWin::VerifyInternal(
   if (CertSubjectCommonNameHasNull(cert_handle))
     verify_result->cert_status |= CERT_STATUS_INVALID;
 
-  if (crl_set && !CheckRevocationWithCRLSet(chain_context, crl_set))
-    verify_result->cert_status |= CERT_STATUS_REVOKED;
-
   std::wstring wstr_hostname = ASCIIToWide(hostname);
 
   SSL_EXTRA_CERT_CHAIN_POLICY_PARA extra_policy_para;
diff --git a/net/cert/crl_set.cc b/net/cert/crl_set.cc
index 2ef1b1f..de06516 100644
--- a/net/cert/crl_set.cc
+++ b/net/cert/crl_set.cc
@@ -581,12 +581,30 @@ const CRLSet::CRLList& CRLSet::crls() const {
 
 // static
 CRLSet* CRLSet::EmptyCRLSetForTesting() {
-  return new CRLSet;
+  return ForTesting(false, NULL, "");
 }
 
 CRLSet* CRLSet::ExpiredCRLSetForTesting() {
+  return ForTesting(true, NULL, "");
+}
+
+// static
+CRLSet* CRLSet::ForTesting(bool is_expired,
+                           const SHA256HashValue* issuer_spki,
+                           const std::string& serial_number) {
   CRLSet* crl_set = new CRLSet;
-  crl_set->not_after_ = 1;
+  if (is_expired)
+    crl_set->not_after_ = 1;
+  if (issuer_spki != NULL) {
+    const std::string spki(reinterpret_cast<const char*>(issuer_spki->data),
+                           sizeof(issuer_spki->data));
+    crl_set->crls_.push_back(make_pair(spki, std::vector<std::string>()));
+    crl_set->crls_index_by_issuer_[spki] = 0;
+  }
+
+  if (!serial_number.empty())
+    crl_set->crls_[0].second.push_back(serial_number);
+
   return crl_set;
 }
 
diff --git a/net/cert/crl_set.h b/net/cert/crl_set.h
index 511654d..348005f 100644
--- a/net/cert/crl_set.h
+++ b/net/cert/crl_set.h
@@ -13,6 +13,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/strings/string_piece.h"
 #include "net/base/net_export.h"
+#include "net/cert/x509_cert_types.h"
 
 namespace base {
 class DictionaryValue;
@@ -88,6 +89,15 @@ class NET_EXPORT CRLSet : public base::RefCountedThreadSafe<CRLSet> {
   // ExpiredCRLSetForTesting returns a expired, empty CRLSet for unit tests.
   static CRLSet* ExpiredCRLSetForTesting();
 
+  // ForTesting returns a CRLSet for testing. If |is_expired| is true, calling
+  // IsExpired on the result will return true. If |issuer_spki| is not NULL,
+  // the CRLSet will cover certificates issued by that SPKI. If |serial_number|
+  // is not emtpy, then that big-endian serial number will be considered to
+  // have been revoked by |issuer_spki|.
+  static CRLSet* ForTesting(bool is_expired,
+                            const SHA256HashValue* issuer_spki,
+                            const std::string& serial_number);
+
  private:
   CRLSet();
   ~CRLSet();
diff --git a/net/test/spawned_test_server/base_test_server.cc b/net/test/spawned_test_server/base_test_server.cc
index bf38b4e..fc586fd 100644
--- a/net/test/spawned_test_server/base_test_server.cc
+++ b/net/test/spawned_test_server/base_test_server.cc
@@ -56,6 +56,7 @@ void GetCiphersList(int cipher, base::ListValue* values) {
 BaseTestServer::SSLOptions::SSLOptions()
     : server_certificate(CERT_OK),
       ocsp_status(OCSP_OK),
+      cert_serial(0),
       request_client_certificate(false),
       bulk_ciphers(SSLOptions::BULK_CIPHER_ANY),
       record_resume(false),
@@ -64,6 +65,8 @@ BaseTestServer::SSLOptions::SSLOptions()
 BaseTestServer::SSLOptions::SSLOptions(
     BaseTestServer::SSLOptions::ServerCertificate cert)
     : server_certificate(cert),
+      ocsp_status(OCSP_OK),
+      cert_serial(0),
       request_client_certificate(false),
       bulk_ciphers(SSLOptions::BULK_CIPHER_ANY),
       record_resume(false),
@@ -375,6 +378,10 @@ bool BaseTestServer::GenerateArguments(base::DictionaryValue* arguments) const {
     if (!ocsp_arg.empty())
       arguments->SetString("ocsp", ocsp_arg);
 
+    if (ssl_options_.cert_serial != 0)
+      arguments->Set("cert-serial",
+                     base::Value::CreateIntegerValue(ssl_options_.cert_serial));
+
     // Check bulk cipher argument.
     scoped_ptr<base::ListValue> bulk_cipher_values(new base::ListValue());
     GetCiphersList(ssl_options_.bulk_ciphers, bulk_cipher_values.get());
diff --git a/net/test/spawned_test_server/base_test_server.h b/net/test/spawned_test_server/base_test_server.h
index 9691f5d..289c76b 100644
--- a/net/test/spawned_test_server/base_test_server.h
+++ b/net/test/spawned_test_server/base_test_server.h
@@ -120,6 +120,10 @@ class BaseTestServer {
     // response returned.
     OCSPStatus ocsp_status;
 
+    // If not zero, |serial| will be the serial number of the auto-generated
+    // leaf certificate when |server_certificate==CERT_AUTO|.
+    uint64 cert_serial;
+
     // True if a CertificateRequest should be sent to the client during
     // handshaking.
     bool request_client_certificate;
diff --git a/net/tools/testserver/minica.py b/net/tools/testserver/minica.py
index bfe896f..2dd38ef 100644
--- a/net/tools/testserver/minica.py
+++ b/net/tools/testserver/minica.py
@@ -323,14 +323,16 @@ unauthorizedDER = '30030a0106'.decode('hex')
 
 def GenerateCertKeyAndOCSP(subject = "127.0.0.1",
                            ocsp_url = "http://127.0.0.1",
-                           ocsp_state = OCSP_STATE_GOOD):
+                           ocsp_state = OCSP_STATE_GOOD,
+                           serial = 0):
   '''GenerateCertKeyAndOCSP returns a (cert_and_key_pem, ocsp_der) where:
        * cert_and_key_pem contains a certificate and private key in PEM format
          with the given subject common name and OCSP URL.
        * ocsp_der contains a DER encoded OCSP response or None if ocsp_url is
          None'''
 
-  serial = RandomNumber(16)
+  if serial == 0:
+    serial = RandomNumber(16)
   cert_der = MakeCertificate(ISSUER_CN, bytes(subject), serial, KEY, KEY,
                              bytes(ocsp_url))
   cert_pem = DERToPEM(cert_der)
diff --git a/net/tools/testserver/testserver.py b/net/tools/testserver/testserver.py
index 0857847..77a3142 100755
--- a/net/tools/testserver/testserver.py
+++ b/net/tools/testserver/testserver.py
@@ -1884,7 +1884,8 @@ class ServerRunner(testserver_base.TestServerRunner):
               subject = "127.0.0.1",
               ocsp_url = ("http://%s:%d/ocsp" %
                   (host, self.__ocsp_server.server_port)),
-              ocsp_state = ocsp_state)
+              ocsp_state = ocsp_state,
+              serial = self.options.cert_serial)
 
           self.__ocsp_server.ocsp_response = ocsp_der
 
@@ -2024,6 +2025,10 @@ class ServerRunner(testserver_base.TestServerRunner):
                                   help='The type of OCSP response generated '
                                   'for the automatically generated '
                                   'certificate. One of [ok,revoked,invalid]')
+    self.option_parser.add_option('--cert-serial', dest='cert_serial',
+                                  default=0, type=int,
+                                  help='If non-zero then the generated '
+                                  'certificate will have this serial number')
     self.option_parser.add_option('--tls-intolerant', dest='tls_intolerant',
                                   default='0', type='int',
                                   help='If nonzero, certain TLS connections '
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index bbffbe2..64f5ac1 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -5439,6 +5439,15 @@ static const SHA1HashValue kOCSPTestCertFingerprint =
   { { 0xf1, 0xad, 0xf6, 0xce, 0x42, 0xac, 0xe7, 0xb4, 0xf4, 0x24,
       0xdb, 0x1a, 0xf7, 0xa0, 0x9f, 0x09, 0xa1, 0xea, 0xf1, 0x5c } };
 
+// This is the SHA256, SPKI hash of the "Testing CA" certificate used by the
+// testserver.
+static const SHA256HashValue kOCSPTestCertSPKI = { {
+  0xee, 0xe6, 0x51, 0x2d, 0x4c, 0xfa, 0xf7, 0x3e,
+  0x6c, 0xd8, 0xca, 0x67, 0xed, 0xb5, 0x5d, 0x49,
+  0x76, 0xe1, 0x52, 0xa7, 0x6e, 0x0e, 0xa0, 0x74,
+  0x09, 0x75, 0xe6, 0x23, 0x24, 0xbd, 0x1b, 0x28,
+} };
+
 // This is the policy OID contained in the certificates that testserver
 // generates.
 static const char kOCSPTestCertPolicy[] = "1.3.6.1.4.1.11129.2.4.1";
@@ -5747,24 +5756,55 @@ TEST_F(HTTPSEVCRLSetTest, ExpiredCRLSet) {
             static_cast<bool>(cert_status & CERT_STATUS_REV_CHECKING_ENABLED));
 }
 
-TEST_F(HTTPSEVCRLSetTest, FreshCRLSet) {
+TEST_F(HTTPSEVCRLSetTest, FreshCRLSetCovered) {
+  if (!SystemSupportsOCSP()) {
+    LOG(WARNING) << "Skipping test because system doesn't support OCSP";
+    return;
+  }
+
   SpawnedTestServer::SSLOptions ssl_options(
       SpawnedTestServer::SSLOptions::CERT_AUTO);
   ssl_options.ocsp_status = SpawnedTestServer::SSLOptions::OCSP_INVALID;
   SSLConfigService::SetCRLSet(
-      scoped_refptr<CRLSet>(CRLSet::EmptyCRLSetForTesting()));
+      scoped_refptr<CRLSet>(CRLSet::ForTesting(
+          false, &kOCSPTestCertSPKI, "")));
 
   CertStatus cert_status;
   DoConnection(ssl_options, &cert_status);
 
-  // With a valid, fresh CRLSet the bad OCSP response shouldn't matter because
-  // we wont check it.
+  // With a fresh CRLSet that covers the issuing certificate, we shouldn't do a
+  // revocation check for EV.
   EXPECT_EQ(0u, cert_status & CERT_STATUS_ALL_ERRORS);
-
   EXPECT_EQ(SystemUsesChromiumEVMetadata(),
             static_cast<bool>(cert_status & CERT_STATUS_IS_EV));
+  EXPECT_FALSE(
+      static_cast<bool>(cert_status & CERT_STATUS_REV_CHECKING_ENABLED));
+}
 
-  EXPECT_FALSE(cert_status & CERT_STATUS_REV_CHECKING_ENABLED);
+TEST_F(HTTPSEVCRLSetTest, FreshCRLSetNotCovered) {
+  if (!SystemSupportsOCSP()) {
+    LOG(WARNING) << "Skipping test because system doesn't support OCSP";
+    return;
+  }
+
+  SpawnedTestServer::SSLOptions ssl_options(
+      SpawnedTestServer::SSLOptions::CERT_AUTO);
+  ssl_options.ocsp_status = SpawnedTestServer::SSLOptions::OCSP_INVALID;
+  SSLConfigService::SetCRLSet(
+      scoped_refptr<CRLSet>(CRLSet::EmptyCRLSetForTesting()));
+
+  CertStatus cert_status = 0;
+  DoConnection(ssl_options, &cert_status);
+
+  // Even with a fresh CRLSet, we should still do online revocation checks when
+  // the certificate chain isn't covered by the CRLSet, which it isn't in this
+  // test.
+  EXPECT_EQ(ExpectedCertStatusForFailedOnlineRevocationCheck(),
+            cert_status & CERT_STATUS_ALL_ERRORS);
+
+  EXPECT_FALSE(cert_status & CERT_STATUS_IS_EV);
+  EXPECT_EQ(SystemUsesChromiumEVMetadata(),
+            static_cast<bool>(cert_status & CERT_STATUS_REV_CHECKING_ENABLED));
 }
 
 TEST_F(HTTPSEVCRLSetTest, ExpiredCRLSetAndRevokedNonEVCert) {
@@ -5822,6 +5862,31 @@ TEST_F(HTTPSCRLSetTest, ExpiredCRLSet) {
   EXPECT_FALSE(cert_status & CERT_STATUS_IS_EV);
   EXPECT_FALSE(cert_status & CERT_STATUS_REV_CHECKING_ENABLED);
 }
+
+TEST_F(HTTPSCRLSetTest, CRLSetRevoked) {
+#if defined(USE_OPENSSL)
+  LOG(WARNING) << "Skipping test because system doesn't support CRLSets";
+  return;
+#endif
+
+  SpawnedTestServer::SSLOptions ssl_options(
+      SpawnedTestServer::SSLOptions::CERT_AUTO);
+  ssl_options.ocsp_status = SpawnedTestServer::SSLOptions::OCSP_OK;
+  ssl_options.cert_serial = 10;
+  SSLConfigService::SetCRLSet(
+      scoped_refptr<CRLSet>(CRLSet::ForTesting(
+          false, &kOCSPTestCertSPKI, "\x0a")));
+
+  CertStatus cert_status = 0;
+  DoConnection(ssl_options, &cert_status);
+
+  // If the certificate is recorded as revoked in the CRLSet, that should be
+  // reflected without online revocation checking.
+  EXPECT_EQ(CERT_STATUS_REVOKED, cert_status & CERT_STATUS_ALL_ERRORS);
+  EXPECT_FALSE(cert_status & CERT_STATUS_IS_EV);
+  EXPECT_FALSE(
+      static_cast<bool>(cert_status & CERT_STATUS_REV_CHECKING_ENABLED));
+}
 #endif  // !defined(OS_IOS)
 
 #if !defined(DISABLE_FTP_SUPPORT)