From 8fce134634c8ee3346ff6d1ae380e570f832cf83 Mon Sep 17 00:00:00 2001
From: Harshil Jain <twitharshil@gmail.com>
Date: Wed, 19 Oct 2022 23:10:03 +0530
Subject: [PATCH] crypto: addressed PR comments

---
 doc/api/cli.md               |  4 +-
 src/crypto/crypto_context.cc | 73 ++++++++++++++++++------------------
 src/crypto/crypto_context.h  |  8 ++++
 3 files changed, 46 insertions(+), 39 deletions(-)

diff --git a/doc/api/cli.md b/doc/api/cli.md
index d90abc4e5ad47d..39c4e5e855b8bd 100644
--- a/doc/api/cli.md
+++ b/doc/api/cli.md
@@ -1540,8 +1540,8 @@ See `SSL_CERT_DIR` and `SSL_CERT_FILE`.
 Node.js uses the trusted CA certificates present in the system store along with
 the `--use-bundled-ca`, `--use-openssl-ca` options.
 
-Note, Only current user certificates are accessible using this method, not the
-local machine store. This option is available to Windows only.
+Only current user certificates are accessible using this method, not the local
+machine store. This option is available to Windows only.
 
 ### `--use-largepages=mode`
 
diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc
index 5d428fc35fe49f..afcf0299574c88 100644
--- a/src/crypto/crypto_context.cc
+++ b/src/crypto/crypto_context.cc
@@ -200,60 +200,59 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
 void ReadSystemStoreCertificates(
     std::vector<std::string>* system_root_certificates) {
 #ifdef _WIN32
-  const HCERTSTORE hStore = CertOpenSystemStoreW(0, L"ROOT");
-  CHECK_NE(hStore, nullptr);
+  CertStorePointer system_store;
 
-  auto cleanup =
-      OnScopeLeave([hStore]() { CHECK_EQ(CertCloseStore(hStore, 0), TRUE); });
+  PCCERT_CONTEXT certificate_context_ptr = nullptr;
 
-  PCCERT_CONTEXT pCtx = nullptr;
+  std::vector<X509*> system_root_certificates_X509;
 
-  while ((pCtx = CertEnumCertificatesInStore(hStore, pCtx)) != nullptr) {
-    const DWORD cbSize = CertGetNameStringW(
-        pCtx, CERT_NAME_SIMPLE_DISPLAY_TYPE, 0, nullptr, nullptr, 0);
+  while ((certificate_context_ptr = CertEnumCertificatesInStore(
+              system_store.ref_, certificate_context_ptr)) != nullptr) {
+    const DWORD certificate_buffer_size =
+        CertGetNameStringW(certificate_context_ptr,
+                           CERT_NAME_SIMPLE_DISPLAY_TYPE,
+                           0,
+                           nullptr,
+                           nullptr,
+                           0);
 
-    CHECK_GT(cbSize, 0);
+    CHECK_GT(certificate_buffer_size, 0);
 
-    std::vector<wchar_t> pszName(cbSize);
+    std::vector<wchar_t> certificate_name(certificate_buffer_size);
 
-    CHECK_GT(CertGetNameStringW(pCtx,
+    CHECK_GT(CertGetNameStringW(certificate_context_ptr,
                                 CERT_NAME_SIMPLE_DISPLAY_TYPE,
                                 0,
                                 nullptr,
-                                pszName.data(),
-                                cbSize),
+                                certificate_name.data(),
+                                certificate_buffer_size),
              0);
+    const unsigned char* certificate_src_ptr =
+        reinterpret_cast<const unsigned char*>(
+            certificate_context_ptr->pbCertEncoded);
+    const size_t certificate_src_length =
+        certificate_context_ptr->cbCertEncoded;
 
-    const char* certificate_src_ptr =
-        reinterpret_cast<const char*>(pCtx->pbCertEncoded);
-    const size_t slen = pCtx->cbCertEncoded;
-    const size_t dlen = base64_encoded_size(slen);
+    X509* cert =
+        d2i_X509(nullptr, &certificate_src_ptr, certificate_src_length);
 
-    char* certificate_dst_ptr = UncheckedMalloc(dlen);
-
-    CHECK_NOT_NULL(certificate_dst_ptr);
-
-    auto cleanup =
-        OnScopeLeave([certificate_dst_ptr]() { free(certificate_dst_ptr); });
-
-    const size_t written =
-        base64_encode(certificate_src_ptr, slen, certificate_dst_ptr, dlen);
-    CHECK_EQ(written, dlen);
+    system_root_certificates_X509.emplace_back(cert);
+  }
 
-    std::string base64_string_output(certificate_dst_ptr, dlen);
+  for (size_t i = 0; i < system_root_certificates_X509.size(); i++) {
+    int result = 0;
 
-    constexpr size_t distance = 72;
-    size_t pos = distance;
+    BIOPointer bio(BIO_new(BIO_s_mem()));
+    CHECK(bio);
 
-    while (pos < base64_string_output.size()) {
-      base64_string_output.insert(pos, "\n");
-      pos += distance + 1;
-    }
+    BUF_MEM* mem = nullptr;
+    result = PEM_write_bio_X509(bio.get(), system_root_certificates_X509[i]);
 
-    base64_string_output = "-----BEGIN CERTIFICATE-----\n" +
-                           base64_string_output + "\n-----END CERTIFICATE-----";
+    BIO_get_mem_ptr(bio.get(), &mem);
+    std::string certificate_string_pem(mem->data, mem->length);
+    system_root_certificates->emplace_back(certificate_string_pem);
 
-    system_root_certificates->emplace_back(std::move(base64_string_output));
+    bio.reset();
   }
 #endif
 }
diff --git a/src/crypto/crypto_context.h b/src/crypto/crypto_context.h
index 4dfd0dfa032cf7..f7d34346419607 100644
--- a/src/crypto/crypto_context.h
+++ b/src/crypto/crypto_context.h
@@ -15,6 +15,14 @@ namespace crypto {
 // Node.js doesn't, so pin the max to what we do support.
 constexpr int kMaxSupportedVersion = TLS1_3_VERSION;
 
+#if _WIN32
+struct CertStorePointer {
+  const HCERTSTORE ref_;
+  CertStorePointer() : ref_(CertOpenSystemStoreW(0, L"ROOT")) {}
+  ~CertStorePointer() { CHECK_EQ(CertCloseStore(ref_, 0), TRUE); }
+};
+#endif
+
 void GetRootCertificates(
     const v8::FunctionCallbackInfo<v8::Value>& args);