From 13c8240eb65e5e759aff1bbd69796a6d47279308 Mon Sep 17 00:00:00 2001 From: Adam Langley Date: Sun, 30 Oct 2016 13:22:50 -0700 Subject: [PATCH 1/4] crypto: fix handling of root_cert_store. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-By: Shigeki Ohtsu --- src/node_crypto.cc | 135 ++++++++++++++++++++------------ src/node_crypto.h | 27 +++---- test/parallel/test-crypto.js | 4 + test/parallel/test-tls-addca.js | 62 +++++++++++++++ 4 files changed, 162 insertions(+), 66 deletions(-) create mode 100644 test/parallel/test-tls-addca.js diff --git a/src/node_crypto.cc b/src/node_crypto.cc index e58b0bc25bb65b..7e351d6b75499c 100644 --- a/src/node_crypto.cc +++ b/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* root_certs_vector; // Just to generate static methods template class SSLWrap; @@ -404,8 +405,6 @@ void SecureContext::Init(const FunctionCallbackInfo& args) { SSL_SESS_CACHE_NO_AUTO_CLEAR); SSL_CTX_sess_set_get_cb(sc->ctx_, SSLWrap::GetSessionCallback); SSL_CTX_sess_set_new_cb(sc->ctx_, SSLWrap::NewSessionCallback); - - sc->ca_store_ = nullptr; } @@ -689,8 +688,52 @@ void SecureContext::SetCert(const FunctionCallbackInfo& 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; + + 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); + + if (x509 == nullptr) { + // Parse errors from the built-in roots are fatal. + abort(); + return 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& args) { - bool newCAStore = false; Environment* env = Environment::GetCurrent(args); SecureContext* sc; @@ -702,23 +745,24 @@ void SecureContext::AddCACert(const FunctionCallbackInfo& 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 +783,27 @@ void SecureContext::AddCRL(const FunctionCallbackInfo& 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) { + return env->ThrowError("Failed to parse CRL"); BIO_free_all(bio); return; } - 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 +846,8 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo& 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 +862,9 @@ void SecureContext::AddRootCerts(const FunctionCallbackInfo& 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 +1065,8 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo& 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 +1079,11 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo& 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; diff --git a/src/node_crypto.h b/src/node_crypto.h index 9a4d936f305ee1..f09ce3e7cab520 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 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 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; } }; diff --git a/test/parallel/test-crypto.js b/test/parallel/test-crypto.js index a325ee131cbb2e..0cbb46ea8f2a63 100644 --- a/test/parallel/test-crypto.js +++ b/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/); diff --git a/test/parallel/test-tls-addca.js b/test/parallel/test-tls-addca.js new file mode 100644 index 00000000000000..0e9571efdf0abf --- /dev/null +++ b/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); From 6d0ce653f2abec5270fae6f80d4a00ab7fb0637e Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 14 Nov 2016 13:39:40 +0100 Subject: [PATCH 2/4] src: fix memory leak introduced in 34febfbf4 Fix leaking the BIO in the error path. Introduced in commit 34febfbf4 ("crypto: fix handling of root_cert_store"). PR-URL: https://github.com/nodejs/node/pull/9604 Refs: https://github.com/nodejs/node/pull/9409 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: Fedor Indutny Reviewed-By: James M Snell Reviewed-By: Jeremiah Senkpiel Reviewed-By: Michael Dawson Reviewed-By: Rod Vagg --- src/node_crypto.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 7e351d6b75499c..c5f5871617c222 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -787,9 +787,8 @@ void SecureContext::AddCRL(const FunctionCallbackInfo& args) { PEM_read_bio_X509_CRL(bio, nullptr, CryptoPemCallback, nullptr); if (crl == nullptr) { - return env->ThrowError("Failed to parse CRL"); BIO_free_all(bio); - return; + return env->ThrowError("Failed to parse CRL"); } X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_); From 129be41a195a218f9fac32d6694f4ec53ad720d3 Mon Sep 17 00:00:00 2001 From: Evan Lucas Date: Tue, 15 Nov 2016 05:29:38 -0600 Subject: [PATCH 3/4] src: use ABORT() macro instead of abort() This makes sure that we dump a backtrace and use raise(SIGABRT) on Windows. PR-URL: https://github.com/nodejs/node/pull/9613 Reviewed-By: Ben Noordhuis Reviewed-By: Colin Ihrig --- src/node_crypto.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index c5f5871617c222..2573035d9efb8f 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -715,7 +715,7 @@ static X509_STORE* NewRootCertStore() { if (x509 == nullptr) { // Parse errors from the built-in roots are fatal. - abort(); + ABORT(); return nullptr; } From f92302842ee4adf9cf35456ea4f3c20cf440d77a Mon Sep 17 00:00:00 2001 From: Sam Roberts Date: Thu, 22 Dec 2016 08:50:33 -0800 Subject: [PATCH 4/4] crypto: use CHECK_NE instead of ABORT or abort Use of abort() was added in 34febfbf4, and changed to ABORT() in 21826ef21ad, but conditional+ABORT() is better expressesed using a CHECK_xxx() macro. See: https://github.com/nodejs/node/pull/9409#discussion_r93575328 PR-URL: https://github.com/nodejs/node/pull/10413 Reviewed-By: Colin Ihrig Reviewed-By: Gibson Fahnestock Reviewed-By: Ben Noordhuis Reviewed-By: Anna Henningsen Reviewed-By: James M Snell --- src/node_crypto.cc | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 2573035d9efb8f..b06c1d698be8c1 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -713,11 +713,8 @@ static X509_STORE* NewRootCertStore() { X509 *x509 = PEM_read_bio_X509(bp, nullptr, CryptoPemCallback, nullptr); BIO_free(bp); - if (x509 == nullptr) { - // Parse errors from the built-in roots are fatal. - ABORT(); - return nullptr; - } + // Parse errors from the built-in roots are fatal. + CHECK_NE(x509, nullptr); root_certs_vector->push_back(x509); }