From 56a430a208df9583db12e852f8dcbdcc5d2c7454 Mon Sep 17 00:00:00 2001 From: Eric Bickle Date: Fri, 8 May 2020 07:35:17 -0700 Subject: [PATCH 1/2] Revert "src: fix missing extra ca in tls.rootCertificates" A fix to tls.rootCertificates to have it correctly return the process' current root certificates resulted in non-deterministic behavior when Node.js is configured to use an OpenSSL system or file-based certificate store. The safest action is to revert the change and change the specification for tls.rootCertificates to state that it only returns the bundled certificates instead of the current ones. Fixes: #32229 Refs: #32074 --- src/node_crypto.cc | 63 ++++++------------- test/parallel/test-tls-root-certificates.js | 69 ++++++++------------- 2 files changed, 43 insertions(+), 89 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index d53a6d2f2325fe..715a724413c161 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1008,6 +1008,24 @@ 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; + } + } + + args.GetReturnValue().Set( + Array::New(env->isolate(), result, arraysize(root_certs))); +} + + void SecureContext::AddCACert(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -2684,21 +2702,6 @@ 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) { @@ -6648,36 +6651,6 @@ 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/test/parallel/test-tls-root-certificates.js b/test/parallel/test-tls-root-certificates.js index f200231fa301a5..5f7aa418ac05a3 100644 --- a/test/parallel/test-tls-root-certificates.js +++ b/test/parallel/test-tls-root-certificates.js @@ -2,49 +2,30 @@ const common = require('../common'); if (!common.hasCrypto) common.skip('missing crypto'); -const fixtures = require('../common/fixtures'); const assert = require('assert'); const tls = require('tls'); -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)); -} + +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-----'); +})); From 3a65605b22238703b891e5943860ceb56bd25545 Mon Sep 17 00:00:00 2001 From: Eric Bickle Date: Fri, 8 May 2020 07:37:50 -0700 Subject: [PATCH 2/2] doc: correct tls.rootCertificates to match implementation Update tls.rootCertificates documentation to clarify that it returns the bundled Node.js root certificates rather than the root certificates used by tls.createSecureContext. Fixes: #32074 Refs: #32229 --- doc/api/tls.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/doc/api/tls.md b/doc/api/tls.md index c29dfcb48ae58f..4921761a3246d4 100644 --- a/doc/api/tls.md +++ b/doc/api/tls.md @@ -1791,8 +1791,10 @@ added: v12.3.0 * {string[]} An immutable array of strings representing the root certificates (in PEM format) -used for verifying peer certificates. This is the default value of the `ca` -option to [`tls.createSecureContext()`][]. +from the bundled Mozilla CA store as supplied by current Node.js version. + +The bundled CA store, as supplied by Node.js, is a snapshot of Mozilla CA store +that is fixed at release time. It is identical on all supported platforms. ## `tls.DEFAULT_ECDH_CURVE`