From 771716980677f9d93a606311daa358fe8ea12320 Mon Sep 17 00:00:00 2001 From: Eric Bickle Date: Tue, 3 Mar 2020 13:35:28 -0800 Subject: [PATCH 1/4] src: fix missing extra ca in tls.rootCertificates Fixes tls.rootCertificates missing certificates loaded from NODE_EXTRA_CA_CERTS. Fixes: https://github.com/nodejs/node/issues/32074 --- src/node_crypto.cc | 30 ++++++--- src/node_crypto_common.cc | 10 +++ src/node_crypto_common.h | 4 ++ test/parallel/test-tls-root-certificates.js | 69 +++++++++++++-------- 4 files changed, 80 insertions(+), 33 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index fdaf91acdc0a1c..716dafdf946a98 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -988,19 +988,33 @@ static X509_STORE* NewRootCertStore() { void GetRootCertificates(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - Local result[arraysize(root_certs)]; - for (size_t i = 0; i < arraysize(root_certs); i++) { - if (!String::NewFromOneByte( - env->isolate(), - reinterpret_cast(root_certs[i]), - NewStringType::kNormal).ToLocal(&result[i])) { - return; + if (root_cert_store == nullptr) { + root_cert_store = NewRootCertStore(); + } + + stack_st_X509_OBJECT* objs = X509_STORE_get0_objects(root_cert_store); + int num_objs = sk_X509_OBJECT_num(objs); + + std::vector> result; + result.reserve(num_objs); + + for (size_t i = 0; i < num_objs; i++) { + X509_OBJECT* obj = sk_X509_OBJECT_value(objs, i); + if (X509_OBJECT_get_type(obj) == X509_LU_X509) { + X509* cert = X509_OBJECT_get0_X509(obj); + + Local value; + if (!X509ToPEM(env, cert).ToLocal(&value)) { + return; + } + + result.push_back(value); } } args.GetReturnValue().Set( - Array::New(env->isolate(), result, arraysize(root_certs))); + Array::New(env->isolate(), result.data(), result.size())); } diff --git a/src/node_crypto_common.cc b/src/node_crypto_common.cc index 197bc5cd5913a4..7a725f6b500dbc 100644 --- a/src/node_crypto_common.cc +++ b/src/node_crypto_common.cc @@ -968,6 +968,16 @@ MaybeLocal GetPeerCert( return result; } +MaybeLocal X509ToPEM(Environment* env, X509* cert) { + BIOPointer bio(BIO_new(BIO_s_mem())); + + if (PEM_write_bio_X509(bio.get(), cert) == 0) { + return Undefined(env->isolate()); + } + + return ToV8Value(env, bio); +} + MaybeLocal X509ToObject(Environment* env, X509* cert) { EscapableHandleScope scope(env->isolate()); Local context = env->context(); diff --git a/src/node_crypto_common.h b/src/node_crypto_common.h index e42e249ef2ba2e..992959c1c44439 100644 --- a/src/node_crypto_common.h +++ b/src/node_crypto_common.h @@ -131,6 +131,10 @@ v8::MaybeLocal X509ToObject( Environment* env, X509* cert); +v8::MaybeLocal X509ToPEM( + Environment* env, + X509* cert); + } // namespace crypto } // namespace node diff --git a/test/parallel/test-tls-root-certificates.js b/test/parallel/test-tls-root-certificates.js index 5f7aa418ac05a3..f200231fa301a5 100644 --- a/test/parallel/test-tls-root-certificates.js +++ b/test/parallel/test-tls-root-certificates.js @@ -2,30 +2,49 @@ const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); +const fixtures = require('../common/fixtures'); const assert = require('assert'); const tls = require('tls'); - -assert(Array.isArray(tls.rootCertificates)); -assert(tls.rootCertificates.length > 0); - -// Getter should return the same object. -assert.strictEqual(tls.rootCertificates, tls.rootCertificates); - -// Array is immutable... -assert.throws(() => tls.rootCertificates[0] = 0, /TypeError/); -assert.throws(() => tls.rootCertificates.sort(), /TypeError/); - -// ...and so is the property. -assert.throws(() => tls.rootCertificates = 0, /TypeError/); - -// Does not contain duplicates. -assert.strictEqual(tls.rootCertificates.length, - new Set(tls.rootCertificates).size); - -assert(tls.rootCertificates.every((s) => { - return s.startsWith('-----BEGIN CERTIFICATE-----\n'); -})); - -assert(tls.rootCertificates.every((s) => { - return s.endsWith('\n-----END CERTIFICATE-----'); -})); +const { fork } = require('child_process'); + +if (process.argv[2] !== 'child') { + // Parent + const NODE_EXTRA_CA_CERTS = fixtures.path('keys', 'ca1-cert.pem'); + + fork( + __filename, + ['child'], + { env: { ...process.env, NODE_EXTRA_CA_CERTS } } + ).on('exit', common.mustCall(function(status) { + assert.strictEqual(status, 0); + })); +} else { + // Child + assert(Array.isArray(tls.rootCertificates)); + assert(tls.rootCertificates.length > 0); + + // Getter should return the same object. + assert.strictEqual(tls.rootCertificates, tls.rootCertificates); + + // Array is immutable... + assert.throws(() => tls.rootCertificates[0] = 0, /TypeError/); + assert.throws(() => tls.rootCertificates.sort(), /TypeError/); + + // ...and so is the property. + assert.throws(() => tls.rootCertificates = 0, /TypeError/); + + // Does not contain duplicates. + assert.strictEqual(tls.rootCertificates.length, + new Set(tls.rootCertificates).size); + + assert(tls.rootCertificates.every((s) => { + return s.startsWith('-----BEGIN CERTIFICATE-----\n'); + })); + + assert(tls.rootCertificates.every((s) => { + return s.endsWith('\n-----END CERTIFICATE-----\n'); + })); + + const extraCert = fixtures.readKey('ca1-cert.pem', 'utf8'); + assert(tls.rootCertificates.includes(extraCert)); +} From ce40db3ac7252ba32aa0ba18f17e6f043e9de91c Mon Sep 17 00:00:00 2001 From: Eric Bickle Date: Tue, 3 Mar 2020 15:38:39 -0800 Subject: [PATCH 2/4] src: fix unsigned data type warning in node_crypto --- 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 716dafdf946a98..0f54134ee864eb 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -999,7 +999,7 @@ void GetRootCertificates(const FunctionCallbackInfo& args) { std::vector> result; result.reserve(num_objs); - for (size_t i = 0; i < num_objs; i++) { + for (int i = 0; i < num_objs; i++) { X509_OBJECT* obj = sk_X509_OBJECT_value(objs, i); if (X509_OBJECT_get_type(obj) == X509_LU_X509) { X509* cert = X509_OBJECT_get0_X509(obj); From 177a36c036f80228150d0ce1e34e6902dd10ac13 Mon Sep 17 00:00:00 2001 From: Eric Bickle Date: Wed, 4 Mar 2020 16:56:53 -0800 Subject: [PATCH 3/4] src: add exceptions to X509ToPEM - Moved X509ToPEM function to node_crypto.cc - Removed braces for single line statements - Modified X509ToPEM so that it throws crypto errors for unexpected OpenSSL errors --- src/node_crypto.cc | 21 +++++++++++++++++---- src/node_crypto.h | 2 ++ src/node_crypto_common.cc | 10 ---------- src/node_crypto_common.h | 4 ---- 4 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 0f54134ee864eb..101a29ddca30dd 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -989,9 +989,8 @@ static X509_STORE* NewRootCertStore() { void GetRootCertificates(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); - if (root_cert_store == nullptr) { + if (root_cert_store == nullptr) root_cert_store = NewRootCertStore(); - } stack_st_X509_OBJECT* objs = X509_STORE_get0_objects(root_cert_store); int num_objs = sk_X509_OBJECT_num(objs); @@ -1005,9 +1004,8 @@ void GetRootCertificates(const FunctionCallbackInfo& args) { X509* cert = X509_OBJECT_get0_X509(obj); Local value; - if (!X509ToPEM(env, cert).ToLocal(&value)) { + if (!X509ToPEM(env, cert).ToLocal(&value)) return; - } result.push_back(value); } @@ -2694,6 +2692,21 @@ static inline Local BIOToStringOrBuffer(Environment* env, } } +static MaybeLocal X509ToPEM(Environment* env, X509* cert) { + BIOPointer bio(BIO_new(BIO_s_mem())); + if (!bio) { + ThrowCryptoError(env, ERR_get_error(), "BIO_new"); + return MaybeLocal(); + } + + if (PEM_write_bio_X509(bio.get(), cert) == 0) { + ThrowCryptoError(env, ERR_get_error(), "PEM_write_bio_X509"); + return MaybeLocal(); + } + + return BIOToStringOrBuffer(env, bio.get(), kKeyFormatPEM); +} + static bool WritePublicKeyInner(EVP_PKEY* pkey, const BIOPointer& bio, const PublicKeyEncodingConfig& config) { diff --git a/src/node_crypto.h b/src/node_crypto.h index 772a34a7da7699..1d7f91ac97a6e2 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -785,6 +785,8 @@ void ThrowCryptoError(Environment* env, unsigned long err, // NOLINT(runtime/int) const char* message = nullptr); +static v8::MaybeLocal X509ToPEM(Environment* env, X509* cert); + template inline T* MallocOpenSSL(size_t count) { void* mem = OPENSSL_malloc(MultiplyWithOverflowCheck(count, sizeof(T))); diff --git a/src/node_crypto_common.cc b/src/node_crypto_common.cc index 7a725f6b500dbc..197bc5cd5913a4 100644 --- a/src/node_crypto_common.cc +++ b/src/node_crypto_common.cc @@ -968,16 +968,6 @@ MaybeLocal GetPeerCert( return result; } -MaybeLocal X509ToPEM(Environment* env, X509* cert) { - BIOPointer bio(BIO_new(BIO_s_mem())); - - if (PEM_write_bio_X509(bio.get(), cert) == 0) { - return Undefined(env->isolate()); - } - - return ToV8Value(env, bio); -} - MaybeLocal X509ToObject(Environment* env, X509* cert) { EscapableHandleScope scope(env->isolate()); Local context = env->context(); diff --git a/src/node_crypto_common.h b/src/node_crypto_common.h index 992959c1c44439..e42e249ef2ba2e 100644 --- a/src/node_crypto_common.h +++ b/src/node_crypto_common.h @@ -131,10 +131,6 @@ v8::MaybeLocal X509ToObject( Environment* env, X509* cert); -v8::MaybeLocal X509ToPEM( - Environment* env, - X509* cert); - } // namespace crypto } // namespace node From 2ff1257ee129db5bc575dc5de857fe4c7c7516ca Mon Sep 17 00:00:00 2001 From: Eric Bickle Date: Wed, 4 Mar 2020 20:31:04 -0800 Subject: [PATCH 4/4] src: remove header decl of X509ToPEM --- src/node_crypto.cc | 60 +++++++++++++++++++++++----------------------- src/node_crypto.h | 2 -- 2 files changed, 30 insertions(+), 32 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 101a29ddca30dd..34534662312605 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -986,36 +986,6 @@ static X509_STORE* NewRootCertStore() { } -void GetRootCertificates(const FunctionCallbackInfo& args) { - Environment* env = Environment::GetCurrent(args); - - if (root_cert_store == nullptr) - root_cert_store = NewRootCertStore(); - - stack_st_X509_OBJECT* objs = X509_STORE_get0_objects(root_cert_store); - int num_objs = sk_X509_OBJECT_num(objs); - - std::vector> result; - result.reserve(num_objs); - - for (int i = 0; i < num_objs; i++) { - X509_OBJECT* obj = sk_X509_OBJECT_value(objs, i); - if (X509_OBJECT_get_type(obj) == X509_LU_X509) { - X509* cert = X509_OBJECT_get0_X509(obj); - - Local value; - if (!X509ToPEM(env, cert).ToLocal(&value)) - return; - - result.push_back(value); - } - } - - args.GetReturnValue().Set( - Array::New(env->isolate(), result.data(), result.size())); -} - - void SecureContext::AddCACert(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -6687,6 +6657,36 @@ void ExportChallenge(const FunctionCallbackInfo& args) { } +void GetRootCertificates(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + + if (root_cert_store == nullptr) + root_cert_store = NewRootCertStore(); + + stack_st_X509_OBJECT* objs = X509_STORE_get0_objects(root_cert_store); + int num_objs = sk_X509_OBJECT_num(objs); + + std::vector> result; + result.reserve(num_objs); + + for (int i = 0; i < num_objs; i++) { + X509_OBJECT* obj = sk_X509_OBJECT_value(objs, i); + if (X509_OBJECT_get_type(obj) == X509_LU_X509) { + X509* cert = X509_OBJECT_get0_X509(obj); + + Local value; + if (!X509ToPEM(env, cert).ToLocal(&value)) + return; + + result.push_back(value); + } + } + + args.GetReturnValue().Set( + Array::New(env->isolate(), result.data(), result.size())); +} + + // Convert the input public key to compressed, uncompressed, or hybrid formats. void ConvertKey(const FunctionCallbackInfo& args) { MarkPopErrorOnReturn mark_pop_error_on_return; diff --git a/src/node_crypto.h b/src/node_crypto.h index 1d7f91ac97a6e2..772a34a7da7699 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -785,8 +785,6 @@ void ThrowCryptoError(Environment* env, unsigned long err, // NOLINT(runtime/int) const char* message = nullptr); -static v8::MaybeLocal X509ToPEM(Environment* env, X509* cert); - template inline T* MallocOpenSSL(size_t count) { void* mem = OPENSSL_malloc(MultiplyWithOverflowCheck(count, sizeof(T)));