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

Mirror the behaviour of SChannel by creating a new in-memory HCERTSTORE containing the certificate and its intermediate CA certificates whenever it is necessary to pass in a PCCERT_CONTEXT to a Windows API that may need to access the PCCERT_CONTEXT->hCertStore - such as certificate chain verification or display.

This also paves the way for removing the GlobalCertStore on Windows, which was necessary in order to link certificates with their intermediates for these same APIs.

BUG=47648
TEST=net_unittests:X509CertificateTest.* should cover this. Additionally, on a fresh profile, navigate to different HTTPS sites. From the Page Info Bubble, select Certificate Information, and in the Windows Certificate Viewer, click "Certification Path" and confirm the entire certificate chain is displayed. This is a variation of testing for http://crbug.com/45706, which should not regress.

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

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

7 files changed, 163 insertions, 79 deletions

diff --git a/chrome/browser/ui/cocoa/certificate_viewer.mm b/chrome/browser/ui/cocoa/certificate_viewer.mm
index 3b6a656..8fbe2af 100644
--- a/chrome/browser/ui/cocoa/certificate_viewer.mm
+++ b/chrome/browser/ui/cocoa/certificate_viewer.mm
@@ -9,30 +9,15 @@
 
 #include <vector>
 
-#include "base/logging.h"
+#include "base/mac/foundation_util.h"
 #include "base/mac/scoped_cftyperef.h"
 #include "net/base/x509_certificate.h"
 
 void ShowCertificateViewer(gfx::NativeWindow parent,
                            net::X509Certificate* cert) {
-  SecCertificateRef cert_mac = cert->os_cert_handle();
-  if (!cert_mac)
-    return;
-
-  base::mac::ScopedCFTypeRef<CFMutableArrayRef> certificates(
-      CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks));
-  if (!certificates.get()) {
-    NOTREACHED();
-    return;
-  }
-  CFArrayAppendValue(certificates, cert_mac);
-
-  // Server certificate must be first in the array; subsequent certificates
-  // in the chain can be in any order.
-  const std::vector<SecCertificateRef>& ca_certs =
-      cert->GetIntermediateCertificates();
-  for (size_t i = 0; i < ca_certs.size(); ++i)
-    CFArrayAppendValue(certificates, ca_certs[i]);
+  base::mac::ScopedCFTypeRef<CFArrayRef> cert_chain(
+      cert->CreateOSCertChainForCert());
+  NSArray* certificates = base::mac::CFToNSCast(cert_chain.get());
 
   // Explicitly disable revocation checking, regardless of user preferences
   // or system settings. The behaviour of SFCertificatePanel is to call
@@ -77,7 +62,7 @@ void ShowCertificateViewer(gfx::NativeWindow parent,
                modalDelegate:nil
               didEndSelector:NULL
                  contextInfo:NULL
-                certificates:reinterpret_cast<NSArray*>(certificates.get())
+                certificates:certificates
                    showGroup:YES];
   // The SFCertificatePanel releases itself when the sheet is dismissed.
 }
diff --git a/chrome/browser/ui/views/certificate_viewer_win.cc b/chrome/browser/ui/views/certificate_viewer_win.cc
index ba54267..9e5007c 100644
--- a/chrome/browser/ui/views/certificate_viewer_win.cc
+++ b/chrome/browser/ui/views/certificate_viewer_win.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -8,10 +8,16 @@
 #include <cryptuiapi.h>
 #pragma comment(lib, "cryptui.lib")
 
+#include "base/logging.h"
 #include "net/base/x509_certificate.h"
 
 void ShowCertificateViewer(gfx::NativeWindow parent,
                            net::X509Certificate* cert) {
+  // Create a new cert context and store containing just the certificate
+  // and its intermediate certificates.
+  PCCERT_CONTEXT cert_list = cert->CreateOSCertChainForCert();
+  CHECK(cert_list);
+
   CRYPTUI_VIEWCERTIFICATE_STRUCT view_info = { 0 };
   view_info.dwSize = sizeof(view_info);
   // We set our parent to the tab window. This makes the cert dialog created
@@ -19,9 +25,8 @@ void ShowCertificateViewer(gfx::NativeWindow parent,
   view_info.hwndParent = parent;
   view_info.dwFlags = CRYPTUI_DISABLE_EDITPROPERTIES |
                       CRYPTUI_DISABLE_ADDTOSTORE;
-  view_info.pCertContext = cert->os_cert_handle();
-  // Search the cert store that 'cert' is in when building the cert chain.
-  HCERTSTORE cert_store = view_info.pCertContext->hCertStore;
+  view_info.pCertContext = cert_list;
+  HCERTSTORE cert_store = cert_list->hCertStore;
   view_info.cStores = 1;
   view_info.rghStores = &cert_store;
   BOOL properties_changed;
@@ -29,4 +34,6 @@ void ShowCertificateViewer(gfx::NativeWindow parent,
   // This next call blocks but keeps processing windows messages, making it
   // modal to the browser window.
   BOOL rv = ::CryptUIDlgViewCertificate(&view_info, &properties_changed);
+
+  CertFreeCertificateContext(cert_list);
 }
diff --git a/net/base/scoped_cert_chain_context.h b/net/base/scoped_cert_chain_context.h
deleted file mode 100644
index 668e46e..0000000
--- a/net/base/scoped_cert_chain_context.h
+++ /dev/null
@@ -1,30 +0,0 @@
-// 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.
-
-#ifndef NET_BASE_SCOPED_CERT_CHAIN_CONTEXT_H_
-#define NET_BASE_SCOPED_CERT_CHAIN_CONTEXT_H_
-#pragma once
-
-#include <windows.h>
-#include <wincrypt.h>
-
-#include "base/memory/scoped_ptr.h"
-
-namespace net {
-
-// This class wraps the CertFreeCertificateChain function in a class that can
-// be passed as a template argument to scoped_ptr_malloc.
-class ScopedPtrMallocFreeCertChain {
- public:
-  void operator()(const CERT_CHAIN_CONTEXT* x) const {
-    CertFreeCertificateChain(x);
-  }
-};
-
-typedef scoped_ptr_malloc<const CERT_CHAIN_CONTEXT,
-                          ScopedPtrMallocFreeCertChain> ScopedCertChainContext;
-
-}  // namespace net
-
-#endif  // NET_BASE_SCOPED_CERT_CHAIN_CONTEXT_H_
diff --git a/net/base/x509_certificate.h b/net/base/x509_certificate.h
index 8a16088..d862110 100644
--- a/net/base/x509_certificate.h
+++ b/net/base/x509_certificate.h
@@ -28,7 +28,7 @@
 #include "base/synchronization/lock.h"
 #elif defined(USE_OPENSSL)
 // Forward declaration; real one in <x509.h>
-struct x509_st;
+typedef struct x509_st X509;
 typedef struct x509_store_st X509_STORE;
 #elif defined(USE_NSS)
 // Forward declaration; real one in <cert.h>
@@ -56,15 +56,15 @@ typedef std::vector<scoped_refptr<X509Certificate> > CertificateList;
 class NET_EXPORT X509Certificate
     : public base::RefCountedThreadSafe<X509Certificate> {
  public:
-  // A handle to the certificate object in the underlying crypto library.
-  // We assume that OSCertHandle is a pointer type on all platforms and
-  // NULL is an invalid OSCertHandle.
+  // An OSCertHandle is a handle to a certificate object in the underlying
+  // crypto library. We assume that OSCertHandle is a pointer type on all
+  // platforms and that NULL represents an invalid OSCertHandle.
 #if defined(OS_WIN)
   typedef PCCERT_CONTEXT OSCertHandle;
 #elif defined(OS_MACOSX)
   typedef SecCertificateRef OSCertHandle;
 #elif defined(USE_OPENSSL)
-  typedef struct x509_st* OSCertHandle;
+  typedef X509* OSCertHandle;
 #elif defined(USE_NSS)
   typedef struct CERTCertificateStr* OSCertHandle;
 #else
@@ -288,6 +288,13 @@ class NET_EXPORT X509Certificate
 
   // Creates the chain of certs to use for this client identity cert.
   CFArrayRef CreateClientCertificateChain() const;
+
+  // Returns a new CFArrayRef containing this certificate and its intermediate
+  // certificates in the form expected by Security.framework and Keychain
+  // Services, or NULL on failure.
+  // The first item in the array will be this certificate, followed by its
+  // intermediates, if any.
+  CFArrayRef CreateOSCertChainForCert() const;
 #endif
 
 #if defined(OS_WIN)
@@ -299,6 +306,52 @@ class NET_EXPORT X509Certificate
   //    this store so that we can close the system store when we finish
   //    searching for client certificates.
   static HCERTSTORE cert_store();
+
+  // Returns a new PCCERT_CONTEXT containing this certificate and its
+  // intermediate certificates, or NULL on failure. The returned
+  // PCCERT_CONTEXT *MUST NOT* be stored in an X509Certificate, as then
+  // os_cert_handle() will not return the correct result. This function is
+  // only necessary if the CERT_CONTEXT.hCertStore member will be accessed or
+  // enumerated, which is generally true for any CryptoAPI functions involving
+  // certificate chains, including validation or certificate display.
+  //
+  // Remarks:
+  // Depending on the CryptoAPI function, Windows may need to access the
+  // HCERTSTORE that the passed-in PCCERT_CONTEXT belongs to, such as to
+  // locate additional intermediates. However, in the current X509Certificate
+  // implementation on Windows, all X509Certificate::OSCertHandles belong to
+  // the same HCERTSTORE - X509Certificate::cert_store(). Since certificates
+  // may be created and accessed on any number of threads, if CryptoAPI is
+  // trying to read this global store while additional certificates are being
+  // added, it may return inconsistent results while enumerating the store.
+  // While the memory accesses themselves are thread-safe, the resultant view
+  // of what is in the store may be altered.
+  //
+  // If OSCertHandles were instead added to a NULL HCERTSTORE, which is valid
+  // in CryptoAPI, then Windows would be unable to locate any of the
+  // intermediates supplied in |intermediate_ca_certs_|, because the
+  // hCertStore will refer to a magic value that indicates "only this
+  // certificate."
+  //
+  // To avoid these problems, a new in-memory HCERTSTORE is created containing
+  // just this certificate and its intermediates. The handle to the version of
+  // this certificate in the new HCERTSTORE is then returned, with the
+  // HCERTSTORE set to be automatically freed when the returned certificate
+  // is freed.
+  //
+  // This function is only needed when the HCERTSTORE of the os_cert_handle()
+  // will be accessed, which is generally only during certificate validation
+  // or display. While the returned PCCERT_CONTEXT and its HCERTSTORE can
+  // safely be used on multiple threads if no further modifications happen, it
+  // is generally preferable for each thread that needs such a context to
+  // obtain its own, rather than risk thread-safety issues by sharing.
+  //
+  // Because of how X509Certificate caching is implemented, attempting to
+  // create an X509Certificate from the returned PCCERT_CONTEXT may result in
+  // the original handle (and thus the originall HCERTSTORE) being returned by
+  // os_cert_handle(). For this reason, the returned PCCERT_CONTEXT *MUST NOT*
+  // be stored in an X509Certificate.
+  PCCERT_CONTEXT CreateOSCertChainForCert() const;
 #endif
 
 #if defined(OS_ANDROID)
@@ -345,6 +398,11 @@ class NET_EXPORT X509Certificate
   // The content of the DER encoded certificate is written to |encoded|.
   bool GetDEREncoded(std::string* encoded);
 
+  // Returns the OSCertHandle of this object. Because of caching, this may
+  // differ from the OSCertHandle originally supplied during initialization.
+  // Note: On Windows, CryptoAPI may return unexpected results if this handle
+  // is used across multiple threads. For more details, see
+  // CreateOSCertChainForCert().
   OSCertHandle os_cert_handle() const { return cert_handle_; }
 
   // Returns true if two OSCertHandles refer to identical certificates.
diff --git a/net/base/x509_certificate_mac.cc b/net/base/x509_certificate_mac.cc
index 3adaed4..61dbbe2 100644
--- a/net/base/x509_certificate_mac.cc
+++ b/net/base/x509_certificate_mac.cc
@@ -747,14 +747,7 @@ int X509Certificate::VerifyInternal(const std::string& hostname,
   // array of certificates, the first of which is the certificate we're
   // verifying, and the subsequent (optional) certificates are used for
   // chain building.
-  CFMutableArrayRef cert_array = CFArrayCreateMutable(kCFAllocatorDefault, 0,
-                                                      &kCFTypeArrayCallBacks);
-  if (!cert_array)
-    return ERR_OUT_OF_MEMORY;
-  ScopedCFTypeRef<CFArrayRef> scoped_cert_array(cert_array);
-  CFArrayAppendValue(cert_array, cert_handle_);
-  for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i)
-    CFArrayAppendValue(cert_array, intermediate_ca_certs_[i]);
+  ScopedCFTypeRef<CFArrayRef> cert_array(CreateOSCertChainForCert());
 
   // From here on, only one thread can be active at a time. We have had a number
   // of sporadic crashes in the SecTrustEvaluate call below, way down inside
@@ -1343,6 +1336,20 @@ CFArrayRef X509Certificate::CreateClientCertificateChain() const {
   return chain.release();
 }
 
+CFArrayRef X509Certificate::CreateOSCertChainForCert() const {
+  CFMutableArrayRef cert_list =
+      CFArrayCreateMutable(kCFAllocatorDefault, 0,
+                           &kCFTypeArrayCallBacks);
+  if (!cert_list)
+    return NULL;
+
+  CFArrayAppendValue(cert_list, os_cert_handle());
+  for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i)
+    CFArrayAppendValue(cert_list, intermediate_ca_certs_[i]);
+
+  return cert_list;
+}
+
 // static
 X509Certificate::OSCertHandle
 X509Certificate::ReadOSCertHandleFromPickle(const Pickle& pickle,
diff --git a/net/base/x509_certificate_win.cc b/net/base/x509_certificate_win.cc
index 0c355b6..efa5e5c 100644
--- a/net/base/x509_certificate_win.cc
+++ b/net/base/x509_certificate_win.cc
@@ -9,6 +9,7 @@
 
 #include "base/lazy_instance.h"
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/pickle.h"
 #include "base/sha1.h"
 #include "base/string_tokenizer.h"
@@ -21,7 +22,6 @@
 #include "net/base/cert_verify_result.h"
 #include "net/base/ev_root_ca_metadata.h"
 #include "net/base/net_errors.h"
-#include "net/base/scoped_cert_chain_context.h"
 #include "net/base/test_root_certs.h"
 #include "net/base/x509_certificate_known_roots_win.h"
 
@@ -33,11 +33,6 @@ namespace net {
 
 namespace {
 
-typedef crypto::ScopedCAPIHandle<
-    HCERTSTORE,
-    crypto::CAPIDestroyerWithFlags<HCERTSTORE,
-                                   CertCloseStore, 0> > ScopedHCERTSTORE;
-
 struct FreeChainEngineFunctor {
   void operator()(HCERTCHAINENGINE engine) const {
     if (engine)
@@ -45,9 +40,35 @@ struct FreeChainEngineFunctor {
   }
 };
 
+struct FreeCertContextFunctor {
+  void operator()(PCCERT_CONTEXT context) const {
+    if (context)
+      CertFreeCertificateContext(context);
+  }
+};
+
+struct FreeCertChainContextFunctor {
+  void operator()(PCCERT_CHAIN_CONTEXT chain_context) const {
+    if (chain_context)
+      CertFreeCertificateChain(chain_context);
+  }
+};
+
 typedef crypto::ScopedCAPIHandle<HCERTCHAINENGINE, FreeChainEngineFunctor>
     ScopedHCERTCHAINENGINE;
 
+typedef crypto::ScopedCAPIHandle<
+    HCERTSTORE,
+    crypto::CAPIDestroyerWithFlags<HCERTSTORE,
+                                   CertCloseStore, 0> > ScopedHCERTSTORE;
+
+typedef scoped_ptr_malloc<const CERT_CONTEXT,
+                          FreeCertContextFunctor> ScopedPCCERT_CONTEXT;
+
+typedef scoped_ptr_malloc<const CERT_CHAIN_CONTEXT,
+                          FreeCertChainContextFunctor>
+    ScopedPCCERT_CHAIN_CONTEXT;
+
 //-----------------------------------------------------------------------------
 
 // TODO(wtc): This is a copy of the MapSecurityError function in
@@ -695,6 +716,40 @@ HCERTSTORE X509Certificate::cert_store() {
   return g_cert_store.Get().cert_store();
 }
 
+PCCERT_CONTEXT X509Certificate::CreateOSCertChainForCert() const {
+  // Create an in-memory certificate store to hold this certificate and
+  // any intermediate certificates in |intermediate_ca_certs_|. The store
+  // will be referenced in the returned PCCERT_CONTEXT, and will not be freed
+  // until the PCCERT_CONTEXT is freed.
+  ScopedHCERTSTORE store(CertOpenStore(
+      CERT_STORE_PROV_MEMORY, 0, NULL,
+      CERT_STORE_DEFER_CLOSE_UNTIL_LAST_FREE_FLAG, NULL));
+  if (!store.get())
+    return NULL;
+
+  // NOTE: This preserves all of the properties of |os_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.
+  PCCERT_CONTEXT primary_cert;
+  BOOL ok = CertAddCertificateContextToStore(store.get(), os_cert_handle(),
+                                             CERT_STORE_ADD_ALWAYS,
+                                             &primary_cert);
+  if (!ok || !primary_cert)
+    return NULL;
+
+  for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i) {
+    CertAddCertificateContextToStore(store.get(), intermediate_ca_certs_[i],
+                                     CERT_STORE_ADD_ALWAYS, NULL);
+  }
+
+  // Note: |store| is explicitly not released, as the call to CertCloseStore()
+  // when |store| goes out of scope will not actually free the store. Instead,
+  // the store will be freed when |primary_cert| is freed.
+  return primary_cert;
+}
+
 int X509Certificate::VerifyInternal(const std::string& hostname,
                                     int flags,
                                     CRLSet* crl_set,
@@ -762,21 +817,23 @@ int X509Certificate::VerifyInternal(const std::string& hostname,
   if (TestRootCerts::HasInstance())
     chain_engine.reset(TestRootCerts::GetInstance()->GetChainEngine());
 
+  ScopedPCCERT_CONTEXT cert_list(CreateOSCertChainForCert());
   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.get(),
            NULL,  // current system time
-           cert_handle_->hCertStore,
+           cert_list->hCertStore,
            &chain_para,
            chain_flags,
            NULL,  // reserved
            &chain_context)) {
     return MapSecurityError(GetLastError());
   }
+
   if (chain_context->TrustStatus.dwErrorStatus &
       CERT_TRUST_IS_NOT_VALID_FOR_USAGE) {
     ev_policy_oid = NULL;
@@ -785,9 +842,9 @@ int X509Certificate::VerifyInternal(const std::string& hostname,
     CertFreeCertificateChain(chain_context);
     if (!CertGetCertificateChain(
              chain_engine,
-             cert_handle_,
+             cert_list.get(),
              NULL,  // current system time
-             cert_handle_->hCertStore,
+             cert_list->hCertStore,
              &chain_para,
              chain_flags,
              NULL,  // reserved
@@ -795,7 +852,8 @@ int X509Certificate::VerifyInternal(const std::string& hostname,
       return MapSecurityError(GetLastError());
     }
   }
-  ScopedCertChainContext scoped_chain_context(chain_context);
+
+  ScopedPCCERT_CHAIN_CONTEXT scoped_chain_context(chain_context);
 
   GetCertChainInfo(chain_context, verify_result);
   verify_result->cert_status |= MapCertChainErrorStatusToCertStatus(
diff --git a/net/net.gyp b/net/net.gyp
index a6fd6ed..98446ce 100644
--- a/net/net.gyp
+++ b/net/net.gyp
@@ -195,7 +195,6 @@
         'base/rand_callback.h',
         'base/registry_controlled_domain.cc',
         'base/registry_controlled_domain.h',
-        'base/scoped_cert_chain_context.h',
         'base/sdch_filter.cc',
         'base/sdch_filter.h',
         'base/sdch_manager.cc',