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

net/base/x509_certificate_win.cc

 // Copyright (c) 2011 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/x509_certificate.h"
 
 #include "base/logging.h"
 #include "base/pickle.h"
 #include "base/sha1.h"
 #include "base/string_tokenizer.h"
@@ skipping 612 matching lines @@
         if (alt_name->rgAltEntry[i].dwAltNameChoice == CERT_ALT_NAME_DNS_NAME)
           dns_names->push_back(
               WideToASCII(alt_name->rgAltEntry[i].pwszDNSName));
       }
     }
   }
   if (dns_names->empty())
     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 {
   if (!cert_handle_)
     return ERR_UNEXPECTED;
 
   // Build and validate certificate chain.
   CERT_CHAIN_PARA chain_para;
   memset(&chain_para, 0, sizeof(chain_para));
   chain_para.cbSize = sizeof(chain_para);
@@ skipping 46 matching lines @@
   // corresponds to HCCE_CURRENT_USER and is is initialized as needed by
   // crypt32. However, when testing, it is necessary to create a new
   // HCERTCHAINENGINE and use that instead. This is because each
   // HCERTCHAINENGINE maintains a cache of information about certificates
   // encountered, and each test run may modify the trust status of a
   // certificate.
   ScopedHCERTCHAINENGINE chain_engine(NULL);
   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;
     chain_para.RequestedIssuancePolicy.Usage.cUsageIdentifier = 0;
     chain_para.RequestedIssuancePolicy.Usage.rgpszUsageIdentifier = NULL;
     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);
   verify_result->cert_status |= MapCertChainErrorStatusToCertStatus(
       chain_context->TrustStatus.dwErrorStatus);
 
   // Treat certificates signed using broken signature algorithms as invalid.
   if (verify_result->has_md4)
     verify_result->cert_status |= CERT_STATUS_INVALID;
 
@@ skipping 191 matching lines @@
     OSCertHandle cert_handle) {
   return CertDuplicateCertificateContext(cert_handle);
 }
 
 // static
 void X509Certificate::FreeOSCertHandle(OSCertHandle cert_handle) {
   CertFreeCertificateContext(cert_handle);
 }
 
 // static
+void X509Certificate::FreeOSCertListHandle(OSCertListHandle cert_list) {
+  CertFreeCertificateContext(cert_list);
+}
+
+// static
 SHA1Fingerprint X509Certificate::CalculateFingerprint(
     OSCertHandle cert) {
   DCHECK(NULL != cert->pbCertEncoded);
   DCHECK_NE(static_cast<DWORD>(0), cert->cbCertEncoded);
 
   BOOL rv;
   SHA1Fingerprint sha1;
   DWORD sha1_size = sizeof(sha1.data);
   rv = CryptHashCertificate(NULL, CALG_SHA1, 0, cert->pbCertEncoded,
                             cert->cbCertEncoded, sha1.data, &sha1_size);
@@ skipping 38 matching lines @@
   if (!CertSerializeCertificateStoreElement(cert_handle, 0, &buffer[0],
                                             &length)) {
     return false;
   }
 
   return pickle->WriteData(reinterpret_cast<const char*>(&buffer[0]),
                            length);
 }
 
 }  // namespace net