Cache the underlying OS certificate handle within the X509CertificateCache, rather than caching raw X509Certificate pointers.

TEST=X509CertificateTest.Cache, X509CertificateTest.Intermediates
BUG=32623, 47648, 49377, 68448, 70216, 77374, 78038

Review URL: http://codereview.chromium.org/2944008

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

12 files changed, 208 insertions, 316 deletions

diff --git a/net/base/cert_database_nss.cc b/net/base/cert_database_nss.cc
index 179ee82..8fb51e0 100644
--- a/net/base/cert_database_nss.cc
+++ b/net/base/cert_database_nss.cc
@@ -102,9 +102,7 @@ void CertDatabase::ListCerts(CertificateList* certs) {
        !CERT_LIST_END(node, cert_list);
        node = CERT_LIST_NEXT(node)) {
     certs->push_back(X509Certificate::CreateFromHandle(
-        node->cert,
-        X509Certificate::SOURCE_LONE_CERT_IMPORT,
-        X509Certificate::OSCertHandles()));
+        node->cert, X509Certificate::OSCertHandles()));
   }
   CERT_DestroyCertList(cert_list);
 }
diff --git a/net/base/cert_database_nss_unittest.cc b/net/base/cert_database_nss_unittest.cc
index e6cf147..63f2a43 100644
--- a/net/base/cert_database_nss_unittest.cc
+++ b/net/base/cert_database_nss_unittest.cc
@@ -51,11 +51,8 @@ CertificateList ListCertsInSlot(PK11SlotInfo* slot) {
   for (CERTCertListNode* node = CERT_LIST_HEAD(cert_list);
        !CERT_LIST_END(node, cert_list);
        node = CERT_LIST_NEXT(node)) {
-    result.push_back(
-        X509Certificate::CreateFromHandle(
-            node->cert,
-            X509Certificate::SOURCE_LONE_CERT_IMPORT,
-            X509Certificate::OSCertHandles()));
+    result.push_back(X509Certificate::CreateFromHandle(
+        node->cert, X509Certificate::OSCertHandles()));
   }
   CERT_DestroyCertList(cert_list);
 
diff --git a/net/base/x509_certificate.cc b/net/base/x509_certificate.cc
index 72d4f81..76fa4dc 100644
--- a/net/base/x509_certificate.cc
+++ b/net/base/x509_certificate.cc
@@ -19,6 +19,7 @@
 #include "base/sha1.h"
 #include "base/string_piece.h"
 #include "base/string_util.h"
+#include "base/synchronization/lock.h"
 #include "base/time.h"
 #include "googleurl/src/url_canon_ip.h"
 #include "net/base/cert_status_flags.h"
@@ -53,29 +54,62 @@ const char kCertificateHeader[] = "CERTIFICATE";
 // The PEM block header used for PKCS#7 data
 const char kPKCS7Header[] = "PKCS7";
 
-// A thread-safe cache for X509Certificate objects.
+// A thread-safe cache for OS certificate handles.
 //
-// The cache does not hold a reference to the certificate objects.  The objects
-// must |Remove| themselves from the cache upon destruction (or else the cache
-// will be holding dead pointers to the objects).
-// TODO(rsleevi): There exists a chance of a use-after-free, due to a race
-// between Find() and Remove(). See http://crbug.com/49377
+// Within each of the supported underlying crypto libraries, a certificate
+// handle is represented as a ref-counted object that contains the parsed
+// data for the certificate. In addition, the underlying OS handle may also
+// contain a copy of the original ASN.1 DER used to constructed the handle.
+//
+// In order to reduce the memory usage when multiple SSL connections exist,
+// with each connection storing the server's identity certificate plus any
+// intermediates supplied, the certificate handles are cached. Any two
+// X509Certificates that were created from the same ASN.1 DER data,
+// regardless of where that data came from, will share the same underlying
+// OS certificate handle.
 class X509CertificateCache {
  public:
-  void Insert(X509Certificate* cert);
-  void Remove(X509Certificate* cert);
-  X509Certificate* Find(const SHA1Fingerprint& fingerprint);
+  // Performs a compare-and-swap like operation. If an OS certificate handle
+  // for the same certificate data as |*cert_handle| already exists in the
+  // cache, the original |*cert_handle| will be freed and |cert_handle|
+  // will be updated to point to a duplicated reference to the existing cached
+  // certificate, with the caller taking ownership of this duplicated handle.
+  // If an equivalent OS certificate handle is not found, a duplicated
+  // reference to |*cert_handle| will be added to the cache. In either case,
+  // upon return, the caller fully owns |*cert_handle| and is responsible for
+  // calling FreeOSCertHandle(), after first calling Remove().
+  void InsertOrUpdate(X509Certificate::OSCertHandle* cert_handle);
+
+  // Decrements the cache reference count for |cert_handle|, a handle that was
+  // previously obtained by calling InsertOrUpdate(). If this is the last
+  // cached reference held, this will remove the handle from the cache. The
+  // caller retains ownership of |cert_handle| and remains responsible for
+  // calling FreeOSCertHandle() to release the underlying OS certificate
+  void Remove(X509Certificate::OSCertHandle cert_handle);
 
  private:
-  typedef std::map<SHA1Fingerprint, X509Certificate*, SHA1FingerprintLessThan>
-      CertMap;
+  // A single entry in the cache. Certificates will be keyed by their SHA1
+  // fingerprints, but will not be considered equivalent unless the entire
+  // certificate data matches.
+  struct Entry {
+    Entry() : cert_handle(NULL), ref_count(0) {}
+
+    X509Certificate::OSCertHandle cert_handle;
+
+    // Increased by each call to InsertOrUpdate(), and balanced by each call
+    // to Remove(). When it equals 0, all references created by
+    // InsertOrUpdate() have been released, so the cache entry will be removed
+    // the cached OS certificate handle will be freed.
+    int ref_count;
+  };
+  typedef std::map<SHA1Fingerprint, Entry, SHA1FingerprintLessThan> CertMap;
 
   // Obtain an instance of X509CertificateCache via a LazyInstance.
   X509CertificateCache() {}
   ~X509CertificateCache() {}
   friend struct base::DefaultLazyInstanceTraits<X509CertificateCache>;
 
-  // You must acquire this lock before using any private data of this object.
+  // You must acquire this lock before using any private data of this object
   // You must not block while holding this lock.
   base::Lock lock_;
 
@@ -89,39 +123,76 @@ base::LazyInstance<X509CertificateCache,
                    base::LeakyLazyInstanceTraits<X509CertificateCache> >
     g_x509_certificate_cache(base::LINKER_INITIALIZED);
 
-// Insert |cert| into the cache.  The cache does NOT AddRef |cert|.
-// Any existing certificate with the same fingerprint will be replaced.
-void X509CertificateCache::Insert(X509Certificate* cert) {
-  base::AutoLock lock(lock_);
-
-  DCHECK(!IsNullFingerprint(cert->fingerprint())) <<
-      "Only insert certs with real fingerprints.";
-  cache_[cert->fingerprint()] = cert;
-};
-
-// Remove |cert| from the cache.  The cache does not assume that |cert| is
-// already in the cache.
-void X509CertificateCache::Remove(X509Certificate* cert) {
-  base::AutoLock lock(lock_);
-
-  CertMap::iterator pos(cache_.find(cert->fingerprint()));
-  if (pos == cache_.end())
-    return;  // It is not an error to remove a cert that is not in the cache.
-  cache_.erase(pos);
-};
+void X509CertificateCache::InsertOrUpdate(
+    X509Certificate::OSCertHandle* cert_handle) {
+  DCHECK(cert_handle);
+  SHA1Fingerprint fingerprint =
+      X509Certificate::CalculateFingerprint(*cert_handle);
+
+  X509Certificate::OSCertHandle old_handle = NULL;
+  {
+    base::AutoLock lock(lock_);
+    CertMap::iterator pos = cache_.find(fingerprint);
+    if (pos == cache_.end()) {
+      // A cached entry was not found, so initialize a new entry. The entry
+      // assumes ownership of the current |*cert_handle|.
+      Entry cache_entry;
+      cache_entry.cert_handle = *cert_handle;
+      cache_entry.ref_count = 0;
+      CertMap::value_type cache_value(fingerprint, cache_entry);
+      pos = cache_.insert(cache_value).first;
+    } else {
+      bool is_same_cert =
+          X509Certificate::IsSameOSCert(*cert_handle, pos->second.cert_handle);
+      if (!is_same_cert) {
+        // Two certificates don't match, due to a SHA1 hash collision. Given
+        // the low probability, the simplest solution is to not cache the
+        // certificate, which should not affect performance too negatively.
+        return;
+      }
+      // A cached entry was found and will be used instead of the caller's
+      // handle. Ensure the caller's original handle will be freed, since
+      // ownership is assumed.
+      old_handle = *cert_handle;
+    }
+    // Whether an existing cached handle or a new handle, increment the
+    // cache's reference count and return a handle that the caller can own.
+    ++pos->second.ref_count;
+    *cert_handle = X509Certificate::DupOSCertHandle(pos->second.cert_handle);
+  }
+  // If the caller's handle was replaced with a cached handle, free the
+  // original handle now. This is done outside of the lock because
+  // |old_handle| may be the only handle for this particular certificate, so
+  // freeing it may be complex or resource-intensive and does not need to
+  // be guarded by the lock.
+  if (old_handle) {
+    X509Certificate::FreeOSCertHandle(old_handle);
+    DHISTOGRAM_COUNTS("X509CertificateReuseCount", 1);
+  }
+}
 
-// Find a certificate in the cache with the given fingerprint.  If one does
-// not exist, this method returns NULL.
-X509Certificate* X509CertificateCache::Find(
-    const SHA1Fingerprint& fingerprint) {
+void X509CertificateCache::Remove(X509Certificate::OSCertHandle cert_handle) {
+  SHA1Fingerprint fingerprint =
+      X509Certificate::CalculateFingerprint(cert_handle);
   base::AutoLock lock(lock_);
 
-  CertMap::iterator pos(cache_.find(fingerprint));
+  CertMap::iterator pos = cache_.find(fingerprint);
   if (pos == cache_.end())
-    return NULL;
-
-  return pos->second;
-};
+    return;  // A hash collision where the winning cert was already freed.
+
+  bool is_same_cert = X509Certificate::IsSameOSCert(cert_handle,
+                                                    pos->second.cert_handle);
+  if (!is_same_cert)
+    return;  // A hash collision where the winning cert is still around.
+
+  if (--pos->second.ref_count == 0) {
+    // The last reference to |cert_handle| has been removed, so release the
+    // Entry's OS handle and remove the Entry. The caller still holds a
+    // reference to |cert_handle| and is responsible for freeing it.
+    X509Certificate::FreeOSCertHandle(pos->second.cert_handle);
+    cache_.erase(pos);
+  }
+}
 
 // CompareSHA1Hashes is a helper function for using bsearch() with an array of
 // SHA1 hashes.
@@ -165,57 +236,22 @@ X509Certificate::X509Certificate(const std::string& subject,
       issuer_(issuer),
       valid_start_(start_date),
       valid_expiry_(expiration_date),
-      cert_handle_(NULL),
-      source_(SOURCE_UNUSED) {
+      cert_handle_(NULL) {
   memset(fingerprint_.data, 0, sizeof(fingerprint_.data));
 }
 
 // static
 X509Certificate* X509Certificate::CreateFromHandle(
     OSCertHandle cert_handle,
-    Source source,
     const OSCertHandles& intermediates) {
   DCHECK(cert_handle);
-  DCHECK_NE(source, SOURCE_UNUSED);
-
-  // Check if we already have this certificate in memory.
-  X509CertificateCache* cache = g_x509_certificate_cache.Pointer();
-  X509Certificate* cached_cert =
-      cache->Find(CalculateFingerprint(cert_handle));
-  if (cached_cert) {
-    DCHECK_NE(cached_cert->source_, SOURCE_UNUSED);
-    if (cached_cert->source_ > source ||
-        (cached_cert->source_ == source &&
-         cached_cert->HasIntermediateCertificates(intermediates))) {
-      // Return the certificate with the same fingerprint from our cache.
-      DHISTOGRAM_COUNTS("X509CertificateReuseCount", 1);
-      return cached_cert;
-    }
-    // Else the new cert is better and will replace the old one in the cache.
-  }
-
-  // Otherwise, allocate and cache a new object.
-  X509Certificate* cert = new X509Certificate(cert_handle, source,
-                                              intermediates);
-  cache->Insert(cert);
-  return cert;
+  return new X509Certificate(cert_handle, intermediates);
 }
 
-#if defined(OS_WIN)
-static X509Certificate::OSCertHandle CreateOSCert(base::StringPiece der_cert) {
-  X509Certificate::OSCertHandle cert_handle = NULL;
-  BOOL ok = CertAddEncodedCertificateToStore(
-      X509Certificate::cert_store(), X509_ASN_ENCODING | PKCS_7_ASN_ENCODING,
-      reinterpret_cast<const BYTE*>(der_cert.data()), der_cert.size(),
-      CERT_STORE_ADD_USE_EXISTING, &cert_handle);
-  return ok ? cert_handle : NULL;
-}
-#else
 static X509Certificate::OSCertHandle CreateOSCert(base::StringPiece der_cert) {
   return X509Certificate::CreateOSCertHandleFromBytes(
       const_cast<char*>(der_cert.data()), der_cert.size());
 }
-#endif
 
 // static
 X509Certificate* X509Certificate::CreateFromDERCertChain(
@@ -232,8 +268,7 @@ X509Certificate* X509Certificate::CreateFromDERCertChain(
 
   OSCertHandle handle = CreateOSCert(der_certs[0]);
   DCHECK(handle);
-  X509Certificate* cert =
-      CreateFromHandle(handle, SOURCE_FROM_NETWORK, intermediate_ca_certs);
+  X509Certificate* cert = CreateFromHandle(handle, intermediate_ca_certs);
   FreeOSCertHandle(handle);
   for (size_t i = 0; i < intermediate_ca_certs.size(); i++)
     FreeOSCertHandle(intermediate_ca_certs[i]);
@@ -248,9 +283,7 @@ X509Certificate* X509Certificate::CreateFromBytes(const char* data,
   if (!cert_handle)
     return NULL;
 
-  X509Certificate* cert = CreateFromHandle(cert_handle,
-                                           SOURCE_LONE_CERT_IMPORT,
-                                           OSCertHandles());
+  X509Certificate* cert = CreateFromHandle(cert_handle, OSCertHandles());
   FreeOSCertHandle(cert_handle);
   return cert;
 }
@@ -282,7 +315,7 @@ X509Certificate* X509Certificate::CreateFromPickle(const Pickle& pickle,
 
   X509Certificate* cert = NULL;
   if (intermediates.size() == num_intermediates)
-    cert = CreateFromHandle(cert_handle, SOURCE_FROM_CACHE, intermediates);
+    cert = CreateFromHandle(cert_handle, intermediates);
   FreeOSCertHandle(cert_handle);
   for (size_t i = 0; i < intermediates.size(); ++i)
     FreeOSCertHandle(intermediates[i]);
@@ -359,8 +392,7 @@ CertificateList X509Certificate::CreateCertificateListFromBytes(
 
   for (OSCertHandles::iterator it = certificates.begin();
        it != certificates.end(); ++it) {
-    X509Certificate* result = CreateFromHandle(*it, SOURCE_LONE_CERT_IMPORT,
-                                               OSCertHandles());
+    X509Certificate* result = CreateFromHandle(*it, OSCertHandles());
     results.push_back(scoped_refptr<X509Certificate>(result));
     FreeOSCertHandle(*it);
   }
@@ -554,24 +586,34 @@ bool X509Certificate::VerifyNameMatch(const std::string& hostname) const {
 #endif
 
 X509Certificate::X509Certificate(OSCertHandle cert_handle,
-                                 Source source,
                                  const OSCertHandles& intermediates)
-    : cert_handle_(DupOSCertHandle(cert_handle)),
-      source_(source) {
-  // Copy/retain the intermediate cert handles.
-  for (size_t i = 0; i < intermediates.size(); ++i)
-    intermediate_ca_certs_.push_back(DupOSCertHandle(intermediates[i]));
+    : cert_handle_(DupOSCertHandle(cert_handle)) {
+  X509CertificateCache* cache = g_x509_certificate_cache.Pointer();
+  cache->InsertOrUpdate(&cert_handle_);
+  for (size_t i = 0; i < intermediates.size(); ++i) {
+    // Duplicate the incoming certificate, as the caller retains ownership
+    // of |intermediates|.
+    OSCertHandle intermediate = DupOSCertHandle(intermediates[i]);
+    // Update the cache, which will assume ownership of the duplicated
+    // handle and return a suitable equivalent, potentially from the cache.
+    cache->InsertOrUpdate(&intermediate);
+    intermediate_ca_certs_.push_back(intermediate);
+  }
   // Platform-specific initialization.
   Initialize();
 }
 
 X509Certificate::~X509Certificate() {
   // We might not be in the cache, but it is safe to remove ourselves anyway.
-  g_x509_certificate_cache.Get().Remove(this);
-  if (cert_handle_)
+  X509CertificateCache* cache = g_x509_certificate_cache.Pointer();
+  if (cert_handle_) {
+    cache->Remove(cert_handle_);
     FreeOSCertHandle(cert_handle_);
-  for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i)
+  }
+  for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i) {
+    cache->Remove(intermediate_ca_certs_[i]);
     FreeOSCertHandle(intermediate_ca_certs_[i]);
+  }
 }
 
 bool X509Certificate::IsBlacklisted() const {
diff --git a/net/base/x509_certificate.h b/net/base/x509_certificate.h
index 8e560980..08aee59 100644
--- a/net/base/x509_certificate.h
+++ b/net/base/x509_certificate.h
@@ -48,7 +48,10 @@ class CertVerifyResult;
 
 typedef std::vector<scoped_refptr<X509Certificate> > CertificateList;
 
-// X509Certificate represents an X.509 certificate used by SSL.
+// X509Certificate represents a X.509 certificate, which is comprised a
+// particular identity or end-entity certificate, such as an SSL server
+// identity or an SSL client certificate, and zero or more intermediate
+// certificates that may be used to build a path to a root certificate.
 class NET_API X509Certificate
     : public base::RefCountedThreadSafe<X509Certificate> {
  public:
@@ -76,18 +79,6 @@ class NET_API X509Certificate
     bool operator() (X509Certificate* lhs,  X509Certificate* rhs) const;
   };
 
-  // Where the certificate comes from.  The enumeration constants are
-  // listed in increasing order of preference.
-  enum Source {
-    SOURCE_UNUSED = 0,            // The source_ member is not used.
-    SOURCE_LONE_CERT_IMPORT = 1,  // From importing a certificate without
-                                  // any intermediate CA certificates.
-    SOURCE_FROM_CACHE = 2,        // From the disk cache - which contains
-                                  // intermediate CA certificates, but may be
-                                  // stale.
-    SOURCE_FROM_NETWORK = 3,      // From the network.
-  };
-
   enum VerifyFlags {
     VERIFY_REV_CHECKING_ENABLED = 1 << 0,
     VERIFY_EV_CERT = 1 << 1,
@@ -136,22 +127,16 @@ class NET_API X509Certificate
                   base::Time start_date, base::Time expiration_date);
 
   // Create an X509Certificate from a handle to the certificate object in the
-  // underlying crypto library. |source| specifies where |cert_handle| comes
-  // from.  Given two certificate handles for the same certificate, our
-  // certificate cache prefers the handle from the network because our HTTP
-  // cache isn't caching the corresponding intermediate CA certificates yet
-  // (http://crbug.com/7065).
-  // The returned pointer must be stored in a scoped_refptr<X509Certificate>.
+  // underlying crypto library. The returned pointer must be stored in a
+  // scoped_refptr<X509Certificate>.
   static X509Certificate* CreateFromHandle(OSCertHandle cert_handle,
-                                           Source source,
                                            const OSCertHandles& intermediates);
 
   // Create an X509Certificate from a chain of DER encoded certificates. The
   // first certificate in the chain is the end-entity certificate to which a
   // handle is returned. The other certificates in the chain are intermediate
-  // certificates. See the comment for |CreateFromHandle| about the |source|
-  // argument.
-  // The returned pointer must be stored in a scoped_refptr<X509Certificate>.
+  // certificates. The returned pointer must be stored in a
+  // scoped_refptr<X509Certificate>.
   static X509Certificate* CreateFromDERCertChain(
       const std::vector<base::StringPiece>& der_certs);
 
@@ -301,17 +286,6 @@ class NET_API X509Certificate
   CFArrayRef CreateClientCertificateChain() const;
 #endif
 
-#if defined(OS_WIN)
-  // Returns a handle to a global, in-memory certificate store. We use it for
-  // two purposes:
-  // 1. Import server certificates into this store so that we can verify and
-  //    display the certificates using CryptoAPI.
-  // 2. Copy client certificates from the "MY" system certificate store into
-  //    this store so that we can close the system store when we finish
-  //    searching for client certificates.
-  static HCERTSTORE cert_store();
-#endif
-
 #if defined(USE_OPENSSL)
   // Returns a handle to a global, in-memory certificate store. We
   // use it for test code, e.g. importing the test server's certificate.
@@ -371,6 +345,10 @@ class NET_API X509Certificate
   // Frees (or releases a reference to) an OS certificate handle.
   static void FreeOSCertHandle(OSCertHandle cert_handle);
 
+  // Calculates the SHA-1 fingerprint of the certificate.  Returns an empty
+  // (all zero) fingerprint on failure.
+  static SHA1Fingerprint CalculateFingerprint(OSCertHandle cert_handle);
+
  private:
   friend class base::RefCountedThreadSafe<X509Certificate>;
   friend class TestRootCerts;  // For unit tests
@@ -381,7 +359,7 @@ class NET_API X509Certificate
 
   // Construct an X509Certificate from a handle to the certificate object
   // in the underlying crypto library.
-  X509Certificate(OSCertHandle cert_handle, Source source,
+  X509Certificate(OSCertHandle cert_handle,
                   const OSCertHandles& intermediates);
 
   ~X509Certificate();
@@ -406,10 +384,6 @@ class NET_API X509Certificate
   static void ResetCertStore();
 #endif
 
-  // Calculates the SHA-1 fingerprint of the certificate.  Returns an empty
-  // (all zero) fingerprint on failure.
-  static SHA1Fingerprint CalculateFingerprint(OSCertHandle cert_handle);
-
   // Verifies that |hostname| matches one of the certificate names or IP
   // addresses supplied, based on TLS name matching rules - specifically,
   // following http://tools.ietf.org/html/rfc6125.
@@ -491,9 +465,6 @@ class NET_API X509Certificate
   mutable base::Lock verification_lock_;
 #endif
 
-  // Where the certificate comes from.
-  Source source_;
-
   DISALLOW_COPY_AND_ASSIGN(X509Certificate);
 };
 
diff --git a/net/base/x509_certificate_mac.cc b/net/base/x509_certificate_mac.cc
index 8f4ba4e..1edcedd 100644
--- a/net/base/x509_certificate_mac.cc
+++ b/net/base/x509_certificate_mac.cc
@@ -742,9 +742,7 @@ X509Certificate* X509Certificate::CreateSelfSigned(
   }
   scoped_cert.reset(certificate_ref);
 
-  return CreateFromHandle(
-     scoped_cert, X509Certificate::SOURCE_LONE_CERT_IMPORT,
-     X509Certificate::OSCertHandles());
+  return CreateFromHandle(scoped_cert, X509Certificate::OSCertHandles());
 }
 
 void X509Certificate::GetDNSNames(std::vector<std::string>* dns_names) const {
@@ -1148,9 +1146,7 @@ bool X509Certificate::IsIssuedBy(
     SecCertificateRef cert_handle = reinterpret_cast<SecCertificateRef>(
         const_cast<void*>(CFArrayGetValueAtIndex(cert_chain, i)));
     scoped_refptr<X509Certificate> cert(X509Certificate::CreateFromHandle(
-        cert_handle,
-        X509Certificate::SOURCE_LONE_CERT_IMPORT,
-        X509Certificate::OSCertHandles()));
+        cert_handle, X509Certificate::OSCertHandles()));
     for (unsigned j = 0; j < valid_issuers.size(); j++) {
       if (cert->issuer().Matches(valid_issuers[j]))
         return true;
@@ -1290,8 +1286,7 @@ bool X509Certificate::GetSSLClientCertificates(
     ScopedCFTypeRef<SecCertificateRef> scoped_cert_handle(cert_handle);
 
     scoped_refptr<X509Certificate> cert(
-        CreateFromHandle(cert_handle, SOURCE_LONE_CERT_IMPORT,
-                         OSCertHandles()));
+        CreateFromHandle(cert_handle, OSCertHandles()));
     if (cert->HasExpired() || !cert->SupportsSSLClientAuth())
       continue;
 
diff --git a/net/base/x509_certificate_nss.cc b/net/base/x509_certificate_nss.cc
index 9c634a8..db9f6a5 100644
--- a/net/base/x509_certificate_nss.cc
+++ b/net/base/x509_certificate_nss.cc
@@ -750,8 +750,7 @@ X509Certificate* X509Certificate::CreateSelfSigned(
   // Save the signed result to the cert.
   cert->derCert = *result;
 
-  X509Certificate* x509_cert =
-      CreateFromHandle(cert, SOURCE_LONE_CERT_IMPORT, OSCertHandles());
+  X509Certificate* x509_cert = CreateFromHandle(cert, OSCertHandles());
   CERT_DestroyCertificate(cert);
   return x509_cert;
 }
diff --git a/net/base/x509_certificate_unittest.cc b/net/base/x509_certificate_unittest.cc
index 5c23193..7adad4f 100644
--- a/net/base/x509_certificate_unittest.cc
+++ b/net/base/x509_certificate_unittest.cc
@@ -475,7 +475,6 @@ TEST(X509CertificateTest, IntermediateCARequireExplicitPolicy) {
   intermediates.push_back(intermediate_cert->os_cert_handle());
   scoped_refptr<X509Certificate> cert_chain =
       X509Certificate::CreateFromHandle(server_cert->os_cert_handle(),
-                                        X509Certificate::SOURCE_FROM_NETWORK,
                                         intermediates);
 
   int flags = 0;
@@ -510,7 +509,6 @@ TEST(X509CertificateTest, DISABLED_GlobalSignR3EVTest) {
   intermediates.push_back(intermediate_cert->os_cert_handle());
   scoped_refptr<X509Certificate> cert_chain =
       X509Certificate::CreateFromHandle(server_cert->os_cert_handle(),
-                                        X509Certificate::SOURCE_FROM_NETWORK,
                                         intermediates);
 
   CertVerifyResult verify_result;
@@ -539,7 +537,6 @@ TEST(X509CertificateTest, TestKnownRoot) {
   intermediates.push_back(intermediate_cert->os_cert_handle());
   scoped_refptr<X509Certificate> cert_chain =
       X509Certificate::CreateFromHandle(cert->os_cert_handle(),
-                                        X509Certificate::SOURCE_FROM_NETWORK,
                                         intermediates);
 
   int flags = 0;
@@ -615,7 +612,6 @@ TEST(X509CertificateTest, PublicKeyHashes) {
   intermediates.push_back(intermediate_cert->os_cert_handle());
   scoped_refptr<X509Certificate> cert_chain =
       X509Certificate::CreateFromHandle(cert->os_cert_handle(),
-                                        X509Certificate::SOURCE_FROM_NETWORK,
                                         intermediates);
 
   int flags = 0;
@@ -663,66 +659,54 @@ TEST(X509CertificateTest, InvalidKeyUsage) {
 #endif
 }
 
-// Tests X509Certificate::Cache via X509Certificate::CreateFromHandle.  We
+// Tests X509CertificateCache via X509Certificate::CreateFromHandle.  We
 // call X509Certificate::CreateFromHandle several times and observe whether
-// it returns a cached or new X509Certificate object.
-//
-// All the OS certificate handles in this test are actually from the same
-// source (the bytes of a lone certificate), but we pretend that some of them
-// come from the network.
+// it returns a cached or new OSCertHandle.
 TEST(X509CertificateTest, Cache) {
   X509Certificate::OSCertHandle google_cert_handle;
+  X509Certificate::OSCertHandle thawte_cert_handle;
 
-  // Add a certificate from the source SOURCE_LONE_CERT_IMPORT to our
-  // certificate cache.
+  // Add a single certificate to the certificate cache.
   google_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
       reinterpret_cast<const char*>(google_der), sizeof(google_der));
   scoped_refptr<X509Certificate> cert1(X509Certificate::CreateFromHandle(
-      google_cert_handle, X509Certificate::SOURCE_LONE_CERT_IMPORT,
-      X509Certificate::OSCertHandles()));
+      google_cert_handle, X509Certificate::OSCertHandles()));
   X509Certificate::FreeOSCertHandle(google_cert_handle);
 
-  // Add a certificate from the same source (SOURCE_LONE_CERT_IMPORT).  This
-  // should return the cached certificate (cert1).
+  // Add the same certificate, but as a new handle.
   google_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
       reinterpret_cast<const char*>(google_der), sizeof(google_der));
   scoped_refptr<X509Certificate> cert2(X509Certificate::CreateFromHandle(
-      google_cert_handle, X509Certificate::SOURCE_LONE_CERT_IMPORT,
-      X509Certificate::OSCertHandles()));
+      google_cert_handle, X509Certificate::OSCertHandles()));
   X509Certificate::FreeOSCertHandle(google_cert_handle);
 
-  EXPECT_EQ(cert1, cert2);
+  // A new X509Certificate should be returned.
+  EXPECT_NE(cert1.get(), cert2.get());
+  // But both instances should share the underlying OS certificate handle.
+  EXPECT_EQ(cert1->os_cert_handle(), cert2->os_cert_handle());
+  EXPECT_EQ(0u, cert1->GetIntermediateCertificates().size());
+  EXPECT_EQ(0u, cert2->GetIntermediateCertificates().size());
 
-  // Add a certificate from the network.  This should kick out the original
-  // cached certificate (cert1) and return a new certificate.
+  // Add the same certificate, but this time with an intermediate. This
+  // should result in the intermediate being cached. Note that this is not
+  // a legitimate chain, but is suitable for testing.
   google_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
       reinterpret_cast<const char*>(google_der), sizeof(google_der));
+  thawte_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
+      reinterpret_cast<const char*>(thawte_der), sizeof(thawte_der));
+  X509Certificate::OSCertHandles intermediates;
+  intermediates.push_back(thawte_cert_handle);
   scoped_refptr<X509Certificate> cert3(X509Certificate::CreateFromHandle(
-      google_cert_handle, X509Certificate::SOURCE_FROM_NETWORK,
-      X509Certificate::OSCertHandles()));
-  X509Certificate::FreeOSCertHandle(google_cert_handle);
-
-  EXPECT_NE(cert1, cert3);
-
-  // Add one certificate from each source.  Both should return the new cached
-  // certificate (cert3).
-  google_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
-      reinterpret_cast<const char*>(google_der), sizeof(google_der));
-  scoped_refptr<X509Certificate> cert4(X509Certificate::CreateFromHandle(
-      google_cert_handle, X509Certificate::SOURCE_FROM_NETWORK,
-      X509Certificate::OSCertHandles()));
-  X509Certificate::FreeOSCertHandle(google_cert_handle);
-
-  EXPECT_EQ(cert3, cert4);
-
-  google_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
-      reinterpret_cast<const char*>(google_der), sizeof(google_der));
-  scoped_refptr<X509Certificate> cert5(X509Certificate::CreateFromHandle(
-      google_cert_handle, X509Certificate::SOURCE_FROM_NETWORK,
-      X509Certificate::OSCertHandles()));
+      google_cert_handle, intermediates));
   X509Certificate::FreeOSCertHandle(google_cert_handle);
+  X509Certificate::FreeOSCertHandle(thawte_cert_handle);
 
-  EXPECT_EQ(cert3, cert5);
+  // Test that the new certificate, even with intermediates, results in the
+  // same underlying handle being used.
+  EXPECT_EQ(cert1->os_cert_handle(), cert3->os_cert_handle());
+  // Though they use the same OS handle, the intermediates should be different.
+  EXPECT_NE(cert1->GetIntermediateCertificates().size(),
+      cert3->GetIntermediateCertificates().size());
 }
 
 TEST(X509CertificateTest, Pickle) {
@@ -735,13 +719,8 @@ TEST(X509CertificateTest, Pickle) {
 
   X509Certificate::OSCertHandles intermediates;
   intermediates.push_back(thawte_cert_handle);
-  // Faking SOURCE_LONE_CERT_IMPORT so that when the pickled certificate is
-  // read, it successfully evicts |cert| from the X509Certificate::Cache.
-  // This will be fixed when http://crbug.com/49377 is fixed.
   scoped_refptr<X509Certificate> cert = X509Certificate::CreateFromHandle(
-      google_cert_handle,
-      X509Certificate::SOURCE_LONE_CERT_IMPORT,
-      intermediates);
+      google_cert_handle, intermediates);
   ASSERT_NE(static_cast<X509Certificate*>(NULL), cert.get());
 
   X509Certificate::FreeOSCertHandle(google_cert_handle);
@@ -755,7 +734,6 @@ TEST(X509CertificateTest, Pickle) {
       X509Certificate::CreateFromPickle(
           pickle, &iter, X509Certificate::PICKLETYPE_CERTIFICATE_CHAIN);
   ASSERT_NE(static_cast<X509Certificate*>(NULL), cert_from_pickle);
-  EXPECT_NE(cert.get(), cert_from_pickle.get());
   EXPECT_TRUE(X509Certificate::IsSameOSCert(
       cert->os_cert_handle(), cert_from_pickle->os_cert_handle()));
   EXPECT_TRUE(cert->HasIntermediateCertificates(
@@ -798,7 +776,6 @@ TEST(X509CertificateTest, Policy) {
   EXPECT_TRUE(policy.HasDeniedCert());
 }
 
-#if defined(OS_MACOSX) || defined(OS_WIN)
 TEST(X509CertificateTest, IntermediateCertificates) {
   scoped_refptr<X509Certificate> webkit_cert(
       X509Certificate::CreateFromBytes(
@@ -819,8 +796,7 @@ TEST(X509CertificateTest, IntermediateCertificates) {
       reinterpret_cast<const char*>(google_der), sizeof(google_der));
   X509Certificate::OSCertHandles intermediates1;
   scoped_refptr<X509Certificate> cert1;
-  cert1 = X509Certificate::CreateFromHandle(
-      google_handle, X509Certificate::SOURCE_FROM_NETWORK, intermediates1);
+  cert1 = X509Certificate::CreateFromHandle(google_handle, intermediates1);
   EXPECT_TRUE(cert1->HasIntermediateCertificates(intermediates1));
   EXPECT_FALSE(cert1->HasIntermediateCertificate(
       webkit_cert->os_cert_handle()));
@@ -830,11 +806,7 @@ TEST(X509CertificateTest, IntermediateCertificates) {
   intermediates2.push_back(webkit_cert->os_cert_handle());
   intermediates2.push_back(thawte_cert->os_cert_handle());
   scoped_refptr<X509Certificate> cert2;
-  cert2 = X509Certificate::CreateFromHandle(
-      google_handle, X509Certificate::SOURCE_FROM_NETWORK, intermediates2);
-
-  // The cache should have stored cert2 'cause it has more intermediates:
-  EXPECT_NE(cert1, cert2);
+  cert2 = X509Certificate::CreateFromHandle(google_handle, intermediates2);
 
   // Verify it has all the intermediates:
   EXPECT_TRUE(cert2->HasIntermediateCertificate(
@@ -844,20 +816,9 @@ TEST(X509CertificateTest, IntermediateCertificates) {
   EXPECT_FALSE(cert2->HasIntermediateCertificate(
       paypal_cert->os_cert_handle()));
 
-  // Create object with 1 intermediate:
-  X509Certificate::OSCertHandles intermediates3;
-  intermediates2.push_back(thawte_cert->os_cert_handle());
-  scoped_refptr<X509Certificate> cert3;
-  cert3 = X509Certificate::CreateFromHandle(
-      google_handle, X509Certificate::SOURCE_FROM_NETWORK, intermediates3);
-
-  // The cache should have returned cert2 'cause it has more intermediates:
-  EXPECT_EQ(cert3, cert2);
-
   // Cleanup
   X509Certificate::FreeOSCertHandle(google_handle);
 }
-#endif
 
 #if defined(OS_MACOSX)
 TEST(X509CertificateTest, IsIssuedBy) {
diff --git a/net/base/x509_certificate_win.cc b/net/base/x509_certificate_win.cc
index a9c0f5e..8442fdbc 100644
--- a/net/base/x509_certificate_win.cc
+++ b/net/base/x509_certificate_win.cc
@@ -4,7 +4,6 @@
 
 #include "net/base/x509_certificate.h"
 
-#include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/pickle.h"
 #include "base/sha1.h"
@@ -605,9 +604,7 @@ X509Certificate* X509Certificate::CreateSelfSigned(
   if (!cert_handle)
     return NULL;
 
-  X509Certificate* cert = CreateFromHandle(cert_handle,
-                                           SOURCE_LONE_CERT_IMPORT,
-                                           OSCertHandles());
+  X509Certificate* cert = CreateFromHandle(cert_handle, OSCertHandles());
   FreeOSCertHandle(cert_handle);
   return cert;
 }
@@ -633,36 +630,6 @@ void X509Certificate::GetDNSNames(std::vector<std::string>* dns_names) const {
     dns_names->push_back(subject_.common_name);
 }
 
-class GlobalCertStore {
- public:
-  HCERTSTORE cert_store() {
-    return cert_store_;
-  }
-
- private:
-  friend struct base::DefaultLazyInstanceTraits<GlobalCertStore>;
-
-  GlobalCertStore()
-      : cert_store_(CertOpenStore(CERT_STORE_PROV_MEMORY, 0, NULL, 0, NULL)) {
-  }
-
-  ~GlobalCertStore() {
-    CertCloseStore(cert_store_, 0 /* flags */);
-  }
-
-  const HCERTSTORE cert_store_;
-
-  DISALLOW_COPY_AND_ASSIGN(GlobalCertStore);
-};
-
-static base::LazyInstance<GlobalCertStore> g_cert_store(
-    base::LINKER_INITIALIZED);
-
-// static
-HCERTSTORE X509Certificate::cert_store() {
-  return g_cert_store.Get().cert_store();
-}
-
 int X509Certificate::VerifyInternal(const std::string& hostname,
                                     int flags,
                                     CertVerifyResult* verify_result) const {
diff --git a/net/socket/ssl_client_socket_mac.cc b/net/socket/ssl_client_socket_mac.cc
index c38b78a..3f7821f 100644
--- a/net/socket/ssl_client_socket_mac.cc
+++ b/net/socket/ssl_client_socket_mac.cc
@@ -438,8 +438,8 @@ X509Certificate* GetServerCert(SSLContextRef ssl_context) {
 
   SecCertificateRef server_cert = static_cast<SecCertificateRef>(
       const_cast<void*>(CFArrayGetValueAtIndex(certs, 0)));
-  return X509Certificate::CreateFromHandle(
-      server_cert, X509Certificate::SOURCE_FROM_NETWORK, intermediate_ca_certs);
+  return X509Certificate::CreateFromHandle(server_cert,
+                                           intermediate_ca_certs);
 }
 
 // Dynamically look up a pointer to a function exported by a bundle.
diff --git a/net/socket/ssl_client_socket_nss.cc b/net/socket/ssl_client_socket_nss.cc
index e827f39..f0a4ee0 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -2041,11 +2041,10 @@ SECStatus SSLClientSocketNSS::PlatformClientAuthHandler(
     // Get the leaf certificate.
     PCCERT_CONTEXT cert_context =
         chain_context->rgpChain[0]->rgpElement[0]->pCertContext;
-    // Copy it to our own certificate store, so that we can close the "MY"
-    // certificate store before returning from this function.
+    // Copy the certificate into a NULL store, so that we can close the "MY"
+    // store before returning from this function.
     PCCERT_CONTEXT cert_context2;
-    BOOL ok = CertAddCertificateContextToStore(X509Certificate::cert_store(),
-                                               cert_context,
+    BOOL ok = CertAddCertificateContextToStore(NULL, cert_context,
                                                CERT_STORE_ADD_USE_EXISTING,
                                                &cert_context2);
     if (!ok) {
@@ -2060,8 +2059,8 @@ SECStatus SSLClientSocketNSS::PlatformClientAuthHandler(
     net::X509Certificate::OSCertHandles intermediates;
     for (DWORD i = 1; i < chain_context->rgpChain[0]->cElement; i++) {
       PCCERT_CONTEXT intermediate_copy;
-      ok = CertAddCertificateContextToStore(X509Certificate::cert_store(),
-          chain_context->rgpChain[0]->rgpElement[i]->pCertContext,
+      ok = CertAddCertificateContextToStore(
+          NULL, chain_context->rgpChain[0]->rgpElement[i]->pCertContext,
           CERT_STORE_ADD_USE_EXISTING, &intermediate_copy);
       if (!ok) {
         NOTREACHED();
@@ -2071,8 +2070,7 @@ SECStatus SSLClientSocketNSS::PlatformClientAuthHandler(
     }
 
     scoped_refptr<X509Certificate> cert = X509Certificate::CreateFromHandle(
-        cert_context2, X509Certificate::SOURCE_LONE_CERT_IMPORT,
-        intermediates);
+        cert_context2, intermediates);
     that->client_certs_.push_back(cert);
 
     X509Certificate::FreeOSCertHandle(cert_context2);
@@ -2235,8 +2233,7 @@ SECStatus SSLClientSocketNSS::ClientAuthHandler(
           NSS_CmpCertChainWCANames(node->cert, ca_names) != SECSuccess)
         continue;
       X509Certificate* x509_cert = X509Certificate::CreateFromHandle(
-          node->cert, X509Certificate::SOURCE_LONE_CERT_IMPORT,
-          net::X509Certificate::OSCertHandles());
+          node->cert, net::X509Certificate::OSCertHandles());
       that->client_certs_.push_back(x509_cert);
     }
     CERT_DestroyCertList(client_certs);
diff --git a/net/socket/ssl_client_socket_openssl.cc b/net/socket/ssl_client_socket_openssl.cc
index 3e25e2b..f679bcc 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -869,8 +869,7 @@ X509Certificate* SSLClientSocketOpenSSL::UpdateServerCert() {
     for (int i = 0; i < sk_X509_num(chain); ++i)
       intermediates.push_back(sk_X509_value(chain, i));
   }
-  server_cert_ = X509Certificate::CreateFromHandle(
-      cert.get(), X509Certificate::SOURCE_FROM_NETWORK, intermediates);
+  server_cert_ = X509Certificate::CreateFromHandle(cert.get(), intermediates);
   DCHECK(server_cert_);
 
   return server_cert_;
diff --git a/net/socket/ssl_client_socket_win.cc b/net/socket/ssl_client_socket_win.cc
index 4cc3103..ecdf8cf 100644
--- a/net/socket/ssl_client_socket_win.cc
+++ b/net/socket/ssl_client_socket_win.cc
@@ -337,40 +337,6 @@ static BOOL WINAPI ClientCertFindCallback(PCCERT_CONTEXT cert_context,
 
 //-----------------------------------------------------------------------------
 
-// A memory certificate store for client certificates.  This allows us to
-// close the "MY" system certificate store when we finish searching for
-// client certificates.
-class ClientCertStore {
- public:
-  ClientCertStore() {
-    store_ = CertOpenStore(CERT_STORE_PROV_MEMORY, 0, NULL, 0, NULL);
-  }
-
-  ~ClientCertStore() {
-    if (store_) {
-      BOOL ok = CertCloseStore(store_, CERT_CLOSE_STORE_CHECK_FLAG);
-      DCHECK(ok);
-    }
-  }
-
-  PCCERT_CONTEXT CopyCertContext(PCCERT_CONTEXT client_cert) {
-    PCCERT_CONTEXT copy;
-    BOOL ok = CertAddCertificateContextToStore(store_, client_cert,
-                                               CERT_STORE_ADD_USE_EXISTING,
-                                               &copy);
-    DCHECK(ok);
-    return ok ? copy : NULL;
-  }
-
- private:
-  HCERTSTORE store_;
-};
-
-static base::LazyInstance<ClientCertStore> g_client_cert_store(
-    base::LINKER_INITIALIZED);
-
-//-----------------------------------------------------------------------------
-
 // Size of recv_buffer_
 //
 // Ciphertext is decrypted one SSL record at a time, so recv_buffer_ needs to
@@ -522,17 +488,18 @@ void SSLClientSocketWin::GetSSLCertRequestInfo(
     // Get the leaf certificate.
     PCCERT_CONTEXT cert_context =
         chain_context->rgpChain[0]->rgpElement[0]->pCertContext;
-    // Copy it to our own certificate store, so that we can close the "MY"
-    // certificate store before returning from this function.
-    PCCERT_CONTEXT cert_context2 =
-        g_client_cert_store.Get().CopyCertContext(cert_context);
-    if (!cert_context2) {
+    // Copy the certificate into a NULL store, so that we can close the "MY"
+    // store before returning from this function.
+    PCCERT_CONTEXT cert_context2 = NULL;
+    BOOL ok = CertAddCertificateContextToStore(NULL, cert_context,
+                                               CERT_STORE_ADD_USE_EXISTING,
+                                               &cert_context2);
+    if (!ok) {
       NOTREACHED();
       continue;
     }
     scoped_refptr<X509Certificate> cert = X509Certificate::CreateFromHandle(
-        cert_context2, X509Certificate::SOURCE_LONE_CERT_IMPORT,
-        X509Certificate::OSCertHandles());
+        cert_context2, X509Certificate::OSCertHandles());
     cert_request_info->client_certs.push_back(cert);
     CertFreeCertificateContext(cert_context2);
   }
@@ -1514,8 +1481,7 @@ int SSLClientSocketWin::DidCompleteHandshake() {
     DidCompleteRenegotiation();
   } else {
     server_cert_ = X509Certificate::CreateFromHandle(
-        server_cert_handle, X509Certificate::SOURCE_FROM_NETWORK,
-        X509Certificate::OSCertHandles());
+        server_cert_handle, X509Certificate::OSCertHandles());
 
     next_state_ = STATE_VERIFY_CERT;
   }