crypto: keep the root cert store per Environment

The root cert store and the certificates set through
tls.setDefaultCACertificates() were thread_local, with a cleanup hook on
whichever Environment used TLS first. When several Environments share a
thread, setting the default CA certificates in one of them replaced the
trusted CAs of the others. Keep both on the Environment, next to its
other OpenSSL state.

Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
PR-URL: #66411
Refs: #66239
Reviewed-By: Anna Henningsen <anna@addaleax.net>

Author: codebytere

src/crypto/crypto_context.cc

@@ -95,37 +95,27 @@ struct X509Less {
 };
 using X509Set = std::set<ncrypto::X509Pointer, X509Less>;
 
-// Per-thread root cert store. See NewRootCertStore() on what it contains.
-static thread_local DeleteFnPtr<X509_STORE, X509_STORE_free> root_cert_store;
-// If the user calls tls.setDefaultCACertificates() this will be used
-// to hold the user-provided certificates, the root_cert_store and any new
-// copy generated by NewRootCertStore() will then contain the certificates
-// from this set.
-static thread_local std::unique_ptr<X509Set> root_certs_from_users;
-static thread_local bool has_cleanup_hook = false;
-
-static void CleanupRootCertStore(void*) {
-  root_cert_store.reset();
-  root_certs_from_users.reset();
-  has_cleanup_hook = false;
-}
-
-static void EnsureRootCertStoreCleanupHook(Environment* env) {
-  if (env == nullptr || has_cleanup_hook) {
-    return;
-  }
+struct RootCertStore {
+  // See NewRootCertStore() on what it contains.
+  DeleteFnPtr<X509_STORE, X509_STORE_free> store;
+  // Set by tls.setDefaultCACertificates(). Once set, NewRootCertStore()
+  // copies these certificates instead of loading the defaults.
+  std::unique_ptr<X509Set> certs_from_users;
+};
 
-  env->AddCleanupHook(CleanupRootCertStore, nullptr);
-  has_cleanup_hook = true;
+void FreeRootCertStore(RootCertStore* root_certs) {
+  delete root_certs;
+}
+
+static RootCertStore* GetRootCertStore(Environment* env) {
+  if (!env->root_cert_store) env->root_cert_store.reset(new RootCertStore());
+  return env->root_cert_store.get();
 }
 
 X509_STORE* GetOrCreateRootCertStore(Environment* env) {
-  EnsureRootCertStoreCleanupHook(env);
-  if (root_cert_store != nullptr) {
-    return root_cert_store.get();
-  }
-  root_cert_store.reset(NewRootCertStore(env));
-  return root_cert_store.get();
+  RootCertStore* root_certs = GetRootCertStore(env);
+  if (!root_certs->store) root_certs->store.reset(NewRootCertStore(env));
+  return root_certs->store.get();
 }
 
 // Takes a string or buffer and loads it into a BIO.
@@ -1062,8 +1052,10 @@ X509_STORE* NewRootCertStore(Environment* env) {
   // If the root cert store is already reset by users through
   // tls.setDefaultCACertificates(), just create a copy from the
   // user-provided certificates.
-  if (root_certs_from_users != nullptr) {
-    for (const auto& cert : *root_certs_from_users) {
+  const X509Set* certs_from_users =
+      env != nullptr ? GetRootCertStore(env)->certs_from_users.get() : nullptr;
+  if (certs_from_users != nullptr) {
+    for (const auto& cert : *certs_from_users) {
       CHECK_EQ(1, X509_STORE_add_cert(store, cert.get()));
     }
     return store;
@@ -1230,12 +1222,13 @@ MaybeLocal<Array> X509sToArrayOfStrings(Environment* env,
 
 void GetUserRootCertificates(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
-  CHECK_NOT_NULL(root_certs_from_users);
+  const auto& certs_from_users = GetRootCertStore(env)->certs_from_users;
+  CHECK(certs_from_users);
   Local<Array> results;
   if (X509sToArrayOfStrings(env,
-                            root_certs_from_users->begin(),
-                            root_certs_from_users->end(),
-                            root_certs_from_users->size())
+                            certs_from_users->begin(),
+                            certs_from_users->end(),
+                            certs_from_users->size())
           .ToLocal(&results)) {
     args.GetReturnValue().Set(results);
   }
@@ -1246,12 +1239,12 @@ void ResetRootCertStore(const FunctionCallbackInfo<Value>& args) {
   CHECK(args[0]->IsArray());
   Local<Array> cert_array = args[0].As<Array>();
   Environment* env = Environment::GetCurrent(context);
-  EnsureRootCertStoreCleanupHook(env);
+  RootCertStore* root_certs = GetRootCertStore(env);
 
   if (cert_array->Length() == 0) {
     // If the array is empty, just clear the user certs and reset the store.
-    root_cert_store.reset();
-    root_certs_from_users = std::make_unique<X509Set>();
+    root_certs->store.reset();
+    root_certs->certs_from_users = std::make_unique<X509Set>();
     return;
   }
 
@@ -1263,7 +1256,6 @@ void ResetRootCertStore(const FunctionCallbackInfo<Value>& args) {
   }
 
   if (certs->empty()) {
-    Environment* env = Environment::GetCurrent(context);
     return THROW_ERR_CRYPTO_OPERATION_FAILED(
         env, "No valid certificates found in the provided array");
   }
@@ -1275,11 +1267,11 @@ void ResetRootCertStore(const FunctionCallbackInfo<Value>& args) {
     // is not consumed by insert (element already exists).
   }
 
-  root_certs_from_users = std::move(new_set);
+  root_certs->certs_from_users = std::move(new_set);
 
-  // Reset the global root cert store so it will be recreated with the
-  // new certificates.
-  root_cert_store.reset();
+  // Reset the root cert store so it will be recreated with the new
+  // certificates.
+  root_certs->store.reset();
 }
 
 void GetSystemCACertificates(const FunctionCallbackInfo<Value>& args) {

src/env.h

@@ -76,6 +76,13 @@ class MacCache;
 
 namespace node {
 
+#if HAVE_OPENSSL
+namespace crypto {
+struct RootCertStore;
+void FreeRootCertStore(RootCertStore* root_certs);
+}  // namespace crypto
+#endif  // HAVE_OPENSSL
+
 namespace shadow_realm {
 class ShadowRealm;
 }
@@ -1219,6 +1226,7 @@ class Environment final : public MemoryRetainer {
   std::unique_ptr<ncrypto::MacCache> provider_mac_cache;
   std::vector<std::string> supported_mac_algorithms;
   bool supported_mac_algorithms_initialized = false;
+  DeleteFnPtr<crypto::RootCertStore, crypto::FreeRootCertStore> root_cert_store;
 #endif  // HAVE_OPENSSL
 
   v8::Global<v8::Module> temporary_required_module_facade_original;

test/cctest/test_environment_shared_isolate.cc

@@ -6,8 +6,12 @@
 
 #include "cppgc/allocation.h"
 #include "cppgc/garbage-collected.h"
+#include "env-inl.h"
 #include "node_test_fixture.h"
 #include "v8-cppgc.h"
+#if HAVE_OPENSSL
+#include "crypto/crypto_context.h"
+#endif
 
 #include <string>
 #include <vector>
@@ -446,6 +450,36 @@ TEST_P(SharedIsolateTest, FreeIsolateDataBeforeItsEnvironmentAsserts) {
   FreeInstance(std::move(instance));
 }
 
+#if HAVE_OPENSSL
+TEST_P(SharedIsolateTest, RootCertStoreIsPerEnvironment) {
+  const HandleScope handle_scope(isolate_);
+  std::unique_ptr<Instance> first =
+      CreateInstance(0, EnvironmentFlags::kNoCreateInspector);
+  std::unique_ptr<Instance> second =
+      CreateInstance(1, EnvironmentFlags::kNoCreateInspector);
+  auto store_size = [](Instance* instance) {
+    return sk_X509_OBJECT_num(X509_STORE_get0_objects(
+        node::crypto::GetOrCreateRootCertStore(instance->env)));
+  };
+  auto set_default_ca_count = [this](Instance* instance, int count) {
+    std::string source =
+        "const tls = process.getBuiltinModule('tls');"
+        "tls.setDefaultCACertificates(tls.rootCertificates.slice(0, " +
+        std::to_string(count) + "))";
+    Evaluate(instance, source.c_str());
+  };
+
+  set_default_ca_count(first.get(), 1);
+  set_default_ca_count(second.get(), 2);
+  EXPECT_EQ(store_size(first.get()), 1);
+  EXPECT_EQ(store_size(second.get()), 2);
+
+  FreeInstance(std::move(first));
+  EXPECT_EQ(store_size(second.get()), 2);
+  FreeInstance(std::move(second));
+}
+#endif  // HAVE_OPENSSL
+
 INSTANTIATE_TEST_SUITE_P(
     EnvironmentTest,
     SharedIsolateTest,