From 9f848d238f4d382959e94de64e56ab12e2ddbbd4 Mon Sep 17 00:00:00 2001
From: Tim Perry <1526883+pimterry@users.noreply.github.com>
Date: Fri, 29 Sep 2023 13:56:33 +0300
Subject: [PATCH] crypto: return clear errors when loading invalid PFX data

PR-URL: https://github.com/nodejs/node/pull/49566
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
---
 src/crypto/crypto_context.cc            |  76 ++++++++++++++++--------
 test/fixtures/keys/cert-without-key.pfx | Bin 0 -> 1483 bytes
 test/parallel/test-tls-invalid-pfx.js   |  23 +++++++
 3 files changed, 74 insertions(+), 25 deletions(-)
 create mode 100644 test/fixtures/keys/cert-without-key.pfx
 create mode 100644 test/parallel/test-tls-invalid-pfx.js

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<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
new file mode 100644
index 0000000000000000000000000000000000000000..6d3dfca11fe98446931033e13becbac383c4ffe0
GIT binary patch
literal 1483
zcmV;+1vL6Ff(6F{0Ru3C1$_nyDuzgg_YDCD0ic2fZUlk_YA}KYW-x*UVg?B+hDe6@
z4FLxRpn?TcFoFe70s#Opf(1wh2`Yw2hW8Bt2LUiw1_>&LNQU<f0R;^(N-!w~2`Yw2
zhW8Bt1q?781PD3tO{Kz>{y72y2mmk)1_&yKNQU<f0tp2GFdYU7V1`HmWdj5ODg+Q*
zH1x#u!;U@;PhZ29wz<oIf&}1Iq@$^^d9Z!N2;#q7(kS(S5wlUq9j$!upDU-Ds_A48
zjoqZP32#wVNyw<i4PgeCZS}lz*Ilo?H>#O#UHS2`M*uHl*Y}gTW4Q|oITZZb;6sl8
z(4$~E$;+L?JrMzk8(BxR;3u%tVo07nMt4gkG}qqa&eGuX8Jmo0gyfwfC#ZZv7#Od>
zf7Ms*50>9X^O`|_z%csxE%??5SQ`}<3H|n^MevmqNw||p0`xmubT1pdlzs~~0zM)p
z1@-J>32N$x3_xU<)7aAWlH0acz*gg(3PyHh=0ZQj=dD$qXfsfPxV^wS=`Pif&Pi?E
zvXX+YyS%WMbm?6JA4Bn$Y(!(vOQNi{tj+`B*4X_K$6&6UVxv6N?k>{B#WU#1YC-bm
zxK=>q`{1xwwx^F`t0<O43u13NtKQGNA9S$gBx*0Lc6z8<Wc-QRXv1zRBiF#!Ca1$J
z_Wi|PxC&(^Q@R3`8G*H9Vg%IO#V(=iB!v1=C<eJ&MS5&q`_i4X`qvOK*|XDcNVeSF
zrla5Yf<XQDuOdk8<vK1o?#V->DN>0z-T+A-W9ho+f4=^kUF$McklhPYxMM7+q~lrO
z$l@`7JSoQS)bY5nyqGtLipeawJbqhxYX1Ro90l&dUG*OCZ8I5dxg003PcVVJv?pNG
zynCut%73q?{bv5~Ojsm};it&!X=wOgB1RL?v&%*yf!V}7<6tQ}-f`6<o2E!;54%hA
zupLElk2EQ{jUp?UM1Zp@3P^n_hgTKq6xt@tp$at0QPEk2XZZlQ?ou-97%$SK$1Wh#
zC^C(5wCqY>fZ}nj?&ZmR0MPr4{Y`66nsSo6FrFtZHsE!}%3^IzQ|FLg5t_dSYR(W`
zCQBF-bw|o=c41Sw{xV@QRZtl|Fy_elo%iw@O$d(Zxiw)dQ5OP+)V?^)B{78g<>`m;
z?`0GJcLvrYR{!a#H7;7wn3qcNJSjUPvezmA3_jH$Kpg7WT?r3p<r^RpS*myY<eW{F
ztXD^MgS>XjP<Y(COa}Ko#>Q(~l|vF1!kwnm=ZE_FKb}?L%KJ5`<KIQi88wR4M563e
zIR|{aB0cSg8pgV`?V^mI(2M%|;Voa?{1P4Jdpv;ku{E(u0ji$MITgvU{M$Y<f_HRn
zBOD9A!6-`5FnKJwWz~$>%fSKnRIjZnr<VQEy2Ih{MryCZl)LTAa^zy7iDZE*u^-Pn
z%&K7Orb;$?lCF`N_?YZ=efqg?EBwIN`fqAQ$@$!;LLmN3xq9CYk_d#8vspc{c+1wP
zRkP*T?;GyixjuBK0?<M0#mwam0h$Txi&-sI&I^8$q-=BqJKSV3R_r<uc%l6w?)x2%
zSjRaaxBnCkAX#p`8TrAHD~U*M33o|Mfqcr7@#@?!4kkn|F$re4GHrT+&ijJz%FCVZ
zDaCjf)U1$G^_ZHG4G?I{hWWRFXSw5P=H*W}f%01C2RcD7Rp5;&)i>fBbTuY|g$l~k
z)`xjW;3Xl`Gm5HcIC<RZ<w{sw()IYPtMuseaRu?~WRt>Gd3Bd$ggIk$Pu2yQ8Ap;p
zi#xF3uFLq=^EZfVMTuX~Zg>2^14sU<7{ZP@a!kD+x=J0tz<^OaOcor%%YI>!O~A-V
zUrp-(Fm?m4RFZRa!gxNTwB;8tK`=2e4F(BdhDZTr0|WvA1povfbA{bAVR(y9g{P(W
l8fsA}>cmtx`2HsZuA<a;wKGA#1PHXq$F=IcG^_#w2ml4~&T0Sv

literal 0
HcmV?d00001

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