v4 backport: crypto: fix handling of root_cert_store.

src/node_crypto.cc

@@ -123,6 +123,7 @@ const char* const root_certs[] = {
 std::string extra_root_certs_file;  // NOLINT(runtime/string)
 
 X509_STORE* root_cert_store;
+std::vector<X509*>* root_certs_vector;
 
 // Just to generate static methods
 template class SSLWrap<TLSWrap>;
@@ -404,8 +405,6 @@ void SecureContext::Init(const FunctionCallbackInfo<Value>& args) {
                                  SSL_SESS_CACHE_NO_AUTO_CLEAR);
   SSL_CTX_sess_set_get_cb(sc->ctx_, SSLWrap<Connection>::GetSessionCallback);
   SSL_CTX_sess_set_new_cb(sc->ctx_, SSLWrap<Connection>::NewSessionCallback);
-
-  sc->ca_store_ = nullptr;
 }
 
 
@@ -689,8 +688,49 @@ void SecureContext::SetCert(const FunctionCallbackInfo<Value>& args) {
 }
 
 
+#if OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(OPENSSL_IS_BORINGSSL)
+// This section contains OpenSSL 1.1.0 functions reimplemented for OpenSSL
+// 1.0.2 so that the following code can be written without lots of #if lines.
+
+static int X509_STORE_up_ref(X509_STORE* store) {
+  CRYPTO_add(&store->references, 1, CRYPTO_LOCK_X509_STORE);
+  return 1;
+}
+
+static int X509_up_ref(X509* cert) {
+  CRYPTO_add(&cert->references, 1, CRYPTO_LOCK_X509);
+  return 1;
+}
+#endif  // OPENSSL_VERSION_NUMBER < 0x10100000L && !OPENSSL_IS_BORINGSSL
+
+
+static X509_STORE* NewRootCertStore() {
+  if (!root_certs_vector) {
+    root_certs_vector = new std::vector<X509*>;
+
+    for (size_t i = 0; i < arraysize(root_certs); i++) {
+      BIO* bp = NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i]));
+      X509 *x509 = PEM_read_bio_X509(bp, nullptr, CryptoPemCallback, nullptr);
+      BIO_free(bp);
+
+      // Parse errors from the built-in roots are fatal.
+      CHECK_NE(x509, nullptr);
+
+      root_certs_vector->push_back(x509);
+    }
+  }
+
+  X509_STORE* store = X509_STORE_new();
+  for (auto& cert : *root_certs_vector) {
+    X509_up_ref(cert);
+    X509_STORE_add_cert(store, cert);
+  }
+
+  return store;
+}
+
+
 void SecureContext::AddCACert(const FunctionCallbackInfo<Value>& args) {
-  bool newCAStore = false;
   Environment* env = Environment::GetCurrent(args);
 
   SecureContext* sc;
@@ -702,23 +742,24 @@ void SecureContext::AddCACert(const FunctionCallbackInfo<Value>& args) {
     return env->ThrowTypeError("Bad parameter");
   }
 
-  if (!sc->ca_store_) {
-    sc->ca_store_ = X509_STORE_new();
-    newCAStore = true;
-  }
-
-  X509* x509 = LoadX509(env, args[0]);
-  if (!x509)
+  BIO* bio = LoadBIO(env, args[0]);
+  if (!bio) {
     return;
+  }
 
-  X509_STORE_add_cert(sc->ca_store_, x509);
-  SSL_CTX_add_client_CA(sc->ctx_, x509);
-
-  X509_free(x509);
-
-  if (newCAStore) {
-    SSL_CTX_set_cert_store(sc->ctx_, sc->ca_store_);
+  X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_);
+  while (X509* x509 =
+             PEM_read_bio_X509(bio, nullptr, CryptoPemCallback, nullptr)) {
+    if (cert_store == root_cert_store) {
+      cert_store = NewRootCertStore();
+      SSL_CTX_set_cert_store(sc->ctx_, cert_store);
+    }
+    X509_STORE_add_cert(cert_store, x509);
+    SSL_CTX_add_client_CA(sc->ctx_, x509);
+    X509_free(x509);
   }
+
+  BIO_free_all(bio);
 }
 
 
@@ -739,19 +780,26 @@ void SecureContext::AddCRL(const FunctionCallbackInfo<Value>& args) {
   if (!bio)
     return;
 
-  X509_CRL *x509 =
+  X509_CRL* crl =
       PEM_read_bio_X509_CRL(bio, nullptr, CryptoPemCallback, nullptr);
 
-  if (x509 == nullptr) {
+  if (crl == nullptr) {
     BIO_free_all(bio);
-    return;
+    return env->ThrowError("Failed to parse CRL");
   }
 
-  X509_STORE_add_crl(sc->ca_store_, x509);
-  X509_STORE_set_flags(sc->ca_store_, X509_V_FLAG_CRL_CHECK |
-                                      X509_V_FLAG_CRL_CHECK_ALL);
+  X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_);
+  if (cert_store == root_cert_store) {
+    cert_store = NewRootCertStore();
+    SSL_CTX_set_cert_store(sc->ctx_, cert_store);
+  }
+
+  X509_STORE_add_crl(cert_store, crl);
+  X509_STORE_set_flags(cert_store,
+                       X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL);
+
   BIO_free_all(bio);
-  X509_CRL_free(x509);
+  X509_CRL_free(crl);
 }
 
 
@@ -794,28 +842,8 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo<Value>& args) {
   ClearErrorOnReturn clear_error_on_return;
   (void) &clear_error_on_return;  // Silence compiler warning.
 
-  CHECK_EQ(sc->ca_store_, nullptr);
-
   if (!root_cert_store) {
-    root_cert_store = X509_STORE_new();
-
-    for (size_t i = 0; i < arraysize(root_certs); i++) {
-      BIO* bp = NodeBIO::NewFixed(root_certs[i], strlen(root_certs[i]));
-      if (bp == nullptr) {
-        return;
-      }
-
-      X509 *x509 = PEM_read_bio_X509(bp, nullptr, CryptoPemCallback, nullptr);
-      if (x509 == nullptr) {
-        BIO_free_all(bp);
-        return;
-      }
-
-      X509_STORE_add_cert(root_cert_store, x509);
-
-      BIO_free_all(bp);
-      X509_free(x509);
-    }
+    root_cert_store = NewRootCertStore();
 
     if (!extra_root_certs_file.empty()) {
       unsigned long err = AddCertsFromFile(  // NOLINT(runtime/int)
@@ -830,10 +858,9 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo<Value>& args) {
     }
   }
 
-  sc->ca_store_ = root_cert_store;
   // Increment reference count so global store is not deleted along with CTX.
-  CRYPTO_add(&root_cert_store->references, 1, CRYPTO_LOCK_X509_STORE);
-  SSL_CTX_set_cert_store(sc->ctx_, sc->ca_store_);
+  X509_STORE_up_ref(root_cert_store);
+  SSL_CTX_set_cert_store(sc->ctx_, root_cert_store);
 }
 
 
@@ -1034,6 +1061,8 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
     sc->cert_ = nullptr;
   }
 
+  X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_);
+
   if (d2i_PKCS12_bio(in, &p12) &&
       PKCS12_parse(p12, pass, &pkey, &cert, &extra_certs) &&
       SSL_CTX_use_certificate_chain(sc->ctx_,
@@ -1046,11 +1075,11 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
     for (int i = 0; i < sk_X509_num(extra_certs); i++) {
       X509* ca = sk_X509_value(extra_certs, i);
 
-      if (!sc->ca_store_) {
-        sc->ca_store_ = X509_STORE_new();
-        SSL_CTX_set_cert_store(sc->ctx_, sc->ca_store_);
+      if (cert_store == root_cert_store) {
+        cert_store = NewRootCertStore();
+        SSL_CTX_set_cert_store(sc->ctx_, cert_store);
       }
-      X509_STORE_add_cert(sc->ca_store_, ca);
+      X509_STORE_add_cert(cert_store, ca);
       SSL_CTX_add_client_CA(sc->ctx_, ca);
     }
     ret = true;

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;
   }
 };
 

test/parallel/test-crypto.js

@@ -140,3 +140,7 @@ assert.throws(function() {
 
 // Make sure memory isn't released before being returned
 console.log(crypto.randomBytes(16));
+
+assert.throws(function() {
+  tls.createSecureContext({ crl: 'not a CRL' });
+}, /Failed to parse CRL/);

test/parallel/test-tls-addca.js

@@ -0,0 +1,62 @@
+'use strict';
+const common = require('../common');
+const fs = require('fs');
+
+if (!common.hasCrypto) {
+  common.skip('missing crypto');
+  return;
+}
+const tls = require('tls');
+
+function filenamePEM(n) {
+  return require('path').join(common.fixturesDir, 'keys', n + '.pem');
+}
+
+function loadPEM(n) {
+  return fs.readFileSync(filenamePEM(n));
+}
+
+const caCert = loadPEM('ca1-cert');
+const contextWithoutCert = tls.createSecureContext({});
+const contextWithCert = tls.createSecureContext({});
+// Adding a CA certificate to contextWithCert should not also add it to
+// contextWithoutCert. This is tested by trying to connect to a server that
+// depends on that CA using contextWithoutCert.
+contextWithCert.context.addCACert(caCert);
+
+const serverOptions = {
+  key: loadPEM('agent1-key'),
+  cert: loadPEM('agent1-cert'),
+};
+const server = tls.createServer(serverOptions, function() {});
+
+const clientOptions = {
+  port: undefined,
+  ca: [caCert],
+  servername: 'agent1',
+  rejectUnauthorized: true,
+};
+
+function startTest() {
+  // This client should fail to connect because it doesn't trust the CA
+  // certificate.
+  clientOptions.secureContext = contextWithoutCert;
+  clientOptions.port = server.address().port;
+  const client = tls.connect(clientOptions, common.fail);
+  client.on('error', common.mustCall(() => {
+    client.destroy();
+
+    // This time it should connect because contextWithCert includes the needed
+    // CA certificate.
+    clientOptions.secureContext = contextWithCert;
+    const client2 = tls.connect(clientOptions, common.mustCall(() => {
+      client2.destroy();
+      server.close();
+    }));
+    client2.on('error', (e) => {
+      console.log(e);
+    });
+  }));
+}
+
+server.listen(0, startTest);