crypto: return clear errors when loading invalid PFX data

PR-URL: nodejs#49566 Reviewed-By: Ben Noordhuis <[email protected]>
debadree25 · Apr 15, 2024 · 9f848d2 · 9f848d2
1 parent b36d25f
commit 9f848d2
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 25 deletions.
diff --git a/src/crypto/crypto_context.cc b/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)

diff --git a/test/fixtures/keys/cert-without-key.pfx b/test/fixtures/keys/cert-without-key.pfx
diff --git a/test/parallel/test-tls-invalid-pfx.js 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();
+}));