Ensure X509Certificate::OSCertHandles are safe to be used on both UI and IO threads on Win

net/base/x509_certificate_win.cc

Index: net/base/x509_certificate_win.cc
diff --git a/net/base/x509_certificate_win.cc b/net/base/x509_certificate_win.cc
index 8442fdbcd7e10ebdc686835ccdecab8fda4169ef..064ac606c91921ff8ba49284702a1887316d044d 100644
--- a/net/base/x509_certificate_win.cc
+++ b/net/base/x509_certificate_win.cc
@@ -630,6 +630,46 @@ void X509Certificate::GetDNSNames(std::vector<std::string>* dns_names) const {
     dns_names->push_back(subject_.common_name);
 }
 
+X509Certificate::OSCertListHandle
+X509Certificate::CreateOSCertListHandle() const {
+  // Create a temporary certificate store to hold |cert_handle_| and any

[wtc, 2011/10/04 00:26:34]

temporary => in-memory

+  // associated intermediates. The store will be referenced in the returned

[wtc, 2011/10/04 00:26:34]

intermediates => intermediate certificates
            or   intermediate CA certificates

+  // OSCertListHandle, and will not be freed until the OSCertListHandle is
+  // freed.
+  OSCertListHandle cert_list = NULL;
+  DWORD flags = CERT_STORE_DEFER_CLOSE_UNTIL_LAST_FREE_FLAG;
+  HCERTSTORE store = CertOpenStore(CERT_STORE_PROV_MEMORY, 0,
+                                   NULL, flags, NULL);
+
+  if (store) {
+    // NOTE: This preserves all of the properties of |cert_handle_| except for
+    // CERT_KEY_PROV_HANDLE_PROP_ID and CERT_KEY_CONTEXT_PROP_ID - the two
+    // properties that hold access to already-opened private keys. If a handle
+    // has already been unlocked (eg: PIN prompt), then the first time that
+    // the identity is used for client auth, it may prompt the user again.

[wtc, 2011/10/04 00:26:34]

Nit: identity => certificate handle?

Do we ever use the return value of CreateOSCertListHandle()
for client auth?  I think we only call CreateOSCertListHandle()
in VerifyInternal() and the certificate viewer dialog, so
I don't think the scenario this comment describes will ever
happen.

[Ryan Sleevi, 2011/10/04 03:38:07]

Now that certificate prompting has been moved into native UI, this probably
doesn't matter as much. The native UI will access to HCERSTOREs of incoming
certs in order to build chains (used to prioritize the order in the UI).

I feel like leaving this subtle note documented is important, although I agree,
in the current code, this shouldn't be hit. Would you prefer I remove it
entirely?

[wtc, 2011/10/04 18:00:51]

You can keep this comment.  I asked the question to verify
my understanding of your code.  It did confuse me a little
bit, but not much.

+    BOOL ok = CertAddCertificateContextToStore(store, cert_handle_,
+                                               CERT_STORE_ADD_ALWAYS,
+                                               &cert_list);
+    if (ok && cert_list) {
+      for (size_t i = 0; ok && i < intermediate_ca_certs_.size(); ++i) {
+        ok = CertAddCertificateContextToStore(store, intermediate_ca_certs_[i],
+                                              CERT_STORE_ADD_ALWAYS, NULL);
+      }
+    }
+
+    // If |cert_list| points to a valid certificate, this will not actually
+    // close and free |store| until |cert_list| is freed.
+    CertCloseStore(store, 0);
+
+    if (!ok) {
+      FreeOSCertListHandle(cert_list);
+      return NULL;
+    }
+  }
+
+  return cert_list;
+}
+
 int X509Certificate::VerifyInternal(const std::string& hostname,
                                     int flags,
                                     CertVerifyResult* verify_result) const {
@@ -696,21 +736,24 @@ int X509Certificate::VerifyInternal(const std::string& hostname,
   if (TestRootCerts::HasInstance())
     chain_engine.reset(TestRootCerts::GetInstance()->GetChainEngine());
 
+  OSCertListHandle cert_list = CreateOSCertListHandle();
   PCCERT_CHAIN_CONTEXT chain_context;
   // IE passes a non-NULL pTime argument that specifies the current system
   // time.  IE passes CERT_CHAIN_REVOCATION_CHECK_CHAIN_EXCLUDE_ROOT as the
   // chain_flags argument.
   if (!CertGetCertificateChain(
            chain_engine,
-           cert_handle_,
+           cert_list,
            NULL,  // current system time
-           cert_handle_->hCertStore,
+           cert_list->hCertStore,
            &chain_para,
            chain_flags,
            NULL,  // reserved
            &chain_context)) {
+    FreeOSCertListHandle(cert_list);

[wtc, 2011/10/04 00:26:34]

Do we have a scoped type for PCCERTCONTEXT?  We could use it
here for |cert_list|.

     return MapSecurityError(GetLastError());
   }
+
   if (chain_context->TrustStatus.dwErrorStatus &
       CERT_TRUST_IS_NOT_VALID_FOR_USAGE) {
     ev_policy_oid = NULL;
@@ -719,16 +762,19 @@ int X509Certificate::VerifyInternal(const std::string& hostname,
     CertFreeCertificateChain(chain_context);
     if (!CertGetCertificateChain(
              chain_engine,
-             cert_handle_,
+             cert_list,
              NULL,  // current system time
-             cert_handle_->hCertStore,
+             cert_list->hCertStore,
              &chain_para,
              chain_flags,
              NULL,  // reserved
              &chain_context)) {
+      FreeOSCertListHandle(cert_list);
       return MapSecurityError(GetLastError());
     }
   }
+  FreeOSCertListHandle(cert_list);
+
   ScopedCertChainContext scoped_chain_context(chain_context);
 
   GetCertChainInfo(chain_context, verify_result);
@@ -940,6 +986,11 @@ void X509Certificate::FreeOSCertHandle(OSCertHandle cert_handle) {
 }
 
 // static
+void X509Certificate::FreeOSCertListHandle(OSCertListHandle cert_list) {
+  CertFreeCertificateContext(cert_list);
+}
+
+// static
 SHA1Fingerprint X509Certificate::CalculateFingerprint(
     OSCertHandle cert) {
   DCHECK(NULL != cert->pbCertEncoded);