crypto: fix handling of root_cert_store.

SecureContext::AddRootCerts only parses the root certificates once and
keeps the result in root_cert_store, a global X509_STORE. This change
addresses the following issues:

1. SecureContext::AddCACert would add certificates to whatever
X509_STORE was being used, even if that happened to be root_cert_store.
Thus adding a CA certificate to a SecureContext would also cause it to
be included in unrelated SecureContexts.

2. AddCRL would crash if neither AddRootCerts nor AddCACert had been
called first.

3. Calling AddCACert without calling AddRootCerts first, and with an
input that didn't contain any certificates, would leak an X509_STORE.

4. AddCRL would add the CRL to whatever X509_STORE was being used. Thus,
like AddCACert, unrelated SecureContext objects could be affected.

The following, non-obvious behaviour remains: calling AddRootCerts
doesn't /add/ them, rather it sets the CA certs to be the root set and
overrides any previous CA certificates.

Points 1–3 are probably unimportant because the SecureContext is
typically configured by `createSecureContext` in `lib/_tls_common.js`.
This function either calls AddCACert or AddRootCerts and only calls
AddCRL after setting up CA certificates. Point four could still apply in
the unlikely case that someone configures a CRL without explicitly
configuring the CAs.

PR-URL: https://github.com/nodejs/node/pull/9409
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>

1 files changed, 12 insertions, 15 deletions

diff --git a/src/node_crypto.h b/src/node_crypto.h
index fd3e2ce895..31cbb4e64f 100644
--- a/src/node_crypto.h
+++ b/src/node_crypto.h
@@ -76,7 +76,6 @@ class SecureContext : public BaseObject {
 
   static void Initialize(Environment* env, v8::Local<v8::Object> target);
 
-  X509_STORE* ca_store_;
   SSL_CTX* ctx_;
   X509* cert_;
   X509* issuer_;
@@ -131,7 +130,6 @@ class SecureContext : public BaseObject {
 
   SecureContext(Environment* env, v8::Local<v8::Object> wrap)
       : BaseObject(env, wrap),
-        ca_store_(nullptr),
         ctx_(nullptr),
         cert_(nullptr),
         issuer_(nullptr) {
@@ -140,20 +138,19 @@ class SecureContext : public BaseObject {
   }
 
   void FreeCTXMem() {
-    if (ctx_) {
-      env()->isolate()->AdjustAmountOfExternalAllocatedMemory(-kExternalSize);
-      SSL_CTX_free(ctx_);
-      if (cert_ != nullptr)
-        X509_free(cert_);
-      if (issuer_ != nullptr)
-        X509_free(issuer_);
-      ctx_ = nullptr;
-      ca_store_ = nullptr;
-      cert_ = nullptr;
-      issuer_ = nullptr;
-    } else {
-      CHECK_EQ(ca_store_, nullptr);
+    if (!ctx_) {
+      return;
     }
+
+    env()->isolate()->AdjustAmountOfExternalAllocatedMemory(-kExternalSize);
+    SSL_CTX_free(ctx_);
+    if (cert_ != nullptr)
+      X509_free(cert_);
+    if (issuer_ != nullptr)
+      X509_free(issuer_);
+    ctx_ = nullptr;
+    cert_ = nullptr;
+    issuer_ = nullptr;
   }
 };