Implement LoadTemporaryRoot for Windows

net/base/test_root_certs_win.cc

+// Copyright (c) 2010 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/test_root_certs.h"
+
+#include <windows.h>
+#include <wincrypt.h>
+
+#include "base/basictypes.h"
+#include "base/lazy_instance.h"
+#include "base/logging.h"
+#include "net/base/x509_certificate.h"
+
+namespace net {
+
+namespace {
+
+BOOL WINAPI InterceptedOpenStoreW(LPCSTR store_provider,

[joth, 2010/11/22 13:05:40]

Maybe comment
// Provides a custom implementation that will be called in place of
CertOpenStore

[wtc, 2010/11/23 00:30:11]

Please add a comment to note that this is a
CertDllOpenStoreProv callback function.  Optionally, link
to http://msdn.microsoft.com/en-us/library/aa376043%28v=VS.85%29.aspx

+                                  DWORD encoding,
+                                  HCRYPTPROV crypt_provider,
+                                  DWORD flags,
+                                  const void* extra,
+                                  HCERTSTORE memory_store,
+                                  PCERT_STORE_PROV_INFO store_info);
+
+// CryptoAPIInjector is used to inject a store provider function for system
+// certificate stores before the one provided internally by Crypt32.dll.
+// Once injected, there is no way to remove, so every call to open a system
+// store will be redirected to the injected function.
+class CryptoAPIInjector {
+ public:
+  // The previous default function for opening system stores. For most
+  // configurations, this should point to Crypt32's internal
+  // I_CertDllOpenSystemStoreProvW function.
+  PFN_CERT_DLL_OPEN_STORE_PROV_FUNC original_function;

[joth, 2010/11/22 13:05:40]

make this private (with trailing _ ) and provide a getter:
PFN_CERT_DLL_OPEN_STORE_PROV_FUNC original_function() { return
original_function_; }

[Ryan Sleevi, 2010/12/03 03:28:06]

Since this class is entirely internal to this implementation, I'm not sure I
understand the reason for the visibility guard. I've made this class a struct,
which seemed to be the preferred/acceptable form in the Style Guide, and exposed
both members as public.

[joth, 2010/12/03 10:40:59]

I was mostly just quoting the rule that on classes, all data members must be
private -- regardless of the class definition only being visible to this .cc.
Fully public structs are fine for passive data-carrying objects (I tend to think
of this in terms of "value" vs "identity" classes). What muddies the water here
is the boiler plating to make it lazy instance, as it enforces identity rather
than value semantics. (that is, no default/copy c'tor and singleton-like
properties imposed).
Also the name xxx "Injector" doesn't really suggest it is a passive object.

In line with the view that it's private to the .cc and access control isn't
important, I'd suggest go the full way and remove the private label and friend
declarations too, leaving it a full public struct.
The alternative is to extract the constructor logic from here into a custom
LeakyLazyInstanceTraits::New overload, which really would leave this struct as
pure passive data-carrier, but does seem to be getting excessive just to avoid
defining one public getter!

+
+ private:
+  friend struct base::DefaultLazyInstanceTraits<CryptoAPIInjector>;
+
+  CryptoAPIInjector()
+      : original_function(NULL),
+        original_handle_(NULL) {
+    HCRYPTOIDFUNCSET registered_functions =
+        CryptInitOIDFunctionSet(CRYPT_OID_OPEN_STORE_PROV_FUNC, 0);
+
+    // Preserve the original handler function in |original_function|. If other
+    // functions are overridden, they will also need to be preserved.
+    BOOL ok = CryptGetOIDFunctionAddress(
+        registered_functions, 0, CERT_STORE_PROV_SYSTEM_W, 0,
+        reinterpret_cast<void**>(&original_function), &original_handle_);
+    DCHECK(ok);
+
+    // For now, intercept only the numeric form of the system store
+    // function, CERT_STORE_PROV_SYSTEM_W (0x0A), which is what Crypt32
+    // functionality uses exclusively. Depending on the machine that tests
+    // are being run on, it may prove necessary to also intercept
+    // sz_CERT_STORE_PROV_SYSTEM_[A/W] and CERT_STORE_PROV_SYSTEM_A, based
+    // on whether or not any third-party CryptoAPI modules have been
+    // installed.
+    const CRYPT_OID_FUNC_ENTRY kFunctionToIntercept =
+        { CERT_STORE_PROV_SYSTEM_W, &InterceptedOpenStoreW };
+
+    // Inject kFunctionToIntercept at the front of the linked list that
+    // crypt32 uses when CertOpenStore is called, replacing the existing
+    // registered function.
+    ok = CryptInstallOIDFunctionAddress(NULL, 0,
+                                        CRYPT_OID_OPEN_STORE_PROV_FUNC, 1,
+                                        &kFunctionToIntercept,
+                                        CRYPT_INSTALL_OID_FUNC_BEFORE_FLAG);
+    DCHECK(ok);
+  }
+
+  // Never called, because this object is intentionally leaked. Certificate
+  // verification happens on a non-joinable worker thread which may still be
+  // running when ~AtExitManager is called.
+  ~CryptoAPIInjector() {
+    original_function = NULL;
+    CryptFreeOIDFunctionAddress(original_handle_, NULL);
+  }
+
+  HCRYPTOIDFUNCADDR original_handle_;
+};
+
+base::LazyInstance<CryptoAPIInjector,
+                   base::LeakyLazyInstanceTraits<CryptoAPIInjector> >
+    g_capi_injector(base::LINKER_INITIALIZED);
+
+BOOL WINAPI InterceptedOpenStoreW(LPCSTR store_provider,
+                                  DWORD encoding,
+                                  HCRYPTPROV crypt_provider,
+                                  DWORD flags,
+                                  const void* store_name,
+                                  HCERTSTORE memory_store,
+                                  PCERT_STORE_PROV_INFO store_info) {
+  // If the high word is all zeroes, then |store_provider| is a numeric ID.
+  // Otherwise, it's a pointer to a null-terminated ASCII string. See the
+  // documentation for CryptGetOIDFunctionAddress for more information.
+  uint32 store_as_uint = reinterpret_cast<uint32>(store_provider);
+  if (store_as_uint > 0xFFFF || store_provider != CERT_STORE_PROV_SYSTEM_W ||
+      !g_capi_injector.Get().original_function)
+    return FALSE;

[wtc, 2010/11/23 00:30:11]

Do we need to call SetLastError() to set an error code?

[Ryan Sleevi, 2010/12/03 03:28:06]

Nothing I've seen in the documentation (MSDN, Header, SDK samples) has stated
it's required. CryptoAPI will propagate the error back to the caller of
CertOpenStore, but as far as I can tell, the error code is not used internally
by CryptoAPI.

+
+  BOOL ok = g_capi_injector.Get().original_function(store_provider, encoding,
+                                                    crypt_provider, flags,
+                                                    store_name, memory_store,
+                                                    store_info);
+  // Only inject certificates for the Root store. If

[joth, 2010/11/22 13:05:40]

Not quite a complete sentence. Maybe:
Conditionally inject certificates for the Root store.

+  // CERT_SYSTEM_STORE_RELOCATE_FLAG is set, then |store_name| points to a
+  // CERT_SYSTEM_STORE_RELOCATE_PARA structure, rather than a
+  // NULL-terminated wide string, so check before making a string
+  // comparison.
+  if (!ok || TestRootCerts::GetInstance()->IsEmpty() ||
+      (flags & CERT_SYSTEM_STORE_RELOCATE_FLAG) ||
+      lstrcmpiW(reinterpret_cast<LPCWSTR>(store_name), L"root"))
+    return ok;
+
+  // The result of CertOpenStore with CERT_STORE_PROV_SYSTEM_W is documented
+  // to be a collection store, and that appears to hold for |memory_store|.
+  // Attempting to add an individual certificate to |memory_store| causes
+  // the request to be forwarded to the first physical store in the
+  // collection that accepts modifications, which will cause a secure
+  // confirmation dialog to be displayed, confirming the user wishes to
+  // trust the certificate. In order to bypass this prompt, the temporary
+  // root store is inserted to the front of the collection, which allows the

[wtc, 2010/11/23 00:30:11]

COMMENT BUG: you pass 0 as the dwPriority argument to
CertAddStoreToCollection, and MSDN says 0 means
TestRootCerts::GetInstance()->temporary_roots() is appended
as the last store in the collection.  This contradicts what
your comment says here.

[Ryan Sleevi, 2010/12/03 03:28:06]

Thanks for spotting this. You're right that it was only a comment bug - the
exact position of temporary_roots() does not matter for this method to work. It
would only matter if we wanted to add certificates/CRLs to the root store
without the dialog, in which case the memory store would need to be the highest
priority and CERT_PHYSICAL_STORE_ADD_ENABLE_FLAG would have had to be passed as
the third parameter (instead of zero). I've updated the comment to better
describe the behaviour of "how/why it works", without the buggy portion.

+  // trust settings to be injected, and without the dialog being displayed.
+  return CertAddStoreToCollection(
+      memory_store, TestRootCerts::GetInstance()->temporary_roots(), 0, 0);
+}
+
+}  // namespace
+
+bool TestRootCerts::Add(X509Certificate* certificate) {
+  // Ensure that the default CryptoAPI functionality has been intercepted.
+  // If a test certificate is never added, then no interception should
+  // happen.
+  g_capi_injector.Get();
+
+  BOOL ok = CertAddCertificateContextToStore(
+      temporary_roots_, certificate->os_cert_handle(),
+      CERT_STORE_ADD_NEW, NULL);
+  if (!ok) {
+    // If the certificate is already added, return successfully.
+    return GetLastError() == CRYPT_E_EXISTS;
+  }
+
+  empty_ = false;
+  return true;
+}
+
+void TestRootCerts::Clear() {
+  empty_ = true;
+
+  PCCERT_CONTEXT prev_cert = NULL;
+  while (prev_cert = CertEnumCertificatesInStore(temporary_roots_, NULL))
+    CertDeleteCertificateFromStore(prev_cert);
+}
+
+bool TestRootCerts::IsEmpty() const {
+  return empty_;
+}
+
+HCERTCHAINENGINE TestRootCerts::GetChainEngine() const {
+  if (IsEmpty())
+    return NULL;  // Default chain engine will suffice.
+
+  // Each HCERTCHAINENGINE caches both the configured system stores and
+  // information about each chain that has been built. In order to ensure
+  // that changes to |temporary_roots_| are properly propagated and that the
+  // various caches are flushed, when at least one certificate is added,
+  // return a new chain engine for every call. Each chain engine creation
+  // should re-open the root store, ensuring the most recent changes are
+  // visible.
+  CERT_CHAIN_ENGINE_CONFIG engine_config = {
+    sizeof(engine_config)
+  };
+  engine_config.dwFlags =
+      CERT_CHAIN_ENABLE_CACHE_AUTO_UPDATE |
+      CERT_CHAIN_ENABLE_SHARE_STORE;
+  HCERTCHAINENGINE chain_engine = NULL;
+  BOOL ok = CertCreateCertificateChainEngine(&engine_config, &chain_engine);
+  DCHECK(ok);
+  return chain_engine;
+}
+
+TestRootCerts::TestRootCerts()
+    : empty_(true) {
+  temporary_roots_ = CertOpenStore(
+      CERT_STORE_PROV_MEMORY, 0, NULL,
+      CERT_STORE_DEFER_CLOSE_UNTIL_LAST_FREE_FLAG, NULL);
+  DCHECK(temporary_roots_);
+}
+
+TestRootCerts::~TestRootCerts() {
+  CertCloseStore(temporary_roots_, 0);
+}
+
+}  // namespace net