crypto: load PFX chain the same way as regular one

Load the certificate chain from the PFX file the same as we do it for a
regular certificate chain.

Fix: #4127
PR-URL: https://github.com/nodejs/node/pull/4165
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

src/node_crypto.cc

@@ -517,46 +517,35 @@ int SSL_CTX_get_issuer(SSL_CTX* ctx, X509* cert, X509** issuer) {
 }
 
 
-// Read a file that contains our certificate in "PEM" format,
-// possibly followed by a sequence of CA certificates that should be
-// sent to the peer in the Certificate message.
-//
-// Taken from OpenSSL - editted for style.
 int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
-                                  BIO* in,
+                                  X509* x,
+                                  STACK_OF(X509)* extra_certs,
                                   X509** cert,
                                   X509** issuer) {
-  int ret = 0;
+  CHECK_EQ(*issuer, nullptr);
-  X509* x = nullptr;
+  CHECK_EQ(*cert, nullptr);
 
-  x = PEM_read_bio_X509_AUX(in, nullptr, CryptoPemCallback, nullptr);
+  int ret = SSL_CTX_use_certificate(ctx, x);
-
-  if (x == nullptr) {
-    SSLerr(SSL_F_SSL_CTX_USE_CERTIFICATE_CHAIN_FILE, ERR_R_PEM_LIB);
-    goto end;
-  }
-
-  ret = SSL_CTX_use_certificate(ctx, x);
 
   if (ret) {
     // If we could set up our certificate, now proceed to
     // the CA certificates.
-    X509 *ca;
     int r;
-    unsigned long err;
 
     if (ctx->extra_certs != nullptr) {
       sk_X509_pop_free(ctx->extra_certs, X509_free);
       ctx->extra_certs = nullptr;
     }
 
-    while ((ca = PEM_read_bio_X509(in, nullptr, CryptoPemCallback, nullptr))) {
+    for (int i = 0; i < sk_X509_num(extra_certs); i++) {
+      X509* ca = sk_X509_value(extra_certs, i);
+
       // NOTE: Increments reference count on `ca`
       r = SSL_CTX_add1_chain_cert(ctx, ca);
 
       if (!r) {
-        X509_free(ca);
         ret = 0;
+        *issuer = nullptr;
         goto end;
       }
       // Note that we must not free r if it was successfully
@@ -567,17 +556,8 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
       // Find issuer
       if (*issuer != nullptr || X509_check_issued(ca, x) != X509_V_OK)
         continue;
-      *issuer = ca;
-    }
 
-    // When the while loop ends, it's usually just EOF.
+      *issuer = ca;
-    err = ERR_peek_last_error();
-    if (ERR_GET_LIB(err) == ERR_LIB_PEM &&
-        ERR_GET_REASON(err) == PEM_R_NO_START_LINE) {
-      ERR_clear_error();
-    } else  {
-      // some real error
-      ret = 0;
     }
   }
 
@@ -590,13 +570,88 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
       // no need to free `store`
     } else {
       // Increment issuer reference count
-      CRYPTO_add(&(*issuer)->references, 1, CRYPTO_LOCK_X509);
+      *issuer = X509_dup(*issuer);
+      if (*issuer == nullptr) {
+        ret = 0;
+        goto end;
+      }
     }
   }
 
  end:
+  if (ret && x != nullptr) {
+    *cert = X509_dup(x);
+    if (*cert == nullptr)
+      ret = 0;
+  }
+  return ret;
+}
+
+
+// Read a file that contains our certificate in "PEM" format,
+// possibly followed by a sequence of CA certificates that should be
+// sent to the peer in the Certificate message.
+//
+// Taken from OpenSSL - edited for style.
+int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
+                                  BIO* in,
+                                  X509** cert,
+                                  X509** issuer) {
+  X509* x = nullptr;
+
+  // Just to ensure that `ERR_peek_last_error` below will return only errors
+  // that we are interested in
+  ERR_clear_error();
+
+  x = PEM_read_bio_X509_AUX(in, nullptr, CryptoPemCallback, nullptr);
+
+  if (x == nullptr) {
+    SSLerr(SSL_F_SSL_CTX_USE_CERTIFICATE_CHAIN_FILE, ERR_R_PEM_LIB);
+    return 0;
+  }
+
+  X509* extra = nullptr;
+  int ret = 0;
+  unsigned long err = 0;
+
+  // Read extra certs
+  STACK_OF(X509)* extra_certs = sk_X509_new_null();
+  if (extra_certs == nullptr) {
+    SSLerr(SSL_F_SSL_CTX_USE_CERTIFICATE_CHAIN_FILE, ERR_R_MALLOC_FAILURE);
+    goto done;
+  }
+
+  while ((extra = PEM_read_bio_X509(in, nullptr, CryptoPemCallback, nullptr))) {
+    if (sk_X509_push(extra_certs, extra))
+      continue;
+
+    // Failure, free all certs
+    goto done;
+  }
+  extra = nullptr;
+
+  // When the while loop ends, it's usually just EOF.
+  err = ERR_peek_last_error();
+  if (ERR_GET_LIB(err) == ERR_LIB_PEM &&
+      ERR_GET_REASON(err) == PEM_R_NO_START_LINE) {
+    ERR_clear_error();
+  } else  {
+    // some real error
+    goto done;
+  }
+
+  ret = SSL_CTX_use_certificate_chain(ctx, x, extra_certs, cert, issuer);
+  if (!ret)
+    goto done;
+
+ done:
+  if (extra_certs != nullptr)
+    sk_X509_pop_free(extra_certs, X509_free);
+  if (extra != nullptr)
+    X509_free(extra);
   if (x != nullptr)
-    *cert = x;
+    X509_free(x);
+
   return ret;
 }
 
@@ -614,6 +669,16 @@ void SecureContext::SetCert(const FunctionCallbackInfo<Value>& args) {
   if (!bio)
     return;
 
+  // Free previous certs
+  if (sc->issuer_ != nullptr) {
+    X509_free(sc->issuer_);
+    sc->issuer_ = nullptr;
+  }
+  if (sc->cert_ != nullptr) {
+    X509_free(sc->cert_);
+    sc->cert_ = nullptr;
+  }
+
   int rv = SSL_CTX_use_certificate_chain(sc->ctx_,
                                          bio,
                                          &sc->cert_,
@@ -888,7 +953,7 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
   PKCS12* p12 = nullptr;
   EVP_PKEY* pkey = nullptr;
   X509* cert = nullptr;
-  STACK_OF(X509)* extraCerts = nullptr;
+  STACK_OF(X509)* extra_certs = nullptr;
   char* pass = nullptr;
   bool ret = false;
 
@@ -913,29 +978,34 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
     pass[passlen] = '\0';
   }
 
+  // Free previous certs
+  if (sc->issuer_ != nullptr) {
+    X509_free(sc->issuer_);
+    sc->issuer_ = nullptr;
+  }
+  if (sc->cert_ != nullptr) {
+    X509_free(sc->cert_);
+    sc->cert_ = nullptr;
+  }
+
   if (d2i_PKCS12_bio(in, &p12) &&
-      PKCS12_parse(p12, pass, &pkey, &cert, &extraCerts) &&
+      PKCS12_parse(p12, pass, &pkey, &cert, &extra_certs) &&
-      SSL_CTX_use_certificate(sc->ctx_, cert) &&
+      SSL_CTX_use_certificate_chain(sc->ctx_,
+                                    cert,
+                                    extra_certs,
+                                    &sc->cert_,
+                                    &sc->issuer_) &&
       SSL_CTX_use_PrivateKey(sc->ctx_, pkey)) {
-    // set extra certs
-    while (X509* x509 = sk_X509_pop(extraCerts)) {
-      if (!sc->ca_store_) {
-        sc->ca_store_ = X509_STORE_new();
-        SSL_CTX_set_cert_store(sc->ctx_, sc->ca_store_);
-      }
-
-      X509_STORE_add_cert(sc->ca_store_, x509);
-      SSL_CTX_add_client_CA(sc->ctx_, x509);
-      X509_free(x509);
-    }
-
-    EVP_PKEY_free(pkey);
-    X509_free(cert);
-    sk_X509_free(extraCerts);
-
     ret = true;
   }
 
+  if (pkey != nullptr)
+    EVP_PKEY_free(pkey);
+  if (cert != nullptr)
+    X509_free(cert);
+  if (extra_certs != nullptr)
+    sk_X509_free(extra_certs);
+
   PKCS12_free(p12);
   BIO_free_all(in);
   delete[] pass;

test/fixtures/keys/Makefile

@@ -79,6 +79,14 @@ agent1-cert.pem: agent1-csr.pem ca1-cert.pem ca1-key.pem
 		-CAcreateserial \
 		-out agent1-cert.pem
 
+agent1-pfx.pem: agent1-cert.pem agent1-key.pem ca1-cert.pem
+	openssl pkcs12 -export \
+		-in agent1-cert.pem \
+		-inkey agent1-key.pem \
+		-certfile ca1-cert.pem \
+		-out agent1-pfx.pem \
+		-password pass:sample
+
 agent1-verify: agent1-cert.pem ca1-cert.pem
 	openssl verify -CAfile ca1-cert.pem agent1-cert.pem
 

test/parallel/test-tls-ocsp-callback.js

@@ -22,11 +22,7 @@ var constants = require('constants');
 var fs = require('fs');
 var join = require('path').join;
 
-test({ response: false }, function() {
+var pfx = fs.readFileSync(join(common.fixturesDir, 'keys', 'agent1-pfx.pem'));
-  test({ response: 'hello world' }, function() {
-    test({ ocsp: false });
-  });
-});
 
 function test(testOptions, cb) {
 
@@ -47,6 +43,13 @@ function test(testOptions, cb) {
   var ocspResponse;
   var session;
 
+  if (testOptions.pfx) {
+    delete options.key;
+    delete options.cert;
+    options.pfx = testOptions.pfx;
+    options.passphrase = testOptions.passphrase;
+  }
+
   var server = tls.createServer(options, function(cleartext) {
     cleartext.on('error', function(er) {
       // We're ok with getting ECONNRESET in this test, but it's
@@ -106,3 +109,23 @@ function test(testOptions, cb) {
     assert.equal(ocspCount, 1);
   });
 }
+
+var tests = [
+  { response: false },
+  { response: 'hello world' },
+  { ocsp: false }
+];
+
+if (!common.hasFipsCrypto) {
+  tests.push({ pfx: pfx, passphrase: 'sample', response: 'hello pfx' });
+}
+
+function runTests(i) {
+  if (i === tests.length) return;
+
+  test(tests[i], common.mustCall(function() {
+    runTests(i + 1);
+  }));
+}
+
+runTests(0);