crypto: return clear errors when loading invalid PFX data

Author: pimterry

src/crypto/crypto_context.cc

@@ -1054,30 +1054,41 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
   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);
+      PKCS12_parse(
+          p12.get(), pass.data(), &pkey_ptr, &cert_ptr, &extra_certs_ptr) &&
+      (pkey.reset(pkey_ptr),
+       cert.reset(cert_ptr),  // Move ownership.
+       extra_certs.reset(extra_certs_ptr),
+       true)) {
+    if (pkey.get() == nullptr) {
+      return THROW_ERR_CRYPTO_OPERATION_FAILED(
+          env, "Unable to load private key from PFX data");
+    }
+
+    if (cert.get() == nullptr) {
+      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_) &&
+        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);
       }
-      X509_STORE_add_cert(cert_store, ca);
-      SSL_CTX_add_client_CA(sc->ctx_.get(), ca);
+      ret = true;
     }
-    ret = true;
   }
 
   if (!ret) {

test/fixtures/keys/cert-without-key.pfx

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