Thread-safety for X509Certificate's intermediate-certs list.

BUG=32553,30001
TEST=none

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

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

10 files changed, 239 insertions, 89 deletions

diff --git a/net/base/x509_certificate.cc b/net/base/x509_certificate.cc
index 24388d8..adf73b9 100644
--- a/net/base/x509_certificate.cc
+++ b/net/base/x509_certificate.cc
@@ -4,6 +4,10 @@
 
 #include "net/base/x509_certificate.h"
 
+#if defined(USE_NSS)
+#include <cert.h>
+#endif
+
 #include "base/histogram.h"
 #include "base/logging.h"
 #include "base/time.h"
@@ -24,6 +28,33 @@ bool IsNullFingerprint(const X509Certificate::Fingerprint& fingerprint) {
 
 }  // namespace
 
+// static
+bool X509Certificate::IsSameOSCert(X509Certificate::OSCertHandle a,
+                                   X509Certificate::OSCertHandle b) {
+  DCHECK(a && b);
+  if (a == b)
+    return true;
+#if defined(OS_WIN)
+  return a->cbCertEncoded == b->cbCertEncoded &&
+      memcmp(a->pbCertEncoded, b->pbCertEncoded, a->cbCertEncoded) == 0;
+#elif defined(OS_MACOSX)
+  if (CFEqual(a, b))
+    return true;
+  CSSM_DATA a_data, b_data;
+  return SecCertificateGetData(a, &a_data) == noErr &&
+      SecCertificateGetData(b, &b_data) == noErr &&
+      a_data.Length == b_data.Length &&
+      memcmp(a_data.Data, b_data.Data, a_data.Length) == 0;
+#elif defined(USE_NSS)
+  return a->derCert.len == b->derCert.len &&
+      memcmp(a->derCert.data, b->derCert.data, a->derCert.len) == 0;
+#else
+  // TODO(snej): not implemented
+  UNREACHED();
+  return false;
+#endif
+}
+
 bool X509Certificate::FingerprintLessThan::operator()(
     const Fingerprint& lhs,
     const Fingerprint& rhs) const {
@@ -57,14 +88,13 @@ X509Certificate::Cache* X509Certificate::Cache::GetInstance() {
   return Singleton<X509Certificate::Cache>::get();
 }
 
-// Insert |cert| into the cache.  The cache does NOT AddRef |cert|.  The cache
-// must not already contain a certificate with the same fingerprint.
+// Insert |cert| into the cache.  The cache does NOT AddRef |cert|.
+// Any existing certificate with the same fingerprint will be replaced.
 void X509Certificate::Cache::Insert(X509Certificate* cert) {
   AutoLock lock(lock_);
 
   DCHECK(!IsNullFingerprint(cert->fingerprint())) <<
       "Only insert certs with real fingerprints.";
-  DCHECK(cache_.find(cert->fingerprint()) == cache_.end());
   cache_[cert->fingerprint()] = cert;
 };
 
@@ -133,8 +163,10 @@ bool X509Certificate::Policy::HasDeniedCert() const {
 }
 
 // static
-X509Certificate* X509Certificate::CreateFromHandle(OSCertHandle cert_handle,
-                                                   Source source) {
+X509Certificate* X509Certificate::CreateFromHandle(
+    OSCertHandle cert_handle,
+    Source source,
+    const OSCertHandles& intermediates) {
   DCHECK(cert_handle);
   DCHECK(source != SOURCE_UNUSED);
 
@@ -144,18 +176,23 @@ X509Certificate* X509Certificate::CreateFromHandle(OSCertHandle cert_handle,
       cache->Find(CalculateFingerprint(cert_handle));
   if (cached_cert) {
     DCHECK(cached_cert->source_ != SOURCE_UNUSED);
-    if (cached_cert->source_ >= source) {
-      // We've found a certificate with the same fingerprint in our cache.  We
-      // own the |cert_handle|, which makes it our job to free it.
+    if (cached_cert->source_ > source ||
+        (cached_cert->source_ == source &&
+         cached_cert->HasIntermediateCertificates(intermediates))) {
+      // Return the certificate with the same fingerprint from our cache.
+      // But we own the input OSCertHandle, which makes it our job to free it.
       FreeOSCertHandle(cert_handle);
       DHISTOGRAM_COUNTS("X509CertificateReuseCount", 1);
       return cached_cert;
     }
-    // Kick out the old certificate from our cache.  The new one is better.
-    cache->Remove(cached_cert);
+    // Else the new cert is better and will replace the old one in the cache.
   }
-  // Otherwise, allocate a new object.
-  return new X509Certificate(cert_handle, source);
+
+  // Otherwise, allocate and cache a new object.
+  X509Certificate* cert = new X509Certificate(cert_handle, source,
+                                              intermediates);
+  cache->Insert(cert);
+  return cert;
 }
 
 // static
@@ -165,12 +202,22 @@ X509Certificate* X509Certificate::CreateFromBytes(const char* data,
   if (!cert_handle)
     return NULL;
 
-  return CreateFromHandle(cert_handle, SOURCE_LONE_CERT_IMPORT);
+  return CreateFromHandle(cert_handle,
+                          SOURCE_LONE_CERT_IMPORT,
+                          OSCertHandles());
 }
 
-X509Certificate::X509Certificate(OSCertHandle cert_handle, Source source)
+X509Certificate::X509Certificate(OSCertHandle cert_handle,
+                                 Source source,
+                                 const OSCertHandles& intermediates)
     : cert_handle_(cert_handle),
       source_(source) {
+#if defined(OS_MACOSX) || defined(OS_WIN)
+  // Copy/retain the intermediate cert handles.
+  for (size_t i = 0; i < intermediates.size(); ++i)
+    intermediate_ca_certs_.push_back(DupOSCertHandle(intermediates[i]));
+#endif
+  // Platform-specific initialization.
   Initialize();
 }
 
@@ -202,4 +249,24 @@ bool X509Certificate::HasExpired() const {
   return base::Time::Now() > valid_expiry();
 }
 
+bool X509Certificate::HasIntermediateCertificate(OSCertHandle cert) {
+#if defined(OS_MACOSX) || defined(OS_WIN)
+  for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i) {
+    if (IsSameOSCert(cert, intermediate_ca_certs_[i]))
+      return true;
+  }
+  return false;
+#else
+  return true;
+#endif
+}
+
+bool X509Certificate::HasIntermediateCertificates(const OSCertHandles& certs) {
+  for (size_t i = 0; i < certs.size(); ++i) {
+    if (!HasIntermediateCertificate(certs[i]))
+      return false;
+  }
+  return true;
+}
+
 }  // namespace net
diff --git a/net/base/x509_certificate.h b/net/base/x509_certificate.h
index f3f3fea..ec287ce 100644
--- a/net/base/x509_certificate.h
+++ b/net/base/x509_certificate.h
@@ -72,6 +72,8 @@ class X509Certificate : public base::RefCountedThreadSafe<X509Certificate> {
   typedef void* OSCertHandle;
 #endif
 
+  typedef std::vector<OSCertHandle> OSCertHandles;
+
   // Principal represent an X.509 principal.
   struct Principal {
     Principal() { }
@@ -152,10 +154,11 @@ class X509Certificate : public base::RefCountedThreadSafe<X509Certificate> {
   // prefers the handle from the network because our HTTP cache isn't
   // caching the corresponding intermediate CA certificates yet
   // (http://crbug.com/7065).
-  //
+  // The list of intermediate certificates is ignored under NSS (i.e. Linux.)
   // The returned pointer must be stored in a scoped_refptr<X509Certificate>.
   static X509Certificate* CreateFromHandle(OSCertHandle cert_handle,
-                                           Source source);
+      Source source,
+      const OSCertHandles& intermediates);
 
   // Create an X509Certificate from the BER-encoded representation.
   // Returns NULL on failure.
@@ -210,20 +213,20 @@ class X509Certificate : public base::RefCountedThreadSafe<X509Certificate> {
   bool HasExpired() const;
 
 #if defined(OS_MACOSX) || defined(OS_WIN)
-  // Adds an untrusted intermediate certificate that may be needed for
-  // chain building.
-  void AddIntermediateCertificate(OSCertHandle cert) {
-    intermediate_ca_certs_.push_back(cert);
-  }
-
   // Returns intermediate certificates added via AddIntermediateCertificate().
   // Ownership follows the "get" rule: it is the caller's responsibility to
   // retain the elements of the result.
-  const std::vector<OSCertHandle>& GetIntermediateCertificates() const {
+  const OSCertHandles& GetIntermediateCertificates() const {
     return intermediate_ca_certs_;
   }
 #endif
 
+  // Returns true if I already contain the given intermediate cert.
+  bool HasIntermediateCertificate(OSCertHandle cert);
+
+  // Returns true if I already contain all the given intermediate certs.
+  bool HasIntermediateCertificates(const OSCertHandles& certs);
+
 #if defined(OS_MACOSX)
   // Does this certificate's usage allow SSL client authentication?
   bool SupportsSSLClientAuth() const;
@@ -262,9 +265,14 @@ class X509Certificate : public base::RefCountedThreadSafe<X509Certificate> {
 
   OSCertHandle os_cert_handle() const { return cert_handle_; }
 
+  // Returns true if two OSCertHandles refer to identical certificates.
+  static bool IsSameOSCert(OSCertHandle a, OSCertHandle b);
+
+
  private:
   friend class base::RefCountedThreadSafe<X509Certificate>;
   FRIEND_TEST(X509CertificateTest, Cache);
+  FRIEND_TEST(X509CertificateTest, IntermediateCertificates);
 
   // A cache of X509Certificate objects.
   class Cache {
@@ -294,7 +302,8 @@ class X509Certificate : public base::RefCountedThreadSafe<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, Source source,
+                  const OSCertHandles& intermediates);
 
   ~X509Certificate();
 
@@ -308,7 +317,10 @@ class X509Certificate : public base::RefCountedThreadSafe<X509Certificate> {
   static OSCertHandle CreateOSCertHandleFromBytes(const char* data,
                                                   int length);
 
-  // Frees an OS certificate handle.
+  // Duplicates (or adds a reference to) an OS certificate handle.
+  static OSCertHandle DupOSCertHandle(OSCertHandle cert_handle);
+
+  // 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
@@ -335,8 +347,8 @@ class X509Certificate : public base::RefCountedThreadSafe<X509Certificate> {
 
 #if defined(OS_MACOSX) || defined(OS_WIN)
   // Untrusted intermediate certificates associated with this certificate
-  // that may be needed for chain building.
-  std::vector<OSCertHandle> intermediate_ca_certs_;
+  // that may be needed for chain building. (NSS impl does not need these.)
+  OSCertHandles intermediate_ca_certs_;
 #endif
 
   // Where the certificate comes from.
diff --git a/net/base/x509_certificate_mac.cc b/net/base/x509_certificate_mac.cc
index 64098f4..16e7604 100644
--- a/net/base/x509_certificate_mac.cc
+++ b/net/base/x509_certificate_mac.cc
@@ -405,9 +405,6 @@ void X509Certificate::Initialize() {
                     &valid_expiry_);
 
   fingerprint_ = CalculateFingerprint(cert_handle_);
-
-  // Store the certificate in the cache in case we need it later.
-  X509Certificate::Cache::GetInstance()->Insert(this);
 }
 
 // static
@@ -689,6 +686,14 @@ X509Certificate::OSCertHandle X509Certificate::CreateOSCertHandleFromBytes(
 }
 
 // static
+X509Certificate::OSCertHandle X509Certificate::DupOSCertHandle(
+    OSCertHandle handle) {
+  if (!handle)
+    return NULL;
+  return reinterpret_cast<OSCertHandle>(const_cast<void*>(CFRetain(handle)));
+}
+
+// static
 void X509Certificate::FreeOSCertHandle(OSCertHandle cert_handle) {
   CFRelease(cert_handle);
 }
@@ -784,7 +789,8 @@ bool X509Certificate::GetSSLClientCertificates (
       continue;
 
     scoped_refptr<X509Certificate> cert(
-        CreateFromHandle(cert_handle, SOURCE_LONE_CERT_IMPORT));
+        CreateFromHandle(cert_handle, SOURCE_LONE_CERT_IMPORT,
+                         OSCertHandles()));
     // cert_handle is adoped by cert, so I don't need to release it myself.
     if (cert->HasExpired() || !cert->SupportsSSLClientAuth())
       continue;
diff --git a/net/base/x509_certificate_nss.cc b/net/base/x509_certificate_nss.cc
index b25688e..705690a 100644
--- a/net/base/x509_certificate_nss.cc
+++ b/net/base/x509_certificate_nss.cc
@@ -472,9 +472,6 @@ void X509Certificate::Initialize() {
   ParseDate(&cert_handle_->validity.notAfter, &valid_expiry_);
 
   fingerprint_ = CalculateFingerprint(cert_handle_);
-
-  // Store the certificate in the cache in case we need it later.
-  X509Certificate::Cache::GetInstance()->Insert(this);
 }
 
 // static
@@ -630,6 +627,12 @@ X509Certificate::OSCertHandle X509Certificate::CreateOSCertHandleFromBytes(
 }
 
 // static
+X509Certificate::OSCertHandle X509Certificate::DupOSCertHandle(
+    OSCertHandle cert_handle) {
+  return CERT_DupCertificate(cert_handle);
+}
+
+// static
 void X509Certificate::FreeOSCertHandle(OSCertHandle cert_handle) {
   CERT_DestroyCertificate(cert_handle);
 }
diff --git a/net/base/x509_certificate_unittest.cc b/net/base/x509_certificate_unittest.cc
index 7904cf0..54de7a0 100644
--- a/net/base/x509_certificate_unittest.cc
+++ b/net/base/x509_certificate_unittest.cc
@@ -309,14 +309,16 @@ TEST(X509CertificateTest, 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);
+      google_cert_handle, X509Certificate::SOURCE_LONE_CERT_IMPORT,
+      X509Certificate::OSCertHandles());
 
   // Add a certificate from the same source (SOURCE_LONE_CERT_IMPORT).  This
   // should return the cached certificate (cert1).
   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);
+      google_cert_handle, X509Certificate::SOURCE_LONE_CERT_IMPORT,
+      X509Certificate::OSCertHandles());
 
   EXPECT_EQ(cert1, cert2);
 
@@ -325,7 +327,8 @@ TEST(X509CertificateTest, Cache) {
   google_cert_handle = X509Certificate::CreateOSCertHandleFromBytes(
       reinterpret_cast<const char*>(google_der), sizeof(google_der));
   scoped_refptr<X509Certificate> cert3 = X509Certificate::CreateFromHandle(
-      google_cert_handle, X509Certificate::SOURCE_FROM_NETWORK);
+      google_cert_handle, X509Certificate::SOURCE_FROM_NETWORK,
+      X509Certificate::OSCertHandles());
 
   EXPECT_NE(cert1, cert3);
 
@@ -334,14 +337,16 @@ TEST(X509CertificateTest, Cache) {
   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);
+      google_cert_handle, X509Certificate::SOURCE_FROM_NETWORK,
+      X509Certificate::OSCertHandles());
 
   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);
+      google_cert_handle, X509Certificate::SOURCE_FROM_NETWORK,
+      X509Certificate::OSCertHandles());
 
   EXPECT_EQ(cert3, cert5);
 }
@@ -396,4 +401,61 @@ TEST(X509CertificateTest, Policy) {
   EXPECT_TRUE(policy.HasDeniedCert());
 }
 
+#if defined(OS_MACOSX) || defined(OS_WIN)
+TEST(X509CertificateTest, IntermediateCertificates) {
+  X509Certificate::OSCertHandle handle1, handle2, handle3, handle4;
+
+  // Create object with no intermediates:
+  handle1 = X509Certificate::CreateOSCertHandleFromBytes(
+      reinterpret_cast<const char*>(google_der), sizeof(google_der));
+  X509Certificate::OSCertHandles intermediates1;
+  scoped_refptr<X509Certificate> cert1;
+  cert1 = X509Certificate::CreateFromHandle(handle1,
+      X509Certificate::SOURCE_FROM_NETWORK,
+      intermediates1);
+  EXPECT_TRUE(cert1->HasIntermediateCertificates(intermediates1));
+  handle2 = X509Certificate::CreateOSCertHandleFromBytes(
+      reinterpret_cast<const char*>(webkit_der), sizeof(webkit_der));
+  EXPECT_FALSE(cert1->HasIntermediateCertificate(handle2));
+
+  // Create object with 2 intermediates:
+  handle1 = X509Certificate::CreateOSCertHandleFromBytes(
+      reinterpret_cast<const char*>(google_der), sizeof(google_der));
+  X509Certificate::OSCertHandles intermediates2;
+  handle3 = X509Certificate::CreateOSCertHandleFromBytes(
+      reinterpret_cast<const char*>(thawte_der), sizeof(thawte_der));
+  intermediates2.push_back(handle2);
+  intermediates2.push_back(handle3);
+  scoped_refptr<X509Certificate> cert2;
+  cert2 = X509Certificate::CreateFromHandle(
+      X509Certificate::DupOSCertHandle(handle1),
+      X509Certificate::SOURCE_FROM_NETWORK,
+      intermediates2);
+
+  // The cache should have stored cert2 'cause it has more intermediates:
+  EXPECT_NE(cert1, cert2);
+
+  // Verify it has all the intermediates:
+  EXPECT_TRUE(cert2->HasIntermediateCertificate(handle2));
+  EXPECT_TRUE(cert2->HasIntermediateCertificate(handle3));
+  handle4 = X509Certificate::CreateOSCertHandleFromBytes(
+      reinterpret_cast<const char*>(paypal_null_der), sizeof(paypal_null_der));
+  EXPECT_FALSE(cert2->HasIntermediateCertificate(handle4));
+
+  // Create object with 1 intermediate:
+  handle3 = X509Certificate::CreateOSCertHandleFromBytes(
+      reinterpret_cast<const char*>(thawte_der), sizeof(thawte_der));
+  X509Certificate::OSCertHandles intermediates3;
+  intermediates2.push_back(handle3);
+  scoped_refptr<X509Certificate> cert3;
+  cert3 = X509Certificate::CreateFromHandle(
+      X509Certificate::DupOSCertHandle(handle1),
+      X509Certificate::SOURCE_FROM_NETWORK,
+      intermediates3);
+
+  // The cache should have returned cert2 'cause it has more intermediates:
+  EXPECT_EQ(cert3, cert2);
+}
+#endif
+
 }  // namespace net
diff --git a/net/base/x509_certificate_win.cc b/net/base/x509_certificate_win.cc
index df43814..7f3f09e 100644
--- a/net/base/x509_certificate_win.cc
+++ b/net/base/x509_certificate_win.cc
@@ -463,9 +463,6 @@ void X509Certificate::Initialize() {
   valid_expiry_ = Time::FromFileTime(cert_handle_->pCertInfo->NotAfter);
 
   fingerprint_ = CalculateFingerprint(cert_handle_);
-
-  // Store the certificate in the cache in case we need it later.
-  X509Certificate::Cache::GetInstance()->Insert(this);
 }
 
 // static
@@ -484,7 +481,8 @@ X509Certificate* X509Certificate::CreateFromPickle(const Pickle& pickle,
       NULL, reinterpret_cast<const void **>(&cert_handle)))
     return NULL;
 
-  return CreateFromHandle(cert_handle, SOURCE_LONE_CERT_IMPORT);
+  return CreateFromHandle(cert_handle, SOURCE_LONE_CERT_IMPORT,
+                          OSCertHandles());
 }
 
 void X509Certificate::Persist(Pickle* pickle) {
@@ -746,6 +744,13 @@ X509Certificate::OSCertHandle X509Certificate::CreateOSCertHandleFromBytes(
   return cert_handle;
 }
 
+
+// static
+X509Certificate::OSCertHandle X509Certificate::DupOSCertHandle(
+    OSCertHandle cert_handle) {
+  return CertDuplicateCertificateContext(cert_handle);
+}
+
 // static
 void X509Certificate::FreeOSCertHandle(OSCertHandle cert_handle) {
   CertFreeCertificateContext(cert_handle);
diff --git a/net/socket/ssl_client_socket_mac.cc b/net/socket/ssl_client_socket_mac.cc
index b3cfc44..ebb31f2 100644
--- a/net/socket/ssl_client_socket_mac.cc
+++ b/net/socket/ssl_client_socket_mac.cc
@@ -412,29 +412,22 @@ X509Certificate* GetServerCert(SSLContextRef ssl_context) {
 
   DCHECK_GT(CFArrayGetCount(certs), 0);
 
-  SecCertificateRef server_cert = static_cast<SecCertificateRef>(
-      const_cast<void*>(CFArrayGetValueAtIndex(certs, 0)));
-  CFRetain(server_cert);
-  X509Certificate *x509_cert = X509Certificate::CreateFromHandle(
-      server_cert, X509Certificate::SOURCE_FROM_NETWORK);
-  if (!x509_cert)
-    return NULL;
-
   // Add each of the intermediate certificates in the server's chain to the
   // server's X509Certificate object. This makes them available to
   // X509Certificate::Verify() for chain building.
-  // TODO(wtc): Since X509Certificate::CreateFromHandle may return a cached
-  // X509Certificate object, we may be adding intermediate CA certificates to
-  // it repeatedly!
+  std::vector<SecCertificateRef> intermediate_ca_certs;
   CFIndex certs_length = CFArrayGetCount(certs);
   for (CFIndex i = 1; i < certs_length; ++i) {
     SecCertificateRef cert_ref = reinterpret_cast<SecCertificateRef>(
         const_cast<void*>(CFArrayGetValueAtIndex(certs, i)));
-    CFRetain(cert_ref);
-    x509_cert->AddIntermediateCertificate(cert_ref);
+    intermediate_ca_certs.push_back(cert_ref);
   }
 
-  return x509_cert;
+  SecCertificateRef server_cert = static_cast<SecCertificateRef>(
+      const_cast<void*>(CFArrayGetValueAtIndex(certs, 0)));
+  CFRetain(server_cert);
+  return X509Certificate::CreateFromHandle(
+      server_cert, X509Certificate::SOURCE_FROM_NETWORK, 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 994f1f3..4047c17 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -595,24 +595,11 @@ X509Certificate *SSLClientSocketNSS::UpdateServerCert() {
       if (!cert_store_)
         cert_store_ = CertOpenStore(CERT_STORE_PROV_MEMORY, 0, NULL, 0, NULL);
 
+      // Get each of the intermediate certificates in the server's chain.
+      // These will be added to the server's X509Certificate object, making
+      // them available to X509Certificate::Verify() for chain building.
+      X509Certificate::OSCertHandles intermediate_ca_certs;
       PCCERT_CONTEXT cert_context = NULL;
-      BOOL ok = CertAddEncodedCertificateToStore(
-          cert_store_,
-          X509_ASN_ENCODING | PKCS_7_ASN_ENCODING,
-          server_cert_nss_->derCert.data,
-          server_cert_nss_->derCert.len,
-          CERT_STORE_ADD_USE_EXISTING,
-          &cert_context);
-      DCHECK(ok);
-      server_cert_ = X509Certificate::CreateFromHandle(
-          cert_context, X509Certificate::SOURCE_FROM_NETWORK);
-
-      // Add each of the intermediate certificates in the server's chain to
-      // the server's X509Certificate object. This makes them available to
-      // X509Certificate::Verify() for chain building.
-      // TODO(wtc): Since X509Certificate::CreateFromHandle may return a
-      // cached X509Certificate object, we may be adding intermediate CA
-      // certificates to it repeatedly!
       CERTCertList* cert_list = CERT_GetCertChainFromCert(
           server_cert_nss_, PR_Now(), certUsageSSLCA);
       if (cert_list) {
@@ -620,7 +607,7 @@ X509Certificate *SSLClientSocketNSS::UpdateServerCert() {
              !CERT_LIST_END(node, cert_list);
              node = CERT_LIST_NEXT(node)) {
           cert_context = NULL;
-          ok = CertAddEncodedCertificateToStore(
+          BOOL ok = CertAddEncodedCertificateToStore(
               cert_store_,
               X509_ASN_ENCODING | PKCS_7_ASN_ENCODING,
               node->cert->derCert.data,
@@ -629,14 +616,31 @@ X509Certificate *SSLClientSocketNSS::UpdateServerCert() {
               &cert_context);
           DCHECK(ok);
           if (node->cert != server_cert_nss_)
-            server_cert_->AddIntermediateCertificate(cert_context);
+            intermediate_ca_certs.push_back(cert_context);
         }
         CERT_DestroyCertList(cert_list);
       }
+
+      // Finally create the X509Certificate object.
+      BOOL ok = CertAddEncodedCertificateToStore(
+          cert_store_,
+          X509_ASN_ENCODING | PKCS_7_ASN_ENCODING,
+          server_cert_nss_->derCert.data,
+          server_cert_nss_->derCert.len,
+          CERT_STORE_ADD_USE_EXISTING,
+          &cert_context);
+      DCHECK(ok);
+      server_cert_ = X509Certificate::CreateFromHandle(
+          cert_context,
+          X509Certificate::SOURCE_FROM_NETWORK,
+          intermediate_ca_certs);
+      for (size_t i = 0; i < intermediate_ca_certs.size(); ++i)
+        CertFreeCertificateContext(intermediate_ca_certs[i]);
 #else
       server_cert_ = X509Certificate::CreateFromHandle(
           CERT_DupCertificate(server_cert_nss_),
-          X509Certificate::SOURCE_FROM_NETWORK);
+          X509Certificate::SOURCE_FROM_NETWORK,
+          X509Certificate::OSCertHandles());
 #endif
     }
   }
@@ -1139,7 +1143,8 @@ SECStatus SSLClientSocketNSS::ClientAuthHandler(
         privkey = PK11_FindKeyByAnyCert(cert, wincx);
         if (privkey) {
           X509Certificate* x509_cert = X509Certificate::CreateFromHandle(
-              cert, X509Certificate::SOURCE_LONE_CERT_IMPORT);
+              cert, X509Certificate::SOURCE_LONE_CERT_IMPORT,
+              net::X509Certificate::OSCertHandles());
           that->client_certs_.push_back(x509_cert);
           SECKEY_DestroyPrivateKey(privkey);
           continue;
diff --git a/net/socket/ssl_client_socket_win.cc b/net/socket/ssl_client_socket_win.cc
index 7e76f9e..bf4a547 100644
--- a/net/socket/ssl_client_socket_win.cc
+++ b/net/socket/ssl_client_socket_win.cc
@@ -68,13 +68,6 @@ static int MapSecurityError(SECURITY_STATUS err) {
   }
 }
 
-// Returns true if the two CERT_CONTEXTs contain the same certificate.
-bool SameCert(PCCERT_CONTEXT a, PCCERT_CONTEXT b) {
-  return a == b ||
-         (a->cbCertEncoded == b->cbCertEncoded &&
-         memcmp(a->pbCertEncoded, b->pbCertEncoded, b->cbCertEncoded) == 0);
-}
-
 //-----------------------------------------------------------------------------
 
 // A bitmask consisting of these bit flags encodes which versions of the SSL
@@ -418,7 +411,8 @@ void SSLClientSocketWin::GetSSLCertRequestInfo(
       continue;
     }
     scoped_refptr<X509Certificate> cert = X509Certificate::CreateFromHandle(
-        cert_context2, X509Certificate::SOURCE_LONE_CERT_IMPORT);
+        cert_context2, X509Certificate::SOURCE_LONE_CERT_IMPORT,
+        net::X509Certificate::OSCertHandles());
     cert_request_info->client_certs.push_back(cert);
   }
 
@@ -1303,14 +1297,16 @@ int SSLClientSocketWin::DidCompleteHandshake() {
     return MapSecurityError(status);
   }
   if (renegotiating_ &&
-      SameCert(server_cert_->os_cert_handle(), server_cert_handle)) {
+      X509Certificate::IsSameOSCert(server_cert_->os_cert_handle(),
+                                    server_cert_handle)) {
     // We already verified the server certificate.  Either it is good or the
     // user has accepted the certificate error.
     CertFreeCertificateContext(server_cert_handle);
     DidCompleteRenegotiation();
   } else {
     server_cert_ = X509Certificate::CreateFromHandle(
-        server_cert_handle, X509Certificate::SOURCE_FROM_NETWORK);
+        server_cert_handle, X509Certificate::SOURCE_FROM_NETWORK,
+        net::X509Certificate::OSCertHandles());
 
     next_state_ = STATE_VERIFY_CERT;
   }
diff --git a/net/socket/ssl_test_util.cc b/net/socket/ssl_test_util.cc
index 55d104c..56eba9c 100644
--- a/net/socket/ssl_test_util.cc
+++ b/net/socket/ssl_test_util.cc
@@ -112,7 +112,8 @@ static net::X509Certificate* LoadTemporaryCert(const FilePath& filename) {
       const_cast<void*>(CFArrayGetValueAtIndex(cert_array, 0)));
   CFRetain(cert_ref);
   return net::X509Certificate::CreateFromHandle(cert_ref,
-      net::X509Certificate::SOURCE_FROM_NETWORK);
+      net::X509Certificate::SOURCE_LONE_CERT_IMPORT,
+      net::X509Certificate::OSCertHandles());
 }
 #endif