net: fallback to online revocation checks for EV status when CRLSet has expired.

After this change our CRLSet logic is:
  * If we have a fresh CRLSet then we don't do online revocation checks unless the
    user has configured them. (It can be configured either via the settings UI,
    or with the EnableOnlineRevocationChecks policy option.)
  * If we don't have a CRLSet, or if it has expired, and we're trying EV verification,
    then we require a positive online revocation check in order to show the EV badge.
    An invalid revocation check reply will prevent the EV badge, but not hard-fail
    the whole verification.

BUG=none
TEST=net_unittests

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

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

9 files changed, 58 insertions, 36 deletions

diff --git a/net/base/crl_set.cc b/net/base/crl_set.cc
index 0818a22..76d6bc3 100644
--- a/net/base/crl_set.cc
+++ b/net/base/crl_set.cc
@@ -540,22 +540,6 @@ CRLSet::Result CRLSet::CheckSPKI(const base::StringPiece& spki_hash) const {
 CRLSet::Result CRLSet::CheckSerial(
     const base::StringPiece& serial_number,
     const base::StringPiece& issuer_spki_hash) const {
-  Result result = CheckSerialIsRevoked(serial_number, issuer_spki_hash);
-  // If we get a revoked signal then we return that no matter how old the
-  // CRLSet is.
-  if (result == REVOKED)
-    return result;
-  if (not_after_ > 0) {
-    uint64 now = base::Time::Now().ToTimeT();
-    if (now > not_after_)
-      return CRL_SET_EXPIRED;
-  }
-  return result;
-}
-
-CRLSet::Result CRLSet::CheckSerialIsRevoked(
-    const base::StringPiece& serial_number,
-    const base::StringPiece& issuer_spki_hash) const {
   base::StringPiece serial(serial_number);
 
   if (!serial.empty() && (serial[0] & 0x80) != 0) {
@@ -583,6 +567,14 @@ CRLSet::Result CRLSet::CheckSerialIsRevoked(
   return GOOD;
 }
 
+bool CRLSet::IsExpired() const {
+  if (not_after_ == 0)
+    return false;
+
+  uint64 now = base::Time::Now().ToTimeT();
+  return now > not_after_;
+}
+
 uint32 CRLSet::sequence() const {
   return sequence_;
 }
@@ -591,4 +583,15 @@ const CRLSet::CRLList& CRLSet::crls() const {
   return crls_;
 }
 
+// static
+CRLSet* CRLSet::EmptyCRLSetForTesting() {
+  return new CRLSet;
+}
+
+CRLSet* CRLSet::ExpiredCRLSetForTesting() {
+  CRLSet* crl_set = new CRLSet;
+  crl_set->not_after_ = 1;
+  return crl_set;
+}
+
 }  // namespace net
diff --git a/net/base/crl_set.h b/net/base/crl_set.h
index 04d7203..b75e11a 100644
--- a/net/base/crl_set.h
+++ b/net/base/crl_set.h
@@ -32,7 +32,6 @@ class NET_EXPORT CRLSet : public base::RefCountedThreadSafe<CRLSet> {
     REVOKED,  // the certificate should be rejected.
     UNKNOWN,  // the CRL for the certificate is not included in the set.
     GOOD,     // the certificate is not listed.
-    CRL_SET_EXPIRED, // the CRLSet has expired.
   };
 
   ~CRLSet();
@@ -55,6 +54,10 @@ class NET_EXPORT CRLSet : public base::RefCountedThreadSafe<CRLSet> {
       const base::StringPiece& serial_number,
       const base::StringPiece& issuer_spki_hash) const;
 
+  // IsExpired returns true iff the current time is past the NotAfter time
+  // specified in the CRLSet.
+  bool IsExpired() const;
+
   // ApplyDelta returns a new CRLSet in |out_crl_set| that is the result of
   // updating the current CRL set with the delta information in |delta_bytes|.
   bool ApplyDelta(const base::StringPiece& delta_bytes,
@@ -84,6 +87,12 @@ class NET_EXPORT CRLSet : public base::RefCountedThreadSafe<CRLSet> {
   // testing.
   const CRLList& crls() const;
 
+  // EmptyCRLSetForTesting returns a valid, but empty, CRLSet for unit tests.
+  static CRLSet* EmptyCRLSetForTesting();
+
+  // ExpiredCRLSetForTesting returns a expired, empty CRLSet for unit tests.
+  static CRLSet* ExpiredCRLSetForTesting();
+
  private:
   CRLSet();
 
@@ -91,11 +100,6 @@ class NET_EXPORT CRLSet : public base::RefCountedThreadSafe<CRLSet> {
   // from "BlockedSPKIs" in |header_dict|.
   bool CopyBlockedSPKIsFromHeader(base::DictionaryValue* header_dict);
 
-  // CheckSerialIsRevoked is a helper function for |CheckSerial|.
-  Result CheckSerialIsRevoked(
-      const base::StringPiece& serial_number,
-      const base::StringPiece& issuer_spki_hash) const;
-
   uint32 sequence_;
   CRLList crls_;
   // not_after_ contains the time, in UNIX epoch seconds, after which the
diff --git a/net/base/crl_set_unittest.cc b/net/base/crl_set_unittest.cc
index d1265b7..42fa82a 100644
--- a/net/base/crl_set_unittest.cc
+++ b/net/base/crl_set_unittest.cc
@@ -208,6 +208,8 @@ TEST(CRLSetTest, Parse) {
   EXPECT_EQ(net::CRLSet::GOOD, set->CheckSerial(
       std::string("\x47\x54\x3E\x79\x00\x03\x00\x00\x14\xF5", 10),
       gia_spki_hash));
+
+  EXPECT_FALSE(set->IsExpired());
 }
 
 TEST(CRLSetTest, NoOpDeltaUpdate) {
@@ -311,7 +313,7 @@ TEST(CRLSetTest, BlockedSPKIs) {
       reinterpret_cast<const char*>(spki_hash)));
 }
 
-TEST(CRLSetTest, Expires) {
+TEST(CRLSetTest, Expired) {
   // This CRLSet has an expiry value set to one second past midnight, 1st Jan,
   // 1970.
   base::StringPiece s(reinterpret_cast<const char*>(kExpiredCRLSet),
@@ -320,6 +322,5 @@ TEST(CRLSetTest, Expires) {
   EXPECT_TRUE(net::CRLSet::Parse(s, &set));
   ASSERT_TRUE(set.get() != NULL);
 
-  EXPECT_EQ(net::CRLSet::CRL_SET_EXPIRED, set->CheckSerial(
-      std::string("\x01", 1), ""));
+  EXPECT_TRUE(set->IsExpired());
 }
diff --git a/net/base/x509_certificate.cc b/net/base/x509_certificate.cc
index a4ae89b..ca8299e 100644
--- a/net/base/x509_certificate.cc
+++ b/net/base/x509_certificate.cc
@@ -25,6 +25,7 @@
 #include "googleurl/src/url_canon_ip.h"
 #include "net/base/cert_status_flags.h"
 #include "net/base/cert_verify_result.h"
+#include "net/base/crl_set.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_util.h"
 #include "net/base/pem_tokenizer.h"
@@ -605,6 +606,16 @@ int X509Certificate::Verify(const std::string& hostname,
     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.
+  //
+  // A possible optimisation is to only enable online revocation checking in
+  // the event that the leaf certificate appears to include a EV policy ID.
+  // However, it's expected that having a current CRLSet will be very common.
+  if ((flags & VERIFY_EV_CERT) && (!crl_set || crl_set->IsExpired()))
+    flags |= VERIFY_REV_CHECKING_ENABLED;
+
   int rv = VerifyInternal(hostname, flags, crl_set, verify_result);
 
   // This check is done after VerifyInternal so that VerifyInternal can fill in
diff --git a/net/base/x509_certificate.h b/net/base/x509_certificate.h
index d4d7478..5c07284 100644
--- a/net/base/x509_certificate.h
+++ b/net/base/x509_certificate.h
@@ -495,7 +495,7 @@ class NET_EXPORT X509Certificate
 
 #if defined(OS_WIN)
   bool CheckEV(PCCERT_CHAIN_CONTEXT chain_context,
-               int flags,
+               bool rev_checking_enabled,
                const char* policy_oid) const;
   static bool IsIssuedByKnownRoot(PCCERT_CHAIN_CONTEXT chain_context);
 #endif
diff --git a/net/base/x509_certificate_mac.cc b/net/base/x509_certificate_mac.cc
index 9103152..af72451 100644
--- a/net/base/x509_certificate_mac.cc
+++ b/net/base/x509_certificate_mac.cc
@@ -732,7 +732,6 @@ bool CheckRevocationWithCRLSet(CFArrayRef chain, CRLSet* crl_set) {
         return false;
       case CRLSet::UNKNOWN:
       case CRLSet::GOOD:
-      case CRLSet::CRL_SET_EXPIRED:
         continue;
       default:
         NOTREACHED();
diff --git a/net/base/x509_certificate_nss.cc b/net/base/x509_certificate_nss.cc
index cd9c46d..2b8ad84 100644
--- a/net/base/x509_certificate_nss.cc
+++ b/net/base/x509_certificate_nss.cc
@@ -316,7 +316,6 @@ CRLSetResult CheckRevocationWithCRLSet(CERTCertList* cert_list,
         return kCRLSetRevoked;
       case CRLSet::UNKNOWN:
       case CRLSet::GOOD:
-      case CRLSet::CRL_SET_EXPIRED:
         continue;
       default:
         NOTREACHED();
diff --git a/net/base/x509_certificate_unittest.cc b/net/base/x509_certificate_unittest.cc
index e54e043..82893bd 100644
--- a/net/base/x509_certificate_unittest.cc
+++ b/net/base/x509_certificate_unittest.cc
@@ -365,10 +365,11 @@ TEST(X509CertificateTest, MAYBE_EVVerification) {
       X509Certificate::CreateFromHandle(certs[0]->os_cert_handle(),
                                         intermediates);
 
+  scoped_ptr<CRLSet> crl_set(CRLSet::EmptyCRLSetForTesting());
   CertVerifyResult verify_result;
   int flags = X509Certificate::VERIFY_EV_CERT;
   int error = comodo_chain->Verify(
-      "comodo.com", flags, NULL, &verify_result);
+      "comodo.com", flags, crl_set.get(), &verify_result);
   EXPECT_EQ(OK, error);
   EXPECT_TRUE(verify_result.cert_status & CERT_STATUS_IS_EV);
 }
diff --git a/net/base/x509_certificate_win.cc b/net/base/x509_certificate_win.cc
index 2f1bcac..a80833f 100644
--- a/net/base/x509_certificate_win.cc
+++ b/net/base/x509_certificate_win.cc
@@ -498,7 +498,6 @@ bool CheckRevocationWithCRLSet(PCCERT_CHAIN_CONTEXT chain,
         return false;
       case CRLSet::UNKNOWN:
       case CRLSet::GOOD:
-      case CRLSet::CRL_SET_EXPIRED:
         continue;
       default:
         NOTREACHED();
@@ -730,7 +729,9 @@ int X509Certificate::VerifyInternal(const std::string& hostname,
   // We can set CERT_CHAIN_RETURN_LOWER_QUALITY_CONTEXTS to get more chains.
   DWORD chain_flags = CERT_CHAIN_CACHE_END_CERT |
                       CERT_CHAIN_REVOCATION_CHECK_CHAIN_EXCLUDE_ROOT;
-  if (flags & VERIFY_REV_CHECKING_ENABLED) {
+  const bool rev_checking_enabled = flags & VERIFY_REV_CHECKING_ENABLED;
+
+  if (rev_checking_enabled) {
     verify_result->cert_status |= CERT_STATUS_REV_CHECKING_ENABLED;
   } else {
     chain_flags |= CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY;
@@ -915,8 +916,10 @@ int X509Certificate::VerifyInternal(const std::string& hostname,
   AppendPublicKeyHashes(chain_context, &verify_result->public_key_hashes);
   verify_result->is_issued_by_known_root = IsIssuedByKnownRoot(chain_context);
 
-  if (ev_policy_oid && CheckEV(chain_context, flags, ev_policy_oid))
+  if (ev_policy_oid &&
+      CheckEV(chain_context, rev_checking_enabled, ev_policy_oid)) {
     verify_result->cert_status |= CERT_STATUS_IS_EV;
+  }
   return OK;
 }
 
@@ -937,7 +940,7 @@ bool X509Certificate::GetDEREncoded(X509Certificate::OSCertHandle cert_handle,
 // of the EV Certificate Guidelines Version 1.0 at
 // http://cabforum.org/EV_Certificate_Guidelines.pdf.
 bool X509Certificate::CheckEV(PCCERT_CHAIN_CONTEXT chain_context,
-                              int flags,
+                              bool rev_checking_enabled,
                               const char* policy_oid) const {
   DCHECK_NE(static_cast<DWORD>(0), chain_context->cChain);
   // If the cert doesn't match any of the policies, the
@@ -945,11 +948,12 @@ bool X509Certificate::CheckEV(PCCERT_CHAIN_CONTEXT chain_context,
   // chain_context->TrustStatus.dwErrorStatus is set.
   DWORD error_status = chain_context->TrustStatus.dwErrorStatus;
 
-  if (!(flags & VERIFY_REV_CHECKING_ENABLED)) {
+  if (!rev_checking_enabled) {
     // If online revocation checking is disabled then we will have still
     // requested that the revocation cache be checked. However, that will often
-    // cause the following two error bits to be set. Since they are expected,
-    // we mask them away.
+    // cause the following two error bits to be set. These error bits mean that
+    // the local OCSP/CRL is stale or missing entries for these certificates.
+    // Since they are expected, we mask them away.
     error_status &= ~(CERT_TRUST_IS_OFFLINE_REVOCATION |
                       CERT_TRUST_REVOCATION_STATUS_UNKNOWN);
   }