diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc index 3876adf7d72211..6e5bbe07d0c337 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -1052,34 +1052,60 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo& args) { EVP_PKEY* pkey_ptr = nullptr; X509* cert_ptr = nullptr; STACK_OF(X509)* extra_certs_ptr = nullptr; - if (d2i_PKCS12_bio(in.get(), &p12_ptr) && - (p12.reset(p12_ptr), true) && // Move ownership to the smart pointer. - PKCS12_parse(p12.get(), pass.data(), - &pkey_ptr, - &cert_ptr, - &extra_certs_ptr) && - (pkey.reset(pkey_ptr), cert.reset(cert_ptr), - extra_certs.reset(extra_certs_ptr), true) && // Move ownership. - SSL_CTX_use_certificate_chain(sc->ctx_.get(), - std::move(cert), - extra_certs.get(), - &sc->cert_, - &sc->issuer_) && - SSL_CTX_use_PrivateKey(sc->ctx_.get(), pkey.get())) { - // Add CA certs too - for (int i = 0; i < sk_X509_num(extra_certs.get()); i++) { - X509* ca = sk_X509_value(extra_certs.get(), i); - - if (cert_store == GetOrCreateRootCertStore()) { - cert_store = NewRootCertStore(); - SSL_CTX_set_cert_store(sc->ctx_.get(), cert_store); - } - X509_STORE_add_cert(cert_store, ca); - SSL_CTX_add_client_CA(sc->ctx_.get(), ca); + + if (!d2i_PKCS12_bio(in.get(), &p12_ptr)) { + goto done; + } + + // Move ownership to the smart pointer: + p12.reset(p12_ptr); + + if (!PKCS12_parse( + p12.get(), pass.data(), &pkey_ptr, &cert_ptr, &extra_certs_ptr)) { + goto done; + } + + // Move ownership of the parsed data: + pkey.reset(pkey_ptr); + cert.reset(cert_ptr); + extra_certs.reset(extra_certs_ptr); + + if (!pkey) { + return THROW_ERR_CRYPTO_OPERATION_FAILED( + env, "Unable to load private key from PFX data"); + } + + if (!cert) { + return THROW_ERR_CRYPTO_OPERATION_FAILED( + env, "Unable to load certificate from PFX data"); + } + + if (!SSL_CTX_use_certificate_chain(sc->ctx_.get(), + std::move(cert), + extra_certs.get(), + &sc->cert_, + &sc->issuer_)) { + goto done; + } + + if (!SSL_CTX_use_PrivateKey(sc->ctx_.get(), pkey.get())) { + goto done; + } + + // Add CA certs too + for (int i = 0; i < sk_X509_num(extra_certs.get()); i++) { + X509* ca = sk_X509_value(extra_certs.get(), i); + + if (cert_store == GetOrCreateRootCertStore()) { + cert_store = NewRootCertStore(); + SSL_CTX_set_cert_store(sc->ctx_.get(), cert_store); } - ret = true; + X509_STORE_add_cert(cert_store, ca); + SSL_CTX_add_client_CA(sc->ctx_.get(), ca); } + ret = true; +done: if (!ret) { // TODO(@jasnell): Should this use ThrowCryptoError? unsigned long err = ERR_get_error(); // NOLINT(runtime/int) diff --git a/test/fixtures/keys/cert-without-key.pfx b/test/fixtures/keys/cert-without-key.pfx new file mode 100644 index 00000000000000..6d3dfca11fe984 Binary files /dev/null and b/test/fixtures/keys/cert-without-key.pfx differ diff --git a/test/parallel/test-tls-invalid-pfx.js b/test/parallel/test-tls-invalid-pfx.js new file mode 100644 index 00000000000000..c16858f0f78873 --- /dev/null +++ b/test/parallel/test-tls-invalid-pfx.js @@ -0,0 +1,23 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); +const fixtures = require('../common/fixtures'); + +const { + assert, connect, keys +} = require(fixtures.path('tls-connect')); + +const invalidPfx = fixtures.readKey('cert-without-key.pfx'); + +connect({ + client: { + pfx: invalidPfx, + passphrase: 'test', + rejectUnauthorized: false + }, + server: keys.agent1 +}, common.mustCall((e, pair, cleanup) => { + assert.strictEqual(e.message, 'Unable to load private key from PFX data'); + cleanup(); +}));