Consider the verification time as well as the expiration time when caching certificate verification…

net/base/multi_threaded_cert_verifier.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/base/multi_threaded_cert_verifier.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ skipping 65 matching lines @@
 
 // The number of seconds for which we'll cache a cache entry.
 const unsigned kTTLSecs = 1800;  // 30 minutes.
 
 }  // namespace
 
 MultiThreadedCertVerifier::CachedResult::CachedResult() : error(ERR_FAILED) {}
 
 MultiThreadedCertVerifier::CachedResult::~CachedResult() {}
 
+MultiThreadedCertVerifier::ValidityRange::ValidityRange(const base::Time& now)
+    : verification_time(now),
+      expiration_time(now) {
+}
+
+MultiThreadedCertVerifier::ValidityRange::ValidityRange(
+    const base::Time& now,
+    const base::Time& expiration)
+    : verification_time(now),
+      expiration_time(expiration) {
+}
+
 // Represents the output and result callback of a request.
 class CertVerifierRequest {
  public:
   CertVerifierRequest(const CompletionCallback& callback,
                       CertVerifyResult* verify_result,
                       const BoundNetLog& net_log)
       : callback_(callback),
         verify_result_(verify_result),
         net_log_(net_log) {
     net_log_.BeginEvent(NetLog::TYPE_CERT_VERIFIER_REQUEST);
@@ skipping 225 matching lines @@
     }
   }
 
   const base::TimeTicks start_time_;
   std::vector<CertVerifierRequest*> requests_;
   CertVerifierWorker* worker_;
   const BoundNetLog net_log_;
 };
 
 MultiThreadedCertVerifier::MultiThreadedCertVerifier()
-    : cache_(kMaxCacheEntries),
+    : cache_(kMaxCacheEntries,
+             &MultiThreadedCertVerifier::CompareValidityRange),
       requests_(0),
       cache_hits_(0),
       inflight_joins_(0),
       verify_proc_(CertVerifyProc::CreateDefault()) {
   CertDatabase::AddObserver(this);
 }
 
 MultiThreadedCertVerifier::~MultiThreadedCertVerifier() {
   STLDeleteValues(&inflight_);
 
@@ skipping 13 matching lines @@
   if (callback.is_null() || !verify_result || hostname.empty()) {
     *out_req = NULL;
     return ERR_INVALID_ARGUMENT;
   }
 
   requests_++;
 
   const RequestParams key(cert->fingerprint(), cert->ca_fingerprint(),
                           hostname, flags);
   const CertVerifierCache::value_type* cached_entry =
-      cache_.Get(key, base::TimeTicks::Now());
+      cache_.Get(key, ValidityRange(base::Time::Now()));
   if (cached_entry) {
     ++cache_hits_;
     *out_req = NULL;
     *verify_result = cached_entry->result;
     return cached_entry->error;
   }
 
   // No cache hit. See if an identical request is currently in flight.
   CertVerifierJob* job;
   std::map<RequestParams, CertVerifierJob*>::const_iterator j;
@@ skipping 28 matching lines @@
   *out_req = request;
   return ERR_IO_PENDING;
 }
 
 void MultiThreadedCertVerifier::CancelRequest(RequestHandle req) {
   DCHECK(CalledOnValidThread());
   CertVerifierRequest* request = reinterpret_cast<CertVerifierRequest*>(req);
   request->Cancel();
 }
 
+// static
+bool MultiThreadedCertVerifier::CompareValidityRange(
+    const ValidityRange& now,
+    const ValidityRange& expiration) {
+  // |now| contains only a single time (verification_time), while |expiration|
+  // contains the validity range - both when the certificate was verified and
+  // when the verification should expire.
+  //
+  // If the user receives a "not yet valid" message, and adjusts their clock
+  // foward to the correct time, this will (typically) cause
+  // now.verification_time to advance past expiration.expiration_time, thus
+  // treating the cached result as an expired entry and re-verifying.
+  // If the user receives a "expired" message, and adjusts their clock backwards
+  // to the correct time, this will cause now.verification_time to be less than
+  // expiration_verification_time, thus treating the cached result as an expired

[cbentzel, 2012/06/18 19:08:28]

Unclear to me: Should now.verification_time < expiration.verification_time be
treated as something different than an expiration issue? 

[Ryan Sleevi, 2012/06/19 00:59:28]

It's a cache expiration issue, but not a cert expiration issue. Hooray for
overloaded terminology.

The point is, if we move into the past, we should re-verify. My experience is
that people are mostly moving into the future, after their CMOS battery
dies/fails (or the CrOS case of no battery, AFAICT), but I wanted to cover both
scenarios of user action.

+  // entry and re-verifying.
+  // If the user receives either of those messages, and does not adjust their
+  // clock, then the result will be (typically) be cached until the expiration
+  // TTL.
+  //
+  // This algorithm is only problematic if the user consistently keeps adjusting
+  // their clock backwards in increments smaller than the expiration TTL, in
+  // which case, cached elements continue to be added. However, because the
+  // cache has a fixed upper bound, if no entries are expired, a 'random' entry
+  // will be, thus keeping the memory constraints bounded over time.
+  return now.verification_time >= expiration.verification_time &&

[cbentzel, 2012/06/18 19:08:28]

DCHECK that now.verification_time == now.expiration_time?

+         now.verification_time < expiration.expiration_time;
+}
+
 // HandleResult is called by CertVerifierWorker on the origin message loop.
 // It deletes CertVerifierJob.
 void MultiThreadedCertVerifier::HandleResult(
     X509Certificate* cert,
     const std::string& hostname,
     int flags,
     int error,
     const CertVerifyResult& verify_result) {
   DCHECK(CalledOnValidThread());
 
   const RequestParams key(cert->fingerprint(), cert->ca_fingerprint(),
                           hostname, flags);
 
   CachedResult cached_result;
   cached_result.error = error;
   cached_result.result = verify_result;
-  cache_.Put(key, cached_result, base::TimeTicks::Now(),
-             base::TimeDelta::FromSeconds(kTTLSecs));
+  base::Time now = base::Time::Now();
+  cache_.Put(key, cached_result, ValidityRange(now),
+             ValidityRange(now, now + base::TimeDelta::FromSeconds(kTTLSecs)));

[szym, 2012/06/18 16:11:37]

If the result is successful, shouldn't we take min(kTTLSecs,
cert->valid_expiry()) ?

[Ryan Sleevi, 2012/06/18 18:00:46]

Possibly. Definitely an edge case. Another edge case even with that approach
would be that expiration time may be influenced by intermediates - that is,
chain validity is the min of all certs' in the chain valid_expiry, which is
something not captured.

It's a small edge case though (a recently expiring cert), and accurately
invalidating it may not be reasonable (eg: do we also invalidate any existing
SSL sessions? What about when we load things from the cache? etc). I think I'm
fine with a small window of incorrectness followed by eventual consistency, but
I'll need to think about the impact some more.

[Ryan Sleevi, 2012/06/19 17:57:34]

In thinking about it, I think it's probably more complexity then needed, and it
still doesn't fully capture the dates at play:
- The validity period of the particular chain built (of which there may be
multiple candidates for different validity periods)
- The validity period of any OCSP/CRL responses
- Whether or not we may be re-using any existing connections (SPDY or SSL
sessions)
- SSL session resumption

Validity periods are primarily to ensure that subscriber information, as
represented in certificates, is accurate and regularly validated. To a lesser
degree, it's also to ensure best practices like key rotation. Immediately
invalidating the cache and treating the cert as expired thus doesn't really add
to the security properties, so I'm inclined to omit it, lest it be confused for
something that does.

[szym, 2012/06/19 18:01:13]

This alone convinced me that it's better to leave it out so that it doesn't give
a false sense of security.

 
   std::map<RequestParams, CertVerifierJob*>::iterator j;
   j = inflight_.find(key);
   if (j == inflight_.end()) {
     NOTREACHED();
     return;
   }
   CertVerifierJob* job = j->second;
   inflight_.erase(j);
 
   job->HandleResult(cached_result);
   delete job;
 }
 
 void MultiThreadedCertVerifier::OnCertTrustChanged(
     const X509Certificate* cert) {
   DCHECK(CalledOnValidThread());
 
   ClearCache();
 }
 
 void MultiThreadedCertVerifier::SetCertVerifyProc(CertVerifyProc* verify_proc) {
   verify_proc_ = verify_proc;
 }
 
 }  // namespace net