From fcf632bc6a3e89aef8729b44c295e34ba1b76a63 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Sat, 5 Dec 2015 16:53:30 -0500 Subject: [PATCH] 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 --- src/node_crypto.cc | 170 +++++++++++++++++------- test/fixtures/keys/Makefile | 8 ++ test/fixtures/keys/agent1-pfx.pem | Bin 0 -> 2437 bytes test/parallel/test-tls-ocsp-callback.js | 33 ++++- 4 files changed, 156 insertions(+), 55 deletions(-) create mode 100644 test/fixtures/keys/agent1-pfx.pem diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 63d767a1f4cd7c..d120fed7cc70bb 100644 --- a/src/node_crypto.cc +++ b/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; - X509* x = nullptr; + CHECK_EQ(*issuer, nullptr); + CHECK_EQ(*cert, nullptr); - 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); - goto end; - } - - ret = SSL_CTX_use_certificate(ctx, x); + int 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. - 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; + *issuer = ca; } } @@ -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& 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& 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,28 +978,33 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo& 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) && - SSL_CTX_use_certificate(sc->ctx_, cert) && + PKCS12_parse(p12, pass, &pkey, &cert, &extra_certs) && + 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); - } + ret = true; + } + if (pkey != nullptr) EVP_PKEY_free(pkey); + if (cert != nullptr) X509_free(cert); - sk_X509_free(extraCerts); - - ret = true; - } + if (extra_certs != nullptr) + sk_X509_free(extra_certs); PKCS12_free(p12); BIO_free_all(in); diff --git a/test/fixtures/keys/Makefile b/test/fixtures/keys/Makefile index 143986274a6b93..1148e529cd9595 100644 --- a/test/fixtures/keys/Makefile +++ b/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 diff --git a/test/fixtures/keys/agent1-pfx.pem b/test/fixtures/keys/agent1-pfx.pem new file mode 100644 index 0000000000000000000000000000000000000000..a36e746a72e06e8dd913f335c1e889ae7646513e GIT binary patch literal 2437 zcmY+FX*3j!8pq8T!w^H33`WSEit-0U(9ItpEOS1^`BKfLNNlr?OJ1>&rkO z3qvH1X(dN!wDhm`p>`5u2ycJP%BvjJ|M6vKhd;iYzes8nQ|!joljb@5azgeWC}Z z&F_;+#ve_b0>Q#cx75wAv&P%l*yy0F^DM6)tq0GSx^4ui4Ne&udX}a~CPGT*!kk^U zl>Euzs|^nx_e+f0$Gk!N9!3~744Dv^U$~`P!0x_Z7g}{4Nq@>w(WzfAaG0;`xpX)O z;+hNQfx_rI0#h+8m9pd=)bbrRQ``^A-i43*mHg-#4;<-~*K#5_RS{}2ceqX1v zEdc9_Pf0sxcZ-pSm?l;2%x>AZ59gDfU$86)c~C`I_SSaI6$}|-uD0)%FUW5F#8~1H zOe+RPr&*^=Ux`akQ|FS9Sb#p^kjyYC3cA>asIgTzJno6jl znyu8@xmgxn24cp?I(1V584w;%%xrI8TMNs-mnm#TvNcQ@KaSTuRxchYM*q4+by67) zdi}$4(o>ABONd6$JuLNP#<5snxAl_)FE$PYCXNQlrrg?mCyeyAsf=+y-z-i~*h84f zx+yw};9aZAeRp(f$&K^yHOj)9Gd(3q-dOg~CBKEf6lXpAT`HHMx-hEnHac5x{+sYS zc=IrWcujqxEsZV!b+R}4XnC@uUF#l{?fhq%foxab+A%*~$z3CPoCa!S@j-m&6@X5k z+|mED=-a_t3l^zt)Yp3pW3|RgFf`nArD=0rDbII=j=vG$miY=^r^H(GfDrM%;-p5~ zc#^%mG26rS*RH`1*$Q4K)G}}r@xG;KO2stAr6^5txq}oOVLROQckwR^{+)AL#lg>Y zxE$X(v28SMg5*lWnAr5;jt3$oED)Ij1poDQ-`IMG*)$6*Je2}Z9{f3|xkXZVwi8cT zSbp0kaSkp;L++^o3oUta9nuFjR7%APeWXx>Im)Slg6xdo~z#`qp7*|!eNnw%6&j3y zH+)5+Wa`pm>p|RALeA_L5kDqY)jFlIXB7Na#hz$?+aPD2C&hMX6anA?GD90yaX8Im zn8>A^FJq-u)cW#P>1{X<(GcrK{AsYL?hfefg^?WUTEmwurQHziYYr;xoA50hwOY z<@wtlr{|HmUV6&+uf97gwyDPcDXYNDM?7aEyDAp_GWzcXXlKLLP=s>)WaVbuk(5#Z zt*(;X^4um%Kf}k;oTojk!!m>X%N>|6DKpqb@kMZaWF<;X33>Ewriz?}BB$%XW%nMI z+p}9|!Wer5t!Y5hAg2jzheAfejVax5H~D~xsUhXp*ce1%y}K2IN|e1fWsBW#73X~+R&$(GPx^ZuW7+UzLQ zJzCyeRPsG9Ku*Y_w_F{FI;PJV&3u>h84EOM%nhK*|5@7EtOHe>onbsnajAP|yU*{T zK_oxkZfb{Ab16u}9s5@FY@=QYKKl{P*=-#e0qL}7vkYf%5v2yUP5C829F3sl7|)2u zt6GaF;!KUMAORP8fhgt8@xd6~J>r61k7}igApje!~o(+nE`3(5W?;=yp=dQhfSj=T3y`$v7kO@dNR67MEq{|zW{xI BfE@q; literal 0 HcmV?d00001 diff --git a/test/parallel/test-tls-ocsp-callback.js b/test/parallel/test-tls-ocsp-callback.js index d970b2ab013446..a11be7ac22d1c5 100644 --- a/test/parallel/test-tls-ocsp-callback.js +++ b/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() { - test({ response: 'hello world' }, function() { - test({ ocsp: false }); - }); -}); +var pfx = fs.readFileSync(join(common.fixturesDir, 'keys', 'agent1-pfx.pem')); 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);