From 7611f1b7d22bbcfcc4a7ef81638a4fca2fd7cad1 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 14 Jan 2019 12:08:55 +0100 Subject: [PATCH] tls: do not free cert in `.getCertificate()` The documentation of `SSL_get_certificate` states that it returns an internal pointer that must not be freed by the caller. Therefore, using a smart pointer to take ownership is incorrect. Refs: https://man.openbsd.org/SSL_get_certificate.3 Refs: https://github.com/nodejs/node/pull/24261 Fixes: https://github.com/nodejs-private/security/issues/217 --- src/node_crypto.cc | 6 +++--- test/parallel/test-tls-pfx-authorizationerror.js | 9 +++++++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 296b15de5bee38..da5432713ca582 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1963,10 +1963,10 @@ void SSLWrap::GetCertificate( Local result; - X509Pointer cert(SSL_get_certificate(w->ssl_.get())); + X509* cert = SSL_get_certificate(w->ssl_.get()); - if (cert) - result = X509ToObject(env, cert.get()); + if (cert != nullptr) + result = X509ToObject(env, cert); args.GetReturnValue().Set(result); } diff --git a/test/parallel/test-tls-pfx-authorizationerror.js b/test/parallel/test-tls-pfx-authorizationerror.js index 6daf89dff0d82f..5105a60dacd6de 100644 --- a/test/parallel/test-tls-pfx-authorizationerror.js +++ b/test/parallel/test-tls-pfx-authorizationerror.js @@ -37,8 +37,13 @@ const server = tls rejectUnauthorized: false }, function() { - assert.strictEqual(client.getCertificate().serialNumber, - 'ECC9B856270DA9A8'); + for (let i = 0; i < 10; ++i) { + // Calling this repeatedly is a regression test that verifies + // that .getCertificate() does not accidentally decrease the + // reference count of the X509* certificate on the native side. + assert.strictEqual(client.getCertificate().serialNumber, + 'ECC9B856270DA9A8'); + } client.end(); server.close(); }