crypto: return clear errors when loading invalid PFX data

src/crypto/crypto_context.cc

@@ -1052,34 +1052,60 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& 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)

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();
+}));